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 > > > <[email protected]> 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 <[email protected]>
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;
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
