On Fri, Oct 16, 2020 at 12:21:27PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote:
> > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Sent: Thursday, 15 October, 2020 4:12 PM
> > On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@xxxxxxxxx wrote:
> > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@xxxxxxxxx>
>
> ...
>
> > > + priv->clk = devm_clk_get(dev, NULL);
> > > + if (IS_ERR(priv->clk))
> > > + return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get
> > > +clock\n");
> > > +
> > > + ret = clk_prepare_enable(priv->clk);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + priv->base = devm_platform_ioremap_resource(pdev, 0);
> > > + if (IS_ERR(priv->base)) {
> >
> > > + clk_disable_unprepare(priv->clk);
> >
> > See below.
> >
> > > + return PTR_ERR(priv->base);
> > > + }
> > > +
> > > + priv->chip.base = -1;
> > > + priv->chip.dev = dev;
> > > + priv->chip.ops = &keembay_pwm_ops;
> > > + priv->chip.npwm = KMB_TOTAL_PWM_CHANNELS;
> > > +
> > > + ret = pwmchip_add(&priv->chip);
> > > + if (ret) {
> > > + dev_err(dev, "Failed to add PWM chip: %pe\n", ERR_PTR(ret));
> >
> > > + clk_disable_unprepare(priv->clk);
> >
> > This messes up with ordering of things.
> >
> > That's why devm golden rule is either all or none. You may fix this by switching to devm_add_action_or_reset().
> >
> > One of possible way is like in below drivers:
> >
> > % git grep -n devm_add_action_or_reset.*disable_unprepare -- drivers/
> >
> > But it may be fixed in follow up change. Depends on maintainers' wishes.
> >
> > Sure. I shall incorporate and check based on maintainers wish in the next version.
> >
> > > + return ret;
> > > + }
> > > +
> > > + platform_set_drvdata(pdev, priv);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +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
> >
> > > +}
>
> What I said does not contradict with what Uwe said. So, please, fix ordering
> either by dropping devm_ in the middle or adding devm_ action.
>
> Now you moved serious ordering issue in ->remove() (which Uwe noted) to less
> serious in ->probe(). But issue is still present.
The easiest way is probably to do the clk_prepare_enable() only after
all devm_ stuff in .probe. (But I'm not emphatic about that, it would be
fine for me to wait until devm_clk_get_enabled() lands.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
