* [RFC PATCH] i2c: skip of_i2c_register_device() for invalid child nodes
@ 2024-10-30 1:07 Abhinav Kumar
2024-10-31 18:23 ` Dmitry Baryshkov
0 siblings, 1 reply; 5+ messages in thread
From: Abhinav Kumar @ 2024-10-30 1:07 UTC (permalink / raw)
To: Wolfram Sang
Cc: Abhinav Kumar, freedreno, dri-devel, robdclark, swboyd, airlied,
dmitry.baryshkov, quic_jesszhan, lyude, simona, linux-i2c,
linux-kernel
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;
+
client = of_i2c_register_device(adap, node);
if (IS_ERR(client)) {
dev_err(&adap->dev,
--
2.34.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH] i2c: skip of_i2c_register_device() for invalid child nodes
2024-10-30 1:07 [RFC PATCH] i2c: skip of_i2c_register_device() for invalid child nodes Abhinav Kumar
@ 2024-10-31 18:23 ` Dmitry Baryshkov
2024-10-31 18:45 ` Abhinav Kumar
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Baryshkov @ 2024-10-31 18:23 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Wolfram Sang, freedreno, dri-devel, robdclark, swboyd, airlied,
quic_jesszhan, lyude, simona, linux-i2c, linux-kernel
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.
> +
> client = of_i2c_register_device(adap, node);
> if (IS_ERR(client)) {
> dev_err(&adap->dev,
> --
> 2.34.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH] i2c: skip of_i2c_register_device() for invalid child nodes
2024-10-31 18:23 ` Dmitry Baryshkov
@ 2024-10-31 18:45 ` Abhinav Kumar
2024-10-31 19:30 ` Dmitry Baryshkov
0 siblings, 1 reply; 5+ messages in thread
From: Abhinav Kumar @ 2024-10-31 18:45 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Wolfram Sang, freedreno, dri-devel, robdclark, swboyd, airlied,
quic_jesszhan, lyude, simona, linux-i2c, linux-kernel
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
>> +
>> client = of_i2c_register_device(adap, node);
>> if (IS_ERR(client)) {
>> dev_err(&adap->dev,
>> --
>> 2.34.1
>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH] i2c: skip of_i2c_register_device() for invalid child nodes
2024-10-31 18:45 ` Abhinav Kumar
@ 2024-10-31 19:30 ` Dmitry Baryshkov
2024-10-31 20:34 ` Abhinav Kumar
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Baryshkov @ 2024-10-31 19:30 UTC (permalink / raw)
To: Abhinav Kumar
Cc: Wolfram Sang, freedreno, dri-devel, robdclark, swboyd, airlied,
quic_jesszhan, lyude, simona, linux-i2c, linux-kernel
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.
>
> > > +
> > > client = of_i2c_register_device(adap, node);
> > > if (IS_ERR(client)) {
> > > dev_err(&adap->dev,
> > > --
> > > 2.34.1
> > >
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH] i2c: skip of_i2c_register_device() for invalid child nodes
2024-10-31 19:30 ` Dmitry Baryshkov
@ 2024-10-31 20:34 ` Abhinav Kumar
0 siblings, 0 replies; 5+ messages in thread
From: Abhinav Kumar @ 2024-10-31 20:34 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Wolfram Sang, freedreno, dri-devel, robdclark, swboyd, airlied,
quic_jesszhan, lyude, simona, linux-i2c, linux-kernel
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
>>>>
>>>
>>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-10-31 20:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-30 1:07 [RFC PATCH] i2c: skip of_i2c_register_device() for invalid child nodes Abhinav Kumar
2024-10-31 18:23 ` Dmitry Baryshkov
2024-10-31 18:45 ` Abhinav Kumar
2024-10-31 19:30 ` Dmitry Baryshkov
2024-10-31 20:34 ` Abhinav Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®