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

I2C mux pinctrl driver currently determines the number of sub-busses by
counting available pinctrl-names. Unfortunately, this requires each
incarnation of the devicetree node with different available sub-busses
to be rewritten.

This patch reworks i2c-mux-pinctrl driver to count the number of
available sub-nodes instead. The rework should be compatible to the old
way of probing for sub-busses and additionally allows to disable unused
sub-busses with standard DT property status = "disabled".

This also amends the corresponding devicetree binding documentation to
reflect the new functionality to disable unused sub-nodes. While at it,
also fix two references to binding documentation files that miss an "i2c-"
prefix.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Tested-by: Stephen Warren <swarren@nvidia.com>
---
Changelog:

v2->v3:
- remove mention of "I2C bus numbers" (Suggested by Stephen Warren)
- require pinctrl-names property for "each child" instead of
  "each enabled child" (Suggested by Stephen Warren)
- swap enabled/disabled child nodes (Suggested by Stephen Warren)

v1->v2:
- added a Tested-by for i2c-mux-pinctrl changes from Stepen Warren.
- reworded i2c-mux-pinctrl devicetree doc changes
  (Suggested by Stephen Warren).

Patches 2/4 - 4/4 remain unchanged.

Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Gabriel Dobato <dobatog@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: linux-i2c@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    | 21 ++++---
 drivers/i2c/muxes/i2c-mux-pinctrl.c                | 70 ++++++++++++++--------
 2 files changed, 58 insertions(+), 33 deletions(-)

On 03/09/2015 06:21 AM, Sebastian Hesselbarth wrote:
> I2C mux pinctrl driver currently determines the number of sub-busses by
> counting available pinctrl-names. Unfortunately, this requires each
> incarnation of the devicetree node with different available sub-busses
> to be rewritten.
>
> This patch reworks i2c-mux-pinctrl driver to count the number of
> available sub-nodes instead. The rework should be compatible to the old
> way of probing for sub-busses and additionally allows to disable unused
> sub-busses with standard DT property status = "disabled".
>
> This also amends the corresponding devicetree binding documentation to
> reflect the new functionality to disable unused sub-nodes. While at it,
> also fix two references to binding documentation files that miss an "i2c-"
> prefix.

The DT binding changes at least,
Acked-by: Stephen Warren <swarren@nvidia.com>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10.03.2015 17:28, Stephen Warren wrote:
> On 03/09/2015 06:21 AM, Sebastian Hesselbarth wrote:
>> I2C mux pinctrl driver currently determines the number of sub-busses by
>> counting available pinctrl-names. Unfortunately, this requires each
>> incarnation of the devicetree node with different available sub-busses
>> to be rewritten.
>>
>> This patch reworks i2c-mux-pinctrl driver to count the number of
>> available sub-nodes instead. The rework should be compatible to the old
>> way of probing for sub-busses and additionally allows to disable unused
>> sub-busses with standard DT property status = "disabled".
>>
>> This also amends the corresponding devicetree binding documentation to
>> reflect the new functionality to disable unused sub-nodes. While at it,
>> also fix two references to binding documentation files that miss an
>> "i2c-"
>> prefix.
>
> The DT binding changes at least,
> Acked-by: Stephen Warren <swarren@nvidia.com>

Wolfram,

are you going to pick this patch through your tree now that Stephen
ack'ed the binding documentation change, too?

I can also split the patch up into driver/doc changes if you prefer.

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

On Mon, Mar 09, 2015 at 01:21:05PM +0100, Sebastian Hesselbarth wrote:
> I2C mux pinctrl driver currently determines the number of sub-busses by
> counting available pinctrl-names. Unfortunately, this requires each
> incarnation of the devicetree node with different available sub-busses
> to be rewritten.
> 
> This patch reworks i2c-mux-pinctrl driver to count the number of
> available sub-nodes instead. The rework should be compatible to the old
> way of probing for sub-busses and additionally allows to disable unused
> sub-busses with standard DT property status = "disabled".

