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

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

Hello,

On Wed Mar 6, 2024 at 1:51 PM CET, Linus Walleij wrote:
> On Wed, Mar 6, 2024 at 12:20 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Wed, Mar 6, 2024 at 12:09 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.
> > >
> > > Drop in some notes and use a local *dev variable to clarify the
> > > code.
> >
> > LGTM, some minor remarks below.
> >
> > ...
> >
> > > Cc: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> >
> > Note, you can put Cc:s after --- line and it won't go to the commit
> > message while Cc to the respective people.
>
> Yeah old habit, actually b4 handles it fine by recording the
> recipients only in the cover letter.
>
> > > +               dev_dbg(dev, "populate: using default ngpio (%d)\n", ngpio);
> >
> > While at it %d --> %u.
> (...)
> > > +               dev_err(dev, "failed getting reset control: %ld\n",
> > >                         PTR_ERR(reset));
> >
> > Also possible %pe.
>
> Fixed them both when applying! Thanks!

Format specifier %pe takes a pointer, ie it should be reset and not
PTR_ERR(reset). See efaa90ed2cff ("gpio: nomadik: Back out some managed
resources") on linux-pinctrl/ib-nomadik-gpio.

Apart from that, tested efaa90ed2cff on Mobileye hardware.

GCC warning:

In file included from ./include/linux/device.h:15,
                 from ./include/linux/platform_device.h:13,
                 from drivers/gpio/gpio-nomadik.c:28:
drivers/gpio/gpio-nomadik.c: In function ‘nmk_gpio_populate_chip’:
drivers/gpio/gpio-nomadik.c:588:30: warning: format ‘%p’ expects argument of type ‘void *’, but argument 3 has type ‘long int’ [-Wformat=]
  588 |                 dev_err(dev, "failed getting reset control: %pe\n",
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/dev_printk.h:110:30: note: in definition of macro ‘dev_printk_index_wrap’
  110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
      |                              ^~~
./include/linux/dev_printk.h:144:56: note: in expansion of macro ‘dev_fmt’
  144 |         dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
      |                                                        ^~~~~~~
drivers/gpio/gpio-nomadik.c:588:17: note: in expansion of macro ‘dev_err’
  588 |                 dev_err(dev, "failed getting reset control: %pe\n",
      |                 ^~~~~~~
drivers/gpio/gpio-nomadik.c:588:62: note: format string is defined here
  588 |                 dev_err(dev, "failed getting reset control: %pe\n",
      |                                                             ~^
      |                                                              |
      |                                                              void *
      |                                                             %ld



Regards,

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