Hello Ayyathurai,

Can you please fix your MUA to properly quote when replying, this is
really annoying.

On Fri, Oct 16, 2020 at 03:18:08AM +0000, Ayyathurai, Vijayakannan wrote:

quoted

On Thu, Oct 15, 2020 at 03:36:09AM +0800, vijayakannan.ayyathurai@intel.com wrote:

quoted

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |