Hello, On Fri, Sep 04, 2020 at 09:42:37AM +0000, G Jaya Kumaran, Vineetha wrote: > > > + clk_rate = clk_get_rate(priv->clk); > > > > clk_get_rate() must only be called when the clock is enabled. Unless I miss > > something this isn't ensured here. > > My understanding is this would not be a problem, as according to > databook, the GPIO block clock is auto-enabled, and also we are not > doing any disabling in the driver for it. It might not be a problem on your hardware today. But this is an API requirement. > > > + * The upper 16 bits of the KMB_PWM_HIGHLOW_OFFSET register contain > > > + * the high time of the waveform, while the last 16 bits contain > > > + * the low time of the waveform, in terms of clock cycles. > > > + * > > > + * high time = clock rate * duty cycle > > > + * low time = clock rate * (period - duty cycle) > > > + * > > > + * e.g. For period 50us, duty cycle 30us, and clock rate 500MHz: > > > + * high time = 500MHz * 30us = 0x3A98 > > > + * low time = 500MHz * 20us = 0x2710 > > > + * Value written to KMB_PWM_HIGHLOW_OFFSET = 0x3A982710 > > > + */ > > > + > > > + clk_rate = clk_get_rate(priv->clk); > > > + > > > + /* Configure waveform high time */ > > > + div = clk_rate * state->duty_cycle; > > > > Since v5.9-rc1 (commit a9d887dc1c60ed67f2271d66560cdcf864c4a578) > > state->duty_cycle is a 64 bit type. So div being unsigned long isn't > > big enough on some platforms. > > div is 64-bit here, so I guess I can keep it as is? unsigned long isn't 64 bits wide on all platforms. > > > + /* Ensure enable bit for each channel is cleared at boot */ > > > + for (ch = 0; ch < KMB_TOTAL_PWM_CHANNELS; ch++) > > > + keembay_pwm_disable(priv, ch); > > > > .probe() is not supposed to change the state of the PWM. > > > > Sorry, I think misunderstood one of your comments in V2 and added this. > The reset value of the enable bit (and all other bits) in the LEADIN register is 0, so this may not be needed. > If it's ok, I'll remove it. I think the right approach is to set the LEADIN register to 0 in .apply(). Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