Not sure about this change. With DYNAMIC_OF these days, you can't rely
that 'disabled' stays disabled all the time. My gut feeling tells me
that people will want to use this someday.

> 
> This also amends the corresponding devicetree binding documentation to
> reflect the new functionality to disable unused sub-nodes. While at it,
> also fix two references to binding documentation files that miss an "i2c-"
> prefix.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> ---
> Changelog:
> 
> v2->v3:
> - remove mention of "I2C bus numbers" (Suggested by Stephen Warren)
> - require pinctrl-names property for "each child" instead of
>   "each enabled child" (Suggested by Stephen Warren)
> - swap enabled/disabled child nodes (Suggested by Stephen Warren)
> 
> v1->v2:
> - added a Tested-by for i2c-mux-pinctrl changes from Stepen Warren.
> - reworded i2c-mux-pinctrl devicetree doc changes
>   (Suggested by Stephen Warren).
> 
> Patches 2/4 - 4/4 remain unchanged.
> 
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Gabriel Dobato <dobatog@gmail.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: linux-i2c@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  .../devicetree/bindings/i2c/i2c-mux-pinctrl.txt    | 21 ++++---
>  drivers/i2c/muxes/i2c-mux-pinctrl.c                | 70 ++++++++++++++--------
>  2 files changed, 58 insertions(+), 33 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
> index ae8af1694e95..cd94a0f64d76 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pinctrl.txt
> @@ -28,17 +28,18 @@ Also required are:
>  * Standard pinctrl properties that specify the pin mux state for each child
>    bus. See ../pinctrl/pinctrl-bindings.txt.
>  
> -* Standard I2C mux properties. See mux.txt in this directory.
> +* Standard I2C mux properties. See i2c-mux.txt in this directory.
>  
> -* I2C child bus nodes. See mux.txt in this directory.
> +* I2C child bus nodes. See i2c-mux.txt in this directory.
>  
> -For each named state defined in the pinctrl-names property, an I2C child bus
> -will be created. I2C child bus numbers are assigned based on the index into
> -the pinctrl-names property.
> +For each enabled child node an I2C child bus will be created.
>  
> -The only exception is that no bus will be created for a state named "idle". If
> -such a state is defined, it must be the last entry in pinctrl-names. For
> -example:
> +There must be a corresponding pinctrl-names entry for each child node at the
> +position of the child node's "reg" property.
> +
> +Also, there can be an idle pinctrl state defined at the end of possible pinctrl
> +states. If such a state is defined, it must be the last entry in pinctrl-names.
> +For example:
>  
>  	pinctrl-names = "ddc", "pta", "idle"  ->  ddc = bus 0, pta = bus 1
>  	pinctrl-names = "ddc", "idle", "pta"  ->  Invalid ("idle" not last)
> @@ -68,10 +69,12 @@ Example:
>  		pinctrl-1 = <&state_i2cmux_pta>;
>  		pinctrl-2 = <&state_i2cmux_idle>;
>  
> +		/* Disabled child bus 0 */
>  		i2c@0 {
>  			reg = <0>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> +			status = "disabled";
>  
>  			eeprom {
>  				compatible = "eeprom";
> @@ -79,10 +82,12 @@ Example:
>  			};
>  		};
>  
> +		/* Enabled child bus 1 */
>  		i2c@1 {
>  			reg = <1>;
>  			#address-cells = <1>;
>  			#size-cells = <0>;
> +			status = "okay";
>  
>  			eeprom {
>  				compatible = "eeprom";
> diff --git a/drivers/i2c/muxes/i2c-mux-pinctrl.c b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> index b48378c4b40d..033dacfabfdf 100644
> --- a/drivers/i2c/muxes/i2c-mux-pinctrl.c
> +++ b/drivers/i2c/muxes/i2c-mux-pinctrl.c
> @@ -56,9 +56,12 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
>  				struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> -	int num_names, i, ret;
> +	struct device_node *child;
> +	struct property *prop;
> +	int num_names, num_children, ret;
>  	struct device_node *adapter_np;
>  	struct i2c_adapter *adapter;
> +	const char *state;
>  
>  	if (!np)
>  		return 0;
> @@ -77,32 +80,16 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
>  		return num_names;
>  	}
>  
> -	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
> -		sizeof(*mux->pdata->pinctrl_states) * num_names,
> -		GFP_KERNEL);
> -	if (!mux->pdata->pinctrl_states) {
> -		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
> -		return -ENOMEM;
> +	num_children = of_get_available_child_count(np);
> +	if (num_children < 0) {
> +		dev_err(mux->dev, "Unable to count available children: %d\n",
> +			num_children);
> +		return num_children;
>  	}
>  
> -	for (i = 0; i < num_names; i++) {
> -		ret = of_property_read_string_index(np, "pinctrl-names", i,
> -			&mux->pdata->pinctrl_states[mux->pdata->bus_count]);
> -		if (ret < 0) {
> -			dev_err(mux->dev, "Cannot parse pinctrl-names: %d\n",
> -				ret);
> -			return ret;
> -		}
> -		if (!strcmp(mux->pdata->pinctrl_states[mux->pdata->bus_count],
> -			    "idle")) {
> -			if (i != num_names - 1) {
> -				dev_err(mux->dev, "idle state must be last\n");
> -				return -EINVAL;
> -			}
> -			mux->pdata->pinctrl_state_idle = "idle";
> -		} else {
> -			mux->pdata->bus_count++;
> -		}
> +	if (num_names < num_children) {
> +		dev_err(mux->dev, "Found less pinctrl states than children\n");
> +		return -EINVAL;
>  	}
>  
>  	adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> @@ -118,6 +105,39 @@ static int i2c_mux_pinctrl_parse_dt(struct i2c_mux_pinctrl *mux,
>  	mux->pdata->parent_bus_num = i2c_adapter_id(adapter);
>  	put_device(&adapter->dev);
>  
> +	mux->pdata->pinctrl_states = devm_kzalloc(&pdev->dev,
> +		sizeof(*mux->pdata->pinctrl_states) * num_children,
> +		GFP_KERNEL);
> +	if (!mux->pdata->pinctrl_states) {
> +		dev_err(mux->dev, "Cannot allocate pinctrl_states\n");
> +		return -ENOMEM;
> +	}
> +
> +	of_property_for_each_string(np, "pinctrl-names", prop, state)
> +		if (!strcmp(state, "idle"))
> +			mux->pdata->pinctrl_state_idle = "idle";
> +
> +	for_each_available_child_of_node(np, child) {
> +		u32 reg;
> +
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		if (ret < 0) {
> +			dev_err(mux->dev, "Missing reg property for child node: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = of_property_read_string_index(np,
> +				    "pinctrl-names", reg, &state);
> +		if (ret < 0) {
> +			dev_err(mux->dev, "Cannot parse pinctrl-names for mux %d: %d\n",
> +				reg, ret);
> +			return ret;
> +		}
> +
> +		mux->pdata->pinctrl_states[mux->pdata->bus_count++] = state;
> +	}
> +
>  	return 0;
>  }
>  #else
> -- 
> 2.1.0
>

On 18.03.2015 13:30, Wolfram Sang wrote:
> On Mon, Mar 09, 2015 at 01:21:05PM +0100, Sebastian Hesselbarth wrote:
>> I2C mux pinctrl driver currently determines the number of sub-busses by
>> counting available pinctrl-names. Unfortunately, this requires each
>> incarnation of the devicetree node with different available sub-busses
>> to be rewritten.
>>
>> This patch reworks i2c-mux-pinctrl driver to count the number of
>> available sub-nodes instead. The rework should be compatible to the old
>> way of probing for sub-busses and additionally allows to disable unused
>> sub-busses with standard DT property status = "disabled".
>
> Not sure about this change. With DYNAMIC_OF these days, you can't rely
> that 'disabled' stays disabled all the time. My gut feeling tells me
> that people will want to use this someday.

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.

I agree that this driver still does not cope well with DYNAMIC_OF but
neither did the former implementation. How about we settle this driver
to this implementation now and wait for any maniac that wants to use it
the way you are suggesting above?

The other option would be to leave the driver as is - but at least on
Dove where the muxing-options are not used often, it will always create
4 i2c-busses (controller plus the three muxing options) on any board
even though the pins are not accessible at all. I think that will just
create massive confusion from the user point-of-view?

BTW, I have received a patchwork update notification - it may be
unrelated but I prefer the Dove dts/dtsi changes to go through mvebu
tree.

Sebastian

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

On Wed, Mar 18, 2015 at 02:23:18PM +0100, Sebastian Hesselbarth wrote:
> On 18.03.2015 13:30, Wolfram Sang wrote:
> >On Mon, Mar 09, 2015 at 01:21:05PM +0100, Sebastian Hesselbarth wrote:
> >>I2C mux pinctrl driver currently determines the number of sub-busses by
> >>counting available pinctrl-names. Unfortunately, this requires each
> >>incarnation of the devicetree node with different available sub-busses
> >>to be rewritten.
> >>
> >>This patch reworks i2c-mux-pinctrl driver to count the number of
> >>available sub-nodes instead. The rework should be compatible to the old
> >>way of probing for sub-busses and additionally allows to disable unused
> >>sub-busses with standard DT property status = "disabled".
> >
> >Not sure about this change. With DYNAMIC_OF these days, you can't rely
> >that 'disabled' stays disabled all the time. My gut feeling tells me
> >that people will want to use this someday.
> 
> 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()?

> I agree that this driver still does not cope well with DYNAMIC_OF but
> neither did the former implementation. How about we settle this driver
> to this implementation now and wait for any maniac that wants to use it
> the way you are suggesting above?

Sure. I don't want you to make this driver OF_DYNAMIC compatible. I just
thought it makes it harder, though, e.g. you allocate memory for the
number of active busses not the number of possibilities, so that would
have to be reverted by the "maniac". I am still at the glimpse level,
but what if we let the mux-pinctrl parse all the data (even for disabled
busses), but only the enabled ones will get a muxed adapter because this
is handled in of_i2c_register_devices()?

> BTW, I have received a patchwork update notification - it may be
> unrelated but I prefer the Dove dts/dtsi changes to go through mvebu
> tree.

Yes, I only take dts patches in rare cases.

On 18.03.2015 15:00, Wolfram Sang wrote:
> On Wed, Mar 18, 2015 at 02:23:18PM +0100, Sebastian Hesselbarth 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.

>> I agree that this driver still does not cope well with DYNAMIC_OF but
>> neither did the former implementation. How about we settle this driver
>> to this implementation now and wait for any maniac that wants to use it
>> the way you are suggesting above?
>
> Sure. I don't want you to make this driver OF_DYNAMIC compatible. I just
> thought it makes it harder, though, e.g. you allocate memory for the
> number of active busses not the number of possibilities, so that would
> have to be reverted by the "maniac". I am still at the glimpse level,
> but what if we let the mux-pinctrl parse all the data (even for disabled
> busses), but only the enabled ones will get a muxed adapter because this
> is handled in of_i2c_register_devices()?

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.

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.

i2c_mux_pinctrl_probe() is basically DT-agnostic and it should
definitely stay that way. Currently, each mux within pd->bus_count
requires a non-NULL pd->pinctrl_states[i] otherwise _probe() will bail
out for all sub-busses.

We could rework it to

(a) deal with each sub-bus individually with respect to 
pinctrl_lookup_state() and i2c_add_mux_adapter()

and

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

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

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

> > 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 :)