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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3650 bytes --]

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]