| Message ID | 20221031-gpiolib-swnode-v1-5-a0ab48d229c7@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add support for software nodes to gpiolib | expand |
On Thu, Nov 03, 2022 at 11:10:15PM -0700, Dmitry Torokhov wrote: > Ensure that all paths to obtain/look up GPIOD from generic > consumer-visible APIs go through the new gpiod_find_and_request() > helper, so that we can easily extend it with support for new firmware > mechanisms. ... > +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode, > + struct device *consumer, > + const char *con_id, > + unsigned int idx, > + enum gpiod_flags *flags, > + unsigned long *lookupflags) > { > + struct gpio_desc *desc = ERR_PTR(-ENOENT); No need, just return directly. > + dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n", > + con_id, fwnode_get_name(fwnode)); %pfwP ? > + > + /* Using device tree? */ > if (is_of_node(fwnode)) { > + dev_dbg(consumer, "using device tree for GPIO lookup\n"); > + desc = of_find_gpio(to_of_node(fwnode), > + con_id, idx, lookupflags); > } else if (is_acpi_node(fwnode)) { With direct return, no need for 'else' here. > + dev_dbg(consumer, "using ACPI for GPIO lookup\n"); > + desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags); > } > > + return desc; > +} ... > +static struct gpio_desc *gpiod_find_and_request(struct device *consumer, > + struct fwnode_handle *fwnode, > + const char *con_id, > + unsigned int idx, > + enum gpiod_flags flags, > + const char *label, > + bool platform_lookup_allowed) > +{ > + struct gpio_desc *desc = ERR_PTR(-ENOENT); We can get rid of the assignment, see below. > + unsigned long lookupflags; > + int ret; > + if (fwnode) Do we need this check? Debug message above (when %pfw is used) would be even useful when fwnode == NULL. > + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, > + &flags, &lookupflags); > + The blank line can be removed after above comments being addressed. > + if (gpiod_not_found(desc) && platform_lookup_allowed) { > + /* > + * Either we are not using DT or ACPI, or their lookup did not > + * return a result. In that case, use platform lookup as a > + * fallback. > + */ > + dev_dbg(consumer, "using lookup tables for GPIO lookup\n"); > + desc = gpiod_find(consumer, con_id, idx, &lookupflags); > + } > + > + if (IS_ERR(desc)) { > + dev_dbg(consumer, "No GPIO consumer %s found\n", con_id); > + return desc; > + } > + > + /* > + * If a connection label was passed use that, else attempt to use > + * the device name as label > + */ > ret = gpiod_request(desc, label); > + if (ret) { > + if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE)) > + return ERR_PTR(ret); > + > + /* > + * This happens when there are several consumers for > + * the same GPIO line: we just return here without > + * further initialization. It is a bit of a hack. > + * This is necessary to support fixed regulators. > + * > + * FIXME: Make this more sane and safe. > + */ > + dev_info(consumer, > + "nonexclusive access to GPIO for %s\n", con_id); Cam be one line. > + return desc; > + } > > + ret = gpiod_configure_flags(desc, con_id, lookupflags, flags); > if (ret < 0) { > + dev_dbg(consumer, "setup of GPIO %s failed\n", con_id); > gpiod_put(desc); > return ERR_PTR(ret); > } ... > struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode, > + const char *con_id, > + int index, > enum gpiod_flags flags, > const char *label) > { > Unnecessary blank line? > + return gpiod_find_and_request(NULL, fwnode, con_id, index, flags, label, > + false); Can be one line. > } ... > + return gpiod_find_and_request(dev, fwnode, con_id, idx, flags, label, > + true); Ditto.
Hi Andy, On Fri, Nov 04, 2022 at 07:17:27PM +0200, Andy Shevchenko wrote: > On Thu, Nov 03, 2022 at 11:10:15PM -0700, Dmitry Torokhov wrote: > > Ensure that all paths to obtain/look up GPIOD from generic > > consumer-visible APIs go through the new gpiod_find_and_request() > > helper, so that we can easily extend it with support for new firmware > > mechanisms. > > ... > > > +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode, > > + struct device *consumer, > > + const char *con_id, > > + unsigned int idx, > > + enum gpiod_flags *flags, > > + unsigned long *lookupflags) > > { > > > + struct gpio_desc *desc = ERR_PTR(-ENOENT); > > No need, just return directly. > > > + dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n", > > + con_id, fwnode_get_name(fwnode)); > > %pfwP ? OK. Although, I think I like %pfw (without 'P') better as it gives results like: /soc/i2c@11007000/edp-bridge@8 or \_SB.PCI0.I2C1.D010 which should help identifying the exact node. > > > + > > + /* Using device tree? */ > > if (is_of_node(fwnode)) { > > + dev_dbg(consumer, "using device tree for GPIO lookup\n"); > > + desc = of_find_gpio(to_of_node(fwnode), > > + con_id, idx, lookupflags); > > } else if (is_acpi_node(fwnode)) { > > With direct return, no need for 'else' here. When we have several branches of equal weight I prefer not to have early/inline returns, but I can add: } else { desc = ERR_PTR(-ENOENT); } at the end, what do you think? > > > + dev_dbg(consumer, "using ACPI for GPIO lookup\n"); > > + desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags); > > } > > > > + return desc; > > +} > > ... > > > +static struct gpio_desc *gpiod_find_and_request(struct device *consumer, > > + struct fwnode_handle *fwnode, > > + const char *con_id, > > + unsigned int idx, > > + enum gpiod_flags flags, > > + const char *label, > > + bool platform_lookup_allowed) > > +{ > > > + struct gpio_desc *desc = ERR_PTR(-ENOENT); > > We can get rid of the assignment, see below. > > > > + unsigned long lookupflags; > > + int ret; > > > + if (fwnode) > > Do we need this check? Yes, I would prefer to have it as it clearly informs the reader that we are only doing lookup by node if we actually have a node. gpiod_find_and_request() expects that it gets a valid node and in the followup change it will be dereferencing fwnode without checking for NULL-ness. > > Debug message above (when %pfw is used) would be even useful when > fwnode == NULL. > > > + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, > > + &flags, &lookupflags); > > > + > > The blank line can be removed after above comments being addressed. > > > + if (gpiod_not_found(desc) && platform_lookup_allowed) { > > + /* > > + * Either we are not using DT or ACPI, or their lookup did not > > + * return a result. In that case, use platform lookup as a > > + * fallback. > > + */ > > + dev_dbg(consumer, "using lookup tables for GPIO lookup\n"); > > + desc = gpiod_find(consumer, con_id, idx, &lookupflags); > > + } > > + > > + if (IS_ERR(desc)) { > > + dev_dbg(consumer, "No GPIO consumer %s found\n", con_id); > > + return desc; > > + } > > + > > + /* > > + * If a connection label was passed use that, else attempt to use > > + * the device name as label > > + */ > > ret = gpiod_request(desc, label); > > + if (ret) { > > + if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE)) > > + return ERR_PTR(ret); > > + > > + /* > > + * This happens when there are several consumers for > > + * the same GPIO line: we just return here without > > + * further initialization. It is a bit of a hack. > > + * This is necessary to support fixed regulators. > > + * > > + * FIXME: Make this more sane and safe. > > + */ > > > + dev_info(consumer, > > + "nonexclusive access to GPIO for %s\n", con_id); > > Cam be one line. I still have not embraced the new 100 columns limit. Linus, Bart, are you OK with moving to 100 or do you want to stay with 80 for a while? > > > + return desc; > > + } > > > > + ret = gpiod_configure_flags(desc, con_id, lookupflags, flags); > > if (ret < 0) { > > + dev_dbg(consumer, "setup of GPIO %s failed\n", con_id); > > gpiod_put(desc); > > return ERR_PTR(ret); > > } > > ... > > > struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode, > > + const char *con_id, > > + int index, > > enum gpiod_flags flags, > > const char *label) > > { > > > > Unnecessary blank line? Indeed, I'll fix it. > > > + return gpiod_find_and_request(NULL, fwnode, con_id, index, flags, label, > > + false); > > Can be one line. Yep, depending on 80/100 column answer. Thanks for the review!
On Fri, Nov 04, 2022 at 11:52:26AM -0700, Dmitry Torokhov wrote: > On Fri, Nov 04, 2022 at 07:17:27PM +0200, Andy Shevchenko wrote: > > On Thu, Nov 03, 2022 at 11:10:15PM -0700, Dmitry Torokhov wrote: ... > > > +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode, > > > + struct device *consumer, > > > + const char *con_id, > > > + unsigned int idx, > > > + enum gpiod_flags *flags, > > > + unsigned long *lookupflags) > > > { > > > > > + struct gpio_desc *desc = ERR_PTR(-ENOENT); > > > > No need, just return directly. > > > > > + dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n", > > > + con_id, fwnode_get_name(fwnode)); > > > > %pfwP ? > > OK. Although, I think I like %pfw (without 'P') better as it gives > results like: > > /soc/i2c@11007000/edp-bridge@8 > > or > > \_SB.PCI0.I2C1.D010 > > which should help identifying the exact node. I agree. > > > + /* Using device tree? */ > > > if (is_of_node(fwnode)) { > > > + dev_dbg(consumer, "using device tree for GPIO lookup\n"); > > > + desc = of_find_gpio(to_of_node(fwnode), > > > + con_id, idx, lookupflags); > > > } else if (is_acpi_node(fwnode)) { > > > > With direct return, no need for 'else' here. > > When we have several branches of equal weight I prefer not to have > early/inline returns, but I can add: > > } else { > desc = ERR_PTR(-ENOENT); > } > > at the end, what do you think? No strong opinion here. > > > + dev_dbg(consumer, "using ACPI for GPIO lookup\n"); > > > + desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags); > > > } > > > > > > + return desc; > > > +} ... > > > + struct gpio_desc *desc = ERR_PTR(-ENOENT); > > > > We can get rid of the assignment, see below. Still below another thought which affects this. > > > + if (fwnode) > > > > Do we need this check? > > Yes, I would prefer to have it as it clearly informs the reader that we > are only doing lookup by node if we actually have a node. > > gpiod_find_and_request() expects that it gets a valid node and in the > followup change it will be dereferencing fwnode without checking for > NULL-ness. But most of the code will check for the NULL anyway. The exceptions are dev_dbg() and accessing to the secondary fwnode. > > Debug message above (when %pfw is used) would be even useful when > > fwnode == NULL. > > > + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, > > > + &flags, &lookupflags); Looking into drivers/base/property.c makes me realize that you might need to test for error pointer as well. Perhaps something like if (IS_ERR_OR_NULL(fwnode)) return ERR_PTR(-ENOENT); in the gpiod_find_by_fwnode() needs to be added. Can you check this?
On Fri, Nov 04, 2022 at 11:06:58PM +0200, Andy Shevchenko wrote: > On Fri, Nov 04, 2022 at 11:52:26AM -0700, Dmitry Torokhov wrote: > > On Fri, Nov 04, 2022 at 07:17:27PM +0200, Andy Shevchenko wrote: > > > On Thu, Nov 03, 2022 at 11:10:15PM -0700, Dmitry Torokhov wrote: > > ... > > > > > +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode, > > > > + struct device *consumer, > > > > + const char *con_id, > > > > + unsigned int idx, > > > > + enum gpiod_flags *flags, > > > > + unsigned long *lookupflags) > > > > { > > > > > > > + struct gpio_desc *desc = ERR_PTR(-ENOENT); > > > > > > No need, just return directly. > > > > > > > + dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n", > > > > + con_id, fwnode_get_name(fwnode)); > > > > > > %pfwP ? > > > > OK. Although, I think I like %pfw (without 'P') better as it gives > > results like: > > > > /soc/i2c@11007000/edp-bridge@8 > > > > or > > > > \_SB.PCI0.I2C1.D010 > > > > which should help identifying the exact node. > > I agree. > > > > > + /* Using device tree? */ > > > > if (is_of_node(fwnode)) { > > > > + dev_dbg(consumer, "using device tree for GPIO lookup\n"); > > > > + desc = of_find_gpio(to_of_node(fwnode), > > > > + con_id, idx, lookupflags); > > > > } else if (is_acpi_node(fwnode)) { > > > > > > With direct return, no need for 'else' here. > > > > When we have several branches of equal weight I prefer not to have > > early/inline returns, but I can add: > > > > } else { > > desc = ERR_PTR(-ENOENT); > > } > > > > at the end, what do you think? > > No strong opinion here. > > > > > + dev_dbg(consumer, "using ACPI for GPIO lookup\n"); > > > > + desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags); > > > > } > > > > > > > > + return desc; > > > > +} > > ... > > > > > + struct gpio_desc *desc = ERR_PTR(-ENOENT); > > > > > > We can get rid of the assignment, see below. > > Still below another thought which affects this. > > > > > + if (fwnode) > > > > > > Do we need this check? > > > > Yes, I would prefer to have it as it clearly informs the reader that we > > are only doing lookup by node if we actually have a node. > > > > gpiod_find_and_request() expects that it gets a valid node and in the > > followup change it will be dereferencing fwnode without checking for > > NULL-ness. > > But most of the code will check for the NULL anyway. The exceptions are > dev_dbg() and accessing to the secondary fwnode. I think it is just a matter of what I want to express through source. I want to show that the device might not have fwnode, and that we only descend into gpiod_find_by_fwnode() in cases where we actually have fwnode. > > > > Debug message above (when %pfw is used) would be even useful when > > > fwnode == NULL. > > > > > + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, > > > > + &flags, &lookupflags); > > Looking into drivers/base/property.c makes me realize that you might need to > test for error pointer as well. > > Perhaps something like > > if (IS_ERR_OR_NULL(fwnode)) > return ERR_PTR(-ENOENT); > > in the gpiod_find_by_fwnode() needs to be added. Can you check this? No, only fwnode->secondary pointer can be PTR_ERR()-encoded. From comment to set_primary_fwnode() in drivers/base/core.c * Valid fwnode cases are: * - primary --> secondary --> -ENODEV * - primary --> NULL * - secondary --> -ENODEV * - NULL I do not believe we should be concerned about someone passing secondary pointers from fwnodes directly into gpiolib. Thanks.
On Fri, Nov 04, 2022 at 09:56:57PM -0700, Dmitry Torokhov wrote: > On Fri, Nov 04, 2022 at 11:06:58PM +0200, Andy Shevchenko wrote: > > On Fri, Nov 04, 2022 at 11:52:26AM -0700, Dmitry Torokhov wrote: > > > On Fri, Nov 04, 2022 at 07:17:27PM +0200, Andy Shevchenko wrote: > > > > On Thu, Nov 03, 2022 at 11:10:15PM -0700, Dmitry Torokhov wrote: ... > > > > > + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, > > > > > + &flags, &lookupflags); > > > > Looking into drivers/base/property.c makes me realize that you might need to > > test for error pointer as well. > > > > Perhaps something like > > > > if (IS_ERR_OR_NULL(fwnode)) > > return ERR_PTR(-ENOENT); > > > > in the gpiod_find_by_fwnode() needs to be added. Can you check this? > > No, only fwnode->secondary pointer can be PTR_ERR()-encoded. > > From comment to set_primary_fwnode() in drivers/base/core.c > > * Valid fwnode cases are: > * - primary --> secondary --> -ENODEV > * - primary --> NULL > * - secondary --> -ENODEV > * - NULL > > I do not believe we should be concerned about someone passing secondary > pointers from fwnodes directly into gpiolib. It's not only about secondary vs. primary notation, some of fwnode API calls may return error pointer and hence entire stack should be prepared for that. See 002752af7b89 ("device property: Allow error pointer to be passed to fwnode APIs") as an example.
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 1bc386032ca8..bed0380c5136 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -1024,45 +1024,6 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode, return desc; } -/** - * acpi_node_get_gpiod() - get a GPIO descriptor from ACPI resources - * @fwnode: pointer to an ACPI firmware node to get the GPIO information from - * @propname: Property name of the GPIO - * @index: index of GpioIo/GpioInt resource (starting from %0) - * @lflags: bitmask of gpio_lookup_flags GPIO_* values - * @dflags: gpiod initialization flags - * - * If @fwnode is an ACPI device object, call acpi_get_gpiod_by_index() for it. - * Otherwise (i.e. it is a data-only non-device object), use the property-based - * GPIO lookup to get to the GPIO resource with the relevant information and use - * that to obtain the GPIO descriptor to return. - * - * If the GPIO cannot be translated or there is an error an ERR_PTR is - * returned. - */ -struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode, - const char *propname, int index, - unsigned long *lflags, - enum gpiod_flags *dflags) -{ - struct acpi_gpio_info info; - struct acpi_device *adev; - struct gpio_desc *desc; - - adev = to_acpi_device_node(fwnode); - if (adev) - desc = acpi_get_gpiod_by_index(adev, propname, index, &info); - else - desc = acpi_get_gpiod_from_data(fwnode, propname, index, &info); - - if (!IS_ERR(desc)) { - acpi_gpio_update_gpiod_flags(dflags, &info); - acpi_gpio_update_gpiod_lookup_flags(lflags, &info); - } - - return desc; -} - /** * acpi_dev_gpio_irq_wake_get_by() - Find GpioInt and translate it to Linux IRQ number * @adev: pointer to a ACPI device to get IRQ from diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h index 8880615327ac..9475f99a9694 100644 --- a/drivers/gpio/gpiolib-acpi.h +++ b/drivers/gpio/gpiolib-acpi.h @@ -36,10 +36,6 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode, unsigned int idx, enum gpiod_flags *dflags, unsigned long *lookupflags); -struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode, - const char *propname, int index, - unsigned long *lflags, - enum gpiod_flags *dflags); int acpi_gpio_count(struct device *dev, const char *con_id); #else @@ -61,12 +57,6 @@ acpi_find_gpio(struct fwnode_handle *fwnode, const char *con_id, { return ERR_PTR(-ENOENT); } -static inline struct gpio_desc * -acpi_node_get_gpiod(struct fwnode_handle *fwnode, const char *propname, - int index, unsigned long *lflags, enum gpiod_flags *dflags) -{ - return ERR_PTR(-ENXIO); -} static inline int acpi_gpio_count(struct device *dev, const char *con_id) { return -ENODEV; diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 607bf4358ff7..7508628b9f6e 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -390,51 +390,6 @@ static unsigned long of_convert_gpio_flags(enum of_gpio_flags flags) return lflags; } -/** - * gpiod_get_from_of_node() - obtain a GPIO from an OF node - * @node: handle of the OF node - * @propname: name of the DT property representing the GPIO - * @index: index of the GPIO to obtain for the consumer - * @dflags: GPIO initialization flags - * @label: label to attach to the requested GPIO - * - * Returns: - * On successful request the GPIO pin is configured in accordance with - * provided @dflags. - * - * In case of error an ERR_PTR() is returned. - */ -struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node, - const char *propname, int index, - enum gpiod_flags dflags, - const char *label) -{ - unsigned long lflags; - struct gpio_desc *desc; - enum of_gpio_flags of_flags; - int ret; - - desc = of_get_named_gpiod_flags(node, propname, index, &of_flags); - if (!desc || IS_ERR(desc)) - return desc; - - ret = gpiod_request(desc, label); - if (ret == -EBUSY && (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE)) - return desc; - if (ret) - return ERR_PTR(ret); - - lflags = of_convert_gpio_flags(of_flags); - - ret = gpiod_configure_flags(desc, propname, lflags, dflags); - if (ret < 0) { - gpiod_put(desc); - return ERR_PTR(ret); - } - - return desc; -} - static struct gpio_desc *of_find_gpio_rename(struct device_node *np, const char *con_id, unsigned int idx, diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h index c44be0f285f3..16c0848c9a25 100644 --- a/drivers/gpio/gpiolib-of.h +++ b/drivers/gpio/gpiolib-of.h @@ -26,10 +26,6 @@ void of_gpiochip_remove(struct gpio_chip *gc); int of_gpio_get_count(struct device *dev, const char *con_id); bool of_gpio_need_valid_mask(const struct gpio_chip *gc); void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev); -struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node, - const char *propname, int index, - enum gpiod_flags dflags, - const char *label); #else static inline struct gpio_desc *of_find_gpio(struct device_node *np, const char *con_id, @@ -52,14 +48,6 @@ static inline void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev) { } -static inline -struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node, - const char *propname, int index, - enum gpiod_flags dflags, - const char *label) -{ - return ERR_PTR(-ENOSYS); -} #endif /* CONFIG_OF_GPIO */ extern struct notifier_block gpio_of_notifier; diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index f0a7a59ac630..79aaba693c4f 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -3801,58 +3801,87 @@ static int platform_gpio_count(struct device *dev, const char *con_id) return count; } -/** - * fwnode_get_named_gpiod - obtain a GPIO from firmware node - * @fwnode: handle of the firmware node - * @propname: name of the firmware property representing the GPIO - * @index: index of the GPIO to obtain for the consumer - * @dflags: GPIO initialization flags - * @label: label to attach to the requested GPIO - * - * This function can be used for drivers that get their configuration - * from opaque firmware. - * - * The 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. - * - * Returns: - * On successful request the GPIO pin is configured in accordance with - * provided @dflags. - * - * In case of error an ERR_PTR() is returned. - */ -static struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, - const char *propname, int index, - enum gpiod_flags dflags, - const char *label) +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode, + struct device *consumer, + const char *con_id, + unsigned int idx, + enum gpiod_flags *flags, + unsigned long *lookupflags) { - unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT; - struct gpio_desc *desc = ERR_PTR(-ENODEV); - int ret; + struct gpio_desc *desc = ERR_PTR(-ENOENT); + dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n", + con_id, fwnode_get_name(fwnode)); + + /* Using device tree? */ if (is_of_node(fwnode)) { - desc = gpiod_get_from_of_node(to_of_node(fwnode), - propname, index, - dflags, - label); - return desc; + dev_dbg(consumer, "using device tree for GPIO lookup\n"); + desc = of_find_gpio(to_of_node(fwnode), + con_id, idx, lookupflags); } else if (is_acpi_node(fwnode)) { - desc = acpi_node_get_gpiod(fwnode, propname, index, - &lflags, &dflags); - if (IS_ERR(desc)) - return desc; - } else { - return ERR_PTR(-EINVAL); + dev_dbg(consumer, "using ACPI for GPIO lookup\n"); + desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags); } - /* Currently only ACPI takes this path */ + return desc; +} + +static struct gpio_desc *gpiod_find_and_request(struct device *consumer, + struct fwnode_handle *fwnode, + const char *con_id, + unsigned int idx, + enum gpiod_flags flags, + const char *label, + bool platform_lookup_allowed) +{ + struct gpio_desc *desc = ERR_PTR(-ENOENT); + unsigned long lookupflags; + int ret; + + if (fwnode) + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, + &flags, &lookupflags); + + if (gpiod_not_found(desc) && platform_lookup_allowed) { + /* + * Either we are not using DT or ACPI, or their lookup did not + * return a result. In that case, use platform lookup as a + * fallback. + */ + dev_dbg(consumer, "using lookup tables for GPIO lookup\n"); + desc = gpiod_find(consumer, con_id, idx, &lookupflags); + } + + if (IS_ERR(desc)) { + dev_dbg(consumer, "No GPIO consumer %s found\n", con_id); + return desc; + } + + /* + * If a connection label was passed use that, else attempt to use + * the device name as label + */ ret = gpiod_request(desc, label); - if (ret) - return ERR_PTR(ret); + if (ret) { + if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE)) + return ERR_PTR(ret); + + /* + * This happens when there are several consumers for + * the same GPIO line: we just return here without + * further initialization. It is a bit of a hack. + * This is necessary to support fixed regulators. + * + * FIXME: Make this more sane and safe. + */ + dev_info(consumer, + "nonexclusive access to GPIO for %s\n", con_id); + return desc; + } - ret = gpiod_configure_flags(desc, propname, lflags, dflags); + ret = gpiod_configure_flags(desc, con_id, lookupflags, flags); if (ret < 0) { + dev_dbg(consumer, "setup of GPIO %s failed\n", con_id); gpiod_put(desc); return ERR_PTR(ret); } @@ -3885,29 +3914,14 @@ static struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, * In case of error an ERR_PTR() is returned. */ struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode, - const char *con_id, int index, + const char *con_id, + int index, enum gpiod_flags flags, const char *label) { - struct gpio_desc *desc; - char prop_name[32]; /* 32 is max size of property name */ - unsigned int i; - - for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) { - if (con_id) - snprintf(prop_name, sizeof(prop_name), "%s-%s", - con_id, gpio_suffixes[i]); - else - snprintf(prop_name, sizeof(prop_name), "%s", - gpio_suffixes[i]); - desc = fwnode_get_named_gpiod(fwnode, prop_name, index, flags, - label); - if (!gpiod_not_found(desc)) - break; - } - - return desc; + return gpiod_find_and_request(NULL, fwnode, con_id, index, flags, label, + false); } EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index); @@ -4061,72 +4075,12 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev, unsigned int idx, enum gpiod_flags flags) { - unsigned long lookupflags = GPIO_LOOKUP_FLAGS_DEFAULT; - struct gpio_desc *desc = NULL; - int ret; - /* Maybe we have a device name, maybe not */ - const char *devname = dev ? dev_name(dev) : "?"; struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL; + const char *devname = dev ? dev_name(dev) : "?"; + const char *label = con_id ?: devname; - dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id); - - /* Using device tree? */ - if (is_of_node(fwnode)) { - dev_dbg(dev, "using device tree for GPIO lookup\n"); - desc = of_find_gpio(to_of_node(fwnode), - con_id, idx, &lookupflags); - } else if (is_acpi_node(fwnode)) { - dev_dbg(dev, "using ACPI for GPIO lookup\n"); - desc = acpi_find_gpio(fwnode, - con_id, idx, &flags, &lookupflags); - } - - /* - * Either we are not using DT or ACPI, or their lookup did not return - * a result. In that case, use platform lookup as a fallback. - */ - if (!desc || gpiod_not_found(desc)) { - dev_dbg(dev, "using lookup tables for GPIO lookup\n"); - desc = gpiod_find(dev, con_id, idx, &lookupflags); - } - - if (IS_ERR(desc)) { - dev_dbg(dev, "No GPIO consumer %s found\n", con_id); - return desc; - } - - /* - * If a connection label was passed use that, else attempt to use - * the device name as label - */ - ret = gpiod_request(desc, con_id ?: devname); - if (ret) { - if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE)) - return ERR_PTR(ret); - - /* - * This happens when there are several consumers for - * the same GPIO line: we just return here without - * further initialization. It is a bit of a hack. - * This is necessary to support fixed regulators. - * - * FIXME: Make this more sane and safe. - */ - dev_info(dev, "nonexclusive access to GPIO for %s\n", con_id ?: devname); - return desc; - } - - ret = gpiod_configure_flags(desc, con_id, lookupflags, flags); - if (ret < 0) { - dev_dbg(dev, "setup of GPIO %s failed\n", con_id); - gpiod_put(desc); - return ERR_PTR(ret); - } - - blocking_notifier_call_chain(&desc->gdev->notifier, - GPIOLINE_CHANGED_REQUESTED, desc); - - return desc; + return gpiod_find_and_request(dev, fwnode, con_id, idx, flags, label, + true); } EXPORT_SYMBOL_GPL(gpiod_get_index);
Ensure that all paths to obtain/look up GPIOD from generic consumer-visible APIs go through the new gpiod_find_and_request() helper, so that we can easily extend it with support for new firmware mechanisms. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/gpio/gpiolib-acpi.c | 39 --------- drivers/gpio/gpiolib-acpi.h | 10 --- drivers/gpio/gpiolib-of.c | 45 ---------- drivers/gpio/gpiolib-of.h | 12 --- drivers/gpio/gpiolib.c | 206 +++++++++++++++++--------------------------- 5 files changed, 80 insertions(+), 232 deletions(-)