Re: [PATCH v3 1/4] i2c: mux-pinctrl: Rework to honor disabled child nodes

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3495 bytes --]


> >>Possible. But this change just makes i2c-mux-pinctrl honor status
> >>property at all. There is no functional change except it now allows
> >>you to disable any of the sub-busses.
> >
> >Actually, this is the feature I like. However, I wonder if we shouldn't
> >have that in the core, say in of_i2c_register_devices()?
> 
> Hmm, looking at of_i2c_register_devices():
> 
> 	for_each_available_child_of_node(adap->dev.of_node, node)
> 		of_i2c_register_device(adap, node);
> 
> already honors status properties by using for_each_available_foo.
> Therefore, i2c-core will also skip i2c device nodes disabled by
> status property.

Yes, but only child nodes, not the complete bus. Here is an RFC of what
I mean:

From: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
Subject: [RFC] i2c: of: always check if busses are enabled

Allow all busses to have a "status" property which allows busses to not
be probed when set to "disabled". Needed for DTS overlays with i2c mux
scenarios.

Signed-off-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index fe80f85896e267..d9a3ad2149332e 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1305,8 +1305,8 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 {
 	struct device_node *node;
 
-	/* Only register child devices if the adapter has a node pointer set */
-	if (!adap->dev.of_node)
+	/* Only register childs if adapter has a node pointer with enabled status */
+	if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node))
 		return;
 
 	dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");

> I am not too deep into i2c-core, but AFAIKS i2c-mux-pinctrl is not an
> i2c device but an i2c mux that is dealt with differently? It is not
> probed with of_i2c_register_devices() but as a separate platform_device
> with a reference to the parent i2c bus.

Yes, but the busses have seperate nodes. Those nodes with the 'reg =
<0>' properties. And those are matched with of_i2c_register_devices when
the reg-property and the chan_id match.

> About the memory allocation for the maximum potential number of muxes:
> We would need some way to distinguish disabled from enabled muxes in
> i2c-mux-pinctrl's platform_data.

Do we? Can't we claim that every described bus needs a pinctrl entry.
Still, the disabled busses won't be probed? So, we could also think
about putting the above code snippet into i2c-mux when registering
busses, so not only the childs will be skipped but also the whole
bus will not be created.

> i2c_mux_pinctrl_probe() is basically DT-agnostic and it should
> definitely stay that way. Currently, each mux within pd->bus_count

I agree.

> requires a non-NULL pd->pinctrl_states[i] otherwise _probe() will bail
> out for all sub-busses.

And I think that makes sense.

> (b) allow (and skip) muxes with pinctrl_states[i] == NULL for now and
> let the "maniac" deal with storing/re-probing the corresponding
> pinctrl_state name once it gets dynamically enabled.

Why do you need the NULL?

> I am still not too eager working on it but if you insist, I can see
> what I can do as long as Stephen sticks with testing it on Tegra. ;)

Please decide if you want to work on it. Remember, I am not short of
patches to deal with.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]