In preparation of dropping support for legacy GPIO API from gpio-keys
switch the driver to use software nodes/properties to describe
GPIO-connected LED and button.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

This is untested - no hardware.

 drivers/platform/x86/barco-p50-gpio.c | 104 +++++++++++++++-----------
 1 file changed, 62 insertions(+), 42 deletions(-)

diff --git a/drivers/platform/x86/barco-p50-gpio.c b/drivers/platform/x86/barco-p50-gpio.c
index 28012eebdb10..6f13e81f98fb 100644
--- a/drivers/platform/x86/barco-p50-gpio.c
+++ b/drivers/platform/x86/barco-p50-gpio.c
@@ -11,6 +11,7 @@
 #define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
 
 #include <linux/delay.h>
+#include <linux/dev_printk.h>
 #include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -18,10 +19,11 @@
 #include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/gpio_keys.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
-#include <linux/input.h>
+#include <linux/gpio/property.h>
+#include <linux/input-event-codes.h>
+#include <linux/property.h>
 
 
 #define DRIVER_NAME		"barco-p50-gpio"
@@ -78,44 +80,57 @@ static const char * const gpio_names[] = {
 	[P50_GPIO_LINE_BTN] = "identify-button",
 };
 
-
-static struct gpiod_lookup_table p50_gpio_led_table = {
-	.dev_id = "leds-gpio",
-	.table = {
-		GPIO_LOOKUP_IDX(DRIVER_NAME, P50_GPIO_LINE_LED, NULL, 0, GPIO_ACTIVE_HIGH),
-		{}
-	}
+static const struct software_node gpiochip_node = {
+	.name = DRIVER_NAME,
 };
 
 /* GPIO LEDs */
-static struct gpio_led leds[] = {
-	{ .name = "identify" }
+static const struct software_node gpio_leds_node = {
+	.name = "gpio-leds-identify",
 };
 
-static struct gpio_led_platform_data leds_pdata = {
-	.num_leds = ARRAY_SIZE(leds),
-	.leds = leds,
+static const struct property_entry identify_led_props[] = {
+	PROPERTY_ENTRY_GPIO("gpios", &gpiochip_node, P50_GPIO_LINE_LED, GPIO_ACTIVE_HIGH),
+	{ }
+};
+
+static const struct software_node identify_led_node = {
+	.parent = &gpio_leds_node,
+	.name = "identify",
+	.properties = identify_led_props,
 };
 
 /* GPIO keyboard */
-static struct gpio_keys_button buttons[] = {
-	{
-		.code = KEY_VENDOR,
-		.gpio = P50_GPIO_LINE_BTN,
-		.active_low = 1,
-		.type = EV_KEY,
-		.value = 1,
-	},
+static const struct property_entry gpio_keys_props[] = {
+	PROPERTY_ENTRY_STRING("label", "identify"),
+	PROPERTY_ENTRY_U32("poll-interval", 100),
+	{ }
 };
 
-static struct gpio_keys_platform_data keys_pdata = {
-	.buttons = buttons,
-	.nbuttons = ARRAY_SIZE(buttons),
-	.poll_interval = 100,
-	.rep = 0,
-	.name = "identify",
+static const struct software_node gpio_keys_node = {
+	.name = "gpio-keys-identify",
+	.properties = gpio_keys_props,
 };
 
+static struct property_entry vendor_key_props[] = {
+	PROPERTY_ENTRY_U32("linux,code", KEY_VENDOR),
+	PROPERTY_ENTRY_GPIO("gpios", &gpiochip_node, P50_GPIO_LINE_BTN, GPIO_ACTIVE_LOW),
+	{ }
+};
+
+static const struct software_node vendor_key_node = {
+	.parent = &gpio_keys_node,
+	.properties = vendor_key_props,
+};
+
+static const struct software_node *p50_swnodes[] = {
+	&gpiochip_node,
+	&gpio_leds_node,
+	&identify_led_node,
+	&gpio_keys_node,
+	&vendor_key_node,
+	NULL
+};
 
 /* low level access routines */
 
@@ -285,6 +300,16 @@ static int p50_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
 
 static int p50_gpio_probe(struct platform_device *pdev)
 {
+	struct platform_device_info key_info = {
+		.name	= "gpio-keys-polled",
+		.id	= PLATFORM_DEVID_NONE,
+		.parent	= &pdev->dev,
+	};
+	struct platform_device_info led_info = {
+		.name	= "leds-gpio",
+		.id	= PLATFORM_DEVID_NONE,
+		.parent	= &pdev->dev,
+	};
 	struct p50_gpio *p50;
 	struct resource *res;
 	int ret;
@@ -339,25 +364,20 @@ static int p50_gpio_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	gpiod_add_lookup_table(&p50_gpio_led_table);
-
-	p50->leds_pdev = platform_device_register_data(&pdev->dev,
-		"leds-gpio", PLATFORM_DEVID_NONE, &leds_pdata, sizeof(leds_pdata));
+	ret = software_node_register_node_group(p50_swnodes);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to register software nodes");
 
+	led_info.fwnode = software_node_fwnode(&gpio_leds_node);
+	p50->leds_pdev = platform_device_register_full(&led_info);
 	if (IS_ERR(p50->leds_pdev)) {
 		ret = PTR_ERR(p50->leds_pdev);
 		dev_err(&pdev->dev, "Could not register leds-gpio: %d\n", ret);
 		goto err_leds;
 	}
 
-	/* gpio-keys-polled uses old-style gpio interface, pass the right identifier */
-	buttons[0].gpio += p50->gc.base;
-
-	p50->keys_pdev =
-		platform_device_register_data(&pdev->dev, "gpio-keys-polled",
-					      PLATFORM_DEVID_NONE,
-					      &keys_pdata, sizeof(keys_pdata));
-
+	key_info.fwnode = software_node_fwnode(&gpio_keys_node);
+	p50->keys_pdev = platform_device_register_full(&key_info);
 	if (IS_ERR(p50->keys_pdev)) {
 		ret = PTR_ERR(p50->keys_pdev);
 		dev_err(&pdev->dev, "Could not register gpio-keys-polled: %d\n", ret);
@@ -369,7 +389,7 @@ static int p50_gpio_probe(struct platform_device *pdev)
 err_keys:
 	platform_device_unregister(p50->leds_pdev);
 err_leds:
-	gpiod_remove_lookup_table(&p50_gpio_led_table);
+	software_node_unregister_node_group(p50_swnodes);
 
 	return ret;
 }
@@ -381,7 +401,7 @@ static void p50_gpio_remove(struct platform_device *pdev)
 	platform_device_unregister(p50->keys_pdev);
 	platform_device_unregister(p50->leds_pdev);
 
-	gpiod_remove_lookup_table(&p50_gpio_led_table);
+	software_node_unregister_node_group(p50_swnodes);
 }
 
 static struct platform_driver p50_gpio_driver = {
-- 
2.51.0.rc0.155.g4a0f42376b-goog


-- 
Dmitry

On Sun, 10 Aug 2025 21:31:37 -0700, Dmitry Torokhov wrote:

> In preparation of dropping support for legacy GPIO API from gpio-keys
> switch the driver to use software nodes/properties to describe
> GPIO-connected LED and button.
> 
> 


Thank you for your contribution, it has been applied to my local
review-ilpo-next branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-next branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86: barco-p50-gpio: use software nodes for gpio-leds/keys
      commit: ff289dc249fd7699081494bd70a91b5718d27114

--
 i.

On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
> In preparation of dropping support for legacy GPIO API from gpio-keys
> switch the driver to use software nodes/properties to describe
> GPIO-connected LED and button.

...

>  #include <linux/delay.h>
> +#include <linux/dev_printk.h>
>  #include <linux/dmi.h>
>  #include <linux/err.h>
>  #include <linux/io.h>

>  #include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> -#include <linux/gpio_keys.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/machine.h>
> -#include <linux/input.h>

> +#include <linux/gpio/property.h>
> +#include <linux/input-event-codes.h>
> +#include <linux/property.h>

The idea of sorting here is to have more generic first and then more specific
(per subsystem in use) groups of headers. So with your change it should look
like

#include <linux/delay.h>
#include <linux/dev_printk.h>
#include <linux/dmi.h>
#include <linux/err.h>
#include <linux/io.h>
...
#include <linux/leds.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/property.h>

#include <linux/gpio/driver.h>
#include <linux/gpio/machine.h>
#include <linux/gpio/property.h>

#include <linux/input-event-codes.h>

(I also added blank lines to make it more explicit)

...

Otherwise LGTM as here it looks like we establish platform device ourselves and
hence no need some additional magic Hans mentioned in the other series.

-- 
With Best Regards,
Andy Shevchenko

Hi Andy, Dmitry,

On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
>> In preparation of dropping support for legacy GPIO API from gpio-keys
>> switch the driver to use software nodes/properties to describe
>> GPIO-connected LED and button.
> 
> ...
> 
>>  #include <linux/delay.h>
>> +#include <linux/dev_printk.h>
>>  #include <linux/dmi.h>
>>  #include <linux/err.h>
>>  #include <linux/io.h>
> 
>>  #include <linux/leds.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> -#include <linux/gpio_keys.h>
>>  #include <linux/gpio/driver.h>
>>  #include <linux/gpio/machine.h>
>> -#include <linux/input.h>
> 
>> +#include <linux/gpio/property.h>
>> +#include <linux/input-event-codes.h>
>> +#include <linux/property.h>
> 
> The idea of sorting here is to have more generic first and then more specific
> (per subsystem in use) groups of headers. So with your change it should look
> like
> 
> #include <linux/delay.h>
> #include <linux/dev_printk.h>
> #include <linux/dmi.h>
> #include <linux/err.h>
> #include <linux/io.h>
> ...
> #include <linux/leds.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/property.h>
> 
> #include <linux/gpio/driver.h>
> #include <linux/gpio/machine.h>
> #include <linux/gpio/property.h>
> 
> #include <linux/input-event-codes.h>
> 
> (I also added blank lines to make it more explicit)
> 
> ...
> 
> Otherwise LGTM as here it looks like we establish platform device ourselves and
> hence no need some additional magic Hans mentioned in the other series.

Not entirely like with the x86-android-tablets patches this
declares a software-node for the gpiochip:

static const struct software_node gpiochip_node = {
	.name = DRIVER_NAME,
};

and registers that node, but nowhere does it actually
get assigned to the gpiochip.

This is going to need a line like this added to probe():

	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);

note the software_node_fwnode() call MUST be made after
registering the software-nodes (group).

Other then needing this single line things are indeed
much easier when the code containing the software
properties / nodes is the same code as which is
registering the gpiochip.

Regards,

Hans

On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> > On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:

...

> > Otherwise LGTM as here it looks like we establish platform device ourselves and
> > hence no need some additional magic Hans mentioned in the other series.
> 
> Not entirely like with the x86-android-tablets patches this
> declares a software-node for the gpiochip:
> 
> static const struct software_node gpiochip_node = {
> 	.name = DRIVER_NAME,
> };
> 
> and registers that node, but nowhere does it actually
> get assigned to the gpiochip.
> 
> This is going to need a line like this added to probe():
> 
> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
> 
> note the software_node_fwnode() call MUST be made after
> registering the software-nodes (group).
> 
> Other then needing this single line things are indeed
> much easier when the code containing the software
> properties / nodes is the same code as which is
> registering the gpiochip.

Ah, good point!

-- 
With Best Regards,
Andy Shevchenko

On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
> > On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> > > On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
> 
> ...
> 
> > > Otherwise LGTM as here it looks like we establish platform device ourselves and
> > > hence no need some additional magic Hans mentioned in the other series.
> > 
> > Not entirely like with the x86-android-tablets patches this
> > declares a software-node for the gpiochip:
> > 
> > static const struct software_node gpiochip_node = {
> > 	.name = DRIVER_NAME,
> > };
> > 
> > and registers that node, but nowhere does it actually
> > get assigned to the gpiochip.
> > 
> > This is going to need a line like this added to probe():
> > 
> > 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
> > 
> > note the software_node_fwnode() call MUST be made after
> > registering the software-nodes (group).
> > 
> > Other then needing this single line things are indeed
> > much easier when the code containing the software
> > properties / nodes is the same code as which is
> > registering the gpiochip.
> 
> Ah, good point!

This is wrong though, the software node need not be attached to the
gpiochip (and I wonder if it is even safe to do so). It simply provides
a name by which gpiochip is looked up in swnode_get_gpio_device().

Thanks.

-- 
Dmitry

Hi,

On 11-Aug-25 5:49 PM, Dmitry Torokhov wrote:
> On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
>> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
>>> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
>>>> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
>>
>> ...
>>
>>>> Otherwise LGTM as here it looks like we establish platform device ourselves and
>>>> hence no need some additional magic Hans mentioned in the other series.
>>>
>>> Not entirely like with the x86-android-tablets patches this
>>> declares a software-node for the gpiochip:
>>>
>>> static const struct software_node gpiochip_node = {
>>> 	.name = DRIVER_NAME,
>>> };
>>>
>>> and registers that node, but nowhere does it actually
>>> get assigned to the gpiochip.
>>>
>>> This is going to need a line like this added to probe():
>>>
>>> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
>>>
>>> note the software_node_fwnode() call MUST be made after
>>> registering the software-nodes (group).
>>>
>>> Other then needing this single line things are indeed
>>> much easier when the code containing the software
>>> properties / nodes is the same code as which is
>>> registering the gpiochip.
>>
>> Ah, good point!
> 
> This is wrong though, the software node need not be attached to the
> gpiochip (and I wonder if it is even safe to do so). It simply provides
> a name by which gpiochip is looked up in swnode_get_gpio_device().

Ah interesting. This is very different from how fwnodes generally
work though. Generally speaking when a PROPERTY_ENTRY_REF() is used
like PROPERTY_ENTRY_GPIO() does then the lookup is done by matching
the reference to the fwnode of the type of device to which the
reference points.

IOW the standard way how this works for most other subsystems
is that gpiolib-swnode.c: swnode_get_gpio_device() would call
gpio_device_find() with a compare function which uses
device_match_fwnode().

I see that instead it uses the swnode name and passes that to
gpio_device_find_by_label().

I must say that AFAIK this is not how swnodes are supposed to
be used the swnode name field is supposed to only be there
for debugging use and may normally be left empty all together.

I guess using the swnode-name + gpio_device_find_by_label()
works but it goes against the design of how fw-nodes
and especially fwnode-references are supposed to be used...

Having a fwnode reference pointing to what is in essence
a dangling (not attached to any device) fwnode is weird.

Are there already any users of PROPERTY_ENTRY_GPIO() in
the kernel? If not then I think that we should fix things
up to actually do a reference match and not a name based
lookup.

Andy IIRC you've done quite a bit of work on software-nodes,
what is your take on this ?

Note this is likely my last email in this thread for
a while since I will be traveling without email access.

Regards,

Hans

On Mon, Aug 11, 2025 at 07:40:27PM +0200, Hans de Goede wrote:
> On 11-Aug-25 5:49 PM, Dmitry Torokhov wrote:
> > On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
> >> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
> >>> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> >>>> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:

...

> >>>> Otherwise LGTM as here it looks like we establish platform device ourselves and
> >>>> hence no need some additional magic Hans mentioned in the other series.
> >>>
> >>> Not entirely like with the x86-android-tablets patches this
> >>> declares a software-node for the gpiochip:
> >>>
> >>> static const struct software_node gpiochip_node = {
> >>> 	.name = DRIVER_NAME,
> >>> };
> >>>
> >>> and registers that node, but nowhere does it actually
> >>> get assigned to the gpiochip.
> >>>
> >>> This is going to need a line like this added to probe():
> >>>
> >>> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
> >>>
> >>> note the software_node_fwnode() call MUST be made after
> >>> registering the software-nodes (group).
> >>>
> >>> Other then needing this single line things are indeed
> >>> much easier when the code containing the software
> >>> properties / nodes is the same code as which is
> >>> registering the gpiochip.
> >>
> >> Ah, good point!
> > 
> > This is wrong though, the software node need not be attached to the
> > gpiochip (and I wonder if it is even safe to do so). It simply provides
> > a name by which gpiochip is looked up in swnode_get_gpio_device().
> 
> Ah interesting. This is very different from how fwnodes generally
> work though. Generally speaking when a PROPERTY_ENTRY_REF() is used
> like PROPERTY_ENTRY_GPIO() does then the lookup is done by matching
> the reference to the fwnode of the type of device to which the
> reference points.
> 
> IOW the standard way how this works for most other subsystems
> is that gpiolib-swnode.c: swnode_get_gpio_device() would call
> gpio_device_find() with a compare function which uses
> device_match_fwnode().
> 
> I see that instead it uses the swnode name and passes that to
> gpio_device_find_by_label().
> 
> I must say that AFAIK this is not how swnodes are supposed to
> be used the swnode name field is supposed to only be there
> for debugging use and may normally be left empty all together.
> 
> I guess using the swnode-name + gpio_device_find_by_label()
> works but it goes against the design of how fw-nodes
> and especially fwnode-references are supposed to be used...
> 
> Having a fwnode reference pointing to what is in essence
> a dangling (not attached to any device) fwnode is weird.
> 
> Are there already any users of PROPERTY_ENTRY_GPIO() in
> the kernel? If not then I think that we should fix things
> up to actually do a reference match and not a name based
> lookup.

IIRC we have several users already.

> Andy IIRC you've done quite a bit of work on software-nodes,
> what is your take on this ?

I remember seeing this series that added functionality and even tried reviewing
it, but I must admit I haven't noticed this detail. I tend to agree with you
that it's better to keep handling of fwnodes uniform.

> Note this is likely my last email in this thread for
> a while since I will be traveling without email access.

-- 
With Best Regards,
Andy Shevchenko

On 11-Aug-25 7:40 PM, Hans de Goede wrote:
> Hi,
> 
> On 11-Aug-25 5:49 PM, Dmitry Torokhov wrote:
>> On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
>>> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
>>>> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
>>>>> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
>>>
>>> ...
>>>
>>>>> Otherwise LGTM as here it looks like we establish platform device ourselves and
>>>>> hence no need some additional magic Hans mentioned in the other series.
>>>>
>>>> Not entirely like with the x86-android-tablets patches this
>>>> declares a software-node for the gpiochip:
>>>>
>>>> static const struct software_node gpiochip_node = {
>>>> 	.name = DRIVER_NAME,
>>>> };
>>>>
>>>> and registers that node, but nowhere does it actually
>>>> get assigned to the gpiochip.
>>>>
>>>> This is going to need a line like this added to probe():
>>>>
>>>> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
>>>>
>>>> note the software_node_fwnode() call MUST be made after
>>>> registering the software-nodes (group).
>>>>
>>>> Other then needing this single line things are indeed
>>>> much easier when the code containing the software
>>>> properties / nodes is the same code as which is
>>>> registering the gpiochip.
>>>
>>> Ah, good point!
>>
>> This is wrong though, the software node need not be attached to the
>> gpiochip (and I wonder if it is even safe to do so). It simply provides
>> a name by which gpiochip is looked up in swnode_get_gpio_device().
> 
> Ah interesting. This is very different from how fwnodes generally
> work though. Generally speaking when a PROPERTY_ENTRY_REF() is used
> like PROPERTY_ENTRY_GPIO() does then the lookup is done by matching
> the reference to the fwnode of the type of device to which the
> reference points.
> 
> IOW the standard way how this works for most other subsystems
> is that gpiolib-swnode.c: swnode_get_gpio_device() would call
> gpio_device_find() with a compare function which uses
> device_match_fwnode().
> 
> I see that instead it uses the swnode name and passes that to
> gpio_device_find_by_label().
> 
> I must say that AFAIK this is not how swnodes are supposed to
> be used the swnode name field is supposed to only be there
> for debugging use and may normally be left empty all together.
> 
> I guess using the swnode-name + gpio_device_find_by_label()
> works but it goes against the design of how fw-nodes
> and especially fwnode-references are supposed to be used...
> 
> Having a fwnode reference pointing to what is in essence
> a dangling (not attached to any device) fwnode is weird.
> 
> Are there already any users of PROPERTY_ENTRY_GPIO() in
> the kernel? If not then I think that we should fix things
> up to actually do a reference match and not a name based
> lookup.
> 
> Andy IIRC you've done quite a bit of work on software-nodes,
> what is your take on this ?
> 
> Note this is likely my last email in this thread for
> a while since I will be traveling without email access.

p.s.

It seems that atm device_match_fwnode() only checks
that the passed in fwnode to match on matches the primary
fwnode of the device. This should be modified to also
match on the secondary node if matching the first node
fails. Like how e.g. fwnode_property_present() falls
back to checking the secondary node if the requested
property is not present in the primary fwnode.

Regards,

Hans

On Mon, Aug 11, 2025 at 07:44:01PM +0200, Hans de Goede wrote:
> On 11-Aug-25 7:40 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 11-Aug-25 5:49 PM, Dmitry Torokhov wrote:
> >> On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
> >>> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
> >>>> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> >>>>> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
> >>>
> >>> ...
> >>>
> >>>>> Otherwise LGTM as here it looks like we establish platform device ourselves and
> >>>>> hence no need some additional magic Hans mentioned in the other series.
> >>>>
> >>>> Not entirely like with the x86-android-tablets patches this
> >>>> declares a software-node for the gpiochip:
> >>>>
> >>>> static const struct software_node gpiochip_node = {
> >>>> 	.name = DRIVER_NAME,
> >>>> };
> >>>>
> >>>> and registers that node, but nowhere does it actually
> >>>> get assigned to the gpiochip.
> >>>>
> >>>> This is going to need a line like this added to probe():
> >>>>
> >>>> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
> >>>>
> >>>> note the software_node_fwnode() call MUST be made after
> >>>> registering the software-nodes (group).
> >>>>
> >>>> Other then needing this single line things are indeed
> >>>> much easier when the code containing the software
> >>>> properties / nodes is the same code as which is
> >>>> registering the gpiochip.
> >>>
> >>> Ah, good point!
> >>
> >> This is wrong though, the software node need not be attached to the
> >> gpiochip (and I wonder if it is even safe to do so). It simply provides
> >> a name by which gpiochip is looked up in swnode_get_gpio_device().
> > 
> > Ah interesting. This is very different from how fwnodes generally
> > work though. Generally speaking when a PROPERTY_ENTRY_REF() is used
> > like PROPERTY_ENTRY_GPIO() does then the lookup is done by matching
> > the reference to the fwnode of the type of device to which the
> > reference points.
> > 
> > IOW the standard way how this works for most other subsystems
> > is that gpiolib-swnode.c: swnode_get_gpio_device() would call
> > gpio_device_find() with a compare function which uses
> > device_match_fwnode().
> > 
> > I see that instead it uses the swnode name and passes that to
> > gpio_device_find_by_label().
> > 
> > I must say that AFAIK this is not how swnodes are supposed to
> > be used the swnode name field is supposed to only be there
> > for debugging use and may normally be left empty all together.

Hmm, given that I wrote both the references support for software nodes
and gpiolib-swnode.c they work exactly as I wanted them ;) Yes, in
general name is optional, but for GPIOs it is needed.

> > 
> > I guess using the swnode-name + gpio_device_find_by_label()
> > works but it goes against the design of how fw-nodes
> > and especially fwnode-references are supposed to be used...
> > 
> > Having a fwnode reference pointing to what is in essence
> > a dangling (not attached to any device) fwnode is weird.

I agree it is a bit weird, but this allows to disconnect the board file
from the GPIO driver and makes it easier to convert to device tree down
the road as it can be done in a piecemeal fashion. If you want fwnode
actually attached to the gpiochip then:

1. You can't really have static/const initializers in most of the cases
2. Fishing it out from an unrelated subsystem is much harder than
matching on a name.

> > 
> > Are there already any users of PROPERTY_ENTRY_GPIO() in
> > the kernel? If not then I think that we should fix things
> > up to actually do a reference match and not a name based
> > lookup.

I converted spitz and a few other drivers. Some of that has landed.

> > 
> > Andy IIRC you've done quite a bit of work on software-nodes,
> > what is your take on this ?
> > 
> > Note this is likely my last email in this thread for
> > a while since I will be traveling without email access.
> 
> p.s.
> 
> It seems that atm device_match_fwnode() only checks
> that the passed in fwnode to match on matches the primary
> fwnode of the device. This should be modified to also
> match on the secondary node if matching the first node
> fails. Like how e.g. fwnode_property_present() falls
> back to checking the secondary node if the requested
> property is not present in the primary fwnode.

Thanks.

-- 
Dmitry

Hi,

On 11-Aug-25 7:59 PM, Dmitry Torokhov wrote:
> On Mon, Aug 11, 2025 at 07:44:01PM +0200, Hans de Goede wrote:
>> On 11-Aug-25 7:40 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 11-Aug-25 5:49 PM, Dmitry Torokhov wrote:
>>>> On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
>>>>> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
>>>>>> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
>>>>>>> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>> Otherwise LGTM as here it looks like we establish platform device ourselves and
>>>>>>> hence no need some additional magic Hans mentioned in the other series.
>>>>>>
>>>>>> Not entirely like with the x86-android-tablets patches this
>>>>>> declares a software-node for the gpiochip:
>>>>>>
>>>>>> static const struct software_node gpiochip_node = {
>>>>>> 	.name = DRIVER_NAME,
>>>>>> };
>>>>>>
>>>>>> and registers that node, but nowhere does it actually
>>>>>> get assigned to the gpiochip.
>>>>>>
>>>>>> This is going to need a line like this added to probe():
>>>>>>
>>>>>> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
>>>>>>
>>>>>> note the software_node_fwnode() call MUST be made after
>>>>>> registering the software-nodes (group).
>>>>>>
>>>>>> Other then needing this single line things are indeed
>>>>>> much easier when the code containing the software
>>>>>> properties / nodes is the same code as which is
>>>>>> registering the gpiochip.
>>>>>
>>>>> Ah, good point!
>>>>
>>>> This is wrong though, the software node need not be attached to the
>>>> gpiochip (and I wonder if it is even safe to do so). It simply provides
>>>> a name by which gpiochip is looked up in swnode_get_gpio_device().
>>>
>>> Ah interesting. This is very different from how fwnodes generally
>>> work though. Generally speaking when a PROPERTY_ENTRY_REF() is used
>>> like PROPERTY_ENTRY_GPIO() does then the lookup is done by matching
>>> the reference to the fwnode of the type of device to which the
>>> reference points.
>>>
>>> IOW the standard way how this works for most other subsystems
>>> is that gpiolib-swnode.c: swnode_get_gpio_device() would call
>>> gpio_device_find() with a compare function which uses
>>> device_match_fwnode().
>>>
>>> I see that instead it uses the swnode name and passes that to
>>> gpio_device_find_by_label().
>>>
>>> I must say that AFAIK this is not how swnodes are supposed to
>>> be used the swnode name field is supposed to only be there
>>> for debugging use and may normally be left empty all together.
> 
> Hmm, given that I wrote both the references support for software nodes
> and gpiolib-swnode.c they work exactly as I wanted them ;) Yes, in
> general name is optional, but for GPIOs it is needed.
> 
>>>
>>> I guess using the swnode-name + gpio_device_find_by_label()
>>> works but it goes against the design of how fw-nodes
>>> and especially fwnode-references are supposed to be used...
>>>
>>> Having a fwnode reference pointing to what is in essence
>>> a dangling (not attached to any device) fwnode is weird.
> 
> I agree it is a bit weird, but this allows to disconnect the board file
> from the GPIO driver and makes it easier to convert to device tree down
> the road as it can be done in a piecemeal fashion. If you want fwnode
> actually attached to the gpiochip then:
> 
> 1. You can't really have static/const initializers in most of the cases
> 2. Fishing it out from an unrelated subsystem is much harder than
> matching on a name.

Ok lets keep using the current swnode.name based approach then.

That certainly makes things easier for the x86-android-tablets
code.

Regards,

Hans

On Tue, 12 Aug 2025, Hans de Goede wrote:

> Hi,
> 
> On 11-Aug-25 7:59 PM, Dmitry Torokhov wrote:
> > On Mon, Aug 11, 2025 at 07:44:01PM +0200, Hans de Goede wrote:
> >> On 11-Aug-25 7:40 PM, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 11-Aug-25 5:49 PM, Dmitry Torokhov wrote:
> >>>> On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
> >>>>> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
> >>>>>> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> >>>>>>> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
> >>>>>
> >>>>> ...
> >>>>>
> >>>>>>> Otherwise LGTM as here it looks like we establish platform device ourselves and
> >>>>>>> hence no need some additional magic Hans mentioned in the other series.
> >>>>>>
> >>>>>> Not entirely like with the x86-android-tablets patches this
> >>>>>> declares a software-node for the gpiochip:
> >>>>>>
> >>>>>> static const struct software_node gpiochip_node = {
> >>>>>> 	.name = DRIVER_NAME,
> >>>>>> };
> >>>>>>
> >>>>>> and registers that node, but nowhere does it actually
> >>>>>> get assigned to the gpiochip.
> >>>>>>
> >>>>>> This is going to need a line like this added to probe():
> >>>>>>
> >>>>>> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
> >>>>>>
> >>>>>> note the software_node_fwnode() call MUST be made after
> >>>>>> registering the software-nodes (group).
> >>>>>>
> >>>>>> Other then needing this single line things are indeed
> >>>>>> much easier when the code containing the software
> >>>>>> properties / nodes is the same code as which is
> >>>>>> registering the gpiochip.
> >>>>>
> >>>>> Ah, good point!
> >>>>
> >>>> This is wrong though, the software node need not be attached to the
> >>>> gpiochip (and I wonder if it is even safe to do so). It simply provides
> >>>> a name by which gpiochip is looked up in swnode_get_gpio_device().
> >>>
> >>> Ah interesting. This is very different from how fwnodes generally
> >>> work though. Generally speaking when a PROPERTY_ENTRY_REF() is used
> >>> like PROPERTY_ENTRY_GPIO() does then the lookup is done by matching
> >>> the reference to the fwnode of the type of device to which the
> >>> reference points.
> >>>
> >>> IOW the standard way how this works for most other subsystems
> >>> is that gpiolib-swnode.c: swnode_get_gpio_device() would call
> >>> gpio_device_find() with a compare function which uses
> >>> device_match_fwnode().
> >>>
> >>> I see that instead it uses the swnode name and passes that to
> >>> gpio_device_find_by_label().
> >>>
> >>> I must say that AFAIK this is not how swnodes are supposed to
> >>> be used the swnode name field is supposed to only be there
> >>> for debugging use and may normally be left empty all together.
> > 
> > Hmm, given that I wrote both the references support for software nodes
> > and gpiolib-swnode.c they work exactly as I wanted them ;) Yes, in
> > general name is optional, but for GPIOs it is needed.
> > 
> >>>
> >>> I guess using the swnode-name + gpio_device_find_by_label()
> >>> works but it goes against the design of how fw-nodes
> >>> and especially fwnode-references are supposed to be used...
> >>>
> >>> Having a fwnode reference pointing to what is in essence
> >>> a dangling (not attached to any device) fwnode is weird.
> > 
> > I agree it is a bit weird, but this allows to disconnect the board file
> > from the GPIO driver and makes it easier to convert to device tree down
> > the road as it can be done in a piecemeal fashion. If you want fwnode
> > actually attached to the gpiochip then:
> > 
> > 1. You can't really have static/const initializers in most of the cases
> > 2. Fishing it out from an unrelated subsystem is much harder than
> > matching on a name.
> 
> Ok lets keep using the current swnode.name based approach then.
> 
> That certainly makes things easier for the x86-android-tablets
> code.

Hi all,

I'm left uncertain if there are any remaining concerns with all these gpio 
conversion patches (the 3 independent ones and the larger 
x86-android-tablets series)? While I see there were a few back and forth
items between you three, it sounded like there's nothing left to do and it 
was all just based on wrong impressions/understanding, is that correct 
deduction from my part?

There was also an almost promise from Hans to test the x86-android-tablets 
series, is that still on plan/pending?

-- 
 i.

Hi Ilpo,

On 8-Sep-25 2:48 PM, Ilpo Järvinen wrote:
> On Tue, 12 Aug 2025, Hans de Goede wrote:
> 
>> Hi,
>>
>> On 11-Aug-25 7:59 PM, Dmitry Torokhov wrote:
>>> On Mon, Aug 11, 2025 at 07:44:01PM +0200, Hans de Goede wrote:
>>>> On 11-Aug-25 7:40 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 11-Aug-25 5:49 PM, Dmitry Torokhov wrote:
>>>>>> On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
>>>>>>> On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
>>>>>>>> On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
>>>>>>>>> On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
>>>>>>>
>>>>>>> ...
>>>>>>>
>>>>>>>>> Otherwise LGTM as here it looks like we establish platform device ourselves and
>>>>>>>>> hence no need some additional magic Hans mentioned in the other series.
>>>>>>>>
>>>>>>>> Not entirely like with the x86-android-tablets patches this
>>>>>>>> declares a software-node for the gpiochip:
>>>>>>>>
>>>>>>>> static const struct software_node gpiochip_node = {
>>>>>>>> 	.name = DRIVER_NAME,
>>>>>>>> };
>>>>>>>>
>>>>>>>> and registers that node, but nowhere does it actually
>>>>>>>> get assigned to the gpiochip.
>>>>>>>>
>>>>>>>> This is going to need a line like this added to probe():
>>>>>>>>
>>>>>>>> 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
>>>>>>>>
>>>>>>>> note the software_node_fwnode() call MUST be made after
>>>>>>>> registering the software-nodes (group).
>>>>>>>>
>>>>>>>> Other then needing this single line things are indeed
>>>>>>>> much easier when the code containing the software
>>>>>>>> properties / nodes is the same code as which is
>>>>>>>> registering the gpiochip.
>>>>>>>
>>>>>>> Ah, good point!
>>>>>>
>>>>>> This is wrong though, the software node need not be attached to the
>>>>>> gpiochip (and I wonder if it is even safe to do so). It simply provides
>>>>>> a name by which gpiochip is looked up in swnode_get_gpio_device().
>>>>>
>>>>> Ah interesting. This is very different from how fwnodes generally
>>>>> work though. Generally speaking when a PROPERTY_ENTRY_REF() is used
>>>>> like PROPERTY_ENTRY_GPIO() does then the lookup is done by matching
>>>>> the reference to the fwnode of the type of device to which the
>>>>> reference points.
>>>>>
>>>>> IOW the standard way how this works for most other subsystems
>>>>> is that gpiolib-swnode.c: swnode_get_gpio_device() would call
>>>>> gpio_device_find() with a compare function which uses
>>>>> device_match_fwnode().
>>>>>
>>>>> I see that instead it uses the swnode name and passes that to
>>>>> gpio_device_find_by_label().
>>>>>
>>>>> I must say that AFAIK this is not how swnodes are supposed to
>>>>> be used the swnode name field is supposed to only be there
>>>>> for debugging use and may normally be left empty all together.
>>>
>>> Hmm, given that I wrote both the references support for software nodes
>>> and gpiolib-swnode.c they work exactly as I wanted them ;) Yes, in
>>> general name is optional, but for GPIOs it is needed.
>>>
>>>>>
>>>>> I guess using the swnode-name + gpio_device_find_by_label()
>>>>> works but it goes against the design of how fw-nodes
>>>>> and especially fwnode-references are supposed to be used...
>>>>>
>>>>> Having a fwnode reference pointing to what is in essence
>>>>> a dangling (not attached to any device) fwnode is weird.
>>>
>>> I agree it is a bit weird, but this allows to disconnect the board file
>>> from the GPIO driver and makes it easier to convert to device tree down
>>> the road as it can be done in a piecemeal fashion. If you want fwnode
>>> actually attached to the gpiochip then:
>>>
>>> 1. You can't really have static/const initializers in most of the cases
>>> 2. Fishing it out from an unrelated subsystem is much harder than
>>> matching on a name.
>>
>> Ok lets keep using the current swnode.name based approach then.
>>
>> That certainly makes things easier for the x86-android-tablets
>> code.
> 
> Hi all,
> 
> I'm left uncertain if there are any remaining concerns with all these gpio 
> conversion patches (the 3 independent ones and the larger 
> x86-android-tablets series)? While I see there were a few back and forth
> items between you three, it sounded like there's nothing left to do and it 
> was all just based on wrong impressions/understanding, is that correct 
> deduction from my part?

Yes that is correct. The current patches theoretically (untested) is good
to go. The only thing which still needs to happen is test it.

> There was also an almost promise from Hans to test the x86-android-tablets 
> series, is that still on plan/pending?

Yes I hope to be able to test this the x86-android-tablets series this
week (likely in the weekend). Once that has been tested I think we can
assume that the 3 independent patches will also work and merge all of
them.

Regards,

Hans

On Mon, Aug 11, 2025 at 08:49:21AM -0700, Dmitry Torokhov wrote:
> On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
> > > On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> > > > On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:

...

> > > > Otherwise LGTM as here it looks like we establish platform device ourselves and
> > > > hence no need some additional magic Hans mentioned in the other series.
> > > 
> > > Not entirely like with the x86-android-tablets patches this
> > > declares a software-node for the gpiochip:
> > > 
> > > static const struct software_node gpiochip_node = {
> > > 	.name = DRIVER_NAME,
> > > };
> > > 
> > > and registers that node, but nowhere does it actually
> > > get assigned to the gpiochip.
> > > 
> > > This is going to need a line like this added to probe():
> > > 
> > > 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
> > > 
> > > note the software_node_fwnode() call MUST be made after
> > > registering the software-nodes (group).
> > > 
> > > Other then needing this single line things are indeed
> > > much easier when the code containing the software
> > > properties / nodes is the same code as which is
> > > registering the gpiochip.
> > 
> > Ah, good point!
> 
> This is wrong though, the software node need not be attached to the
> gpiochip (and I wonder if it is even safe to do so). It simply provides
> a name by which gpiochip is looked up in swnode_get_gpio_device().

Do we have all this being documented somewhere? Perhaps start with that?1

-- 
With Best Regards,
Andy Shevchenko

On Mon, Aug 11, 2025 at 07:01:00PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 11, 2025 at 08:49:21AM -0700, Dmitry Torokhov wrote:
> > On Mon, Aug 11, 2025 at 06:45:23PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 11, 2025 at 04:20:33PM +0200, Hans de Goede wrote:
> > > > On 11-Aug-25 2:44 PM, Andy Shevchenko wrote:
> > > > > On Sun, Aug 10, 2025 at 09:31:37PM -0700, Dmitry Torokhov wrote:
> 
> ...
> 
> > > > > Otherwise LGTM as here it looks like we establish platform device ourselves and
> > > > > hence no need some additional magic Hans mentioned in the other series.
> > > > 
> > > > Not entirely like with the x86-android-tablets patches this
> > > > declares a software-node for the gpiochip:
> > > > 
> > > > static const struct software_node gpiochip_node = {
> > > > 	.name = DRIVER_NAME,
> > > > };
> > > > 
> > > > and registers that node, but nowhere does it actually
> > > > get assigned to the gpiochip.
> > > > 
> > > > This is going to need a line like this added to probe():
> > > > 
> > > > 	p50->gc.fwnode = software_node_fwnode(&gpiochip_node);
> > > > 
> > > > note the software_node_fwnode() call MUST be made after
> > > > registering the software-nodes (group).
> > > > 
> > > > Other then needing this single line things are indeed
> > > > much easier when the code containing the software
> > > > properties / nodes is the same code as which is
> > > > registering the gpiochip.
> > > 
> > > Ah, good point!
> > 
> > This is wrong though, the software node need not be attached to the
> > gpiochip (and I wonder if it is even safe to do so). It simply provides
> > a name by which gpiochip is looked up in swnode_get_gpio_device().
> 
> Do we have all this being documented somewhere? Perhaps start with that?1

This is original commit introducing the functionality:

commit e7f9ff5dc90c3826231343439c35c6b7e9e57378
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date:   Fri Nov 11 14:19:08 2022 -0800

    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)
    - a new PROPERTY_ENTRY_GPIO() macro is supplied to ensure the above
    - name of the software node representing gpiochip must match label of
      the gpiochip, as we use it to locate gpiochip structure at runtime
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

    The following illustrates use of software nodes to describe a "System"
    button that is currently specified via use of gpio_keys_platform_data
    in arch/mips/alchemy/board-mtx1.c. It follows bindings specified in
    Documentation/devicetree/bindings/input/gpio-keys.yaml.

    static const struct software_node mxt1_gpiochip2_node = {
            .name = "alchemy-gpio2",
    };

    static const struct property_entry mtx1_gpio_button_props[] = {
            PROPERTY_ENTRY_U32("linux,code", BTN_0),
            PROPERTY_ENTRY_STRING("label", "System button"),
            PROPERTY_ENTRY_GPIO("gpios", &mxt1_gpiochip2_node, 7, GPIO_ACTIVE_LOW),
            { }
    };

    Similarly, arch/arm/mach-tegra/board-paz00.c can be converted to:

    static const struct software_node tegra_gpiochip_node = {
            .name = "tegra-gpio",
    };

    static struct property_entry wifi_rfkill_prop[] __initdata = {
            PROPERTY_ENTRY_STRING("name", "wifi_rfkill"),
            PROPERTY_ENTRY_STRING("type", "wlan"),
            PROPERTY_ENTRY_GPIO("reset-gpios",
                                &tegra_gpiochip_node, 25, GPIO_ACTIVE_HIGH);
            PROPERTY_ENTRY_GPIO("shutdown-gpios",
                                &tegra_gpiochip_node, 85, GPIO_ACTIVE_HIGH);
            { },
    };

    static struct platform_device wifi_rfkill_device = {
            .name   = "rfkill_gpio",
            .id     = -1,
    };

    ...

            software_node_register(&tegra_gpiochip_node);
            device_create_managed_software_node(&wifi_rfkill_device.dev,
                                                wifi_rfkill_prop, NULL);

I guess we could extract this into somewhere like
Documentation/driver-api/gpio/legacy-boards.rst, but given that after
initial set of conversions we should not be seeing any more users I
wonder if it is worth it.

Thanks.

-- 
Dmitry