Hi Uwe,
-----Original Message-----
From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
Sent: Friday, 16 October, 2020 1:04 PM
To: Ayyathurai, Vijayakannan <vijayakannan.ayyathurai@xxxxxxxxx>
Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Wan Mohamad, Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>; mgross@xxxxxxxxxxxxxxx; Raja Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@xxxxxxxxx>
Subject: Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay
Hello Ayyathurai,
Can you please fix your MUA to properly quote when replying, this is really annoying.
Sorry for that MUA (Made Up Acronym).
On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote:
> > On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@xxxxxxxxx wrote:
> > > +static int keembay_pwm_remove(struct platform_device *pdev) {
> > > + struct keembay_pwm *priv = platform_get_drvdata(pdev);
> > > + int ret;
> > > +
> > > + ret = pwmchip_remove(&priv->chip);
> > > + clk_disable_unprepare(priv->clk);
> > > +
> > > + return ret;
> >
> > ...and this will be simplified to
> >
> > return pwmchip_remove(&priv->chip);
>
> Until v10, It is as per your suggestion. But I have changed it in v11
> to overcome the issue mentioned by Uwe. I have kept the snip of v10
> FYR below.
>
> //Start snip from v10 review mailing list //> +static int
> keembay_pwm_remove(struct platform_device *pdev) {
> //> + struct keembay_pwm *priv = platform_get_drvdata(pdev);
> //> +
> //> + clk_disable_unprepare(priv->clk);
> //> +
> //> + return pwmchip_remove(&priv->chip);
> //
> //You have to call pwmchip_remove first. Otherwise you're stopping the PWM while the framework still believes everything to be fine.
> //
> //> +}
> //End snip from v10 review mailing review
Note that we're both (Andy and I) are right. You must not disable the clocks before pwmchip_remove() (otherwise for a short time the PWM looks ready but isn't). And if you use devm-stuff to enable the clock it will be disabled only after the remove callback completed and your .remove may look like:
static int keembay_pwm_remove(struct platform_device *pdev)
{
struct keembay_pwm *priv = platform_get_drvdata(pdev);
return pwmchip_remove(&priv->chip);
}
because you won't have to care for the clock explicitly.
OK. I will incorporate it in the next version.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Thanks,
Vijay
