| Message ID | 20170113173630.22138-1-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: > The i2c adapter on DP AUX is purely a software construct. Linking > it to the device node of the parent device is wrong, as it leads to > 2 devices sharing the same device node, which is bad practice, as > well as the i2c trying to populate children of the i2c adapter by > looking at the child device nodes of the parent device. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> Since this is about dt, I have no idea what it does or why. Needs someone else to look at & merge. -Daniel > --- > drivers/gpu/drm/drm_dp_helper.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 3e6fe82c6d64..f91ade1edbde 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -1020,7 +1020,6 @@ int drm_dp_aux_register(struct drm_dp_aux *aux) > aux->ddc.class = I2C_CLASS_DDC; > aux->ddc.owner = THIS_MODULE; > aux->ddc.dev.parent = aux->dev; > - aux->ddc.dev.of_node = aux->dev->of_node; > > strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev), > sizeof(aux->ddc.name)); > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: > The i2c adapter on DP AUX is purely a software construct. Linking > it to the device node of the parent device is wrong, as it leads to > 2 devices sharing the same device node, which is bad practice, as Who says that two devices can't share the same device node? It's done all the time. > well as the i2c trying to populate children of the i2c adapter by > looking at the child device nodes of the parent device. A set of patches landed in v4.9 to work around this issue in a better way. See: 98b00488459e dt-bindings: i2c: Add support for 'i2c-bus' subnode 7e4c224abfe8 i2c: core: Add support for 'i2c-bus' subnode Thierry
Am Montag, den 23.01.2017, 17:33 +0100 schrieb Thierry Reding: > On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: > > The i2c adapter on DP AUX is purely a software construct. Linking > > it to the device node of the parent device is wrong, as it leads to > > 2 devices sharing the same device node, which is bad practice, as > > Who says that two devices can't share the same device node? It's done > all the time. Depending on the driver load order this may cause the child device to be probed using the parent device compatible. Clearly not something you would want to have, although it's not a problem in this specific case, as the child device is constructed by the parent driver. But generally this should be avoided. The question in this specific case is: why should the pure software i2c adapter have a DT node at all? The i2c bus isn't there in hardware, so why should it be bound to a hardware node? > > well as the i2c trying to populate children of the i2c adapter by > > looking at the child device nodes of the parent device. > > A set of patches landed in v4.9 to work around this issue in a better > way. See: > > 98b00488459e dt-bindings: i2c: Add support for 'i2c-bus' subnode > 7e4c224abfe8 i2c: core: Add support for 'i2c-bus' subnode I disagree. This isn't a better way for this specific case: it means I have to add an empty i2c-bus node to a device (parallel-to-DP converter) that does not even have a i2c bus in hardware. This is confusing and misleading. Regards, Lucas
On Mon, Jan 23, 2017 at 10:33 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: >> The i2c adapter on DP AUX is purely a software construct. Linking >> it to the device node of the parent device is wrong, as it leads to >> 2 devices sharing the same device node, which is bad practice, as > > Who says that two devices can't share the same device node? It's done > all the time. It's done *some of the time* and I would not consider it best practice. >> well as the i2c trying to populate children of the i2c adapter by >> looking at the child device nodes of the parent device. > > A set of patches landed in v4.9 to work around this issue in a better > way. See: > > 98b00488459e dt-bindings: i2c: Add support for 'i2c-bus' subnode > 7e4c224abfe8 i2c: core: Add support for 'i2c-bus' subnode What does this buy us? I don't see why this needs to be in DT either. Contrary to popular belief, DT is not the only way to instantiate devices, C code can still do it. Also, if this one line removal has no side effects, then how was it even needed? We can always add it back if there's some argument for why it is needed. Rob
Hi Rob, Am Mittwoch, den 29.03.2017, 08:56 -0500 schrieb Rob Herring: > On Mon, Jan 23, 2017 at 10:33 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: > >> The i2c adapter on DP AUX is purely a software construct. Linking > >> it to the device node of the parent device is wrong, as it leads to > >> 2 devices sharing the same device node, which is bad practice, as > > > > Who says that two devices can't share the same device node? It's done > > all the time. > > It's done *some of the time* and I would not consider it best practice. > > >> well as the i2c trying to populate children of the i2c adapter by > >> looking at the child device nodes of the parent device. > > > > A set of patches landed in v4.9 to work around this issue in a better > > way. See: > > > > 98b00488459e dt-bindings: i2c: Add support for 'i2c-bus' subnode > > 7e4c224abfe8 i2c: core: Add support for 'i2c-bus' subnode > > What does this buy us? I don't see why this needs to be in DT either. > Contrary to popular belief, DT is not the only way to instantiate > devices, C code can still do it. > > Also, if this one line removal has no side effects, then how was it > even needed? We can always add it back if there's some argument for > why it is needed. Okay, so I take this as you mostly agreeing with the rationale of this patch. @Daniel or someone with drm-misc commit rights: could you please pull this in if you are fine with it? Regards, Lucas
On Wed, Apr 05, 2017 at 10:52:32AM +0200, Lucas Stach wrote: > Hi Rob, > > Am Mittwoch, den 29.03.2017, 08:56 -0500 schrieb Rob Herring: > > On Mon, Jan 23, 2017 at 10:33 AM, Thierry Reding > > <thierry.reding@gmail.com> wrote: > > > On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: > > >> The i2c adapter on DP AUX is purely a software construct. Linking > > >> it to the device node of the parent device is wrong, as it leads to > > >> 2 devices sharing the same device node, which is bad practice, as > > > > > > Who says that two devices can't share the same device node? It's done > > > all the time. > > > > It's done *some of the time* and I would not consider it best practice. > > > > >> well as the i2c trying to populate children of the i2c adapter by > > >> looking at the child device nodes of the parent device. > > > > > > A set of patches landed in v4.9 to work around this issue in a better > > > way. See: > > > > > > 98b00488459e dt-bindings: i2c: Add support for 'i2c-bus' subnode > > > 7e4c224abfe8 i2c: core: Add support for 'i2c-bus' subnode > > > > What does this buy us? I don't see why this needs to be in DT either. > > Contrary to popular belief, DT is not the only way to instantiate > > devices, C code can still do it. > > > > Also, if this one line removal has no side effects, then how was it > > even needed? We can always add it back if there's some argument for > > why it is needed. > > Okay, so I take this as you mostly agreeing with the rationale of this > patch. For some general background on this: I was originally using this for DP support on Tegra (though that ended up never getting merged because of a particularily frustrating episode of trying to get better link training support into the core helpers) and use it as a means to obtain the I2C controller used for DDC. On Tegra, and I suspect other devices as well, the DP AUX controller is separate from the encoder, so the idea was to link them together using a standard ddc-i2c-bus phandle. I ended up not needing that because the encoder and DP AUX controller are so tightly linked on Tegra that I need direct access to the DP AUX anyway and can therefore directly get the I2C controller from that. If there aren't any other users of this, I suppose we could simply remove the line. Should someone turn up in the future and require the I2C controller to be looked up from a phandle we could add it again, at which point we'd have to investigate again how to get rid of the errors. Acked-by: Thierry Reding <treding@nvidia.com>
On 05.04.2017 14:04, Thierry Reding wrote: > On Wed, Apr 05, 2017 at 10:52:32AM +0200, Lucas Stach wrote: >> Hi Rob, >> >> Am Mittwoch, den 29.03.2017, 08:56 -0500 schrieb Rob Herring: >>> On Mon, Jan 23, 2017 at 10:33 AM, Thierry Reding >>> <thierry.reding@gmail.com> wrote: >>>> On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: >>>>> The i2c adapter on DP AUX is purely a software construct. Linking >>>>> it to the device node of the parent device is wrong, as it leads to >>>>> 2 devices sharing the same device node, which is bad practice, as >>>> Who says that two devices can't share the same device node? It's done >>>> all the time. >>> It's done *some of the time* and I would not consider it best practice. >>> >>>>> well as the i2c trying to populate children of the i2c adapter by >>>>> looking at the child device nodes of the parent device. >>>> A set of patches landed in v4.9 to work around this issue in a better >>>> way. See: >>>> >>>> 98b00488459e dt-bindings: i2c: Add support for 'i2c-bus' subnode >>>> 7e4c224abfe8 i2c: core: Add support for 'i2c-bus' subnode >>> What does this buy us? I don't see why this needs to be in DT either. >>> Contrary to popular belief, DT is not the only way to instantiate >>> devices, C code can still do it. >>> >>> Also, if this one line removal has no side effects, then how was it >>> even needed? We can always add it back if there's some argument for >>> why it is needed. >> Okay, so I take this as you mostly agreeing with the rationale of this >> patch. > For some general background on this: I was originally using this for DP > support on Tegra (though that ended up never getting merged because of a > particularily frustrating episode of trying to get better link training > support into the core helpers) and use it as a means to obtain the I2C > controller used for DDC. On Tegra, and I suspect other devices as well, > the DP AUX controller is separate from the encoder, so the idea was to > link them together using a standard ddc-i2c-bus phandle. > > I ended up not needing that because the encoder and DP AUX controller > are so tightly linked on Tegra that I need direct access to the DP AUX > anyway and can therefore directly get the I2C controller from that. > > If there aren't any other users of this, I suppose we could simply > remove the line. Should someone turn up in the future and require the > I2C controller to be looked up from a phandle we could add it again, > at which point we'd have to investigate again how to get rid of the > errors. > > Acked-by: Thierry Reding <treding@nvidia.com> It looks like the patch has been accepted but not merged. Who can pick it up? or is it OK if I put it int drm-misc? Regards Andrzej
On Tue, Nov 14, 2017 at 03:34:19PM +0100, Andrzej Hajda wrote: > On 05.04.2017 14:04, Thierry Reding wrote: > > On Wed, Apr 05, 2017 at 10:52:32AM +0200, Lucas Stach wrote: > >> Hi Rob, > >> > >> Am Mittwoch, den 29.03.2017, 08:56 -0500 schrieb Rob Herring: > >>> On Mon, Jan 23, 2017 at 10:33 AM, Thierry Reding > >>> <thierry.reding@gmail.com> wrote: > >>>> On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: > >>>>> The i2c adapter on DP AUX is purely a software construct. Linking > >>>>> it to the device node of the parent device is wrong, as it leads to > >>>>> 2 devices sharing the same device node, which is bad practice, as > >>>> Who says that two devices can't share the same device node? It's done > >>>> all the time. > >>> It's done *some of the time* and I would not consider it best practice. > >>> > >>>>> well as the i2c trying to populate children of the i2c adapter by > >>>>> looking at the child device nodes of the parent device. > >>>> A set of patches landed in v4.9 to work around this issue in a better > >>>> way. See: > >>>> > >>>> 98b00488459e dt-bindings: i2c: Add support for 'i2c-bus' subnode > >>>> 7e4c224abfe8 i2c: core: Add support for 'i2c-bus' subnode > >>> What does this buy us? I don't see why this needs to be in DT either. > >>> Contrary to popular belief, DT is not the only way to instantiate > >>> devices, C code can still do it. > >>> > >>> Also, if this one line removal has no side effects, then how was it > >>> even needed? We can always add it back if there's some argument for > >>> why it is needed. > >> Okay, so I take this as you mostly agreeing with the rationale of this > >> patch. > > For some general background on this: I was originally using this for DP > > support on Tegra (though that ended up never getting merged because of a > > particularily frustrating episode of trying to get better link training > > support into the core helpers) and use it as a means to obtain the I2C > > controller used for DDC. On Tegra, and I suspect other devices as well, > > the DP AUX controller is separate from the encoder, so the idea was to > > link them together using a standard ddc-i2c-bus phandle. > > > > I ended up not needing that because the encoder and DP AUX controller > > are so tightly linked on Tegra that I need direct access to the DP AUX > > anyway and can therefore directly get the I2C controller from that. > > > > If there aren't any other users of this, I suppose we could simply > > remove the line. Should someone turn up in the future and require the > > I2C controller to be looked up from a phandle we could add it again, > > at which point we'd have to investigate again how to get rid of the > > errors. > > > > Acked-by: Thierry Reding <treding@nvidia.com> > > It looks like the patch has been accepted but not merged. Who can pick > it up? or is it OK if I put it int drm-misc? I don't have the patch itself, but sounds good. Just go ahead and push. -Daniel
On 20.11.2017 08:54, Daniel Vetter wrote: > On Tue, Nov 14, 2017 at 03:34:19PM +0100, Andrzej Hajda wrote: >> On 05.04.2017 14:04, Thierry Reding wrote: >>> On Wed, Apr 05, 2017 at 10:52:32AM +0200, Lucas Stach wrote: >>>> Hi Rob, >>>> >>>> Am Mittwoch, den 29.03.2017, 08:56 -0500 schrieb Rob Herring: >>>>> On Mon, Jan 23, 2017 at 10:33 AM, Thierry Reding >>>>> <thierry.reding@gmail.com> wrote: >>>>>> On Fri, Jan 13, 2017 at 06:36:30PM +0100, Lucas Stach wrote: >>>>>>> The i2c adapter on DP AUX is purely a software construct. Linking >>>>>>> it to the device node of the parent device is wrong, as it leads to >>>>>>> 2 devices sharing the same device node, which is bad practice, as >>>>>> Who says that two devices can't share the same device node? It's done >>>>>> all the time. >>>>> It's done *some of the time* and I would not consider it best practice. >>>>> >>>>>>> well as the i2c trying to populate children of the i2c adapter by >>>>>>> looking at the child device nodes of the parent device. >>>>>> A set of patches landed in v4.9 to work around this issue in a better >>>>>> way. See: >>>>>> >>>>>> 98b00488459e dt-bindings: i2c: Add support for 'i2c-bus' subnode >>>>>> 7e4c224abfe8 i2c: core: Add support for 'i2c-bus' subnode >>>>> What does this buy us? I don't see why this needs to be in DT either. >>>>> Contrary to popular belief, DT is not the only way to instantiate >>>>> devices, C code can still do it. >>>>> >>>>> Also, if this one line removal has no side effects, then how was it >>>>> even needed? We can always add it back if there's some argument for >>>>> why it is needed. >>>> Okay, so I take this as you mostly agreeing with the rationale of this >>>> patch. >>> For some general background on this: I was originally using this for DP >>> support on Tegra (though that ended up never getting merged because of a >>> particularily frustrating episode of trying to get better link training >>> support into the core helpers) and use it as a means to obtain the I2C >>> controller used for DDC. On Tegra, and I suspect other devices as well, >>> the DP AUX controller is separate from the encoder, so the idea was to >>> link them together using a standard ddc-i2c-bus phandle. >>> >>> I ended up not needing that because the encoder and DP AUX controller >>> are so tightly linked on Tegra that I need direct access to the DP AUX >>> anyway and can therefore directly get the I2C controller from that. >>> >>> If there aren't any other users of this, I suppose we could simply >>> remove the line. Should someone turn up in the future and require the >>> I2C controller to be looked up from a phandle we could add it again, >>> at which point we'd have to investigate again how to get rid of the >>> errors. >>> >>> Acked-by: Thierry Reding <treding@nvidia.com> >> It looks like the patch has been accepted but not merged. Who can pick >> it up? or is it OK if I put it int drm-misc? > I don't have the patch itself, but sounds good. Just go ahead and push. > -Daniel OK, done. Regards Andrzej
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 3e6fe82c6d64..f91ade1edbde 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -1020,7 +1020,6 @@ int drm_dp_aux_register(struct drm_dp_aux *aux) aux->ddc.class = I2C_CLASS_DDC; aux->ddc.owner = THIS_MODULE; aux->ddc.dev.parent = aux->dev; - aux->ddc.dev.of_node = aux->dev->of_node; strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev), sizeof(aux->ddc.name));
The i2c adapter on DP AUX is purely a software construct. Linking it to the device node of the parent device is wrong, as it leads to 2 devices sharing the same device node, which is bad practice, as well as the i2c trying to populate children of the i2c adapter by looking at the child device nodes of the parent device. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- drivers/gpu/drm/drm_dp_helper.c | 1 - 1 file changed, 1 deletion(-)