| Message ID | 20220328073434.44848-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | daa986d5f8d8871e6691b0fe54375f263c754d04 |
| Headers | show |
| Series | pwm: samsung: Implement .apply() callback | expand |
On 28/03/2022 09:34, Uwe Kleine-König wrote: > To eventually get rid of all legacy drivers convert this driver to the > modern world implementing .apply(). > > The size check for state->period is moved to .apply() to make sure that > the values of state->duty_cycle and state->period are passed to > pwm_samsung_config without change while they are discarded to int. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/pwm-samsung.c | 54 ++++++++++++++++++++++++++++++--------- > 1 file changed, 42 insertions(+), 12 deletions(-) > > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c > index 0a4ff55fad04..9c5b4f515641 100644 > --- a/drivers/pwm/pwm-samsung.c > +++ b/drivers/pwm/pwm-samsung.c > @@ -321,14 +321,6 @@ static int __pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm, > struct samsung_pwm_channel *chan = pwm_get_chip_data(pwm); > u32 tin_ns = chan->tin_ns, tcnt, tcmp, oldtcmp; > > - /* > - * We currently avoid using 64bit arithmetic by using the > - * fact that anything faster than 1Hz is easily representable > - * by 32bits. > - */ > - if (period_ns > NSEC_PER_SEC) > - return -ERANGE; > - > tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm)); > oldtcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm)); > > @@ -438,13 +430,51 @@ static int pwm_samsung_set_polarity(struct pwm_chip *chip, > return 0; > } > > +static int pwm_samsung_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + int err, enabled = pwm->state.enabled; > + > + if (state->polarity != pwm->state.polarity) { > + if (enabled) { > + pwm_samsung_disable(chip, pwm); > + enabled = false; > + } > + > + err = pwm_samsung_set_polarity(chip, pwm, state->polarity); > + if (err) > + return err; > + } > + > + if (!state->enabled) { > + if (enabled) > + pwm_samsung_disable(chip, pwm); > + > + return 0; > + } > + > + /* > + * We currently avoid using 64bit arithmetic by using the > + * fact that anything faster than 1Hz is easily representable > + * by 32bits. > + */ > + if (state->period > NSEC_PER_SEC) Isn't this changing a bit logic in case of setting wrong period and inverted signal? Before, the config would return early and do nothing. Now, you disable the PWM, check the condition here and exit with PWM disabled. I think this should reverse the tasks done, or the check should be done early. > + return -ERANGE; > + > + err = pwm_samsung_config(chip, pwm, state->duty_cycle, state->period); > + if (err) > + return err; Best regards, Krzysztof
On Fri, Apr 22, 2022 at 05:11:02PM +0200, Krzysztof Kozlowski wrote: > On 28/03/2022 09:34, Uwe Kleine-König wrote: > > To eventually get rid of all legacy drivers convert this driver to the > > modern world implementing .apply(). > > > > The size check for state->period is moved to .apply() to make sure that > > the values of state->duty_cycle and state->period are passed to > > pwm_samsung_config without change while they are discarded to int. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/pwm-samsung.c | 54 ++++++++++++++++++++++++++++++--------- > > 1 file changed, 42 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c > > index 0a4ff55fad04..9c5b4f515641 100644 > > --- a/drivers/pwm/pwm-samsung.c > > +++ b/drivers/pwm/pwm-samsung.c > > @@ -321,14 +321,6 @@ static int __pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm, > > struct samsung_pwm_channel *chan = pwm_get_chip_data(pwm); > > u32 tin_ns = chan->tin_ns, tcnt, tcmp, oldtcmp; > > > > - /* > > - * We currently avoid using 64bit arithmetic by using the > > - * fact that anything faster than 1Hz is easily representable > > - * by 32bits. > > - */ > > - if (period_ns > NSEC_PER_SEC) > > - return -ERANGE; > > - > > tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm)); > > oldtcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm)); > > > > @@ -438,13 +430,51 @@ static int pwm_samsung_set_polarity(struct pwm_chip *chip, > > return 0; > > } > > > > +static int pwm_samsung_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + int err, enabled = pwm->state.enabled; > > + > > + if (state->polarity != pwm->state.polarity) { > > + if (enabled) { > > + pwm_samsung_disable(chip, pwm); > > + enabled = false; > > + } > > + > > + err = pwm_samsung_set_polarity(chip, pwm, state->polarity); > > + if (err) > > + return err; > > + } > > + > > + if (!state->enabled) { > > + if (enabled) > > + pwm_samsung_disable(chip, pwm); > > + > > + return 0; > > + } > > + > > + /* > > + * We currently avoid using 64bit arithmetic by using the > > + * fact that anything faster than 1Hz is easily representable > > + * by 32bits. > > + */ > > + if (state->period > NSEC_PER_SEC) > > Isn't this changing a bit logic in case of setting wrong period and > inverted signal? > > Before, the config would return early and do nothing. Now, you disable > the PWM, check the condition here and exit with PWM disabled. I think > this should reverse the tasks done, or the check should be done early. The intension here is to just push the legacy implementation of .apply() (i.e. pwm_apply_legacy()) into the driver, to be able to get rid of the legacy handing in the core. I think the problem you point out is real, but it is not introduced by my change which doesn't change behaviour. The problem is just that it's not possible to "see" that period is invalid at the time the polarity is changed and so it exists since at least 5ec803edcb703fe379836f13560b79dfac79b01d, my patch just made it more obvious. So yes, there is potential to further improve the driver, but that's out of scope for this 1:1 conversion patch. Best regards Uwe
On 25/04/2022 19:16, Uwe Kleine-König wrote: >>> + /* >>> + * We currently avoid using 64bit arithmetic by using the >>> + * fact that anything faster than 1Hz is easily representable >>> + * by 32bits. >>> + */ >>> + if (state->period > NSEC_PER_SEC) >> >> Isn't this changing a bit logic in case of setting wrong period and >> inverted signal? >> >> Before, the config would return early and do nothing. Now, you disable >> the PWM, check the condition here and exit with PWM disabled. I think >> this should reverse the tasks done, or the check should be done early. > > The intension here is to just push the legacy implementation of .apply() > (i.e. pwm_apply_legacy()) into the driver, to be able to get rid of the > legacy handing in the core. I think the problem you point out is real, > but it is not introduced by my change which doesn't change behaviour. > > The problem is just that it's not possible to "see" that period is > invalid at the time the polarity is changed and so it exists since at > least 5ec803edcb703fe379836f13560b79dfac79b01d, my patch just made it > more obvious. > > So yes, there is potential to further improve the driver, but that's out > of scope for this 1:1 conversion patch. Sounds reasonable, thanks for explanation. Everything else was looking good, so: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Mon, Mar 28, 2022 at 09:34:34AM +0200, Uwe Kleine-König wrote: > To eventually get rid of all legacy drivers convert this driver to the > modern world implementing .apply(). > > The size check for state->period is moved to .apply() to make sure that > the values of state->duty_cycle and state->period are passed to > pwm_samsung_config without change while they are discarded to int. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/pwm-samsung.c | 54 ++++++++++++++++++++++++++++++--------- > 1 file changed, 42 insertions(+), 12 deletions(-) Applied, thanks. Thierry
diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c index 0a4ff55fad04..9c5b4f515641 100644 --- a/drivers/pwm/pwm-samsung.c +++ b/drivers/pwm/pwm-samsung.c @@ -321,14 +321,6 @@ static int __pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm, struct samsung_pwm_channel *chan = pwm_get_chip_data(pwm); u32 tin_ns = chan->tin_ns, tcnt, tcmp, oldtcmp; - /* - * We currently avoid using 64bit arithmetic by using the - * fact that anything faster than 1Hz is easily representable - * by 32bits. - */ - if (period_ns > NSEC_PER_SEC) - return -ERANGE; - tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm)); oldtcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm)); @@ -438,13 +430,51 @@ static int pwm_samsung_set_polarity(struct pwm_chip *chip, return 0; } +static int pwm_samsung_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + int err, enabled = pwm->state.enabled; + + if (state->polarity != pwm->state.polarity) { + if (enabled) { + pwm_samsung_disable(chip, pwm); + enabled = false; + } + + err = pwm_samsung_set_polarity(chip, pwm, state->polarity); + if (err) + return err; + } + + if (!state->enabled) { + if (enabled) + pwm_samsung_disable(chip, pwm); + + return 0; + } + + /* + * We currently avoid using 64bit arithmetic by using the + * fact that anything faster than 1Hz is easily representable + * by 32bits. + */ + if (state->period > NSEC_PER_SEC) + return -ERANGE; + + err = pwm_samsung_config(chip, pwm, state->duty_cycle, state->period); + if (err) + return err; + + if (!pwm->state.enabled) + err = pwm_samsung_enable(chip, pwm); + + return err; +} + static const struct pwm_ops pwm_samsung_ops = { .request = pwm_samsung_request, .free = pwm_samsung_free, - .enable = pwm_samsung_enable, - .disable = pwm_samsung_disable, - .config = pwm_samsung_config, - .set_polarity = pwm_samsung_set_polarity, + .apply = pwm_samsung_apply, .owner = THIS_MODULE, };
To eventually get rid of all legacy drivers convert this driver to the modern world implementing .apply(). The size check for state->period is moved to .apply() to make sure that the values of state->duty_cycle and state->period are passed to pwm_samsung_config without change while they are discarded to int. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-samsung.c | 54 ++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 12 deletions(-) base-commit: ed14d36498c8d15be098df4af9ca324f96e9de74