Re: [PATCH v11] i2c: designware: defer probe if child GpioInt controllers are not bound

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Jul 15, 2026 at 05:17:01PM +0530, Hardik Prakash wrote:
> I2C controllers may have child devices with GpioInt resources that
> depend on GPIO controllers being fully initialized. If the I2C
> controller probes and enumerates children before the referenced GPIO
> controller has completed probe, GPIO interrupts may not be properly
> configured, leading to device failures.
> 
> On Lenovo Yoga 7 14AGP11, the WACF2200 touchscreen (child of
> AMDI0010:02) has a GpioInt resource pointing to GPIO 157 on the
> pinctrl-amd controller (AMDI0030:00). When i2c-designware probes
> AMDI0010:02 before pinctrl-amd finishes initializing, I2C transactions
> fail with lost arbitration errors:
> 
>   0.285952  amd_gpio_probe: registering gpiochip  <- GPIO chip visible
>   0.287121  amd_gpio_probe: requesting parent IRQ <- probe still running
>   0.301454  AMDI0010:02 dw_i2c_plat_probe: start  <- races here
>   2.348157  lost arbitration
> 
> Add a dependency check that walks ACPI child devices and defers probe
> until any referenced GPIO controller is bound.

...

> v10 -> v11:
> - Replaced custom gpio_controller_ref list with gpio_device_find_by_fwnode(),
>   as suggested by Andy, dropping the linked list and dedup logic (~60 lines)
> - Moved resource-skip explanation from commit message into a code comment
> - Fixed device_is_bound() to check gpio_device_to_device(gdev)->parent
>   rather than the gpio_device's own internal class device, which never
>   has a driver bound to it

Thanks for an update, looks much better!

My comments below. I think v12 will be final if you address everything as
suggested and answer to the Bart's request.

...

> +static int check_gpioint_resource(struct acpi_resource *ares, void *data)
> +{

> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;

This style is discouraged. Please, move this below.

> +	struct acpi_resource_gpio *agpio;
> +	struct acpi_device *gpio_adev;
> +	struct device *gpio_dev;
> +	acpi_handle handle;
> +
> +	if (!acpi_gpio_get_irq_resource(ares, &agpio))
> +		return 1; /* not a GpioInt resource, skip */
> +
> +	if (!agpio->resource_source.string_length)
> +		return 1; /* no named controller, skip */

> +	if (ACPI_FAILURE(acpi_get_handle(NULL, agpio->resource_source.string_ptr, &handle)))
> +		return 1;

Can be rewritten as

	acpi_status status;
	...

	status = acpi_get_handle(NULL, agpio->resource_source.string_ptr, &handle);
	if (ACPI_FAILURE(status))
		return 1;

> +	gpio_adev = acpi_fetch_acpi_dev(handle);
> +	if (!gpio_adev)
> +		return 1;
> +
> +	gdev = gpio_device_find_by_fwnode(acpi_fwnode_handle(gpio_adev));

	struct gpio_device *gdev __free(gpio_device_put) =
		gpio_device_find_by_fwnode(acpi_fwnode_handle(gpio_adev));

> +	if (!gdev)
> +		return -EPROBE_DEFER; /* controller not registered yet: abort walk */
> +
> +	gpio_dev = gpio_device_to_device(gdev)->parent;

> +	scoped_guard(device, gpio_dev) {

Can we use simple guard()() here?

> +		if (!device_is_bound(gpio_dev))
> +			return -EPROBE_DEFER; /* controller not bound yet: abort walk */
> +	}
> +
> +	return 1; /* bound, skip adding to resource list, continue walk */
> +}

...

> +static int check_child_gpioint(struct acpi_device *adev, void *data)
> +{
> +	struct list_head res_list;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&res_list);

Instead just define with LIST_HEAD() macro.

	LIST_HEAD(res_list);
	int ret;

> +	ret = acpi_dev_get_resources(adev, &res_list, check_gpioint_resource, NULL);

> +	acpi_dev_free_resource_list(&res_list);
> +
> +	/*
> +	 * ret is a nonnegative resource count on success, which must not
> +	 * be mistaken for a nonzero "stop iteration" signal by
> +	 * acpi_dev_for_each_child(); only forward genuine errors.
> +	 */
> +	return ret < 0 ? ret : 0;

Check for errors when it's required. I believe you fixed it at some point
and now it's again old version. Please, reduce full rely on AI, it's not
helpful sometimes.

	ret = acpi_dev_get_resources(adev, &res_list, check_gpioint_resource, NULL);
	if (ret < 0)
		return ret;
	...
	return 0;

> +}

...

> +static int i2c_dw_check_gpio_dependencies(struct device *dev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);

This style is discouraged as it brings an unneeded burden on a maintenance.

> +	if (!adev)
> +		return 0;

	struct acpi_device *adev;

	adev = ACPI_COMPANION(dev);
	if (!adev)
		return 0;

> +	return acpi_dev_for_each_child(adev, check_child_gpioint, NULL);
> +}

-- 
With Best Regards,
Andy Shevchenko