Message ID | 20190824001041.11007-7-uwe@kleine-koenig.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Updates for the atmel PWM driver | expand |
On 24.08.2019 03:10, Uwe Kleine-König wrote: > External E-Mail > > > This function reads back the configured parameters from the hardware. As > .apply rounds down (mostly) I'm rounding up in .get_state() to achieve > that applying a state just read from hardware is a no-op. Since this read is only at probing, at least for the moment, and, as far as I remember, the idea w/ .get_state was to reflect in Linux the states of PWMs that were setup before Linux takes control (e.g. PWMs setup in bootloaders) I think it would no problem if it would be no-ops in this scenario. In case of run-time state retrieval, pwm_get_state() should be enough. If one would get the state previously saved w/ this .get_state API he/she would change it, then it would apply the changes to the hardware. No changes of PWM state would be anyway skipped from the beginning, in pwm_apply_state() by this code: if (state->period == pwm->state.period && state->duty_cycle == pwm->state.duty_cycle && state->polarity == pwm->state.polarity && state->enabled == pwm->state.enabled) return 0; But maybe I'm missing something. > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > --- > New in v2 > > drivers/pwm/pwm-atmel.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > index 152c2b7dd6df..f788501ab6ca 100644 > --- a/drivers/pwm/pwm-atmel.c > +++ b/drivers/pwm/pwm-atmel.c > @@ -295,8 +295,47 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > return 0; > } > > +static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + u32 sr, cmr; > + > + sr = atmel_pwm_readl(atmel_pwm, PWM_SR); > + cmr = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); > + > + if (sr & (1 << pwm->hwpwm)) { > + unsigned long rate = clk_get_rate(atmel_pwm->clk); > + u32 cdty, cprd, pres; There is a whitespace at the end of this line. > + u64 tmp; > + > + pres = cmr & PWM_CMR_CPRE_MSK; > + > + cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.period); If this is possible, please try to keep it at 80 chars per line. In my opinion this still looks good: cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.period); > + tmp = (u64)cprd * NSEC_PER_SEC; > + tmp <<= pres; > + state->period = DIV64_U64_ROUND_UP(tmp, rate); > + > + cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.duty); Ditto. > + tmp = (u64)cdty * NSEC_PER_SEC; > + tmp <<= pres; > + state->duty_cycle = DIV64_U64_ROUND_UP(tmp, rate); > + > + state->enabled = true; > + } else { > + state->enabled = false; > + } > + > + if (cmr & PWM_CMR_CPOL) > + state->polarity = PWM_POLARITY_INVERSED; > + else > + state->polarity = PWM_POLARITY_NORMAL; > + > +} > + > static const struct pwm_ops atmel_pwm_ops = { > .apply = atmel_pwm_apply, > + .get_state = atmel_pwm_get_state, > .owner = THIS_MODULE, > }; Other than the minor things above, Acked-by: Claudiu Beznea <claudiu.beznea@microchip.com> > >
Hello Claudiu, On Wed, Aug 28, 2019 at 10:26:18AM +0000, Claudiu.Beznea@microchip.com wrote: > On 24.08.2019 03:10, Uwe Kleine-König wrote: > > External E-Mail > > This function reads back the configured parameters from the hardware. As > > .apply rounds down (mostly) I'm rounding up in .get_state() to achieve > > that applying a state just read from hardware is a no-op. > > Since this read is only at probing, at least for the moment, and, as far as Yes, up to now .get_state() is only called at probing time. There is a patch series (by me) on the list that changes that. (Though I'm not entirely sure this is a good idea. Will comment my doubts in that thread later.) > I remember, the idea w/ .get_state was to reflect in Linux the states of > PWMs that were setup before Linux takes control (e.g. PWMs setup in > bootloaders) I think it would no problem if it would be no-ops in this > scenario. IMHO it should be a no-op. > In case of run-time state retrieval, pwm_get_state() should be > enough. If one would get the state previously saved w/ this .get_state API > he/she would change it, then it would apply the changes to the hardware. No > changes of PWM state would be anyway skipped from the beginning, in > pwm_apply_state() by this code: > > if (state->period == pwm->state.period && > state->duty_cycle == pwm->state.duty_cycle && > state->polarity == pwm->state.polarity && > state->enabled == pwm->state.enabled) > return 0; > > But maybe I'm missing something. There is a problem I want to solve generally, not only for the atmel driver. For example I consider it "expected" that s1 = pwm_get_state(pwm) pwm_apply_state(pwm, s2) pwm_apply_state(pwm, s1) ends in the same configuration as it started. For that it is necessary (even for the atmel driver with the guard you pointed out above) to round up in .get_state if .apply rounds down. Best regards Uwe
diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index 152c2b7dd6df..f788501ab6ca 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -295,8 +295,47 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return 0; } +static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); + u32 sr, cmr; + + sr = atmel_pwm_readl(atmel_pwm, PWM_SR); + cmr = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); + + if (sr & (1 << pwm->hwpwm)) { + unsigned long rate = clk_get_rate(atmel_pwm->clk); + u32 cdty, cprd, pres; + u64 tmp; + + pres = cmr & PWM_CMR_CPRE_MSK; + + cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.period); + tmp = (u64)cprd * NSEC_PER_SEC; + tmp <<= pres; + state->period = DIV64_U64_ROUND_UP(tmp, rate); + + cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.duty); + tmp = (u64)cdty * NSEC_PER_SEC; + tmp <<= pres; + state->duty_cycle = DIV64_U64_ROUND_UP(tmp, rate); + + state->enabled = true; + } else { + state->enabled = false; + } + + if (cmr & PWM_CMR_CPOL) + state->polarity = PWM_POLARITY_INVERSED; + else + state->polarity = PWM_POLARITY_NORMAL; + +} + static const struct pwm_ops atmel_pwm_ops = { .apply = atmel_pwm_apply, + .get_state = atmel_pwm_get_state, .owner = THIS_MODULE, };
This function reads back the configured parameters from the hardware. As .apply rounds down (mostly) I'm rounding up in .get_state() to achieve that applying a state just read from hardware is a no-op. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- New in v2 drivers/pwm/pwm-atmel.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)