| Message ID | 1496134457-19573-1-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 >
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 > > >
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 >>> >>
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 > >>> > >> >
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
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;
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(-) -- 2.7.4