Re: [PATCH v3 2/5] reset: Instantiate reset GPIO controller for shared reset-gpios

[Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>]

On Mo, 2024-01-15 at 17:13 +0100, Krzysztof Kozlowski wrote:
> On 15/01/2024 17:06, Bartosz Golaszewski wrote:
> > > +
> > > +static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
> > > +                                        unsigned int gpio,
> > > +                                        unsigned int of_flags)
> > > +{
> > > +       struct gpiod_lookup_table *lookup __free(kfree) = NULL;
> > > +       struct gpio_device *gdev __free(gpio_device_put) = NULL;
> > > +       char *label __free(kfree) = NULL;
> > 
> > I got yelled at by Linus Torvalds personally for doing it like this. I
> > know this is a common pattern in code using GLib but Linus wants auto
> > variables to be initialized where they're declared...
> 
> Declaration is here. Initialization is here. Therefore this is
> initialized where it is declared. What's more it is initialized to a
> valid value, because __free() accepts NULLs.
[...]
> > ... so this should become:
> > 
> >   struct gpio_device *gdev __free(gpio_device_put) = gpio_device_find(...)
> > 
> > and same for the rest.
> > 
> > Don't get me wrong, I love cleanup.h but there's a (unofficial for
> > now) coding style.
> 
> So you just want to declare it not in top-part of the function but just
> before first use?

IIUC, Linus wants exactly this:

https://lore.kernel.org/all/CAHk-=wgRHiV5VSxtfXA4S6aLUmcQYEuB67u3BJPJPtuESs1JyA@xxxxxxxxxxxxxx/

[...]
> > 
> > > +               goto out_unlock;
> > >         }
> > > 
> > >         rstc_id = rcdev->of_xlate(rcdev, &args);
> > >         if (rstc_id < 0) {
> > >                 rstc = ERR_PTR(rstc_id);
> > > -               goto out;
> > > +               goto out_unlock;
> > >         }
> > > 
> > >         /* reset_list_mutex also protects the rcdev's reset_control list */
> > >         rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
> > > 
> > > -out:
> > > +out_unlock:
> > >         mutex_unlock(&reset_list_mutex);
> > > +out_put:
> > >         of_node_put(args.np);
> > 
> > I suggest reworking this to use cleanup.h as well.
> 
> It's independent task. This is an existing code and any refactoring to
> cleanup or not is independent thing.

Seconded. Separate cleanup very welcome, but this series is about
adding functionality.

regards
Philipp