Re: [PATCH] of: property: add i2c-parent to fw_devlink supplier bindings

On Fri Aug 14, 2026 at 11:15 AM PDT, Rob Herring wrote:
> On Fri, Aug 14, 2026 at 01:09:49AM +0000, [email protected] wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] fw_devlink incorrectly parses I2C slave addresses as phandles due to 0-cell supplier binding definition for `i2c-parent`.
>> --
>> 
>> --- Patch [1]: [PATCH] of: property: add i2c-parent to fw_devlink supplier bindings ---
>> commit 85f34a66eac871d9e36bccb6795b94b17ab065d4
>> Author: Abdurrahman Hussain <[email protected]>
>> 
>> of: property: add i2c-parent to fw_devlink supplier bindings
>> 
>> This commit teaches fw_devlink about i2c-parent so the core creates the
>> corresponding device links. This ensures consumers are unbound before the
>> parent adapter's device, removing the need for -EPROBE_DEFER.
>> 
>> > diff --git a/drivers/of/property.c b/drivers/of/property.c
>> > --- a/drivers/of/property.c
>> > +++ b/drivers/of/property.c
>> > @@ -1409,6 +1409,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
>> >  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
>> >  DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
>> >  DEFINE_SIMPLE_PROP(panel, "panel", NULL)
>> > +DEFINE_SIMPLE_PROP(i2c_parent, "i2c-parent", NULL)
>> 
>> [Severity: High]
>> Will this correctly handle device tree bindings where the i2c-parent
>> property includes a slave address argument?
>> 
>> For example, platforms using devices like the Toshiba TC9563 PCIe switch
>> might use a property such as: i2c-parent = <&qup_i2c 0x77>;
>> 
>> By passing NULL as the cells argument to DEFINE_SIMPLE_PROP(), the parser
>> assumes a 0-cell list. When fw_devlink iterates over the property, it finds
>> the first phandle correctly, but won't it erroneously interpret the slave
>> address argument (0x77 / 119) as a second, independent 0-cell phandle?
>> 
>> If another unrelated device tree node happens to match this phandle ID,
>> could this cause fw_devlink to link the consumer to the wrong device,
>> potentially leading to spurious probe deferrals or circular dependencies?
>
> I think you need custom parsing for this one to read the first cell and 
> ignore the 2nd cell if present.
>
> Rob

Agreed. One wrinkle with unconditionally ignoring everything after the
first phandle: i2c-demux-pinctrl holds a list of several bare phandles
(i2c-parent = <&iic2>, <&i2c2>, <&gpioi2c2>;), and in the flattened
tree that's indistinguishable from tc9563's <phandle addr> form. So
always stopping at entry 0 would silently drop the demux's remaining
supplier links.

The only reliable disambiguator I found is the consumer's compatible —
toshiba,tc9563 is the one binding with a trailing slave-address cell.
I have this queued for v2:

static struct device_node *parse_i2c_parent(struct device_node *np,
                                          const char *prop_name, int index)
{
      if (strcmp(prop_name, "i2c-parent"))
              return NULL;

      /* toshiba,tc9563 is <phandle addr>; every other user is bare phandles */
      if (of_device_is_compatible(np, "toshiba,tc9563")) {
              if (index)
                      return NULL;
              return of_parse_phandle(np, prop_name, 0);
      }

      return of_parse_phandle(np, prop_name, index);
}

Is a compatible check in property.c acceptable here, or would you
rather see something else (e.g. entry 0 only, accepting the lost demux
links)? If this looks fine I'll send it as v2.

Thanks,
Abdurrahman