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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8d754f56-0df3-4d7a-94ce-96d28f4f8003@kernel.org>
Date: Mon, 11 Aug 2025 19:45:24 +0200
From: Hans de Goede <hansg@...nel.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Arnd Bergmann <arnd@...nel.org>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 01/11] platform/x86: x86-android-tablets: convert
 Goodix devices to GPIO references

Hi,

On 11-Aug-25 6:01 PM, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Mon, Aug 11, 2025 at 12:09:18PM +0200, Hans de Goede wrote:
>> 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@...il.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.
> 
> Please see explanation below why it actually is enough.
> 
>>
>>
>>> +
>>> +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:
> 
> No, I'm afraid you misunderstand how software nodes for GPIOs work.

<snip>

Ack. I've already replied to the same remark in the
"[PATCH] platform/x86: barco-p50-gpio: use software nodes for gpio-leds/keys"
thread.

Lets continue discussing this there.

Regards,

Hans