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

> >>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@the-dreams.de>
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@the-dreams.de>
---
 drivers/i2c/i2c-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


> 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.

> > 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.

Maybe sounds more harsh than it was meant. I just want to know if I need
to schedule time for this issue.

On 03/19/2015 04:09 AM, Wolfram Sang wrote:
>
>>>> 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@the-dreams.de>
> 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.

> diff --git 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;

That feels a bit odd to me. For a regular non-mux I2C controller, that 
extra case would never trigger if the controller node was disabled, 
since the device core would never probe the controller device itself. 
So, we'd end up with inconsistent paths through the I2C core for regular 
controllers and muxes.

Perhaps better would be to have a mux-specific function to iterate over 
a mux's child nodes and instantiate buses for those. That function would 
check whether each bus node was disabled or not. That'd isolate the 
special case into the place where it was relevant.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> >-	/* 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;
> 
> That feels a bit odd to me. For a regular non-mux I2C controller, that extra
> case would never trigger if the controller node was disabled, since the
> device core would never probe the controller device itself. So, we'd end up
> with inconsistent paths through the I2C core for regular controllers and
> muxes.

I first thought the no-op for the non-mux case wouldn't hurt, but I
agree about the consistent code path. I mentioned in my previous mail
that i2c-mux might be a better place for this...

> Perhaps better would be to have a mux-specific function to iterate over a
> mux's child nodes and instantiate buses for those. That function would check
> whether each bus node was disabled or not. That'd isolate the special case
> into the place where it was relevant.

... so I wonder what you think about putting the
of_device_is_available() check into i2c_add_mux_adapter() once the
reg-property and chan_id have been matched?

On 03/19/2015 10:02 AM, Wolfram Sang wrote:
>>> -	/* 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;
>>
>> That feels a bit odd to me. For a regular non-mux I2C controller, that extra
>> case would never trigger if the controller node was disabled, since the
>> device core would never probe the controller device itself. So, we'd end up
>> with inconsistent paths through the I2C core for regular controllers and
>> muxes.
>
> I first thought the no-op for the non-mux case wouldn't hurt, but I
> agree about the consistent code path. I mentioned in my previous mail
> that i2c-mux might be a better place for this...
>
>> Perhaps better would be to have a mux-specific function to iterate over a
>> mux's child nodes and instantiate buses for those. That function would check
>> whether each bus node was disabled or not. That'd isolate the special case
>> into the place where it was relevant.
>
> ... so I wonder what you think about putting the
> of_device_is_available() check into i2c_add_mux_adapter() once the
> reg-property and chan_id have been matched?

I think that looks like a good place, yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 19.03.2015 17:02, Wolfram Sang wrote:
>> Perhaps better would be to have a mux-specific function to iterate over a
>> mux's child nodes and instantiate buses for those. That function would check
>> whether each bus node was disabled or not. That'd isolate the special case
>> into the place where it was relevant.
>
> ... so I wonder what you think about putting the
> of_device_is_available() check into i2c_add_mux_adapter() once the
> reg-property and chan_id have been matched?
>

Ok, I see what you mean. I had a look at the place in question and
wonder what to return from i2c_add_mux_adapter() in the disabled
case so that i2c-mux-pinctrl is still happy with the returned value.

I guess what you want to have is that i2c_add_adapter() is not called
for the disabled case, right?

Is the i2c_adapter struct prepared in i2c_mux_add_adapter() still valid
if i2c_add_adapter() is not called?

Sorry, I am not too deep into i2c subsystem, I just reworked i2c-mux-
pinctrl to make it work on Dove. If you are fine with giving me some
guidance how you prefer to have it done, I can try to free some spare
time. Unfortunately there is already little of it, so please don't
expect a quick tested patch.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> Ok, I see what you mean. I had a look at the place in question and
> wonder what to return from i2c_add_mux_adapter() in the disabled
> case so that i2c-mux-pinctrl is still happy with the returned value.

Ouch, you are right. The crux of interfaces returning NULL instead of an
ERR_PTR :( I'll have a look, I maybe started to fix this somewhen.

> I guess what you want to have is that i2c_add_adapter() is not called
> for the disabled case, right?

I think that makes sense.

> Is the i2c_adapter struct prepared in i2c_mux_add_adapter() still valid
> if i2c_add_adapter() is not called?

I will have a closer look to the issue this weekend.

> Sorry, I am not too deep into i2c subsystem, I just reworked i2c-mux-
> pinctrl to make it work on Dove. If you are fine with giving me some
> guidance how you prefer to have it done, I can try to free some spare
> time.

Cool, thanks. Learning by doing is a good way to get such knowledge :)

> Unfortunately there is already little of it, so please don't
> expect a quick tested patch.

I understand.

Thanks,

   Wolfram

> > I guess what you want to have is that i2c_add_adapter() is not called
> > for the disabled case, right?
> 
> I think that makes sense.

But maybe we should just start simple and keep calling i2c_add_adapter()
for the disabled case. We will just skip probing devices on the bus.
Would that help the issue you were originally trying to solve?

On 21.03.2015 22:00, Wolfram Sang wrote:
>>> I guess what you want to have is that i2c_add_adapter() is not called
>>> for the disabled case, right?
>>
>> I think that makes sense.
>
> But maybe we should just start simple and keep calling i2c_add_adapter()
> for the disabled case. We will just skip probing devices on the bus.
> Would that help the issue you were originally trying to solve?

It is not about probing devices on the mux sub-busses, I'd expect no
devices on the optional sub-busses in DT on boards where those pins
are not used as i2c.

The idea was to hide those busses completely in particular from
userspace on boards where they'll never be available.

If modifying i2c-mux-pinctrl to respect the sub-bus status property is
such a big issue, I'd rather leave the driver as is and expose all
sub-busses to userspace.

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> If modifying i2c-mux-pinctrl to respect the sub-bus status property is
> such a big issue, I'd rather leave the driver as is and expose all
> sub-busses to userspace.

Well, dunno what 'big issue' is in your book :) What definately needs to
be done is:

* handle "status" at mux-core level, not mux-driver
* that probably needs us to convert i2c_add_mux_adapter() to return
  ERR_PTRs instead of NULL, so we can distinguish the "disabled" case
* that would mean to fix all existing users

That's all not groundbreaking stuff, but needs caution and thoroughness.
There might be some gory details left, though...

Regards,

   Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 23.03.2015 19:32, Wolfram Sang wrote:
>> If modifying i2c-mux-pinctrl to respect the sub-bus status property is
>> such a big issue, I'd rather leave the driver as is and expose all
>> sub-busses to userspace.
>
> Well, dunno what 'big issue' is in your book :) What definately needs to
> be done is:

Wolfram,

I had a look at the code in question again and prepared some patches
to return ERR_PTRs from i2c_add_mux_adapter(), rework users to check
for IS_ERR() and finally skip i2c_add_adapter for the OF disabled case.

I have them compile-tested but I don't have any hardware available
right now.

Anyway, I still have some questions.

> * handle "status" at mux-core level, not mux-driver

I get that.. but:

> * that probably needs us to convert i2c_add_mux_adapter() to return
>    ERR_PTRs instead of NULL, so we can distinguish the "disabled" case

What do you want to return from i2c_add_mux_adapter() if you find an OF
disabled child node?

AFAIKS, there is no explicit errno for a disabled device (node) so all
I can imagine here is to return either the &priv->adap before actually
calling i2c_add_adapter on it or NULL now that we explicitly check for
errors.

> * that would mean to fix all existing users

If we choose to return NULL, those users who can deal with a
missing/disabled sub-bus on the mux can check with IS_ERR()
the others should check with IS_ERR_OR_NULL().

We could also choose to return some other errno (-ENODEV maybe)
but still we'd have to double-check that return value on i2c-mux-pinctrl
and the others too if we don't care that i2c_add_adapter() wasn't
called for a mux.

> That's all not groundbreaking stuff, but needs caution and thoroughness.
> There might be some gory details left, though...

As I said before, the intention was not disable a possible i2c bus that
can be dynamically added/removed on that specific Dove SBC/CM-A510
board but to have a single i2c-mux-pinctrl in the SoC dtsi just because
the SoC has the optional i2c bus muxing.

While thinking about it (and I still think of it as a 'big issue'
compared to the intention of the initial patch) I came to the
conclusion that I should maybe just go for a board-specific
i2c-mux-pinctrl node instead of adding it to the SoC dtsi. That will
also avoid doubled i2c busses on boards with just the default i2c
option.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> While thinking about it (and I still think of it as a 'big issue'
> compared to the intention of the initial patch) I came to the
> conclusion that I should maybe just go for a board-specific
> i2c-mux-pinctrl node instead of adding it to the SoC dtsi. That will
> also avoid doubled i2c busses on boards with just the default i2c
> option.

Ehrm, then please let me know what you decided on. If you chose the
above road, then I don't need to think about the other questions :)