Aw: Re: [PATCH v3] pwm: bcm2835: Support apply function for atomic configuration

[Lino Sanfilippo]

Hi Uwe

> Hello Lino,
>
> On Tue, Dec 08, 2020 at 11:01:45PM +0100, Lino Sanfilippo wrote:
> > Use the newer .apply function of pwm_ops instead of .config, .enable,
> > .disable and .set_polarity. This guarantees atomic changes of the pwm
> > controller configuration. It also reduces the size of the driver.
> >
> > Since now period is a 64 bit value, add an extra check to reject periods
> > that exceed the possible max value for the 32 bit register.
> >
> > This has been tested on a Raspberry PI 4.
>
> This looks right, just two small nitpicks below.
>

>
> This cast isn't necessary. (And if it was, I *think* the space between
> "(u32)" and "period" is wrong. But my expectation that checkpatch warns
> about this is wrong, so take this with a grain of salt.)

OK, I will omit the cast in the next patch version (it was primarily
meant for documentation purposes but now it seems to me rather
unusual for kernel code)

>
> > -	value = readl(pc->base + PWM_CONTROL);
> > -	value &= ~(PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > -	writel(value, pc->base + PWM_CONTROL);
> > -}
> > +	/* set duty cycle */
> > +	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, scaler);
> > +	writel(val, pc->base + DUTY(pwm->hwpwm));
> >
> > -static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> > -				enum pwm_polarity polarity)
> > -{
> > -	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> > -	u32 value;
> > +	/* set polarity */
> > +	val = readl(pc->base + PWM_CONTROL);
> >
> > -	value = readl(pc->base + PWM_CONTROL);
> > +	if (state->polarity == PWM_POLARITY_NORMAL)
> > +		val &= ~(PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > +	else
> > +		val |= PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm);
> >
> > -	if (polarity == PWM_POLARITY_NORMAL)
> > -		value &= ~(PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > +	/* enable/disable */
> > +	if (state->enabled)
> > +		val |= PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm);
> >  	else
> > -		value |= PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm);
> > +		val &= ~(PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm));
> >
> > -	writel(value, pc->base + PWM_CONTROL);
> > +	writel(val, pc->base + PWM_CONTROL);
> >
> >  	return 0;
> >  }
> >
> > +
>
> I wouldn't have added this empty line. But I guess that's subjective. Or
> did you add this by mistake?

I cannot remember that the line was added by intention, so I am fine to remove it.

Thanks and regards,
Lino