Re: [PATCH] drm: don't link DP aux i2c adapter to the hardware device node

From: Thierry Reding <thierry.reding@gmail.com>
To: Lucas Stach <l.stach@pengutronix.de>
Cc: patchwork-lst@pengutronix.de,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] drm: don't link DP aux i2c adapter to the hardware device node
Date: Wed, 5 Apr 2017 14:04:31 +0200	[thread overview]
Message-ID: <20170405120431.GA9162@ulmo.ba.sec> (raw)
In-Reply-To: <1491382352.2904.9.camel@pengutronix.de>

[-- Attachment #1.1: Type: text/plain, Size: 2622 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel