[v5,10/16] gpio: devres: Add devm_gpiod_get_parent_array
diff mbox series

Message ID 8dd9dad2765d47fd6c6fec20566326d00e48a696.1574059625.git.matti.vaittinen@fi.rohmeurope.com
State Under Review
Headers show
Series
  • Support ROHM BD71828 PMIC
Related show

Commit Message

Vaittinen, Matti Nov. 18, 2019, 6:58 a.m. UTC
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@fi.rohmeurope.com>
---

No changes from v4

 .../driver-api/driver-model/devres.rst        |  1 +
 drivers/gpio/gpiolib-devres.c                 | 65 ++++++++++++++-----
 include/linux/gpio/consumer.h                 |  5 ++
 3 files changed, 56 insertions(+), 15 deletions(-)

Comments

Linus Walleij Nov. 19, 2019, 2:43 p.m. UTC | #1
On Mon, Nov 18, 2019 at 7:58 AM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> 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@fi.rohmeurope.com>
(...)
> +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.

> @@ -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.

> +/**
> + * devm_gpiod_get_array - Resource-managed gpiod_get_array()

And this function is supposed to be resource managed for sure.

> + * @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?

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

Or am I reading it wrong?

Yours,
Linus Walleij
Vaittinen, Matti Nov. 19, 2019, 5:54 p.m. UTC | #2
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@fi.rohmeurope.com> 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@fi.rohmeurope.com>
> (...)
> > +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
Linus Walleij Nov. 21, 2019, 2:13 p.m. UTC | #3
On Tue, Nov 19, 2019 at 6:54 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
> [Me]
> > 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.

I was reading it wrong, so not your bad. I guess lack of focus on
my side, this part is fine!

Yours,
Linus Walleij

Patch
diff mbox series

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index a100bef54952..1b8302ba405b 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -270,6 +270,7 @@  GPIO
   devm_gpiod_get_index()
   devm_gpiod_get_index_optional()
   devm_gpiod_get_optional()
+  devm_gpiod_get_parent_array()
   devm_gpiod_put()
   devm_gpiod_unhinge()
   devm_gpiochip_add_data()
diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 98e3c20d9730..584fa6edfd20 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -270,19 +270,11 @@  struct gpio_desc *__must_check devm_gpiod_get_index_optional(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_gpiod_get_index_optional);
 
-/**
- * devm_gpiod_get_array - Resource-managed gpiod_get_array()
- * @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)
+static struct gpio_descs *__must_check
+__devm_gpiod_get_array(struct device *gpiodev,
+		       struct device *managed,
+		       const char *con_id,
+		       enum gpiod_flags flags)
 {
 	struct gpio_descs **dr;
 	struct gpio_descs *descs;
@@ -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);
 
 	return descs;
 }
+
+/**
+ * devm_gpiod_get_array - Resource-managed gpiod_get_array()
+ * @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);
+}
 EXPORT_SYMBOL_GPL(devm_gpiod_get_array);
 
+/**
+ * devm_gpiod_get_parent_array - Resource-managed gpiod_get_array for subdevices
+ * @dev:	Managed device whose parent is the GPIO consumer
+ * @con_id:	function within the GPIO consumer
+ * @flags:	optional GPIO initialization flags
+ *
+ * Managed gpiod_get_array() for subdevices. This function is intended to be
+ * used by MFD sub-devices whose GPIO bindings are in parent (MFD) device but
+ * whose GPIO reservation should last only for the dub-device life time.
+ * Returns EINVAL if no parent device is found. Rest of the behaviour and
+ * return values are as documented for gpiod_get_array()
+ */
+struct gpio_descs *__must_check
+devm_gpiod_get_parent_array(struct device *dev,
+			    const char *con_id,
+			    enum gpiod_flags flags)
+{
+	if (!dev | !dev->parent)
+		return ERR_PTR(-EINVAL);
+	return __devm_gpiod_get_array(dev->parent, dev, con_id, flags);
+}
+EXPORT_SYMBOL_GPL(devm_gpiod_get_parent_array);
+
 /**
  * devm_gpiod_get_array_optional - Resource-managed gpiod_get_array_optional()
  * @dev:	GPIO consumer
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index b70af921c614..01a82b1c6828 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -103,6 +103,11 @@  struct gpio_descs *__must_check devm_gpiod_get_array(struct device *dev,
 struct gpio_descs *__must_check
 devm_gpiod_get_array_optional(struct device *dev, const char *con_id,
 			      enum gpiod_flags flags);
+struct gpio_descs *__must_check
+devm_gpiod_get_parent_array(struct device *dev,
+			    const char *con_id,
+			    enum gpiod_flags flags);
+
 void devm_gpiod_put(struct device *dev, struct gpio_desc *desc);
 void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc);
 void devm_gpiod_put_array(struct device *dev, struct gpio_descs *descs);