On Wed, Apr 05, 2017 at 02:04:31PM +0200, 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@xxxxxxxxx> 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@xxxxxxxxxx>
I'm going to have to retract that: I just noticed that this patch breaks
eDP for Venice2 (and presumably all Tegra124 Nyan boards as well, though
I don't have those to test with).
My description above isn't quite correct. For eDP device we do use the
ddc-i2c-bus property in DT to denote which I2C bus to use for probing
the EDID. So the reason why eDP now breaks is because the simple-panel
driver will look for the I2C adapter, not find a matching one and defer
probe (indefinitely).
A, perhaps nicer, alternative I found to make it work is the below
patch. Would that be more reasonable? Looping in Wolfram.
Thierry
--- >8 ---
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index 8d474bb1dc15..f88527a61cd1 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -118,6 +118,14 @@ static int of_dev_node_match(struct device *dev, void *data)
return dev->of_node == data;
}
+static int of_parent_node_match(struct device *dev, void *data)
+{
+ if (dev->parent)
+ return dev->parent->of_node == data;
+
+ return 0;
+}
+
/* must call put_device() when done with returned i2c_client device */
struct i2c_client *of_find_i2c_device_by_node(struct device_node *node)
{
@@ -143,6 +151,9 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
struct i2c_adapter *adapter;
dev = bus_find_device(&i2c_bus_type, NULL, node, of_dev_node_match);
+ if (!dev)
+ dev = bus_find_device(&i2c_bus_type, NULL, node, of_parent_node_match);
+
if (!dev)
return NULL;
Attachment:
signature.asc
Description: PGP signature
