[6/6] gpiolib: add support for software nodes

  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(-)
  

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.