| Message ID | 20221031-gpiolib-swnode-v2-0-81f55af5fa0e@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for software nodes to gpiolib | expand |
On Wed, Nov 09, 2022 at 01:29:41PM +0200, Andy Shevchenko wrote: > On Tue, Nov 08, 2022 at 04:26:45PM -0800, 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 device 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. > > Thank for an update! > > I have almost nothing serious except two nit-picks I think we can address: > - dropping const qualifier for no (?) reason > - having a superfluous check and extra dev_dbg() > > If you are are going to address them, feel free to add my Rb tag to > the patches 5 & 6. Thank you for the reviews. I addressed most of your comments, but because I still left the check you were concerned about in I'd like an explicit reviewed-by on the v3 if you OK giving it. Thank you.
On Wed, Nov 09, 2022 at 11:00:29AM -0800, Dmitry Torokhov wrote: > On Wed, Nov 09, 2022 at 01:25:06PM +0200, Andy Shevchenko wrote: > > On Tue, Nov 08, 2022 at 04:26:50PM -0800, 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) > > > { > > > - unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT; > > > > > - struct gpio_desc *desc = ERR_PTR(-ENODEV); > > > > Not sure why this is needed. Now I see that else branch has been changed, > > but looking closer to it, we can drop it completely, while leaving this > > line untouched, correct? > > Yes. I believe removing an initializer and doing a series of if/else > if/else was discussed and [soft] agreed-on in the previous review cycle, > but I can change it back. > > I think we still need to have it return -ENOENT and not -ENODEV/-EINVAL > so that we can fall back to GPIO lookup tables when dealing with an > unsupported node type. Right, okay, let's go with whatever variant you find better. ... > > > + if (!IS_ERR_OR_NULL(fwnode)) > > > > I think this is superfluous check. > > > > Now in the form of this series, you have only a single dev_dbg() that tries to > > dereference it. Do we really need to have it there, since every branch has its > > own dev_dbg() anyway? > > As I mentioned, I like to keep this check to show the reader that we > should only descend into gpiod_find_by_fwnode() if we have a valid > fwnode. It is less about code generation and more about the intent. Yes, but if fwnode is not found, we have a next check for that. I really don't think we lose anything by dropping the check and gaining the code generation as a side effect.
On Wed, Nov 09, 2022 at 11:32:31AM -0800, Dmitry Torokhov wrote: > On Wed, Nov 09, 2022 at 01:29:41PM +0200, Andy Shevchenko wrote: > > On Tue, Nov 08, 2022 at 04:26:45PM -0800, Dmitry Torokhov wrote: ... > > Thank for an update! > > > > I have almost nothing serious except two nit-picks I think we can address: > > - dropping const qualifier for no (?) reason > > - having a superfluous check and extra dev_dbg() > > > > If you are are going to address them, feel free to add my Rb tag to > > the patches 5 & 6. > > Thank you for the reviews. I addressed most of your comments, but > because I still left the check you were concerned about in I'd like an > explicit reviewed-by on the v3 if you OK giving it. I gave for patch 6 and for patch 5 I still think we can get rid of the check.
On Thu, Nov 10, 2022 at 03:42:40PM +0200, Andy Shevchenko wrote: > On Wed, Nov 09, 2022 at 11:00:29AM -0800, Dmitry Torokhov wrote: > > On Wed, Nov 09, 2022 at 01:25:06PM +0200, Andy Shevchenko wrote: > > > On Tue, Nov 08, 2022 at 04:26:50PM -0800, 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) > > > > { > > > > - unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT; > > > > > > > - struct gpio_desc *desc = ERR_PTR(-ENODEV); > > > > > > Not sure why this is needed. Now I see that else branch has been changed, > > > but looking closer to it, we can drop it completely, while leaving this > > > line untouched, correct? > > > > Yes. I believe removing an initializer and doing a series of if/else > > if/else was discussed and [soft] agreed-on in the previous review cycle, > > but I can change it back. > > > > I think we still need to have it return -ENOENT and not -ENODEV/-EINVAL > > so that we can fall back to GPIO lookup tables when dealing with an > > unsupported node type. > > Right, okay, let's go with whatever variant you find better. > > ... > > > > > + if (!IS_ERR_OR_NULL(fwnode)) > > > > > > I think this is superfluous check. > > > > > > Now in the form of this series, you have only a single dev_dbg() that tries to > > > dereference it. Do we really need to have it there, since every branch has its > > > own dev_dbg() anyway? > > > > As I mentioned, I like to keep this check to show the reader that we > > should only descend into gpiod_find_by_fwnode() if we have a valid > > fwnode. It is less about code generation and more about the intent. > > Yes, but if fwnode is not found, we have a next check for that. No, the check you are talking about is for the GPIO not being located. It does not have anything to do with fwnode validity. You are relying on intimate knowledge of gpiod_find_by_fwnode() implementation and the fact that in the current form it will withstand ERR_PTR-encoded or NULL fwnode. I want to have the source code so clear in its intent so that I can be woken up in the middle of the night with a huge hangover and still be able to tell how it is supposed to behave. > I really don't > think we lose anything by dropping the check and gaining the code generation as > a side effect. This is cold path, happening only on startup. I am not saying that we want to make it slow unnecessarily, but a condition branch that might even get optimized out is not something we should be concerned here. Thanks.
On Thu, Nov 10, 2022 at 09:21:59AM -0800, Dmitry Torokhov wrote: > On Thu, Nov 10, 2022 at 03:42:40PM +0200, Andy Shevchenko wrote: > > On Wed, Nov 09, 2022 at 11:00:29AM -0800, Dmitry Torokhov wrote: > > > On Wed, Nov 09, 2022 at 01:25:06PM +0200, Andy Shevchenko wrote: > > > > On Tue, Nov 08, 2022 at 04:26:50PM -0800, Dmitry Torokhov wrote: ... > > > > > + if (!IS_ERR_OR_NULL(fwnode)) > > > > > > > > I think this is superfluous check. > > > > > > > > Now in the form of this series, you have only a single dev_dbg() that tries to > > > > dereference it. Do we really need to have it there, since every branch has its > > > > own dev_dbg() anyway? > > > > > > As I mentioned, I like to keep this check to show the reader that we > > > should only descend into gpiod_find_by_fwnode() if we have a valid > > > fwnode. It is less about code generation and more about the intent. > > > > Yes, but if fwnode is not found, we have a next check for that. > > No, the check you are talking about is for the GPIO not being located. > It does not have anything to do with fwnode validity. You are relying on > intimate knowledge of gpiod_find_by_fwnode() implementation and the fact > that in the current form it will withstand ERR_PTR-encoded or NULL > fwnode. > > I want to have the source code so clear in its intent so that I can be > woken up in the middle of the night with a huge hangover and still be > able to tell how it is supposed to behave. As you said let's leave it to Bart and Linus. > > I really don't > > think we lose anything by dropping the check and gaining the code generation as > > a side effect. > > This is cold path, happening only on startup. I am not saying that we > want to make it slow unnecessarily, but a condition branch that might > even get optimized out is not something we should be concerned here. Agree, that's why I called it "side effect".
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 device 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. 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 --- Changes in v2: - reworked the series to be independent of other in-flight patches. That meant keeping devm_gpiod_get_from_of_node() for now. - removed handling of secondary nodes, it deserves a separate patch series - fixed refcounting when handling swnodes (Andy) - added include/linux/gpio/property.h with PROPERTY_ENTRY_GPIO (Andy) - addressed most of the rest of Andy's comments - collected reviewed-by and acked-by - Link to v1: https://lore.kernel.org/r/20221031-gpiolib-swnode-v1-0-a0ab48d229c7@gmail.com --- 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 | 7 +- drivers/gpio/gpiolib-of.h | 4 +- drivers/gpio/gpiolib-swnode.c | 128 ++++++++++++++++++++++++ drivers/gpio/gpiolib-swnode.h | 14 +++ drivers/gpio/gpiolib.c | 219 +++++++++++++++++------------------------- include/linux/gpio/property.h | 11 +++ 9 files changed, 329 insertions(+), 241 deletions(-) --- base-commit: b6fc3fddade7a194bd141a49f2689e50f796ef46 change-id: 20221031-gpiolib-swnode-948203f49b23