Re: [PATCH] gpio: nomadik: Back out some managed resources

[Théo Lebrun <theo.lebrun@xxxxxxxxxxx>]

Hello,

On Tue Mar 5, 2024 at 6:05 PM CET, Andy Shevchenko wrote:
> On Tue, Mar 5, 2024 at 10:26 AM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> >
> > Several commits introduce managed resources (devm_*) into the
> > nmk_gpio_populate_chip() function.
> >
> > This isn't always working because when called from the Nomadik pin
> > control driver we just want to populate some states for the device as
> > the same states are used by the pin control driver.
> >
> > Some managed resources such as devm_kzalloc() etc will work, as the
> > passed in platform device will be used for lifecycle management,
> > but in some cases where we used the looked-up platform device
> > for the GPIO block, this will cause problems for the combined
> > pin control and GPIO driver, because it adds managed resources
> > to the GPIO device before it is probed, which is something that
> > the device core will not accept, and all of the GPIO blocks will
> > refuse to probe:
> >
> > platform 8012e000.gpio: Resources present before probing
> > platform 8012e080.gpio: Resources present before probing
> > (...)
> >
> > Fix this by not tying any managed resources to the looked-up
> > gpio_pdev/gpio_dev device, let's just live with the fact that
> > these need imperative resource management for now.
>
> ...
>
> > -       clk = devm_clk_get_optional(gpio_dev, NULL);
> > +       /* NOTE: do not use devm_ here! */
> > +       clk = clk_get_optional(gpio_dev, NULL);
> >         if (IS_ERR(clk)) {
> >                 platform_device_put(gpio_pdev);
> >                 return (void *)clk;
>
> > -       reset = devm_reset_control_get_optional_shared(gpio_dev, NULL);
> > +       /* NOTE: do not use devm_ here! */
> > +       reset = reset_control_get_optional_shared(gpio_dev, NULL);
> >         if (IS_ERR(reset)) {
> > -               dev_err(&pdev->dev, "failed getting reset control: %ld\n",
> > +               dev_err(dev, "failed getting reset control: %ld\n",
> >                         PTR_ERR(reset));
> >                 return ERR_CAST(reset);
> >         }
> > @@ -588,7 +594,7 @@ struct nmk_gpio_chip *nmk_gpio_populate_chip(struct fwnode_handle *fwnode,
> >          */
> >         ret = reset_control_deassert(reset);
> >         if (ret) {
> > -               dev_err(&pdev->dev, "failed reset deassert: %d\n", ret);
> > +               dev_err(dev, "failed reset deassert: %d\n", ret);
> >                 return ERR_PTR(ret);
> >         }
>
> Yeah, but you forgot to update the error path as it needs to unroll
> the changes, no?

About error cases: platform_device_put() calls are missing from the last
two error cases (reset-related).

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com