Re: [PATCH v5 10/16] gpio: devres: Add devm_gpiod_get_parent_array

[Vaittinen, Matti]

Hello Linus,

On Tue, 2019-11-19 at 15:43 +0100, Linus Walleij wrote:
> On Mon, Nov 18, 2019 at 7:58 AM Matti Vaittinen
> <matti.vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> 
> > Bunch of MFD sub-devices which are instantiated by MFD do not have
> > own device-tree nodes but have (for example) the GPIO consumer
> > information in parent device's DT node. Add resource managed
> > devm_gpiod_get_array() for such devices so that they can get the
> > consumer information from parent DT while still binding the GPIO
> > reservation life-time to this sub-device life time.
> > 
> > If devm_gpiod_get_array is used as such - then unloading and then
> > re-loading the child device fails as the GPIOs reserved during
> > first
> > load are not freed when driver for sub-device is unload (if parent
> > stays there).
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> (...)
> > +static struct gpio_descs *__must_check
> > +__devm_gpiod_get_array(struct device *gpiodev,
> > +                      struct device *managed,
> > +                      const char *con_id,
> > +                      enum gpiod_flags flags)
> 
> I'm opposed to functions named __underscore_something()
> so find a proper name for this function.
> devm_gpiod_get_array_common() works if nothing else.

Ok. We all have our own pecularitie... I mean preferences :) But as
this is definitely your territory - I'll change the name :) No problem.

> 
> > @@ -292,19 +284,62 @@ struct gpio_descs *__must_check
> > devm_gpiod_get_array(struct device *dev,
> >         if (!dr)
> >                 return ERR_PTR(-ENOMEM);
> > 
> > -       descs = gpiod_get_array(dev, con_id, flags);
> > +       descs = gpiod_get_array(gpiodev, con_id, flags);
> >         if (IS_ERR(descs)) {
> >                 devres_free(dr);
> >                 return descs;
> >         }
> > 
> >         *dr = descs;
> > -       devres_add(dev, dr);
> > +       if (managed)
> > +               devres_add(managed, dr);
> > +       else
> > +               devres_add(gpiodev, dr);
> 
> So we only get managed resources if the "managed" device is
> passed in.

Hmm. Actually, no if I am not misreading this. We get managed devices
in any case, but the lifetime is either bound to exitence of the gpio
consumer device (which is standard way) - or existence of specific
'managed' device.

In case of MFD sub-device (like BD71828 regulator subdev) which has
GPIO consumer properties in MFD node - the GPIO consumer information is
in parent device's (BD71828 MFD device) data - but the lifetime should
be bound to sub-devices (BD71828 regulator device) lifetime. Thus we
need two different devices here.

> 
> > +/**
> > + * devm_gpiod_get_array - Resource-managed gpiod_get_array()
> 
> And this function is supposed to be resource managed for sure.

Yes. This is the standard case where device which has the consumer data
is also the one who 'manages' the GPIO.

> 
> > + * @dev:       GPIO consumer
> > + * @con_id:    function within the GPIO consumer
> > + * @flags:     optional GPIO initialization flags
> > + *
> > + * Managed gpiod_get_array(). GPIO descriptors returned from this
> > function are
> > + * automatically disposed on driver detach. See gpiod_get_array()
> > for detailed
> > + * information about behavior and return values.
> > + */
> > +struct gpio_descs *__must_check devm_gpiod_get_array(struct device
> > *dev,
> > +                                                    const char
> > *con_id,
> > +                                                    enum
> > gpiod_flags flags)
> > +{
> > +       return __devm_gpiod_get_array(dev, NULL, con_id, flags);
> 
> So what is this? NULL?

Here we don't have separate manager device - thus the manager is NULL
-and the consumer device ("dev" here) is what we use to manage GPIO.

> 
> Doesn't that mean you just removed all resource management for this
> call?

No :)

> 
> Or am I reading it wrong?

Either you are reading it wrong or I am writing it wrong xD. In any
case this means I need to drop few comments in code :) Thanks.

Br,
	Matti Vaittinen