RE: [PATCH V3 04/10] gpio: vf610: add optional clock support

["A.s. Dong" <aisheng.dong@xxxxxxx>]

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@xxxxxxxxxx]
> Sent: Friday, November 2, 2018 5:33 PM
[...]
> On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@xxxxxxx> wrote:
> 
> > +       clk_port = devm_clk_get(&pdev->dev, "port");
> > +       if (clk_port == ERR_PTR(-EPROBE_DEFER))
> > +               return -EPROBE_DEFER;
> > +
> > +       clk_gpio = devm_clk_get(&pdev->dev, "gpio");
> > +       if (clk_gpio == ERR_PTR(-EPROBE_DEFER))
> > +               return -EPROBE_DEFER;
> > +
> > +       if (!IS_ERR_OR_NULL(clk_port)) {
> > +               ret = clk_prepare_enable(clk_port);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       if (!IS_ERR_OR_NULL(clk_gpio)) {
> > +               ret = clk_prepare_enable(clk_gpio);
> > +               if (ret) {
> > +                       clk_disable_unprepare(clk_port);
> > +                       return ret;
> > +               }
> > +       }
> 
> I think IS_ERR_OR_NULL() is considered harmful among other things.
> 
> What about this pattern:
> 
>         clk = devm_clk_get(dev, "foo");
>         if (!IS_ERR(clk)) {
>                 ret = clk_prepare_enable(clk);
>                 if (ret)
>                         return ret;
>         } else if (PTR_ERR(clk) == -EPROBE_DEFER) {
>                 /*
>                  * Percolate deferrals, for anything else,
>                  * just live without the clocking.
>                  */
>                 return PTR_ERR(clk);
>         }
> 
> You also need to introduce code to disable the clock on
> .remove() or the clock will always be on after this module has probed. This
> applies also to builtin drivers, unless you suppress the sysfs attributes and use
> platform_driver_probe()
> 

Looks good to me. Will update the patch.
Thanks for the suggestion.

Regards
Dong Aisheng

> Yours,
> Linus Walleij