| Message ID | 8dd9dad2765d47fd6c6fec20566326d00e48a696.1574059625.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD71828 PMIC | expand |
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
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
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
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);
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(-)