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

On Wed, 15 Jul 2026, Bartosz Golaszewski wrote:
> Please include the entire changelog in every new iteration. I believe I gave
> my Ack for this and now it's gone. I have no idea why because there's neither
> a complete changelog nor links to previous versions.

Sorry about that, I will include the full changelog history from now on.

Here's a catch-up: since your v6, v7 Ack, the patch went through the v8
regression (NULL ptr deref + probe deferral loop), a v9 revert+fix
split, and now v11's restructure around gpio_device_find_by_fwnode()
per Andy's suggestion. The core approach (generic GpioInt dependency
check) is the same one you acked at v6.

However, given the amount that's changed since v6, I wonder if you
might want to take another look before re-acking.

Thanks,
Hardik

On Wed, 15 Jul 2026 at 19:47, Andy Shevchenko
<[email protected]> wrote:
>
> 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
>
>