On Wed, Feb 13, 2019 at 01:37:03PM +0100, Thierry Reding wrote: > On Thu, Feb 07, 2019 at 11:16:57AM +0100, Uwe Kleine-König wrote: > > On Tue, Jan 29, 2019 at 05:13:19PM +0530, Yash Shah wrote: > [...] > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c > [...] > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG); > > > + > > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm * SIZE_PWMCMP); > > > + > > > + val &= ~(1 << PWM_SIFIVE_PWMCFG_DEGLITCH); > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG); > > > + > > > + pwm_sifive_get_state(chip, dev, state); > > > > Thierry: This changes the pwm_state. Is this how this should be done? > > Yes, I think that's fine. The PWM state should always reflect the > current hardware state. If the configuration that we program does not > reflect the state that was requested, that should be reflected in the > PWM state. I'm not sure you blessed what is really done here. If I do: state.duty_cycle = state.period; pwm_apply_state(pwm, &state); the call in question doesn't only result in pwm->state.duty_cycle < pwm->state.period, but it also corrects my local state variable (i.e. I have state.duty_cycle < state.period afterwards). Is this what you thought to be fine? Also note that v6 dropped that call because of my doubts. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
