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
