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

[Andy Shevchenko]

On Thu, Apr 30, 2026 at 09:34:06AM +0200, 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.
> 
> The driver uses platform_create_bundle() and expects all required
> providers to be present before it itself is probed. We know the name of
> the device we're waiting for so look them up and assign the appropriate
> software node as the secondary firmware node of the underlying ACPI node.
> 
> Scheduling fine-grained devres actions allows for proper teardown and
> unsetting of the secondary firmware nodes.

...

> +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;
> +}

Why don't we use set_secondary_fwnode() in the above functions?
Drivers are not supposed to know the guts of the fwnode implementation.

...

> +static int auto_secondary_fwnode_init(struct device *parent)
> +{
> +	const struct software_node *const *swnode;
> +	int ret;
> +
> +	if (!gpiochip_node_group)
> +		return 0;
> +
> +	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;
> +
> +	for (swnode = gpiochip_node_group; *swnode; swnode++) {
> +		struct device *dev __free(put_device) =
> +				acpi_bus_find_device_by_name((*swnode)->name);
> +		if (!dev) {
> +			dev_err(parent, "Failed to find the required GPIO controller: %s\n",
> +				(*swnode)->name);

swnode at this point is registered, meaning we have an associated fwnode
handle, hence why not use %pfwP here?

> +			return -ENODEV;
> +		}
> +
> +		ret = acpi_set_secondary_fwnode(parent, dev, *swnode);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko