[PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[Jacopo Mondi]

I2c-mux channels are created as mux siblings while they should be
children of the mux itself. Fix it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Hello,
   while inspecting child nodes of an i2c adapter it has been noted that
child devices of an i2c-mux are listed as children of the i2c adapter itself,
and not of the i2c-mux.

The hierarchy of devices looked like

-- i2c-04
--- eeprom@57
--- video_receiver@70
--- video_receiver@34
--- gmsl-deserializer@0		<-- MUX
--- gmsl-deserializer@0/i2c@0	<-- MUX CHANNEL

It now looks like

-- i2c-04
--- eeprom@57
--- video_receiver@70
--- video_receiver@34
--- gmsl-deserializer@0
---- gmsl-deserializer@0/i2c@0

v1 -> v2:
- change commit message as suggested by Geert

 drivers/i2c/i2c-mux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index 83768e8..37b7804 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	priv->adap.owner = THIS_MODULE;
 	priv->adap.algo = &priv->algo;
 	priv->adap.algo_data = priv;
-	priv->adap.dev.parent = &parent->dev;
+	priv->adap.dev.parent = muxc->dev;
 	priv->adap.retries = parent->retries;
 	priv->adap.timeout = parent->timeout;
 	priv->adap.quirks = parent->quirks;
--
2.7.4

Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[Peter Rosin]

On 2017-05-30 10:54, Jacopo Mondi wrote:
> I2c-mux channels are created as mux siblings while they should be
> children of the mux itself. Fix it.

Has this received any testing at all?

I think it will break various users of i2c_parent_is_i2c_adapter()
that expect the current situation that a i2c mux child adapter is
direct child of the parent i2c adapter. I.e. with no intermediate
device node.

Cheers,
peda

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 
> Hello,
>    while inspecting child nodes of an i2c adapter it has been noted that
> child devices of an i2c-mux are listed as children of the i2c adapter itself,
> and not of the i2c-mux.
> 
> The hierarchy of devices looked like
> 
> -- i2c-04
> --- eeprom@57
> --- video_receiver@70
> --- video_receiver@34
> --- gmsl-deserializer@0		<-- MUX
> --- gmsl-deserializer@0/i2c@0	<-- MUX CHANNEL
> 
> It now looks like
> 
> -- i2c-04
> --- eeprom@57
> --- video_receiver@70
> --- video_receiver@34
> --- gmsl-deserializer@0
> ---- gmsl-deserializer@0/i2c@0
> 
> v1 -> v2:
> - change commit message as suggested by Geert
> 
>  drivers/i2c/i2c-mux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 83768e8..37b7804 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>  	priv->adap.owner = THIS_MODULE;
>  	priv->adap.algo = &priv->algo;
>  	priv->adap.algo_data = priv;
> -	priv->adap.dev.parent = &parent->dev;
> +	priv->adap.dev.parent = muxc->dev;
>  	priv->adap.retries = parent->retries;
>  	priv->adap.timeout = parent->timeout;
>  	priv->adap.quirks = parent->quirks;
> --
> 2.7.4
> 

Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[jmondi]

Hi Peter,

On Tue, May 30, 2017 at 01:04:08PM +0200, Peter Rosin wrote:
> On 2017-05-30 10:54, Jacopo Mondi wrote:
> > I2c-mux channels are created as mux siblings while they should be
> > children of the mux itself. Fix it.
>
> Has this received any testing at all?
>

Yes, but on our specific use case, that apparently does not makes use of
i2c_parent_is_i2c_adapter()

> I think it will break various users of i2c_parent_is_i2c_adapter()
> that expect the current situation that a i2c mux child adapter is
> direct child of the parent i2c adapter. I.e. with no intermediate
> device node.

Oh, I know see..

So when walking the devices sitting on an i2c-adapter we should expect to
see mux children adapters as well there. Do you think is there a way to
easily identify them?

Thanks
   j
>
> Cheers,
> peda
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > Hello,
> >    while inspecting child nodes of an i2c adapter it has been noted that
> > child devices of an i2c-mux are listed as children of the i2c adapter itself,
> > and not of the i2c-mux.
> >
> > The hierarchy of devices looked like
> >
> > -- i2c-04
> > --- eeprom@57
> > --- video_receiver@70
> > --- video_receiver@34
> > --- gmsl-deserializer@0		<-- MUX
> > --- gmsl-deserializer@0/i2c@0	<-- MUX CHANNEL
> >
> > It now looks like
> >
> > -- i2c-04
> > --- eeprom@57
> > --- video_receiver@70
> > --- video_receiver@34
> > --- gmsl-deserializer@0
> > ---- gmsl-deserializer@0/i2c@0
> >
> > v1 -> v2:
> > - change commit message as suggested by Geert
> >
> >  drivers/i2c/i2c-mux.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> > index 83768e8..37b7804 100644
> > --- a/drivers/i2c/i2c-mux.c
> > +++ b/drivers/i2c/i2c-mux.c
> > @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> >  	priv->adap.owner = THIS_MODULE;
> >  	priv->adap.algo = &priv->algo;
> >  	priv->adap.algo_data = priv;
> > -	priv->adap.dev.parent = &parent->dev;
> > +	priv->adap.dev.parent = muxc->dev;
> >  	priv->adap.retries = parent->retries;
> >  	priv->adap.timeout = parent->timeout;
> >  	priv->adap.quirks = parent->quirks;
> > --
> > 2.7.4
> >
>

Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[Peter Rosin]

On 2017-05-31 09:51, jmondi wrote:
> Hi Peter,
> 
> On Tue, May 30, 2017 at 01:04:08PM +0200, Peter Rosin wrote:
>> On 2017-05-30 10:54, Jacopo Mondi wrote:
>>> I2c-mux channels are created as mux siblings while they should be
>>> children of the mux itself. Fix it.
>>
>> Has this received any testing at all?
>>
> 
> Yes, but on our specific use case, that apparently does not makes use of
> i2c_parent_is_i2c_adapter()

Try e.g. adding a client device with some address to the root i2c
adapter, and then add another client device with the same address
to one of the mux child adapters. Do that with and without your
patch. I suspect it will be allowed with your patch even though it
shouldn't.

>> I think it will break various users of i2c_parent_is_i2c_adapter()
>> that expect the current situation that a i2c mux child adapter is
>> direct child of the parent i2c adapter. I.e. with no intermediate
>> device node.
> 
> Oh, I know see..
> 
> So when walking the devices sitting on an i2c-adapter we should expect to
> see mux children adapters as well there. Do you think is there a way to
> easily identify them?

Well, you can use the method from i2c_parent_is_i2c_adapter() and
write a function like so (untested):

struct i2c_adapter *i2c_is_i2c_adapter(struct device *dev)
{
	if (dev->type != &i2c_adapter_type)
		return NULL;

	return to_i2c_adapter(dev);
}

But why do you need to identify them? What problem are you trying to solve?

Also, I forgot to mention this in my first reply, but note that the
device implementing the i2c-mux need not be a child of the i2c adapter.
I.e. in your example, the device I'm talking about is gmsl-deserializer@0.
This "MUX" device could be e.g. a platform device situated in some other
completely random place in the device tree. Oh well, perhaps not random,
but I hope you get what I mean...

Cheers,
peda

> Thanks
>    j
>>
>> Cheers,
>> peda
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>
>>> Hello,
>>>    while inspecting child nodes of an i2c adapter it has been noted that
>>> child devices of an i2c-mux are listed as children of the i2c adapter itself,
>>> and not of the i2c-mux.
>>>
>>> The hierarchy of devices looked like
>>>
>>> -- i2c-04
>>> --- eeprom@57
>>> --- video_receiver@70
>>> --- video_receiver@34
>>> --- gmsl-deserializer@0		<-- MUX
>>> --- gmsl-deserializer@0/i2c@0	<-- MUX CHANNEL
>>>
>>> It now looks like
>>>
>>> -- i2c-04
>>> --- eeprom@57
>>> --- video_receiver@70
>>> --- video_receiver@34
>>> --- gmsl-deserializer@0
>>> ---- gmsl-deserializer@0/i2c@0
>>>
>>> v1 -> v2:
>>> - change commit message as suggested by Geert
>>>
>>>  drivers/i2c/i2c-mux.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
>>> index 83768e8..37b7804 100644
>>> --- a/drivers/i2c/i2c-mux.c
>>> +++ b/drivers/i2c/i2c-mux.c
>>> @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>>>  	priv->adap.owner = THIS_MODULE;
>>>  	priv->adap.algo = &priv->algo;
>>>  	priv->adap.algo_data = priv;
>>> -	priv->adap.dev.parent = &parent->dev;
>>> +	priv->adap.dev.parent = muxc->dev;
>>>  	priv->adap.retries = parent->retries;
>>>  	priv->adap.timeout = parent->timeout;
>>>  	priv->adap.quirks = parent->quirks;
>>> --
>>> 2.7.4
>>>
>>

Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[jmondi]

On Wed, May 31, 2017 at 10:19:07AM +0200, Peter Rosin wrote:
> On 2017-05-31 09:51, jmondi wrote:
> > Hi Peter,
> >
> > On Tue, May 30, 2017 at 01:04:08PM +0200, Peter Rosin wrote:
> >> On 2017-05-30 10:54, Jacopo Mondi wrote:
> >>> I2c-mux channels are created as mux siblings while they should be
> >>> children of the mux itself. Fix it.
> >>
> >> Has this received any testing at all?
> >>
> >
> > Yes, but on our specific use case, that apparently does not makes use of
> > i2c_parent_is_i2c_adapter()
>
> Try e.g. adding a client device with some address to the root i2c
> adapter, and then add another client device with the same address
> to one of the mux child adapters. Do that with and without your
> patch. I suspect it will be allowed with your patch even though it
> shouldn't.
>

Oh I see what's you're point here.
I cannot access the test board and try, but yes I see what you mean.

> >> I think it will break various users of i2c_parent_is_i2c_adapter()
> >> that expect the current situation that a i2c mux child adapter is
> >> direct child of the parent i2c adapter. I.e. with no intermediate
> >> device node.
> >
> > Oh, I know see..
> >
> > So when walking the devices sitting on an i2c-adapter we should expect to
> > see mux children adapters as well there. Do you think is there a way to
> > easily identify them?
>
> Well, you can use the method from i2c_parent_is_i2c_adapter() and
> write a function like so (untested):
>
> struct i2c_adapter *i2c_is_i2c_adapter(struct device *dev)
> {
> 	if (dev->type != &i2c_adapter_type)
> 		return NULL;
>
> 	return to_i2c_adapter(dev);
> }
>
> But why do you need to identify them? What problem are you trying to solve?
>

I want to be able to walk all children devices of an i2c-adapter,
not including the mux channel i2c-adapters. I'm sure I can work around it.
While doing that I stumbled upon this and thought it was wrong.

> Also, I forgot to mention this in my first reply, but note that the
> device implementing the i2c-mux need not be a child of the i2c adapter.
> I.e. in your example, the device I'm talking about is gmsl-deserializer@0.
> This "MUX" device could be e.g. a platform device situated in some other
> completely random place in the device tree. Oh well, perhaps not random,
> but I hope you get what I mean...

Yes, I guess so :)

Please drop this patch then, and thanks for your explanation

Thanks
   j

>
> Cheers,
> peda
>
> > Thanks
> >    j
> >>
> >> Cheers,
> >> peda
> >>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>>
> >>> Hello,
> >>>    while inspecting child nodes of an i2c adapter it has been noted that
> >>> child devices of an i2c-mux are listed as children of the i2c adapter itself,
> >>> and not of the i2c-mux.
> >>>
> >>> The hierarchy of devices looked like
> >>>
> >>> -- i2c-04
> >>> --- eeprom@57
> >>> --- video_receiver@70
> >>> --- video_receiver@34
> >>> --- gmsl-deserializer@0		<-- MUX
> >>> --- gmsl-deserializer@0/i2c@0	<-- MUX CHANNEL
> >>>
> >>> It now looks like
> >>>
> >>> -- i2c-04
> >>> --- eeprom@57
> >>> --- video_receiver@70
> >>> --- video_receiver@34
> >>> --- gmsl-deserializer@0
> >>> ---- gmsl-deserializer@0/i2c@0
> >>>
> >>> v1 -> v2:
> >>> - change commit message as suggested by Geert
> >>>
> >>>  drivers/i2c/i2c-mux.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> >>> index 83768e8..37b7804 100644
> >>> --- a/drivers/i2c/i2c-mux.c
> >>> +++ b/drivers/i2c/i2c-mux.c
> >>> @@ -324,7 +324,7 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
> >>>  	priv->adap.owner = THIS_MODULE;
> >>>  	priv->adap.algo = &priv->algo;
> >>>  	priv->adap.algo_data = priv;
> >>> -	priv->adap.dev.parent = &parent->dev;
> >>> +	priv->adap.dev.parent = muxc->dev;
> >>>  	priv->adap.retries = parent->retries;
> >>>  	priv->adap.timeout = parent->timeout;
> >>>  	priv->adap.quirks = parent->quirks;
> >>> --
> >>> 2.7.4
> >>>
> >>
>

Re: [PATCH v2] i2c: i2c-mux: Fix channel parent node assignment

[Peter Rosin]

On 2017-06-01 04:19, jmondi wrote:
>> But why do you need to identify them? What problem are you trying to solve?
>>
> I want to be able to walk all children devices of an i2c-adapter,
> not including the mux channel i2c-adapters. I'm sure I can work around it.
> While doing that I stumbled upon this and thought it was wrong.

Hey, I'm not saying that the current (ab?)use of dev->parent for the
i2c hierarchy is "right". But fixing things is simply just more involved
than what you proposed.

Some random thoughts on this topic:

- I'm not sure it is sane to rearrange the device hierarchy the way the
  i2c mux code does, and the i2c hierarchy is available by other means
  as parent in struct i2c_mux_core. The trouble is getting to that struct
  from outside the owner driver. Maybe make i2c_mux_alloc create a
  separate device? Then all i2c muxes could store their copy of struct
  i2c_mux_core in a way that could be easily found by generic code.
- Is it ok to add children to unsuspecting(?) devices? (I.e. the parent
  adapter of an i2c mux gets extra children.)
- I think the current scheme prevents the parent adapter from going away
  before the mux child adapter during tare-down, which is a good thing,
  but I think that can be solved with these new device links that I read
  about?
- The i2c hierarchy also needs to be visible in sysfs, which it is with
  the current scheme (but not with your patch) so some kind of new
  i2c-parent attribute is needed for i2c mux child adapters.
- Changes in this area feel subtle, and needs testing...

Cheers,
peda