Re: [PATCH v3 2/2] platform/x86: x86-android-tablets: enable fwnode matching of GPIO chips

[Hans de Goede]

Hi Bartosz,

On 27-Apr-26 14:19, Bartosz Golaszewski wrote:
> In order to allow GPIOLIB to match cherryview and baytrail GPIO
> controllers by their firmware nodes instead of their names, we need to
> attach the - currently "dangling" - existing software nodes to their
> target devices dynamically.
> 
> We deal with devices described in ACPI so set up a bus notifier waiting
> for the ADD events. We know the name of the device we're waiting for so
> match against it and - on match - assign the appropriate software node
> as the secondary firmware node of the underlying ACPI node. In case the
> event was emitted earlier than this driver's probe: also make sure the
> device was not added before.
> 
> Scheduling fine-grained devres actions allows for proper teardown and
> unsetting of the secondary firmware nodes.

Thank you for your work on this.

The x86-android-tablets.ko kernel module uses platform_create_bundle()
so its probe() cannot return -EPROBE_DEFER. IOW it expects all the GPIO
pins which it needs to already be there when it loads (which so far in
practice holds, since these x86 GPIO controllers are always builtin
for various reasons).

This means that there is no need all the notifier stuff. Only adding
an acpi_bus_find_device_by_name() helper as suggested by Rafael and
then finding the GPIO controllers and attaching the swnodes is
necessary.

And if the acpi_bus_find_device_by_name() fails it is ok to fail
the probe() just like it currently fails when gpiod_get() returns
-EPROBE_DEFER (or fails for other reasons).

This should nicely simplify this patch.

Regards,

Hans





> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
> ---
>  drivers/platform/x86/x86-android-tablets/core.c | 127 +++++++++++++++++++++++-
>  1 file changed, 124 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
> index 021009e9085bec3db9c4daa1f6235600210a6099..9e6e8f272dfe16cda421b569802045c3d94fc0ab 100644
> --- a/drivers/platform/x86/x86-android-tablets/core.c
> +++ b/drivers/platform/x86/x86-android-tablets/core.c
> @@ -13,10 +13,12 @@
>  #include <linux/acpi.h>
>  #include <linux/device.h>
>  #include <linux/dmi.h>
> +#include <linux/fwnode.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/irq.h>
>  #include <linux/module.h>
> +#include <linux/notifier.h>
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/serdev.h>
> @@ -360,6 +362,124 @@ static const struct software_node *cherryview_gpiochip_node_group[] = {
>  	NULL
>  };
>  
> +struct auto_secondary_data {
> +	struct notifier_block nb;
> +	struct device *parent;
> +};
> +
> +static void auto_secondary_unset(void *data)
> +{
> +	struct fwnode_handle *fwnode = data;
> +
> +	fwnode->secondary = NULL;
> +}
> +
> +static int acpi_set_secondary_fwnode(struct device *parent, struct device *dev,
> +				     const struct software_node *const swnode)
> +{
> +	struct acpi_device *device = to_acpi_device(dev);
> +	struct fwnode_handle *fwnode;
> +	int ret;
> +
> +	fwnode = software_node_fwnode(swnode);
> +	if (WARN_ON(!fwnode))
> +		return -ENOENT;
> +
> +	fwnode->secondary = ERR_PTR(-ENODEV);
> +	device->fwnode.secondary = fwnode;
> +
> +	ret = devm_add_action_or_reset(parent, auto_secondary_unset, &device->fwnode);
> +	if (ret)
> +		dev_err(parent, "Failed to schedule the unset action for secondary fwnode\n");
> +
> +	return ret;
> +}
> +
> +static int acpi_auto_secondary_notifier(struct notifier_block *nb,
> +					unsigned long action, void *data)
> +{
> +	struct auto_secondary_data *auto_sec = container_of(nb, struct auto_secondary_data, nb);
> +	const struct software_node *const *swnode;
> +	struct device *dev = data;
> +	int ret;
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		for (swnode = gpiochip_node_group; *swnode; swnode++) {
> +			if (strcmp((*swnode)->name, dev_name(dev)) == 0) {
> +				ret = acpi_set_secondary_fwnode(auto_sec->parent, dev, *swnode);
> +				return ret ? NOTIFY_BAD : NOTIFY_OK;
> +			}
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static void auto_secondary_unregister_node_group(void *data)
> +{
> +	const struct software_node **nodes = data;
> +
> +	software_node_unregister_node_group(nodes);
> +}
> +
> +static void auto_secondary_unregister_notifier(void *data)
> +{
> +	struct notifier_block *nb = data;
> +
> +	bus_unregister_notifier(&acpi_bus_type, nb);
> +}
> +
> +static int auto_secondary_fwnode_init(struct device *parent)
> +{
> +	const struct software_node *const *swnode;
> +	struct auto_secondary_data *data;
> +	int ret;
> +
> +	ret = software_node_register_node_group(gpiochip_node_group);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(parent,
> +				       auto_secondary_unregister_node_group,
> +				       gpiochip_node_group);
> +	if (ret)
> +		return ret;
> +
> +	data = devm_kzalloc(parent, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->nb.notifier_call = acpi_auto_secondary_notifier;
> +	data->parent = parent;
> +
> +	ret = bus_register_notifier(&acpi_bus_type, &data->nb);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(parent,
> +				       auto_secondary_unregister_notifier,
> +				       &data->nb);
> +	if (ret)
> +		return ret;
> +
> +	/* Device may have been already added. */
> +	for (swnode = gpiochip_node_group; *swnode; swnode++) {
> +		struct device *dev __free(put_device) =
> +			bus_find_device_by_name(&acpi_bus_type, NULL, (*swnode)->name);
> +		if (dev) {
> +			ret = acpi_set_secondary_fwnode(parent, dev, *swnode);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void x86_android_tablet_remove(struct platform_device *pdev)
>  {
>  	int i;
> @@ -391,7 +511,6 @@ static void x86_android_tablet_remove(struct platform_device *pdev)
>  
>  	software_node_unregister_node_group(gpio_button_swnodes);
>  	software_node_unregister_node_group(swnode_group);
> -	software_node_unregister_node_group(gpiochip_node_group);
>  }
>  
>  static __init int x86_android_tablet_probe(struct platform_device *pdev)
> @@ -427,9 +546,11 @@ static __init int x86_android_tablet_probe(struct platform_device *pdev)
>  		break;
>  	}
>  
> -	ret = software_node_register_node_group(gpiochip_node_group);
> -	if (ret)
> +	ret = auto_secondary_fwnode_init(&pdev->dev);
> +	if (ret) {
> +		x86_android_tablet_remove(pdev);
>  		return ret;
> +	}
>  
>  	ret = software_node_register_node_group(dev_info->swnode_group);
>  	if (ret) {
>