Re: [RFC PATCH] i2c: skip of_i2c_register_device() for invalid child nodes

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 10/31/2024 12:30 PM, Dmitry Baryshkov wrote:
> On Thu, Oct 31, 2024 at 11:45:53AM -0700, Abhinav Kumar wrote:
>>
>>
>> On 10/31/2024 11:23 AM, Dmitry Baryshkov wrote:
>>> On Wed, 30 Oct 2024 at 03:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> of_i2c_register_devices() adds all child nodes of a given i2c bus
>>>> however in certain device trees of_alias_from_compatible() and
>>>> of_property_read_u32() can fail as the child nodes of the device
>>>> might not be valid i2c client devices. One such example is the
>>>> i2c aux device for the DRM MST toplogy manager which uses the
>>>> display controller device node to add the i2c adaptor [1] leading
>>>> to an error spam like below
>>>>
>>>> i2c i2c-20: of_i2c: register /soc@0/display-subsystem@ae00000/display-controller@ae01000/ports
>>>> i2c i2c-20: of_i2c: modalias failure on /soc@0/display-subsystem@ae00000/display-controller@ae01000/ports
>>>> i2c i2c-20: Failed to create I2C device for /soc@0/display-subsystem@ae00000/display-controller@ae01000/ports
>>>> i2c i2c-20: of_i2c: register /soc@0/display-subsystem@ae00000/display-controller@ae01000/opp-table
>>>> i2c i2c-20: of_i2c: invalid reg on /soc@0/display-subsystem@ae00000/display-controller@ae01000/opp-table
>>>> i2c i2c-20: Failed to create I2C device for /soc@0/display-subsystem@ae00000/display-controller@ae01000/opp-table
>>>>
>>>> Add protection against invalid child nodes before trying to register
>>>> i2c devices for all child nodes.
>>>>
>>>> [1] : https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/display/drm_dp_mst_topology.c#L5985
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>    drivers/i2c/i2c-core-of.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
>>>> index a6c407d36800..62a2603c3092 100644
>>>> --- a/drivers/i2c/i2c-core-of.c
>>>> +++ b/drivers/i2c/i2c-core-of.c
>>>> @@ -86,6 +86,8 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>>>>    {
>>>>           struct device_node *bus, *node;
>>>>           struct i2c_client *client;
>>>> +       u32 addr;
>>>> +       char temp[16];
>>>>
>>>>           /* Only register child devices if the adapter has a node pointer set */
>>>>           if (!adap->dev.of_node)
>>>> @@ -101,6 +103,10 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>>>>                   if (of_node_test_and_set_flag(node, OF_POPULATED))
>>>>                           continue;
>>>>
>>>> +               if (of_property_read_u32(node, "reg", &addr) ||
>>>> +                   of_alias_from_compatible(node, temp, sizeof(temp)))
>>>> +                       continue;
>>>
>>> I think just of_property_read_u32() should be enough to skip
>>> non-I2C-device children. If of_alias_from_compatible() fails, it is a
>>> legit error.
>>>
>>
>> Thanks for the review.
>>
>> of_alias_from_compatible() looks for a compatible string but all child nodes
>> such as ports will not have the compatible. Hence below error will still be
>> seen:
>>
>> i2c i2c-20: of_i2c: modalias failure on
>> /soc@0/display-subsystem@ae00000/display-controller@ae01000/ports
> 
> But ports node don't have a reg property too, so it should be skipped
> based on that.
> 

hmmm this is a good point. I see that individual port@ nodes do have a 
reg but ports node does not.

I will re-test this once without the of_alias_from_compatible() and drop 
the of_alias_from_compatible in v2.

>>
>>>> +
>>>>                   client = of_i2c_register_device(adap, node);
>>>>                   if (IS_ERR(client)) {
>>>>                           dev_err(&adap->dev,
>>>> --
>>>> 2.34.1
>>>>
>>>
>>>
>