Messages in this thread | | | Date | Wed, 9 Dec 2020 08:05:16 +0100 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v3] pwm: bcm2835: Support apply function for atomic configuration |
| |
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.
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > --- > > v3: Check against period truncation (based on a review by Uwe Kleine-König) > v2: Fix compiler error for 64 bit builds > > drivers/pwm/pwm-bcm2835.c | 72 +++++++++++++++++------------------------------ > 1 file changed, 26 insertions(+), 46 deletions(-) > > diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c > index 6841dcf..d339898 100644 > --- a/drivers/pwm/pwm-bcm2835.c > +++ b/drivers/pwm/pwm-bcm2835.c > @@ -58,13 +58,15 @@ static void bcm2835_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > writel(value, pc->base + PWM_CONTROL); > } > > -static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > - int duty_ns, int period_ns) > +static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > { > + > struct bcm2835_pwm *pc = to_bcm2835_pwm(chip); > unsigned long rate = clk_get_rate(pc->clk); > + unsigned long long period; > unsigned long scaler; > - u32 period; > + u32 val; > > if (!rate) { > dev_err(pc->dev, "failed to get clock rate\n"); > @@ -72,65 +74,43 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > } > > scaler = DIV_ROUND_CLOSEST(NSEC_PER_SEC, rate); > - period = DIV_ROUND_CLOSEST(period_ns, scaler); > + /* set period */ > + period = DIV_ROUND_CLOSEST_ULL(state->period, scaler); > > - if (period < PERIOD_MIN) > + /* dont accept a period that is too small or has been truncated */ > + if ((period < PERIOD_MIN) || (period > U32_MAX)) > return -EINVAL; > > - writel(DIV_ROUND_CLOSEST(duty_ns, scaler), > - pc->base + DUTY(pwm->hwpwm)); > - writel(period, pc->base + PERIOD(pwm->hwpwm)); > - > - return 0; > -} > - > -static int bcm2835_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > -{ > - struct bcm2835_pwm *pc = to_bcm2835_pwm(chip); > - u32 value; > - > - value = readl(pc->base + PWM_CONTROL); > - value |= PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm); > - writel(value, pc->base + PWM_CONTROL); > - > - return 0; > -} > - > -static void bcm2835_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > -{ > - struct bcm2835_pwm *pc = to_bcm2835_pwm(chip); > - u32 value; > + writel((u32) period, pc->base + PERIOD(pwm->hwpwm));
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.)
> - 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?
> static const struct pwm_ops bcm2835_pwm_ops = { > .request = bcm2835_pwm_request, > .free = bcm2835_pwm_free, > - .config = bcm2835_pwm_config, > - .enable = bcm2835_pwm_enable, > - .disable = bcm2835_pwm_disable, > - .set_polarity = bcm2835_set_polarity, > + .apply = bcm2835_pwm_apply, > .owner = THIS_MODULE, > };
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |