> -----Original Message----- > From: linux-pwm-owner@xxxxxxxxxxxxxxx <linux-pwm- > owner@xxxxxxxxxxxxxxx> On Behalf Of Uwe Kleine-König > Sent: Saturday, September 5, 2020 4:26 AM > To: G Jaya Kumaran, Vineetha <vineetha.g.jaya.kumaran@xxxxxxxxx> > Cc: thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx; linux- > pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Wan Mohamad, Wan > Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>; > andriy.shevchenko@xxxxxxxxxxxxxxx; Raja Subramanian, Lakshmi Bai > <lakshmi.bai.raja.subramanian@xxxxxxxxx>; mgross@xxxxxxxxxxxxxxx; > Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@xxxxxxxxx> > Subject: Re: [PATCH v5 1/2] pwm: Add PWM driver for Intel Keem Bay > > 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. > OK, will add clock enablement before using clk_get_rate(). > > > > + * 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. > Sorry, I meant I would keep it as is since div is an unsigned long long here, not an unsigned long. > > > > + /* 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(). > Okay, will do this. > Best regards > Uwe > Thank you, Vineetha > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
