| Message ID | 1607464905-16630-1-git-send-email-LinoSanfilippo@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] pwm: bcm2835: Support apply function for atomic configuration | expand |
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, > };
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
Hello, 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. > > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Side note: I'm a bit surprised about the output of b4 diff 1607464905-16630-1-git-send-email-LinoSanfilippo@gmx.de This is probably due to the fact that compared to v3 you also rebased. Still the diff is quite big. Best regards and thanks for your patch Uwe
Hi Uwe, > Gesendet: Donnerstag, 10. Dezember 2020 um 12:43 Uhr > Von: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> > An: "Lino Sanfilippo" <LinoSanfilippo@gmx.de> > Cc: thierry.reding@gmail.com, lee.jones@linaro.org, nsaenzjulienne@suse.de, f.fainelli@gmail.com, rjui@broadcom.com, sean@mess.org, sbranden@broadcom.com, bcm-kernel-feedback-list@broadcom.com, linux-pwm@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org > Betreff: Re: [PATCH v3] pwm: bcm2835: Support apply function for atomic configuration > > Side note: I'm a bit surprised about the output of > > b4 diff 1607464905-16630-1-git-send-email-LinoSanfilippo@gmx.de > > This is probably due to the fact that compared to v3 you also rebased. > Still the diff is quite big. You are right, I made a rebase before I created the last patch, sorry for the confusion this caused. Anyway, thanks for the review(s)! Regards, Lino
On Fri, Dec 11, 2020 at 10:28:35AM +0100, Lino Sanfilippo wrote: > Hi Uwe, > > > Gesendet: Donnerstag, 10. Dezember 2020 um 12:43 Uhr > > Von: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> > > An: "Lino Sanfilippo" <LinoSanfilippo@gmx.de> > > Cc: thierry.reding@gmail.com, lee.jones@linaro.org, nsaenzjulienne@suse.de, f.fainelli@gmail.com, rjui@broadcom.com, sean@mess.org, sbranden@broadcom.com, bcm-kernel-feedback-list@broadcom.com, linux-pwm@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org > > Betreff: Re: [PATCH v3] pwm: bcm2835: Support apply function for atomic configuration > > > > > Side note: I'm a bit surprised about the output of > > > > b4 diff 1607464905-16630-1-git-send-email-LinoSanfilippo@gmx.de > > > > This is probably due to the fact that compared to v3 you also rebased. > > Still the diff is quite big. > > You are right, I made a rebase before I created the last patch, sorry for the confusion this caused. > Anyway, thanks for the review(s)! You did everything good enough. (To further improve, you could use git-format-patch's --base option and mention a rebase in the series' changelog; note this is quite high level critic.) This was more me wondering the output is not easier to use. (And note I also showed the wrong commandline, but that doesn't resolve the issue. The right command is: b4 diff 1607546905-20549-1-git-send-email-LinoSanfilippo@gmx.de .) Best regards Uwe
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)); - 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; } + 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, };
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. 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(-)