On Wed, Dec 19, 2012 at 03:10:20PM +0100, Boris BREZILLON wrote:
> On 19/12/2012 12:26, Thierry Reding wrote:
> > On Mon, Dec 17, 2012 at 12:13:30PM +0100, Boris BREZILLON wrote:
[...]
> >> + /* configure new setting */
> >> + cmr |= newcmr;
> >> + __raw_writel(cmr, regs + ATMEL_TC_REG(group, CMR));
> >
> > I wonder why you bother setting newcmr and OR'ing it into cmr. Couldn't
> > you just mask all previous settings in cmr first, then OR the new bits?
>
> I did this to keep the locked portion of code as small as possible:
> I prepare the mask to apply to cmr register before getting the lock.
>
> But I can do it this way if you prefer:
>
> spin_lock(&tcbpwmc->lock);
> cmr = __raw_readl(regs + ATMEL_TC_REG(group, CMR));
>
> /* flush old setting and set the new one */
> if (index == 0) {
> cmr &= ~ATMEL_TC_A_MASK;
> if (polarity == PWM_POLARITY_INVERSED)
> cmr |= ATMEL_TC_ASWTRG_CLEAR;
> else
> cmr |= ATMEL_TC_ASWTRG_SET;
> } else {
> cmr &= ~ATMEL_TC_B_MASK;
> if (polarity == PWM_POLARITY_INVERSED)
> cmr |= ATMEL_TC_BSWTRG_CLEAR;
> else
> cmr |= ATMEL_TC_BSWTRG_SET;
> }
>
> __raw_writel(cmr, regs + ATMEL_TC_REG(group, CMR));
Yes, that's along the lines of what I had in mind. It was more of a
suggestion because I think the above looks more obvious. But if you
think having a shorter critical section is worth it, then that's fine
too.
> >> + /*
> >> + * If duty is 0 or equal to period there's no need to register
> >> + * a specific action on RA/RB and RC compare.
> >> + * The output will be configured on software trigger and keep
> >> + * this config till next config call.
> >> + */
> >> + if (tcbpwm->duty != tcbpwm->period && tcbpwm->duty > 0) {
> >> + if (polarity == PWM_POLARITY_INVERSED) {
> >> + if (index == 0)
> >> + newcmr |=
> >> + ATMEL_TC_ACPA_SET | ATMEL_TC_ACPC_CLEAR;
> >> + else
> >> + newcmr |=
> >> + ATMEL_TC_BCPB_SET | ATMEL_TC_BCPC_CLEAR;
> >> + } else {
> >> + if (index == 0)
> >> + newcmr |=
> >> + ATMEL_TC_ACPA_CLEAR | ATMEL_TC_ACPC_SET;
> >> + else
> >> + newcmr |=
> >> + ATMEL_TC_BCPB_CLEAR | ATMEL_TC_BCPC_SET;
> >
> > If you can get rid of newcmr and OR directly into cmr instead, these
> > will fit on one line.
>
> I'm not sure I understand how you would do this.
> Here is the same function without the newcmr variable:
What I meant to say was: "these will each fit on one line".
> static int atmel_tcb_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct atmel_tcb_pwm_chip *tcbpwmc = to_tcb_chip(chip);
> struct atmel_tcb_pwm_device *tcbpwm = pwm_get_chip_data(pwm);
> struct atmel_tc *tc = tcbpwmc->tc;
> void __iomem *regs = tc->regs;
> unsigned group = pwm->hwpwm / 2;
> unsigned index = pwm->hwpwm % 2;
> u32 cmr;
> enum pwm_polarity polarity = tcbpwm->polarity;
>
> /* If duty is 0 reverse polarity */
> if (tcbpwm->duty == 0)
> polarity = !polarity;
>
> spin_lock(&tcbpwmc->lock);
> cmr = __raw_readl(regs + ATMEL_TC_REG(group, CMR));
>
> /* flush old setting and set the new one */
> cmr &= ~ATMEL_TC_TCCLKS;
> if (index == 0) {
> cmr &= ~ATMEL_TC_A_MASK;
>
> /* Set CMR flags according to given polarity */
> if (polarity == PWM_POLARITY_INVERSED) {
> cmr |= ATMEL_TC_ASWTRG_CLEAR;
>
> /*
> * If duty is 0 or equal to period there's no need to
> register
> * a specific action on RA/RB and RC compare.
> * The output will be configured on software trigger
> and keep
> * this config till next config call.
> */
> if (tcbpwm->duty != tcbpwm->period && tcbpwm->duty > 0)
> cmr |= ATMEL_TC_ACPA_SET | ATMEL_TC_ACPC_CLEAR;
> } else {
> cmr |= ATMEL_TC_ASWTRG_SET;
> if (tcbpwm->duty != tcbpwm->period && tcbpwm->duty > 0)
> cmr |= ATMEL_TC_ACPA_CLEAR | ATMEL_TC_ACPC_SET;
> }
> } else {
> cmr &= ~ATMEL_TC_B_MASK;
> if (polarity == PWM_POLARITY_INVERSED) {
> cmr |= ATMEL_TC_BSWTRG_CLEAR;
> if (tcbpwm->duty != tcbpwm->period && tcbpwm->duty > 0)
> cmr |= ATMEL_TC_BCPB_SET | ATMEL_TC_BCPC_CLEAR;
> } else {
> cmr |= ATMEL_TC_BSWTRG_SET;
> if (tcbpwm->duty != tcbpwm->period && tcbpwm->duty > 0)
> cmr |= ATMEL_TC_BCPA_CLEAR | ATMEL_TC_BCPC_SET;
> }
> }
>
> __raw_writel(cmr, regs + ATMEL_TC_REG(group, CMR));
>
> if (index == 0)
> __raw_writel(tcbpwm->duty, regs + ATMEL_TC_REG(group, RA));
> else
> __raw_writel(tcbpwm->duty, regs + ATMEL_TC_REG(group, RB));
>
> __raw_writel(tcbpwm->period, regs + ATMEL_TC_REG(group, RC));
>
> /* Use software trigger to apply the new setting */
> __raw_writel(ATMEL_TC_CLKEN | ATMEL_TC_SWTRG,
> regs + ATMEL_TC_REG(group, CCR));
> spin_unlock(&tcbpwmc->lock);
> return 0;
> }
>
>
> Is that what you're expecting?
Looking at the code above, maybe reshuffling isn't such a good idea
after all as you have to repeat the "duty 0 or equal to period" check
four times.
Thierry
pgpmmtmkzviXg.pgp
Description: PGP signature
