On Fri, Aug 21, 2020 at 02:07:49PM +0800, Tanwar, Rahul wrote:

On 20/8/2020 6:52 pm, Andy Shevchenko wrote:

quoted

On Thu, Aug 20, 2020 at 12:50:46PM +0800, Rahul Tanwar wrote:

...

quoted

quoted

+	ret = clk_prepare_enable(pc->clk);

Wrap it with devm_add_action_or_reset(). Same for reset_control_deassert().
You probably can even put them under one function.

I did some study and research for using devm_add_action_or_reset(). But
still i have some doubts. Below steps is what i intend to do in order to
switch to using this API. Please do review and let me know it is ok and
i am not missing anything else. Thanks.

1. Call reset_control_assert()
2. Call clk_prepare_enable()

3. Call pwmchip_add()

First of all, I haven't told anything about this.

4. Call devm_add_action_or_reset(dev, my_action, pc)
5. Remove explicit calls to unprepare/reset_control_assert from probe in
failure cases.
6. static void my_action(void *pc)
   {
      pwmchip_remove(&pc->chip);
      clk_disable_upprepare(pc->clk);
      reset_control_assert(pc->rst);
   }
7. Remove platform_driver.remove() entirely.

Nope, pwmchip_add() and pwmchip_remove() stay as is now.

Only wrap clock and reset resources.

quoted

quoted

+	if (ret) {
+		dev_err(dev, "failed to enable clock\n");
+		reset_control_assert(pc->rst);
+		return ret;
+	}

-- 
With Best Regards,
Andy Shevchenko