| Message ID | 2052507.JjE2hdgEYc@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Tue, Oct 7, 2014 at 9:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > Some drivers need to deal with only firmware representation of its > GPIOs. An example would be a GPIO button array driver where each button > is described as a separate firmware node in device tree. Typically these > child nodes do not have physical representation in the Linux device > model. > > In order to help device drivers to handle such firmware child nodes we > add dev[m]_get_named_gpiod_from_child() that takes a child firmware > node pointer as its second argument (the first one is the parent device > itself), finds the GPIO using whatever is the underlying firmware > method, and requests the GPIO properly. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> ... > +/* Child properties interface */ > +struct gpio_desc *dev_get_named_gpiod_from_child(struct device *dev, void *child, > + const char *propname, int index); > +struct gpio_desc *devm_get_named_gpiod_from_child(struct device *dev, void *child, > + const char *propname, int index); I see the reason for these functions and am not opposed to them. However, I wonder if we could not replace propname by a con_id that would be resolved to one of con_id-gpio for DT and whatever naming convention ACPI is using? This would prevent users to name GPIOs outside of the conventions defined in the bindings and be generally safer. Is there a particular reason (used by some old code?) for the current direct property access? If not, maybe we could call a slightly-modified of_find_gpio() to resolve the GPIO property for DT, and the equivalent function for ACPI? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 07, 2014 at 07:22:13PM +0900, Alexandre Courbot wrote: > On Tue, Oct 7, 2014 at 9:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > Some drivers need to deal with only firmware representation of its > > GPIOs. An example would be a GPIO button array driver where each button > > is described as a separate firmware node in device tree. Typically these > > child nodes do not have physical representation in the Linux device > > model. > > > > In order to help device drivers to handle such firmware child nodes we > > add dev[m]_get_named_gpiod_from_child() that takes a child firmware > > node pointer as its second argument (the first one is the parent device > > itself), finds the GPIO using whatever is the underlying firmware > > method, and requests the GPIO properly. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > ... > > > +/* Child properties interface */ > > +struct gpio_desc *dev_get_named_gpiod_from_child(struct device *dev, void *child, > > + const char *propname, int index); > > +struct gpio_desc *devm_get_named_gpiod_from_child(struct device *dev, void *child, > > + const char *propname, int index); > > I see the reason for these functions and am not opposed to them. > However, I wonder if we could not replace propname by a con_id that > would be resolved to one of con_id-gpio for DT and whatever naming > convention ACPI is using? The code in gpio-leds.c and gpio_keys_polled.c refers to "gpios" as the property name. If we can change that somehow to work with con_id-gpio instead without breaking things, then why not. > This would prevent users to name GPIOs outside of the conventions > defined in the bindings and be generally safer. Is there a particular > reason (used by some old code?) for the current direct property > access? If not, maybe we could call a slightly-modified of_find_gpio() > to resolve the GPIO property for DT, and the equivalent function for > ACPI? Only reason I can think of is support for the existing properties that are used directly. Drivers using gpiod_get() and friends do not need dev_get_named_gpiod_from_child() anyway. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 7, 2014 at 7:40 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, Oct 07, 2014 at 07:22:13PM +0900, Alexandre Courbot wrote: >> On Tue, Oct 7, 2014 at 9:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > From: Mika Westerberg <mika.westerberg@linux.intel.com> >> > >> > Some drivers need to deal with only firmware representation of its >> > GPIOs. An example would be a GPIO button array driver where each button >> > is described as a separate firmware node in device tree. Typically these >> > child nodes do not have physical representation in the Linux device >> > model. >> > >> > In order to help device drivers to handle such firmware child nodes we >> > add dev[m]_get_named_gpiod_from_child() that takes a child firmware >> > node pointer as its second argument (the first one is the parent device >> > itself), finds the GPIO using whatever is the underlying firmware >> > method, and requests the GPIO properly. >> > >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> ... >> >> > +/* Child properties interface */ >> > +struct gpio_desc *dev_get_named_gpiod_from_child(struct device *dev, void *child, >> > + const char *propname, int index); >> > +struct gpio_desc *devm_get_named_gpiod_from_child(struct device *dev, void *child, >> > + const char *propname, int index); >> >> I see the reason for these functions and am not opposed to them. >> However, I wonder if we could not replace propname by a con_id that >> would be resolved to one of con_id-gpio for DT and whatever naming >> convention ACPI is using? > > The code in gpio-leds.c and gpio_keys_polled.c refers to "gpios" as the > property name. If we can change that somehow to work with con_id-gpio > instead without breaking things, then why not. > >> This would prevent users to name GPIOs outside of the conventions >> defined in the bindings and be generally safer. Is there a particular >> reason (used by some old code?) for the current direct property >> access? If not, maybe we could call a slightly-modified of_find_gpio() >> to resolve the GPIO property for DT, and the equivalent function for >> ACPI? > > Only reason I can think of is support for the existing properties that > are used directly. Drivers using gpiod_get() and friends do not need > dev_get_named_gpiod_from_child() anyway. Right. Another thing is that the property handling code (active low only for now) is duplicated again, but that can be addressed separately. I will have a look at gpio-leds and gpio_keys_polled to see if we cannot make this work at a higher level. It's easier to have the bindings respected if the code itself enforces them. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, October 07, 2014 07:52:02 PM Alexandre Courbot wrote: > On Tue, Oct 7, 2014 at 7:40 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Tue, Oct 07, 2014 at 07:22:13PM +0900, Alexandre Courbot wrote: > >> On Tue, Oct 7, 2014 at 9:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> > From: Mika Westerberg <mika.westerberg@linux.intel.com> > >> > > >> > Some drivers need to deal with only firmware representation of its > >> > GPIOs. An example would be a GPIO button array driver where each button > >> > is described as a separate firmware node in device tree. Typically these > >> > child nodes do not have physical representation in the Linux device > >> > model. > >> > > >> > In order to help device drivers to handle such firmware child nodes we > >> > add dev[m]_get_named_gpiod_from_child() that takes a child firmware > >> > node pointer as its second argument (the first one is the parent device > >> > itself), finds the GPIO using whatever is the underlying firmware > >> > method, and requests the GPIO properly. > >> > > >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > >> ... > >> > >> > +/* Child properties interface */ > >> > +struct gpio_desc *dev_get_named_gpiod_from_child(struct device *dev, void *child, > >> > + const char *propname, int index); > >> > +struct gpio_desc *devm_get_named_gpiod_from_child(struct device *dev, void *child, > >> > + const char *propname, int index); > >> > >> I see the reason for these functions and am not opposed to them. > >> However, I wonder if we could not replace propname by a con_id that > >> would be resolved to one of con_id-gpio for DT and whatever naming > >> convention ACPI is using? > > > > The code in gpio-leds.c and gpio_keys_polled.c refers to "gpios" as the > > property name. If we can change that somehow to work with con_id-gpio > > instead without breaking things, then why not. > > > >> This would prevent users to name GPIOs outside of the conventions > >> defined in the bindings and be generally safer. Is there a particular > >> reason (used by some old code?) for the current direct property > >> access? If not, maybe we could call a slightly-modified of_find_gpio() > >> to resolve the GPIO property for DT, and the equivalent function for > >> ACPI? > > > > Only reason I can think of is support for the existing properties that > > are used directly. Drivers using gpiod_get() and friends do not need > > dev_get_named_gpiod_from_child() anyway. > > Right. Another thing is that the property handling code (active low > only for now) is duplicated again, but that can be addressed > separately. > > I will have a look at gpio-leds and gpio_keys_polled to see if we > cannot make this work at a higher level. It's easier to have the > bindings respected if the code itself enforces them. I'm wondering if that can be done after merging the current work? We'll be able to use the drivers in question with our hardware in the meantime then ...
On Wed, Oct 8, 2014 at 9:09 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, October 07, 2014 07:52:02 PM Alexandre Courbot wrote: >> On Tue, Oct 7, 2014 at 7:40 PM, Mika Westerberg >> <mika.westerberg@linux.intel.com> wrote: >> > On Tue, Oct 07, 2014 at 07:22:13PM +0900, Alexandre Courbot wrote: >> >> On Tue, Oct 7, 2014 at 9:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> >> > From: Mika Westerberg <mika.westerberg@linux.intel.com> >> >> > >> >> > Some drivers need to deal with only firmware representation of its >> >> > GPIOs. An example would be a GPIO button array driver where each button >> >> > is described as a separate firmware node in device tree. Typically these >> >> > child nodes do not have physical representation in the Linux device >> >> > model. >> >> > >> >> > In order to help device drivers to handle such firmware child nodes we >> >> > add dev[m]_get_named_gpiod_from_child() that takes a child firmware >> >> > node pointer as its second argument (the first one is the parent device >> >> > itself), finds the GPIO using whatever is the underlying firmware >> >> > method, and requests the GPIO properly. >> >> > >> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> >> >> ... >> >> >> >> > +/* Child properties interface */ >> >> > +struct gpio_desc *dev_get_named_gpiod_from_child(struct device *dev, void *child, >> >> > + const char *propname, int index); >> >> > +struct gpio_desc *devm_get_named_gpiod_from_child(struct device *dev, void *child, >> >> > + const char *propname, int index); >> >> >> >> I see the reason for these functions and am not opposed to them. >> >> However, I wonder if we could not replace propname by a con_id that >> >> would be resolved to one of con_id-gpio for DT and whatever naming >> >> convention ACPI is using? >> > >> > The code in gpio-leds.c and gpio_keys_polled.c refers to "gpios" as the >> > property name. If we can change that somehow to work with con_id-gpio >> > instead without breaking things, then why not. >> > >> >> This would prevent users to name GPIOs outside of the conventions >> >> defined in the bindings and be generally safer. Is there a particular >> >> reason (used by some old code?) for the current direct property >> >> access? If not, maybe we could call a slightly-modified of_find_gpio() >> >> to resolve the GPIO property for DT, and the equivalent function for >> >> ACPI? >> > >> > Only reason I can think of is support for the existing properties that >> > are used directly. Drivers using gpiod_get() and friends do not need >> > dev_get_named_gpiod_from_child() anyway. >> >> Right. Another thing is that the property handling code (active low >> only for now) is duplicated again, but that can be addressed >> separately. >> >> I will have a look at gpio-leds and gpio_keys_polled to see if we >> cannot make this work at a higher level. It's easier to have the >> bindings respected if the code itself enforces them. > > I'm wondering if that can be done after merging the current work? > > We'll be able to use the drivers in question with our hardware in the > meantime then ... Yeah, that's probably ok. The properties in question already exist so we will have to support them anyway ; it's just a matter of seeing whether we can improve the proposed way. So for now: Acked-by: Alexandre Courbot <acourbot@nvidia.com> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, October 08, 2014 11:55:08 AM Alexandre Courbot wrote: > On Wed, Oct 8, 2014 at 9:09 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Tuesday, October 07, 2014 07:52:02 PM Alexandre Courbot wrote: > >> On Tue, Oct 7, 2014 at 7:40 PM, Mika Westerberg > >> <mika.westerberg@linux.intel.com> wrote: > >> > On Tue, Oct 07, 2014 at 07:22:13PM +0900, Alexandre Courbot wrote: > >> >> On Tue, Oct 7, 2014 at 9:18 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > >> >> > From: Mika Westerberg <mika.westerberg@linux.intel.com> > >> >> > > >> >> > Some drivers need to deal with only firmware representation of its > >> >> > GPIOs. An example would be a GPIO button array driver where each button > >> >> > is described as a separate firmware node in device tree. Typically these > >> >> > child nodes do not have physical representation in the Linux device > >> >> > model. > >> >> > > >> >> > In order to help device drivers to handle such firmware child nodes we > >> >> > add dev[m]_get_named_gpiod_from_child() that takes a child firmware > >> >> > node pointer as its second argument (the first one is the parent device > >> >> > itself), finds the GPIO using whatever is the underlying firmware > >> >> > method, and requests the GPIO properly. > >> >> > > >> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > >> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> >> > >> >> ... > >> >> > >> >> > +/* Child properties interface */ > >> >> > +struct gpio_desc *dev_get_named_gpiod_from_child(struct device *dev, void *child, > >> >> > + const char *propname, int index); > >> >> > +struct gpio_desc *devm_get_named_gpiod_from_child(struct device *dev, void *child, > >> >> > + const char *propname, int index); > >> >> > >> >> I see the reason for these functions and am not opposed to them. > >> >> However, I wonder if we could not replace propname by a con_id that > >> >> would be resolved to one of con_id-gpio for DT and whatever naming > >> >> convention ACPI is using? > >> > > >> > The code in gpio-leds.c and gpio_keys_polled.c refers to "gpios" as the > >> > property name. If we can change that somehow to work with con_id-gpio > >> > instead without breaking things, then why not. > >> > > >> >> This would prevent users to name GPIOs outside of the conventions > >> >> defined in the bindings and be generally safer. Is there a particular > >> >> reason (used by some old code?) for the current direct property > >> >> access? If not, maybe we could call a slightly-modified of_find_gpio() > >> >> to resolve the GPIO property for DT, and the equivalent function for > >> >> ACPI? > >> > > >> > Only reason I can think of is support for the existing properties that > >> > are used directly. Drivers using gpiod_get() and friends do not need > >> > dev_get_named_gpiod_from_child() anyway. > >> > >> Right. Another thing is that the property handling code (active low > >> only for now) is duplicated again, but that can be addressed > >> separately. > >> > >> I will have a look at gpio-leds and gpio_keys_polled to see if we > >> cannot make this work at a higher level. It's easier to have the > >> bindings respected if the code itself enforces them. > > > > I'm wondering if that can be done after merging the current work? > > > > We'll be able to use the drivers in question with our hardware in the > > meantime then ... > > Yeah, that's probably ok. The properties in question already exist so > we will have to support them anyway ; it's just a matter of seeing > whether we can improve the proposed way. > > So for now: > > Acked-by: Alexandre Courbot <acourbot@nvidia.com> Thanks!
diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c index 954b9f6..a1f7e55 100644 --- a/drivers/gpio/devres.c +++ b/drivers/gpio/devres.c @@ -109,6 +109,40 @@ struct gpio_desc *__must_check __devm_gpiod_get_index(struct device *dev, EXPORT_SYMBOL(__devm_gpiod_get_index); /** + * devm_get_named_gpiod_from_child - managed dev_get_named_gpiod_from_child() + * @dev: GPIO consumer + * @child: firmware node (child of @dev) + * @propname: name of the firmware property + * @index: index of the GPIO in the property value in case of many + * + * GPIO descriptors returned from this function are automatically disposed on + * driver detach. + */ +struct gpio_desc *devm_get_named_gpiod_from_child(struct device *dev, void *child, + const char *propname, int index) +{ + struct gpio_desc **dr; + struct gpio_desc *desc; + + dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), + GFP_KERNEL); + if (!dr) + return ERR_PTR(-ENOMEM); + + desc = dev_get_named_gpiod_from_child(dev, child, propname, index); + if (IS_ERR(desc)) { + devres_free(dr); + return desc; + } + + *dr = desc; + devres_add(dev, dr); + + return desc; +} +EXPORT_SYMBOL(devm_get_named_gpiod_from_child); + +/** * devm_gpiod_get_index_optional - Resource-managed gpiod_get_index_optional() * @dev: GPIO consumer * @con_id: function within the GPIO consumer diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 4c86601..344bc12 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1717,6 +1717,62 @@ struct gpio_desc *__must_check __gpiod_get_index(struct device *dev, EXPORT_SYMBOL_GPL(__gpiod_get_index); /** + * dev_get_named_gpiod_from_child - obtain a GPIO from firmware node + * @dev: parent device + * @child: firmware node (child of @dev) + * @propname: name of the firmware property + * @idx: index of the GPIO in the property value in case of many + * + * This function can be used for drivers that get their configuration + * from firmware in such a way that some properties are described as child + * nodes for the parent device in DT or ACPI. + * + * Function properly finds the corresponding GPIO using whatever is the + * underlying firmware interface and then makes sure that the GPIO + * descriptor is requested before it is returned to the caller. + * + * In case of error an ERR_PTR() is returned. + */ +struct gpio_desc *dev_get_named_gpiod_from_child(struct device *dev, void *child, + const char *propname, int index) +{ + struct gpio_desc *desc = ERR_PTR(-ENODEV); + bool active_low = false; + int ret; + + if (!child) + return ERR_PTR(-EINVAL); + + if (IS_ENABLED(CONFIG_OF) && dev->of_node) { + enum of_gpio_flags flags; + + desc = of_get_named_gpiod_flags(child, propname, index, &flags); + if (!IS_ERR(desc)) + active_low = flags & OF_GPIO_ACTIVE_LOW; + } else if (ACPI_COMPANION(dev)) { + struct acpi_gpio_info info; + + desc = acpi_get_gpiod_by_index(child, propname, index, &info); + if (!IS_ERR(desc)) + active_low = info.active_low; + } + + if (IS_ERR(desc)) + return desc; + + ret = gpiod_request(desc, NULL); + if (ret) + return ERR_PTR(ret); + + /* Only value flag can be set from both DT and ACPI is active_low */ + if (active_low) + set_bit(FLAG_ACTIVE_LOW, &desc->flags); + + return desc; +} +EXPORT_SYMBOL_GPL(dev_get_named_gpiod_from_child); + +/** * gpiod_get_index_optional - obtain an optional GPIO from a multi-index GPIO * function * @dev: GPIO consumer, can be NULL for system-global GPIOs diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 12f146f..033d2fd 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -94,6 +94,11 @@ int gpiod_to_irq(const struct gpio_desc *desc); struct gpio_desc *gpio_to_desc(unsigned gpio); int desc_to_gpio(const struct gpio_desc *desc); +/* Child properties interface */ +struct gpio_desc *dev_get_named_gpiod_from_child(struct device *dev, void *child, + const char *propname, int index); +struct gpio_desc *devm_get_named_gpiod_from_child(struct device *dev, void *child, + const char *propname, int index); #else /* CONFIG_GPIOLIB */ static inline struct gpio_desc *__must_check __gpiod_get(struct device *dev,