| 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
| ||
|
Message-ID: <3ru23uz7mxrjlo77zgkbzdpfzkafzwxt5tvxrbeo3j3h7o2rjx@2ob5m3imsamh>
Date: Mon, 11 Aug 2025 09:01:56 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Hans de Goede <hansg@...nel.org>
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 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.
If you look into drivers/gpio/gpiolib-swnode.c, in particular at
swnode_get_gpio_device(), is uses the name from the software node to
match with the name of registered gpiochip to locate it. The node itself
does not need to be attached to the gpiochip or it's parent device, it
is simply there to establish a link between GPIO reference
(SOFTWARE_NODE_REFEREMCE) and gpiochip structure.
So PROPERTY_ENTRY_GPIO is pretty much PROPERTY_ENTRY_REF which is:
#define PROPERTY_ENTRY_REF(_name_, _ref_, ...) \
(struct property_entry) { \
.name = _name_, \
.length = sizeof(struct software_node_ref_args), \
.type = DEV_PROP_REF, \
{ .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), }, \
}
and SOFTWARE_NODE_REFERENCE is
#define SOFTWARE_NODE_REFERENCE(_ref_, ...) \
(const struct software_node_ref_args) { \
.node = _ref_, \
.nargs = COUNT_ARGS(__VA_ARGS__), \
.args = { __VA_ARGS__ }, \
}
swnode_find_gpio() scans these entries looking for the matching name,
and then resolves the references by taking the _ref_ software node,
using its name and doing:
gdev = gpio_device_find_by_label(gdev_node->name);
Where gdev_node is that potentially unattached software node and name is
"INT33FC:00" or whatever.
>
> 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.
Yes, it actually is.
Thanks.
--
Dmitry
Powered by blists - more mailing lists