Re: [PATCH v3] pwm: atmel: add Timer Counter Block PWM driver

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 5049 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]