Re: [PATCH v2 01/11] platform/x86: x86-android-tablets: convert Goodix devices to GPIO references

[Hans de Goede <hansg@kernel.org>]

Hi,

On 11-Aug-25 4:22 AM, Dmitry Torokhov wrote:
> Now that gpiolib supports software nodes to describe GPIOs, switch the
> driver away from using GPIO lookup tables for Goodix touchscreens to
> using PROPERTY_ENTRY_GPIO() to keep all touchscreen properties together.
> 
> Since the tablets are using either Baytrail or Cherryview GPIO
> controllers x86_dev_info structure has been extended to carry gpiochip
> type information so that the code can instantiate correct set of
> software nodes representing the GPIO chip.
> 
> Because this adds a new point of failure in x86_android_tablet_probe(),
> x86_android_tablet_remove() is rearranged to handle cases where battery
> swnode has not been registered yet, and registering of GPIO lookup
> tables is moved earlier as it can not fail.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

So I was curious and took a quick peek at the code, mainly at
the core changes.

...

> diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
> index 2a9c47178505..b0d63d3c05cd 100644
> --- a/drivers/platform/x86/x86-android-tablets/core.c
> +++ b/drivers/platform/x86/x86-android-tablets/core.c
> @@ -155,6 +155,7 @@ static struct serdev_device **serdevs;
>  static struct gpio_keys_button *buttons;
>  static struct gpiod_lookup_table * const *gpiod_lookup_tables;
>  static const struct software_node *bat_swnode;
> +static const struct software_node **gpiochip_node_group;
>  static void (*exit_handler)(void);
>  
>  static __init struct i2c_adapter *
> @@ -331,6 +332,34 @@ static __init int x86_instantiate_serdev(const struct x86_dev_info *dev_info, in
>  	return ret;
>  }
>  
> +const struct software_node baytrail_gpiochip_nodes[] = {
> +	{ .name = "INT33FC:00" },
> +	{ .name = "INT33FC:01" },
> +	{ .name = "INT33FC:02" },
> +};

I'm afraid that just setting the names here, and then
registering the node group below is not enough, see
the comment below.


> +
> +static const struct software_node *baytrail_gpiochip_node_group[] = {
> +	&baytrail_gpiochip_nodes[0],
> +	&baytrail_gpiochip_nodes[1],
> +	&baytrail_gpiochip_nodes[2],
> +	NULL
> +};

...

> @@ -361,10 +390,14 @@ static void x86_android_tablet_remove(struct platform_device *pdev)
>  	if (exit_handler)
>  		exit_handler();
>  
> +	if (bat_swnode)
> +		software_node_unregister(bat_swnode);
> +
> +	if (gpiochip_node_group)
> +		software_node_unregister_node_group(gpiochip_node_group);
> +
>  	for (i = 0; gpiod_lookup_tables && gpiod_lookup_tables[i]; i++)
>  		gpiod_remove_lookup_table(gpiod_lookup_tables[i]);
> -
> -	software_node_unregister(bat_swnode);
>  }
>  
>  static __init int x86_android_tablet_probe(struct platform_device *pdev)
> @@ -388,16 +421,36 @@ static __init int x86_android_tablet_probe(struct platform_device *pdev)
>  	for (i = 0; dev_info->modules && dev_info->modules[i]; i++)
>  		request_module(dev_info->modules[i]);
>  
> -	bat_swnode = dev_info->bat_swnode;
> -	if (bat_swnode) {
> -		ret = software_node_register(bat_swnode);
> +	gpiod_lookup_tables = dev_info->gpiod_lookup_tables;
> +	for (i = 0; gpiod_lookup_tables && gpiod_lookup_tables[i]; i++)
> +		gpiod_add_lookup_table(gpiod_lookup_tables[i]);
> +
> +	switch (dev_info->gpiochip_type) {
> +	case X86_GPIOCHIP_BAYTRAIL:
> +		gpiochip_node_group = baytrail_gpiochip_node_group;
> +		break;
> +	case X86_GPIOCHIP_CHERRYVIEW:
> +		gpiochip_node_group = cherryview_gpiochip_node_group;
> +		break;
> +	case X86_GPIOCHIP_UNSPECIFIED:
> +		gpiochip_node_group = NULL;
> +		break;
> +	}
> +
> +	if (gpiochip_node_group) {
> +		ret = software_node_register_node_group(gpiochip_node_group);
>  		if (ret)
>  			return ret;
>  	}

As mentioned above just registering the node group here is not enough,
the nodes need to actually be assigned to the platform-devices which
are the parents of the GPIO controller, something like this from
a recent patch of mine which is not upstream yet:

static int __init acer_a1_840_init(struct device *dev)
{
        int ret;

        acer_a1_840_fg_dev = bus_find_device_by_name(&platform_bus_type, NULL, "chtdc_ti_batterry");
        if (!acer_a1_840_fg_dev)
                return dev_err_probe(dev, -EPROBE_DEFER, "getting chtdc_ti_battery dev\n");

        acer_a1_840_fg_node = fwnode_create_software_node(acer_a1_840_fg_props, NULL);
        if (IS_ERR(acer_a1_840_fg_node)) {
                ret = PTR_ERR(acer_a1_840_fg_node);
                goto err_put;
        }

        ret = device_add_software_node(acer_a1_840_fg_dev,
                                       to_software_node(acer_a1_840_fg_node));
        if (ret)
                goto err_put;

Except that this will only work if the GPIO controller gets probed()
after assigning the swnodes. Otherwise the GPIO controller itself will
not have the fwnode_handle inside struct gpio_chip set ...

Oh wait, that fwnode will already be set, it will point to the ACPI
fwnode. So setting the swnode on the platform device will likely work,
because that will be assigned to fwnode_handle->secondary and
the fwnode_handle pointer is shared between the platform-device
and the gpiochip, so adding the software_node to the platform-device
will also set fwnode_handle->secondary for the gpiochio (as it is
the same fwnode_handle).

So thinking more about it calling device_add_software_node() on
the GPIO controller parent platform-device doing something like\
the above should work ...

But only calling software_node_register_node_group() definitely
is not enough.

Regards,

Hans