| Message ID | 20221031-gpiolib-swnode-v1-6-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:16PM -0700, Dmitry Torokhov wrote: > Now that static device properties understand notion of child nodes and > references, let's teach gpiolib to handle them: > > - GPIOs are represented as a references to software nodes representing > gpiochip > - references must have 2 arguments - GPIO number within the chip and > GPIO flags (GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, etc). > - name of the software node representing gpiochip must match label of > the gpiochip, as we use it to locate gpiochip structure at runtime. > > const struct software_node gpio_bank_b_node = { > .name = "B", > }; > > const struct property_entry simone_key_enter_props[] __initconst = { > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > PROPERTY_ENTRY_STRING("label", "enter"), > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), Okay, can we have an example for something like reset-gpios? Because from the above I can't easily get what label is and how in the `gpioinfo` tool the requested line will look like. > { } > }; ... > +#include <linux/err.h> > +#include <linux/errno.h> > +#include <linux/gpio/consumer.h> It seems you are using much more that these ones. ... > + char prop_name[32]; /* 32 is max size of property name */ Why is it not defined then? ... > + /* > + * Note we do not need to try both -gpios and -gpio suffixes, > + * as, unlike OF and ACPI, we can fix software nodes to conform > + * to the proper binding. > + */ True, but for the sake of consistency between providers perhaps it makes sense to check that as well. Dunno, up to Linus and Bart to decide. ... > + /* > + * We expect all swnode-described GPIOs have GPIO number and > + * polarity arguments, hence nargs is set to 2. > + */ Maybe instead you can provide a custom macro wrapper that will check the number of arguments at compile time? ... > + pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n", > + __func__, prop_name, fwnode, idx); __func__ is not needed. Dynamic Debug can automatically add it. Since you have an fwnode, use that as a marker. ... > + chip = gpiochip_find((void *)chip_node->name, > + swnode_gpiochip_match_name); One line? ... > + pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n", > + __func__, prop_name, fwnode, idx, PTR_ERR_OR_ZERO(desc)); Same as above. ... > + char prop_name[32]; > + if (con_id) > + snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id); > + else > + strscpy(prop_name, "gpios", sizeof(prop_name)); I saw this code, please deduplicate. ... > + /* > + * This is not very efficient, but GPIO lists usually have only > + * 1 or 2 entries. > + */ > + count = 0; > + while (fwnode_property_get_reference_args(fwnode, prop_name, NULL, > + 0, count, &args) == 0) I would put it into for loop (and looking into property.h I think propname is fine variable name): for (count = 0; ; count++) { if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args)) break; } Btw, what about reference counting? Do we need to care about it? > + return count ? count : -ENOENT; Elvis would work as well. return count ?: -ENOENT; ... > +struct fwnode_handle; struct gpio_desc; > + > +struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode, > + const char *con_id, unsigned int idx, > + unsigned long *flags); > +int swnode_gpio_count(struct fwnode_handle *fwnode, const char *con_id); ... > + /* > + * First look up GPIO in the secondary software node in case > + * it was used to store updated properties. Why this is done first? We don't try secondary before we have checked primary. > + */ > + if (is_software_node(fwnode->secondary)) { With the previous comments it would become if (fwnode && is_...) > + dev_dbg(consumer, > + "using secondary software node for GPIO lookup\n"); > + desc = swnode_find_gpio(fwnode->secondary, > + con_id, idx, lookupflags); > + if (!gpiod_not_found(desc)) > + return desc; > + } ... > int gpiod_count(struct device *dev, const char *con_id) > { > + struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL; > + int count; > + > + /* > + * First look up GPIO in the secondary software node in case > + * it was used to store updated properties. > + */ Same question as above. > + if (!IS_ERR_OR_NULL(fwnode) && is_software_node(fwnode->secondary)) { > + count = swnode_gpio_count(fwnode->secondary, con_id); > + if (count > 0) > + return count; > + } > > if (is_of_node(fwnode)) > count = of_gpio_get_count(dev, con_id); > else if (is_acpi_node(fwnode)) > count = acpi_gpio_count(dev, con_id); > + else if (is_software_node(fwnode)) > + count = swnode_gpio_count(fwnode, con_id); > + else > + count = -ENOENT; > > if (count < 0) > count = platform_gpio_count(dev, con_id);
On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote: > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote: > > Now that static device properties understand notion of child nodes and > > references, let's teach gpiolib to handle them: > > > > - GPIOs are represented as a references to software nodes representing > > gpiochip > > - references must have 2 arguments - GPIO number within the chip and > > GPIO flags (GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, etc). > > - name of the software node representing gpiochip must match label of > > the gpiochip, as we use it to locate gpiochip structure at runtime. > > > > const struct software_node gpio_bank_b_node = { > > .name = "B", > > }; > > > > const struct property_entry simone_key_enter_props[] __initconst = { > > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > > > PROPERTY_ENTRY_STRING("label", "enter"), > > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), > > Okay, can we have an example for something like reset-gpios? Because from > the above I can't easily get what label is and how in the `gpioinfo` tool > the requested line will look like. The label is something unrelated to gpio. The example was supposed to match gpio-keys binding found in Documentation/devicetree/bindings/input/gpio-keys.yaml > > > { } > > }; > > ... > > > +#include <linux/err.h> > > +#include <linux/errno.h> > > +#include <linux/gpio/consumer.h> > > It seems you are using much more that these ones. Yeah, you are right. > > ... > > > + char prop_name[32]; /* 32 is max size of property name */ > > Why is it not defined then? Not sure. 32 is the limit used throughout gpiolib (see the main gpiolib.c, gpiolib-acpi.c and gpiolib-of.c). We could add a private gpiolib define. Or we could dynamically allocate strings if we belive this is an issue. I'd like to do it as a followup if we decide this needs changing. > > ... > > > + /* > > + * Note we do not need to try both -gpios and -gpio suffixes, > > + * as, unlike OF and ACPI, we can fix software nodes to conform > > + * to the proper binding. > > + */ > > True, but for the sake of consistency between providers perhaps it makes sense > to check that as well. Dunno, up to Linus and Bart to decide. I hear you, however we had to have this fallback for OF and ACPI because of concerns of separate DT/firmware and keeping compatibility. Here we do not have this problem, so I think we should require properly named properties. > > ... > > > + /* > > + * We expect all swnode-described GPIOs have GPIO number and > > + * polarity arguments, hence nargs is set to 2. > > + */ > > Maybe instead you can provide a custom macro wrapper that will check the number > of arguments at compile time? We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF() that enforces needed arguments. > > ... > > > + pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n", > > + __func__, prop_name, fwnode, idx); > > __func__ is not needed. Dynamic Debug can automatically add it. > Since you have an fwnode, use that as a marker. I was mimicking gpiolib-of.c::of_get_named_gpiod_flags(). I guess we can guess the function from other log messages we emit, but does it hurt having it? > > ... > > > + chip = gpiochip_find((void *)chip_node->name, > > + swnode_gpiochip_match_name); > > One line? > > ... > > > + pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n", > > + __func__, prop_name, fwnode, idx, PTR_ERR_OR_ZERO(desc)); > > Same as above. > > ... > > > + char prop_name[32]; > > > + if (con_id) > > + snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id); > > + else > > + strscpy(prop_name, "gpios", sizeof(prop_name)); > > I saw this code, please deduplicate. OK. > > ... > > > + /* > > + * This is not very efficient, but GPIO lists usually have only > > + * 1 or 2 entries. > > + */ > > + count = 0; > > + while (fwnode_property_get_reference_args(fwnode, prop_name, NULL, > > + 0, count, &args) == 0) > > I would put it into for loop (and looking into property.h I think propname > is fine variable name): > > for (count = 0; ; count++) { > if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args)) > break; > } OK on name, but I like explicit counting with the "while" loop as it shows the purpose of the code. > > Btw, what about reference counting? Do we need to care about it? Yes, indeed, we need to drop the reference, thank you for noticing! > > > + return count ? count : -ENOENT; > > Elvis would work as well. > > return count ?: -ENOENT; OK, I like Elvis. > > > ... > > > +struct fwnode_handle; > > struct gpio_desc; > > > + > > +struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode, > > + const char *con_id, unsigned int idx, > > + unsigned long *flags); > > +int swnode_gpio_count(struct fwnode_handle *fwnode, const char *con_id); > > ... > > > + /* > > + * First look up GPIO in the secondary software node in case > > + * it was used to store updated properties. > > Why this is done first? We don't try secondary before we have checked primary. I believe we should check secondary first, so that secondaries can be used not only to add missing properties, but also to override existing ones in case they are incorrect. > > > + */ > > > + if (is_software_node(fwnode->secondary)) { > > With the previous comments it would become > > if (fwnode && is_...) Right, I prefer if we could trust that fwnode passed here is not NULL. > > > + dev_dbg(consumer, > > + "using secondary software node for GPIO lookup\n"); > > + desc = swnode_find_gpio(fwnode->secondary, > > + con_id, idx, lookupflags); > > + if (!gpiod_not_found(desc)) > > + return desc; > > + } > > ... > > > int gpiod_count(struct device *dev, const char *con_id) > > { > > + struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL; > > + int count; > > + > > + /* > > + * First look up GPIO in the secondary software node in case > > + * it was used to store updated properties. > > + */ > > Same question as above. > > > + if (!IS_ERR_OR_NULL(fwnode) && is_software_node(fwnode->secondary)) { > > + count = swnode_gpio_count(fwnode->secondary, con_id); > > + if (count > 0) > > + return count; > > + } > > > > if (is_of_node(fwnode)) > > count = of_gpio_get_count(dev, con_id); > > else if (is_acpi_node(fwnode)) > > count = acpi_gpio_count(dev, con_id); > > + else if (is_software_node(fwnode)) > > + count = swnode_gpio_count(fwnode, con_id); > > + else > > + count = -ENOENT; > > > > if (count < 0) > > count = platform_gpio_count(dev, con_id); > Thanks for the review!
On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote: > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote: > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote: ... > > > const struct property_entry simone_key_enter_props[] __initconst = { > > > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > > > > > PROPERTY_ENTRY_STRING("label", "enter"), > > > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), > > > > Okay, can we have an example for something like reset-gpios? Because from > > the above I can't easily get what label is and how in the `gpioinfo` tool > > the requested line will look like. > > The label is something unrelated to gpio. The example was supposed to > match gpio-keys binding found in > Documentation/devicetree/bindings/input/gpio-keys.yaml Yes, but what would be output of `gpioinfo` for the above example and if GPIO is named properly (with con_id)? > > > { } > > > }; ... > > > + /* > > > + * We expect all swnode-described GPIOs have GPIO number and > > > + * polarity arguments, hence nargs is set to 2. > > > + */ > > > > Maybe instead you can provide a custom macro wrapper that will check the number > > of arguments at compile time? > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF() > that enforces needed arguments. Yes, that's what I meant. ... > > > + pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n", > > > + __func__, prop_name, fwnode, idx); > > > > __func__ is not needed. Dynamic Debug can automatically add it. > > Since you have an fwnode, use that as a marker. > > I was mimicking gpiolib-of.c::of_get_named_gpiod_flags(). I guess we can > guess the function from other log messages we emit, but does it hurt > having it? I think it's redundant. You can modify message itself to improve its uniqueness. ... > > > + /* > > > + * This is not very efficient, but GPIO lists usually have only > > > + * 1 or 2 entries. > > > + */ > > > + count = 0; > > > + while (fwnode_property_get_reference_args(fwnode, prop_name, NULL, > > > + 0, count, &args) == 0) > > > > I would put it into for loop (and looking into property.h I think propname > > is fine variable name): > > > > for (count = 0; ; count++) { > > if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args)) > > break; > > } > > OK on name, but I like explicit counting with the "while" loop as it > shows the purpose of the code. OK, let's see how it will look like with the proper dropped reference. > > Btw, what about reference counting? Do we need to care about it? > > Yes, indeed, we need to drop the reference, thank you for noticing! ... > > > + /* > > > + * First look up GPIO in the secondary software node in case > > > + * it was used to store updated properties. > > > > Why this is done first? We don't try secondary before we have checked primary. > > I believe we should check secondary first, so that secondaries can be > used not only to add missing properties, but also to override existing > ones in case they are incorrect. It contradicts all code we have in the kernel regarding the use of software nodes, you need very strong argument to justify that. Personally I think this must be fixed. > > > + */
On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote: > On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote: > > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote: > > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote: > > ... > > > > > const struct property_entry simone_key_enter_props[] __initconst = { > > > > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > > > > > > > PROPERTY_ENTRY_STRING("label", "enter"), > > > > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), > > > > > > Okay, can we have an example for something like reset-gpios? Because from > > > the above I can't easily get what label is and how in the `gpioinfo` tool > > > the requested line will look like. > > > > The label is something unrelated to gpio. The example was supposed to > > match gpio-keys binding found in > > Documentation/devicetree/bindings/input/gpio-keys.yaml > > Yes, but what would be output of `gpioinfo` for the above example and > if GPIO is named properly (with con_id)? Same as if I am using device tree, or ACPI, etc. I am not changing how labeling is done, so whatever rules were before adding swnode support they will be used with swnodes. With the hack patch to gpio-keys.c below and device using the following DT fragment I see the following from gpioinfo: gpio_keys: gpio-keys { status = "okay"; compatible = "gpio-keys"; pinctrl-names = "default"; pinctrl-0 = <&pen_eject>; pen_insert: pen-insert { label = "Pen Insert"; /* Insert = low, eject = high */ /* gpios = <&pio 18 GPIO_ACTIVE_LOW>; */ linux,code = <SW_PEN_INSERTED>; linux,input-type = <EV_SW>; wakeup-event-action = <EV_ACT_DEASSERTED>; wakeup-source; }; }; Just "gpios" (con_id == NULL): line 18: "PEN_EJECT_OD" "Pen Insert" input active-low [used] With "key-gpios" (con_id == "key") it is exactly the same: line 18: "PEN_EJECT_OD" "Pen Insert" input active-low [used] Ah, I guess you wonder how it will look like if we do not pass this "label" into devm_fwnode_gpiod_get() and instead use NULL? line 18: "PEN_EJECT_OD" "?" input active-low [used] If the driver used gpiod_get() or similar it would either have the "con_id" label or device name (produced with dev_name(dev) if con_id is NULL. Still, not changes from using swnodes compared to ACPI or DT. > > > > > { } > > > > }; > > ... > > > > > + /* > > > > + * We expect all swnode-described GPIOs have GPIO number and > > > > + * polarity arguments, hence nargs is set to 2. > > > > + */ > > > > > > Maybe instead you can provide a custom macro wrapper that will check the number > > > of arguments at compile time? > > > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF() > > that enforces needed arguments. > > Yes, that's what I meant. Where do you think it should go? Not sure if I want to pollute property.h, I guess linux/gpio/matchine.h will need to include property.h? > > ... > > > > > + pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n", > > > > + __func__, prop_name, fwnode, idx); > > > > > > __func__ is not needed. Dynamic Debug can automatically add it. > > > Since you have an fwnode, use that as a marker. > > > > I was mimicking gpiolib-of.c::of_get_named_gpiod_flags(). I guess we can > > guess the function from other log messages we emit, but does it hurt > > having it? > > I think it's redundant. You can modify message itself to improve its > uniqueness. ¯\_(ツ)_/¯ I think we are moving into extreme bikeshedding direction here. > > ... > > > > > + /* > > > > + * This is not very efficient, but GPIO lists usually have only > > > > + * 1 or 2 entries. > > > > + */ > > > > + count = 0; > > > > + while (fwnode_property_get_reference_args(fwnode, prop_name, NULL, > > > > + 0, count, &args) == 0) > > > > > > I would put it into for loop (and looking into property.h I think propname > > > is fine variable name): > > > > > > for (count = 0; ; count++) { > > > if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args)) > > > break; > > > } > > > > OK on name, but I like explicit counting with the "while" loop as it > > shows the purpose of the code. > > OK, let's see how it will look like with the proper dropped reference. > > > > Btw, what about reference counting? Do we need to care about it? > > > > Yes, indeed, we need to drop the reference, thank you for noticing! > > ... > > > > > + /* > > > > + * First look up GPIO in the secondary software node in case > > > > + * it was used to store updated properties. > > > > > > Why this is done first? We don't try secondary before we have checked primary. > > > > I believe we should check secondary first, so that secondaries can be > > used not only to add missing properties, but also to override existing > > ones in case they are incorrect. > > It contradicts all code we have in the kernel regarding the use of software > nodes, you need very strong argument to justify that. > > Personally I think this must be fixed. I agree, the rest of the code should be fixed ;) I'll put it on my TODO list. I gave my argument above already: swnodes should not only be useful to add missing properties, but also allow fixing up existing ones. If I implemented what you are suggesting then I would not be able to create this concise example and would need to model entire DT node for GPIO keys. Thanks.
On Fri, Nov 04, 2022 at 09:48:47PM -0700, Dmitry Torokhov wrote: > On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote: > > On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote: > > > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote: > > > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote: ... > > > > > const struct property_entry simone_key_enter_props[] __initconst = { > > > > > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > > > > > > > > > PROPERTY_ENTRY_STRING("label", "enter"), > > > > > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), > > > > > > > > Okay, can we have an example for something like reset-gpios? Because from > > > > the above I can't easily get what label is and how in the `gpioinfo` tool > > > > the requested line will look like. > > > > > > The label is something unrelated to gpio. The example was supposed to > > > match gpio-keys binding found in > > > Documentation/devicetree/bindings/input/gpio-keys.yaml > > > > Yes, but what would be output of `gpioinfo` for the above example and > > if GPIO is named properly (with con_id)? > > Same as if I am using device tree, or ACPI, etc. I am not changing how > labeling is done, so whatever rules were before adding swnode support > they will be used with swnodes. > > With the hack patch to gpio-keys.c below and device using the following > DT fragment I see the following from gpioinfo: > > gpio_keys: gpio-keys { > status = "okay"; > > compatible = "gpio-keys"; > pinctrl-names = "default"; > pinctrl-0 = <&pen_eject>; > > pen_insert: pen-insert { > label = "Pen Insert"; > /* Insert = low, eject = high */ > /* gpios = <&pio 18 GPIO_ACTIVE_LOW>; */ > linux,code = <SW_PEN_INSERTED>; > linux,input-type = <EV_SW>; > wakeup-event-action = <EV_ACT_DEASSERTED>; > wakeup-source; > }; > }; > > Just "gpios" (con_id == NULL): > > line 18: "PEN_EJECT_OD" "Pen Insert" input active-low [used] > > With "key-gpios" (con_id == "key") it is exactly the same: > > line 18: "PEN_EJECT_OD" "Pen Insert" input active-low [used] > > Ah, I guess you wonder how it will look like if we do not pass this > "label" into devm_fwnode_gpiod_get() and instead use NULL? > > line 18: "PEN_EJECT_OD" "?" input active-low [used] > > If the driver used gpiod_get() or similar it would either have the > "con_id" label or device name (produced with dev_name(dev) if con_id is > NULL. Still, not changes from using swnodes compared to ACPI or DT. Yes, can you add a summary of above to the commit message? > > > > > { } > > > > > }; ... > > > > > + /* > > > > > + * We expect all swnode-described GPIOs have GPIO number and > > > > > + * polarity arguments, hence nargs is set to 2. > > > > > + */ > > > > > > > > Maybe instead you can provide a custom macro wrapper that will check the number > > > > of arguments at compile time? > > > > > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF() > > > that enforces needed arguments. > > > > Yes, that's what I meant. > > Where do you think it should go? Not sure if I want to pollute > property.h, I guess linux/gpio/matchine.h will need to include > property.h? Good question. I was thinking more of a separate header for that, because adding property.h adds tons of stuff which might be not needed otherwise. ... > > > > > + /* > > > > > + * First look up GPIO in the secondary software node in case > > > > > + * it was used to store updated properties. > > > > > > > > Why this is done first? We don't try secondary before we have checked primary. > > > > > > I believe we should check secondary first, so that secondaries can be > > > used not only to add missing properties, but also to override existing > > > ones in case they are incorrect. > > > > It contradicts all code we have in the kernel regarding the use of software > > nodes, you need very strong argument to justify that. > > > > Personally I think this must be fixed. > > I agree, the rest of the code should be fixed ;) I'll put it on my TODO > list. I'm not sure what "rest of the code" you are referring to. The core part of device property APIs? > I gave my argument above already: swnodes should not only be useful to > add missing properties, but also allow fixing up existing ones. If I > implemented what you are suggesting then I would not be able to create > this concise example and would need to model entire DT node for GPIO > keys. Why do you need that in the first place? We should not use swnodes as primary source of the information. The auxiliary one is fine. "Fixing" via priority inversion in current model is not good thing to have. If you really need that you have to first do the following: - convert fwnode to be a list node - unembed it from struct device (leaving only head of list there - update all necessary APIs respectively In such implementation list_add() / list_add_tail() will define a priority and you may have stack of properties. Doing it in a hackish way by allowing priority inversion in _some_ APIs is no go in my opinion.
On Mon, Nov 07, 2022 at 01:08:09PM +0200, Andy Shevchenko wrote: > On Fri, Nov 04, 2022 at 09:48:47PM -0700, Dmitry Torokhov wrote: > > On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote: > > > On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote: > > > > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote: > > > > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote: > > ... > > > > > > > const struct property_entry simone_key_enter_props[] __initconst = { > > > > > > PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), > > > > > > > > > > > PROPERTY_ENTRY_STRING("label", "enter"), > > > > > > PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), > > > > > > > > > > Okay, can we have an example for something like reset-gpios? Because from > > > > > the above I can't easily get what label is and how in the `gpioinfo` tool > > > > > the requested line will look like. > > > > > > > > The label is something unrelated to gpio. The example was supposed to > > > > match gpio-keys binding found in > > > > Documentation/devicetree/bindings/input/gpio-keys.yaml > > > > > > Yes, but what would be output of `gpioinfo` for the above example and > > > if GPIO is named properly (with con_id)? > > > > Same as if I am using device tree, or ACPI, etc. I am not changing how > > labeling is done, so whatever rules were before adding swnode support > > they will be used with swnodes. > > > > With the hack patch to gpio-keys.c below and device using the following > > DT fragment I see the following from gpioinfo: > > > > gpio_keys: gpio-keys { > > status = "okay"; > > > > compatible = "gpio-keys"; > > pinctrl-names = "default"; > > pinctrl-0 = <&pen_eject>; > > > > pen_insert: pen-insert { > > label = "Pen Insert"; > > /* Insert = low, eject = high */ > > /* gpios = <&pio 18 GPIO_ACTIVE_LOW>; */ > > linux,code = <SW_PEN_INSERTED>; > > linux,input-type = <EV_SW>; > > wakeup-event-action = <EV_ACT_DEASSERTED>; > > wakeup-source; > > }; > > }; > > > > Just "gpios" (con_id == NULL): > > > > line 18: "PEN_EJECT_OD" "Pen Insert" input active-low [used] > > > > With "key-gpios" (con_id == "key") it is exactly the same: > > > > line 18: "PEN_EJECT_OD" "Pen Insert" input active-low [used] > > > > Ah, I guess you wonder how it will look like if we do not pass this > > "label" into devm_fwnode_gpiod_get() and instead use NULL? > > > > line 18: "PEN_EJECT_OD" "?" input active-low [used] > > > > If the driver used gpiod_get() or similar it would either have the > > "con_id" label or device name (produced with dev_name(dev) if con_id is > > NULL. Still, not changes from using swnodes compared to ACPI or DT. > > Yes, can you add a summary of above to the commit message? > > > > > > > { } > > > > > > }; > > ... > > > > > > > + /* > > > > > > + * We expect all swnode-described GPIOs have GPIO number and > > > > > > + * polarity arguments, hence nargs is set to 2. > > > > > > + */ > > > > > > > > > > Maybe instead you can provide a custom macro wrapper that will check the number > > > > > of arguments at compile time? > > > > > > > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF() > > > > that enforces needed arguments. > > > > > > Yes, that's what I meant. > > > > Where do you think it should go? Not sure if I want to pollute > > property.h, I guess linux/gpio/matchine.h will need to include > > property.h? > > Good question. I was thinking more of a separate header for that, > because adding property.h adds tons of stuff which might be not > needed otherwise. OK, I guess include/linux/gpio/property.h ? > > ... > > > > > > > + /* > > > > > > + * First look up GPIO in the secondary software node in case > > > > > > + * it was used to store updated properties. > > > > > > > > > > Why this is done first? We don't try secondary before we have checked primary. > > > > > > > > I believe we should check secondary first, so that secondaries can be > > > > used not only to add missing properties, but also to override existing > > > > ones in case they are incorrect. > > > > > > It contradicts all code we have in the kernel regarding the use of software > > > nodes, you need very strong argument to justify that. > > > > > > Personally I think this must be fixed. > > > > I agree, the rest of the code should be fixed ;) I'll put it on my TODO > > list. > > I'm not sure what "rest of the code" you are referring to. The core part of > device property APIs? Yes. > > > I gave my argument above already: swnodes should not only be useful to > > add missing properties, but also allow fixing up existing ones. If I > > implemented what you are suggesting then I would not be able to create > > this concise example and would need to model entire DT node for GPIO > > keys. > > Why do you need that in the first place? We should not use swnodes as primary > source of the information. The auxiliary one is fine. "Fixing" via priority > inversion in current model is not good thing to have. > > If you really need that you have to first do the following: > - convert fwnode to be a list node > - unembed it from struct device (leaving only head of list there > - update all necessary APIs respectively > > In such implementation list_add() / list_add_tail() will define a priority > and you may have stack of properties. Hmm, that will complicate things quite a bit. I wonder why you think that using swnodes to fix up the "bad" firmware data is not desirable. Swnodes are controlled by the kernel and thus we can potentially allow users tweak them from usersoace. There is a desire to allow easier access to various driver's parameters - see for example Hans patches to Goodix and Silead where he adds code that intercepts reading of device properties and instead gets data form module parameter - I would like to have such facility in more general way. https://lore.kernel.org/all/20221025122930.421377-3-hdegoede@redhat.com/ > > Doing it in a hackish way by allowing priority inversion in _some_ APIs > is no go in my opinion. Yes, I agree that we want to have all APIs behave in the similar way. It occurred to me that the topic of handling secondary node is actually separate from swnode hanlding, so I will remove it from this patch and I can start a separate thread/patches for it after we land this series. Thanks.
On Mon, Nov 07, 2022 at 08:12:32AM -0800, Dmitry Torokhov wrote: > On Mon, Nov 07, 2022 at 01:08:09PM +0200, Andy Shevchenko wrote: > > On Fri, Nov 04, 2022 at 09:48:47PM -0700, Dmitry Torokhov wrote: > > > On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote: > > > > On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote: > > > > > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote: > > > > > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote: ... > > > > > > > + /* > > > > > > > + * We expect all swnode-described GPIOs have GPIO number and > > > > > > > + * polarity arguments, hence nargs is set to 2. > > > > > > > + */ > > > > > > > > > > > > Maybe instead you can provide a custom macro wrapper that will check the number > > > > > > of arguments at compile time? > > > > > > > > > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF() > > > > > that enforces needed arguments. > > > > > > > > Yes, that's what I meant. > > > > > > Where do you think it should go? Not sure if I want to pollute > > > property.h, I guess linux/gpio/matchine.h will need to include > > > property.h? > > > > Good question. I was thinking more of a separate header for that, > > because adding property.h adds tons of stuff which might be not > > needed otherwise. > > OK, I guess include/linux/gpio/property.h ? Why not? ... > > > > > > > + /* > > > > > > > + * First look up GPIO in the secondary software node in case > > > > > > > + * it was used to store updated properties. > > > > > > > > > > > > Why this is done first? We don't try secondary before we have checked primary. > > > > > > > > > > I believe we should check secondary first, so that secondaries can be > > > > > used not only to add missing properties, but also to override existing > > > > > ones in case they are incorrect. > > > > > > > > It contradicts all code we have in the kernel regarding the use of software > > > > nodes, you need very strong argument to justify that. > > > > > > > > Personally I think this must be fixed. > > > > > > I agree, the rest of the code should be fixed ;) I'll put it on my TODO > > > list. > > > > I'm not sure what "rest of the code" you are referring to. The core part of > > device property APIs? > > Yes. > > > I gave my argument above already: swnodes should not only be useful to > > > add missing properties, but also allow fixing up existing ones. If I > > > implemented what you are suggesting then I would not be able to create > > > this concise example and would need to model entire DT node for GPIO > > > keys. > > > > Why do you need that in the first place? We should not use swnodes as primary > > source of the information. The auxiliary one is fine. "Fixing" via priority > > inversion in current model is not good thing to have. > > > > If you really need that you have to first do the following: > > - convert fwnode to be a list node > > - unembed it from struct device (leaving only head of list there > > - update all necessary APIs respectively > > > > In such implementation list_add() / list_add_tail() will define a priority > > and you may have stack of properties. > > Hmm, that will complicate things quite a bit. I wonder why you think > that using swnodes to fix up the "bad" firmware data is not desirable. I put there "in current model". It means that for secondary-primary and only them that design doesn't allow to create quirks. > Swnodes are controlled by the kernel and thus we can potentially allow > users tweak them from usersoace. There is a desire to allow easier > access to various driver's parameters - see for example Hans patches to > Goodix and Silead where he adds code that intercepts reading of device > properties and instead gets data form module parameter - I would like to > have such facility in more general way. > > https://lore.kernel.org/all/20221025122930.421377-3-hdegoede@redhat.com/ How can you guarantee that flip-flopping priority of reading properties doesn't break things? Moreover, what problem we are trying to hack up? The DT should be fixed in DT. ACPI? In ACPI properties are not that common, and even that, we shouldn't unleash vendors to make all possible abuse-like mistakes in ACPI, that's why I do not think that allowing property quirks is a good idea at all. > > Doing it in a hackish way by allowing priority inversion in _some_ APIs > > is no go in my opinion. > > Yes, I agree that we want to have all APIs behave in the similar way. It > occurred to me that the topic of handling secondary node is actually > separate from swnode hanlding, so I will remove it from this patch and I > can start a separate thread/patches for it after we land this series. As long as the priority is out of the picture I'm fine with this series, Because as I said the current design seems (I wasn't the author, so I might be mistaken) had been made with "we trust the firmware" in mind.
On Mon, Nov 07, 2022 at 10:59:39PM +0200, Andy Shevchenko wrote: > On Mon, Nov 07, 2022 at 08:12:32AM -0800, Dmitry Torokhov wrote: ... > > Swnodes are controlled by the kernel and thus we can potentially allow > > users tweak them from usersoace. There is a desire to allow easier > > access to various driver's parameters - see for example Hans patches to > > Goodix and Silead where he adds code that intercepts reading of device > > properties and instead gets data form module parameter - I would like to > > have such facility in more general way. > > > > https://lore.kernel.org/all/20221025122930.421377-3-hdegoede@redhat.com/ > > How can you guarantee that flip-flopping priority of reading properties doesn't > break things? > > Moreover, what problem we are trying to hack up? The DT should be fixed in DT. > ACPI? In ACPI properties are not that common, and even that, we shouldn't unleash > vendors to make all possible abuse-like mistakes in ACPI, that's why I do not think > that allowing property quirks is a good idea at all. To clarify. In the context when we consider the reversed priority of their importance. That said, that the "quirk first, firmware later" is NAK by me, while "firmware first, quirk latter" is pretty much fine.
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 8629e9eaf79e..010587025fc8 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_OF_GPIO) += gpiolib-of.o obj-$(CONFIG_GPIO_CDEV) += gpiolib-cdev.o obj-$(CONFIG_GPIO_SYSFS) += gpiolib-sysfs.o obj-$(CONFIG_GPIO_ACPI) += gpiolib-acpi.o +obj-$(CONFIG_GPIOLIB) += gpiolib-swnode.o # Device drivers. Generally keep list sorted alphabetically obj-$(CONFIG_GPIO_REGMAP) += gpio-regmap.o diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c new file mode 100644 index 000000000000..d005ce0b986d --- /dev/null +++ b/drivers/gpio/gpiolib-swnode.c @@ -0,0 +1,106 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Software Node helpers for the GPIO API + * + * Copyright 2022 Google LLC + */ +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/gpio/consumer.h> + +#include "gpiolib.h" +#include "gpiolib-swnode.h" + +static int swnode_gpiochip_match_name(struct gpio_chip *chip, void *data) +{ + return !strcmp(chip->label, data); +} + +struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode, + const char *con_id, unsigned int idx, + unsigned long *flags) +{ + const struct software_node *chip_node; + const struct software_node *swnode; + struct fwnode_reference_args args; + struct gpio_chip *chip; + struct gpio_desc *desc; + char prop_name[32]; /* 32 is max size of property name */ + int error; + + swnode = to_software_node(fwnode); + if (!swnode) + return ERR_PTR(-EINVAL); + + /* + * Note we do not need to try both -gpios and -gpio suffixes, + * as, unlike OF and ACPI, we can fix software nodes to conform + * to the proper binding. + */ + if (con_id) + snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id); + else + strscpy(prop_name, "gpios", sizeof(prop_name)); + + /* + * We expect all swnode-described GPIOs have GPIO number and + * polarity arguments, hence nargs is set to 2. + */ + error = fwnode_property_get_reference_args(fwnode, prop_name, NULL, + 2, idx, &args); + if (error) { + pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n", + __func__, prop_name, fwnode, idx); + return ERR_PTR(error); + } + + chip_node = to_software_node(args.fwnode); + if (!chip_node || !chip_node->name) + return ERR_PTR(-EINVAL); + + chip = gpiochip_find((void *)chip_node->name, + swnode_gpiochip_match_name); + if (!chip) + return ERR_PTR(-EPROBE_DEFER); + + desc = gpiochip_get_desc(chip, args.args[0]); + *flags = args.args[1]; /* We expect native GPIO flags */ + + pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n", + __func__, prop_name, fwnode, idx, PTR_ERR_OR_ZERO(desc)); + + return desc; +} + +/** + * swnode_gpio_count - count the GPIOs associated with a device / function + * @fwnode: firmware node of the GPIO consumer, can be %NULL for + * system-global GPIOs + * @con_id: function within the GPIO consumer + * + * Return: + * The number of GPIOs associated with a device / function or %-ENOENT, + * if no GPIO has been assigned to the requested function. + */ +int swnode_gpio_count(struct fwnode_handle *fwnode, const char *con_id) +{ + struct fwnode_reference_args args; + char prop_name[32]; + int count; + + if (con_id) + snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id); + else + strscpy(prop_name, "gpios", sizeof(prop_name)); + + /* + * This is not very efficient, but GPIO lists usually have only + * 1 or 2 entries. + */ + count = 0; + while (fwnode_property_get_reference_args(fwnode, prop_name, NULL, + 0, count, &args) == 0) + count++; + + return count ? count : -ENOENT; +} diff --git a/drivers/gpio/gpiolib-swnode.h b/drivers/gpio/gpiolib-swnode.h new file mode 100644 index 000000000000..afd51c9b6e34 --- /dev/null +++ b/drivers/gpio/gpiolib-swnode.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef GPIOLIB_SWNODE_H +#define GPIOLIB_SWNODE_H + +struct fwnode_handle; + +struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode, + const char *con_id, unsigned int idx, + unsigned long *flags); +int swnode_gpio_count(struct fwnode_handle *fwnode, const char *con_id); + +#endif /* GPIOLIB_SWNODE_H */ diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 79aaba693c4f..b9976485587d 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -26,6 +26,7 @@ #include "gpiolib.h" #include "gpiolib-of.h" #include "gpiolib-acpi.h" +#include "gpiolib-swnode.h" #include "gpiolib-cdev.h" #include "gpiolib-sysfs.h" @@ -3813,6 +3814,19 @@ static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode, dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n", con_id, fwnode_get_name(fwnode)); + /* + * First look up GPIO in the secondary software node in case + * it was used to store updated properties. + */ + if (is_software_node(fwnode->secondary)) { + dev_dbg(consumer, + "using secondary software node for GPIO lookup\n"); + desc = swnode_find_gpio(fwnode->secondary, + con_id, idx, lookupflags); + if (!gpiod_not_found(desc)) + return desc; + } + /* Using device tree? */ if (is_of_node(fwnode)) { dev_dbg(consumer, "using device tree for GPIO lookup\n"); @@ -3821,6 +3835,9 @@ static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode, } else if (is_acpi_node(fwnode)) { dev_dbg(consumer, "using ACPI for GPIO lookup\n"); desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags); + } else if (is_software_node(fwnode)) { + dev_dbg(consumer, "using software node for GPIO lookup\n"); + desc = swnode_find_gpio(fwnode, con_id, idx, lookupflags); } return desc; @@ -3933,13 +3950,27 @@ EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index); */ int gpiod_count(struct device *dev, const char *con_id) { - const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL; - int count = -ENOENT; + struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL; + int count; + + /* + * First look up GPIO in the secondary software node in case + * it was used to store updated properties. + */ + if (!IS_ERR_OR_NULL(fwnode) && is_software_node(fwnode->secondary)) { + count = swnode_gpio_count(fwnode->secondary, con_id); + if (count > 0) + return count; + } if (is_of_node(fwnode)) count = of_gpio_get_count(dev, con_id); else if (is_acpi_node(fwnode)) count = acpi_gpio_count(dev, con_id); + else if (is_software_node(fwnode)) + count = swnode_gpio_count(fwnode, con_id); + else + count = -ENOENT; if (count < 0) count = platform_gpio_count(dev, con_id);
Now that static device properties understand notion of child nodes and references, let's teach gpiolib to handle them: - GPIOs are represented as a references to software nodes representing gpiochip - references must have 2 arguments - GPIO number within the chip and GPIO flags (GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, etc). - name of the software node representing gpiochip must match label of the gpiochip, as we use it to locate gpiochip structure at runtime. const struct software_node gpio_bank_b_node = { .name = "B", }; const struct property_entry simone_key_enter_props[] __initconst = { PROPERTY_ENTRY_U32("linux,code", KEY_ENTER), PROPERTY_ENTRY_STRING("label", "enter"), PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW), { } }; Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/gpio/Makefile | 1 + drivers/gpio/gpiolib-swnode.c | 106 ++++++++++++++++++++++++++++++++++++++++++ drivers/gpio/gpiolib-swnode.h | 13 ++++++ drivers/gpio/gpiolib.c | 35 +++++++++++++- 4 files changed, 153 insertions(+), 2 deletions(-)