| Message ID | 20221031-gpiolib-swnode-v1-0-a0ab48d229c7@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for software nodes to gpiolib | expand |
On Fri, Nov 4, 2022 at 7:10 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > This series attempts to add support for software nodes to gpiolib, using > software node references. This allows us to convert more drivers to the > generic device properties and drop support for custom platform data. > > To describe a GPIO via software nodes we can create the following data > items: > > /* Node representing the GPIO controller/GPIO bank */ > static const struct software_node gpio_bank_b_node = { > .name = "B", > }; > > /* > * Properties that will be assigned to a software node assigned to > * the devicei that used platform data. > */ > static const struct property_entry simone_key_enter_props[] = { > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > PROPERTY_ENTRY_STRING("label", "enter"), > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), > { } > }; > > The code in gpiolib handling software nodes uses the name in the > software node representing GPIO controller to locate the actual instance > of GPIO controller. > > Note that kbuild robot is likely to complain about this patchset because > it depends on patches removing [devm_]gpiod_get_from_of_node() and > devm_fwnode_get_[index_]gpiod_from_child() APIs that are still pending. > I pushed them to > > git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tmp-gpiolib > > for your reference. > > To: Linus Walleij <linus.walleij@linaro.org> > To: Bartosz Golaszewski <brgl@bgdev.pl> > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: linux-gpio@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-acpi@vger.kernel.org > > --- > Dmitry Torokhov (6): > gpiolib: of: change of_find_gpio() to accept device node > gpiolib: acpi: change acpi_find_gpio() to accept firmware node > gpiolib: acpi: teach acpi_find_gpio() to handle data-only nodes > gpiolib: acpi: avoid leaking ACPI details into upper gpiolib layers > gpiolib: consolidate GPIO lookups > gpiolib: add support for software nodes > > drivers/gpio/Makefile | 1 + > drivers/gpio/gpiolib-acpi.c | 132 +++++++++++++---------- > drivers/gpio/gpiolib-acpi.h | 54 +--------- > drivers/gpio/gpiolib-of.c | 52 +-------- > drivers/gpio/gpiolib-of.h | 16 +-- > drivers/gpio/gpiolib-swnode.c | 106 +++++++++++++++++++ > drivers/gpio/gpiolib-swnode.h | 13 +++ > drivers/gpio/gpiolib.c | 239 ++++++++++++++++++++---------------------- > 8 files changed, 316 insertions(+), 297 deletions(-) > --- > base-commit: dc04f5ab1b1114aa19b9026f816fc01ca9c9941d > change-id: 20221031-gpiolib-swnode-948203f49b23 > > -- > Dmitry > This is great work. I'll wait for Andy to Ack the ACPI patches and let's get it in. Bartosz
On Thu, Nov 03, 2022 at 11:10:10PM -0700, Dmitry Torokhov wrote: > This series attempts to add support for software nodes to gpiolib, using > software node references. This allows us to convert more drivers to the > generic device properties and drop support for custom platform data. > > To describe a GPIO via software nodes we can create the following data > items: > > /* Node representing the GPIO controller/GPIO bank */ > static const struct software_node gpio_bank_b_node = { > .name = "B", > }; > > /* > * Properties that will be assigned to a software node assigned to > * the devicei that used platform data. > */ > static const struct property_entry simone_key_enter_props[] = { > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > PROPERTY_ENTRY_STRING("label", "enter"), > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), > { } > }; > > The code in gpiolib handling software nodes uses the name in the > software node representing GPIO controller to locate the actual instance > of GPIO controller. > > Note that kbuild robot is likely to complain about this patchset because > it depends on patches removing [devm_]gpiod_get_from_of_node() and > devm_fwnode_get_[index_]gpiod_from_child() APIs that are still pending. > I pushed them to > > git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tmp-gpiolib > > for your reference. I agree with Bart, this is nice work! So, for the patches 1-4: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > To: Linus Walleij <linus.walleij@linaro.org> > To: Bartosz Golaszewski <brgl@bgdev.pl> > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: linux-gpio@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-acpi@vger.kernel.org > > --- > Dmitry Torokhov (6): > gpiolib: of: change of_find_gpio() to accept device node > gpiolib: acpi: change acpi_find_gpio() to accept firmware node > gpiolib: acpi: teach acpi_find_gpio() to handle data-only nodes > gpiolib: acpi: avoid leaking ACPI details into upper gpiolib layers > gpiolib: consolidate GPIO lookups > gpiolib: add support for software nodes > > drivers/gpio/Makefile | 1 + > drivers/gpio/gpiolib-acpi.c | 132 +++++++++++++---------- > drivers/gpio/gpiolib-acpi.h | 54 +--------- > drivers/gpio/gpiolib-of.c | 52 +-------- > drivers/gpio/gpiolib-of.h | 16 +-- > drivers/gpio/gpiolib-swnode.c | 106 +++++++++++++++++++ > drivers/gpio/gpiolib-swnode.h | 13 +++ > drivers/gpio/gpiolib.c | 239 ++++++++++++++++++++---------------------- > 8 files changed, 316 insertions(+), 297 deletions(-) > --- > base-commit: dc04f5ab1b1114aa19b9026f816fc01ca9c9941d > change-id: 20221031-gpiolib-swnode-948203f49b23 > > -- > Dmitry >
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: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.
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.
On Fri, Nov 4, 2022 at 7:10 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > This series attempts to add support for software nodes to gpiolib, using > software node references. This allows us to convert more drivers to the > generic device properties and drop support for custom platform data. > > To describe a GPIO via software nodes we can create the following data > items: > > /* Node representing the GPIO controller/GPIO bank */ > static const struct software_node gpio_bank_b_node = { > .name = "B", > }; > > /* > * Properties that will be assigned to a software node assigned to > * the devicei that used platform data. > */ > static const struct property_entry simone_key_enter_props[] = { > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > PROPERTY_ENTRY_STRING("label", "enter"), > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), > { } > }; > > The code in gpiolib handling software nodes uses the name in the > software node representing GPIO controller to locate the actual instance > of GPIO controller. > > Note that kbuild robot is likely to complain about this patchset because > it depends on patches removing [devm_]gpiod_get_from_of_node() and > devm_fwnode_get_[index_]gpiod_from_child() APIs that are still pending. > I pushed them to > > git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tmp-gpiolib > > for your reference. > > To: Linus Walleij <linus.walleij@linaro.org> > To: Bartosz Golaszewski <brgl@bgdev.pl> > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: linux-gpio@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-acpi@vger.kernel.org I have waited literally years for this patch series :D Acked-by: Linus Walleij <linus.walleij@linaro.org> The ACPI details is Andy territory so I dare not speak about those, but for everything else I think this is a go. Yours, Linus Walleij
This series attempts to add support for software nodes to gpiolib, using software node references. This allows us to convert more drivers to the generic device properties and drop support for custom platform data. To describe a GPIO via software nodes we can create the following data items: /* Node representing the GPIO controller/GPIO bank */ static const struct software_node gpio_bank_b_node = { .name = "B", }; /* * Properties that will be assigned to a software node assigned to * the devicei that used platform data. */ static const struct property_entry simone_key_enter_props[] = { PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), PROPERTY_ENTRY_STRING("label", "enter"), PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), { } }; The code in gpiolib handling software nodes uses the name in the software node representing GPIO controller to locate the actual instance of GPIO controller. Note that kbuild robot is likely to complain about this patchset because it depends on patches removing [devm_]gpiod_get_from_of_node() and devm_fwnode_get_[index_]gpiod_from_child() APIs that are still pending. I pushed them to git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tmp-gpiolib for your reference. To: Linus Walleij <linus.walleij@linaro.org> To: Bartosz Golaszewski <brgl@bgdev.pl> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: linux-gpio@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-acpi@vger.kernel.org --- Dmitry Torokhov (6): gpiolib: of: change of_find_gpio() to accept device node gpiolib: acpi: change acpi_find_gpio() to accept firmware node gpiolib: acpi: teach acpi_find_gpio() to handle data-only nodes gpiolib: acpi: avoid leaking ACPI details into upper gpiolib layers gpiolib: consolidate GPIO lookups gpiolib: add support for software nodes drivers/gpio/Makefile | 1 + drivers/gpio/gpiolib-acpi.c | 132 +++++++++++++---------- drivers/gpio/gpiolib-acpi.h | 54 +--------- drivers/gpio/gpiolib-of.c | 52 +-------- drivers/gpio/gpiolib-of.h | 16 +-- drivers/gpio/gpiolib-swnode.c | 106 +++++++++++++++++++ drivers/gpio/gpiolib-swnode.h | 13 +++ drivers/gpio/gpiolib.c | 239 ++++++++++++++++++++---------------------- 8 files changed, 316 insertions(+), 297 deletions(-) --- base-commit: dc04f5ab1b1114aa19b9026f816fc01ca9c9941d change-id: 20221031-gpiolib-swnode-948203f49b23