Re: [PATCH v12 1/2] pwm: Add PWM driver for Intel Keem Bay

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote:
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 
> Sent: Thursday, 15 October, 2020 4:12 PM
> On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote:
> > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@intel.com>

...

> > +	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.

-- 
With Best Regards,
Andy Shevchenko