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
