[PATCH v3] i2c: mux: remove duplicated i2c_algorithm

[PATCH v3] i2c: mux: remove duplicated i2c_algorithm

[Luca Ceresoli]

From: Luca Ceresoli <luca@lucaceresoli.net>

i2c-mux instantiates one i2c_algorithm for each downstream adapter.
However these algorithms are all identical, depending only on the
parent adapter.

Avoid duplication by hoisting the i2c_algorithm from the adapters to
the i2c_mux_core object, and reuse it in all the adapters.

Cc: Peter Rosin <peda@axentia.se>
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---

Changes v2 -> v3:
 - fix coding style of comment when moving it (Peter Rosin)
 - move the 'algo' member between parent and dev (Peter Rosin)

Changes v1 -> v2:
 - include i2c.h (fixes build failure on i2c-mux-ltc4306)
---
 drivers/i2c/i2c-mux.c   | 38 +++++++++++++++++++-------------------
 include/linux/i2c-mux.h |  2 ++
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
index f330690b4125..6ac23004fb2a 100644
--- a/drivers/i2c/i2c-mux.c
+++ b/drivers/i2c/i2c-mux.c
@@ -30,7 +30,6 @@
 /* multiplexer per channel data */
 struct i2c_mux_priv {
 	struct i2c_adapter adap;
-	struct i2c_algorithm algo;
 	struct i2c_mux_core *muxc;
 	u32 chan_id;
 };
@@ -263,6 +262,24 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
 	muxc->deselect = deselect;
 	muxc->max_adapters = max_adapters;
 
+	/*
+	 * Need to do algo dynamically because we don't know ahead of
+	 * time what sort of physical adapter we'll be dealing with.
+	 */
+	if (parent->algo->master_xfer) {
+		if (muxc->mux_locked)
+			muxc->algo.master_xfer = i2c_mux_master_xfer;
+		else
+			muxc->algo.master_xfer = __i2c_mux_master_xfer;
+	}
+	if (parent->algo->smbus_xfer) {
+		if (muxc->mux_locked)
+			muxc->algo.smbus_xfer = i2c_mux_smbus_xfer;
+		else
+			muxc->algo.smbus_xfer = __i2c_mux_smbus_xfer;
+	}
+	muxc->algo.functionality = i2c_mux_functionality;
+
 	return muxc;
 }
 EXPORT_SYMBOL_GPL(i2c_mux_alloc);
@@ -301,28 +318,11 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
 	priv->muxc = muxc;
 	priv->chan_id = chan_id;
 
-	/* Need to do algo dynamically because we don't know ahead
-	 * of time what sort of physical adapter we'll be dealing with.
-	 */
-	if (parent->algo->master_xfer) {
-		if (muxc->mux_locked)
-			priv->algo.master_xfer = i2c_mux_master_xfer;
-		else
-			priv->algo.master_xfer = __i2c_mux_master_xfer;
-	}
-	if (parent->algo->smbus_xfer) {
-		if (muxc->mux_locked)
-			priv->algo.smbus_xfer = i2c_mux_smbus_xfer;
-		else
-			priv->algo.smbus_xfer = __i2c_mux_smbus_xfer;
-	}
-	priv->algo.functionality = i2c_mux_functionality;
-
 	/* Now fill out new adapter structure */
 	snprintf(priv->adap.name, sizeof(priv->adap.name),
 		 "i2c-%d-mux (chan_id %d)", i2c_adapter_id(parent), chan_id);
 	priv->adap.owner = THIS_MODULE;
-	priv->adap.algo = &priv->algo;
+	priv->adap.algo = &muxc->algo;
 	priv->adap.algo_data = priv;
 	priv->adap.dev.parent = &parent->dev;
 	priv->adap.retries = parent->retries;
diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
index bd74d5706f3b..3d89600d5a9e 100644
--- a/include/linux/i2c-mux.h
+++ b/include/linux/i2c-mux.h
@@ -28,9 +28,11 @@
 #ifdef __KERNEL__
 
 #include <linux/bitops.h>
+#include <linux/i2c.h>
 
 struct i2c_mux_core {
 	struct i2c_adapter *parent;
+	struct i2c_algorithm algo;
 	struct device *dev;
 	unsigned int mux_locked:1;
 	unsigned int arbitrator:1;
-- 
2.17.1

Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm

[Peter Rosin]

On 2018-10-03 17:19, Luca Ceresoli wrote:
> From: Luca Ceresoli <luca@lucaceresoli.net>
> 
> i2c-mux instantiates one i2c_algorithm for each downstream adapter.
> However these algorithms are all identical, depending only on the
> parent adapter.
> 
> Avoid duplication by hoisting the i2c_algorithm from the adapters to
> the i2c_mux_core object, and reuse it in all the adapters.

Ouch, while I like the concept of having one i2c_algorithm per mux,
this patch is not working. Various i2c-mux drivers set the
muxc->mux_locked variable *after* the i2c_mux_alloc call, and this
patch breaks such use.

So, the patch needs some reworking. Sorry for not noticing this
earlier.

Cheers,
Peter

> Cc: Peter Rosin <peda@axentia.se>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> 
> Changes v2 -> v3:
>  - fix coding style of comment when moving it (Peter Rosin)
>  - move the 'algo' member between parent and dev (Peter Rosin)
> 
> Changes v1 -> v2:
>  - include i2c.h (fixes build failure on i2c-mux-ltc4306)
> ---
>  drivers/i2c/i2c-mux.c   | 38 +++++++++++++++++++-------------------
>  include/linux/i2c-mux.h |  2 ++
>  2 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index f330690b4125..6ac23004fb2a 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c
> @@ -30,7 +30,6 @@
>  /* multiplexer per channel data */
>  struct i2c_mux_priv {
>  	struct i2c_adapter adap;
> -	struct i2c_algorithm algo;
>  	struct i2c_mux_core *muxc;
>  	u32 chan_id;
>  };
> @@ -263,6 +262,24 @@ struct i2c_mux_core *i2c_mux_alloc(struct i2c_adapter *parent,
>  	muxc->deselect = deselect;
>  	muxc->max_adapters = max_adapters;
>  
> +	/*
> +	 * Need to do algo dynamically because we don't know ahead of
> +	 * time what sort of physical adapter we'll be dealing with.
> +	 */
> +	if (parent->algo->master_xfer) {
> +		if (muxc->mux_locked)
> +			muxc->algo.master_xfer = i2c_mux_master_xfer;
> +		else
> +			muxc->algo.master_xfer = __i2c_mux_master_xfer;
> +	}
> +	if (parent->algo->smbus_xfer) {
> +		if (muxc->mux_locked)
> +			muxc->algo.smbus_xfer = i2c_mux_smbus_xfer;
> +		else
> +			muxc->algo.smbus_xfer = __i2c_mux_smbus_xfer;
> +	}
> +	muxc->algo.functionality = i2c_mux_functionality;
> +
>  	return muxc;
>  }
>  EXPORT_SYMBOL_GPL(i2c_mux_alloc);
> @@ -301,28 +318,11 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>  	priv->muxc = muxc;
>  	priv->chan_id = chan_id;
>  
> -	/* Need to do algo dynamically because we don't know ahead
> -	 * of time what sort of physical adapter we'll be dealing with.
> -	 */
> -	if (parent->algo->master_xfer) {
> -		if (muxc->mux_locked)
> -			priv->algo.master_xfer = i2c_mux_master_xfer;
> -		else
> -			priv->algo.master_xfer = __i2c_mux_master_xfer;
> -	}
> -	if (parent->algo->smbus_xfer) {
> -		if (muxc->mux_locked)
> -			priv->algo.smbus_xfer = i2c_mux_smbus_xfer;
> -		else
> -			priv->algo.smbus_xfer = __i2c_mux_smbus_xfer;
> -	}
> -	priv->algo.functionality = i2c_mux_functionality;
> -
>  	/* Now fill out new adapter structure */
>  	snprintf(priv->adap.name, sizeof(priv->adap.name),
>  		 "i2c-%d-mux (chan_id %d)", i2c_adapter_id(parent), chan_id);
>  	priv->adap.owner = THIS_MODULE;
> -	priv->adap.algo = &priv->algo;
> +	priv->adap.algo = &muxc->algo;
>  	priv->adap.algo_data = priv;
>  	priv->adap.dev.parent = &parent->dev;
>  	priv->adap.retries = parent->retries;
> diff --git a/include/linux/i2c-mux.h b/include/linux/i2c-mux.h
> index bd74d5706f3b..3d89600d5a9e 100644
> --- a/include/linux/i2c-mux.h
> +++ b/include/linux/i2c-mux.h
> @@ -28,9 +28,11 @@
>  #ifdef __KERNEL__
>  
>  #include <linux/bitops.h>
> +#include <linux/i2c.h>
>  
>  struct i2c_mux_core {
>  	struct i2c_adapter *parent;
> +	struct i2c_algorithm algo;
>  	struct device *dev;
>  	unsigned int mux_locked:1;
>  	unsigned int arbitrator:1;
> 

Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm

[Luca Ceresoli]

Hi Peter,

On 08/10/2018 23:43, Peter Rosin wrote:
> On 2018-10-03 17:19, Luca Ceresoli wrote:
>> From: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> i2c-mux instantiates one i2c_algorithm for each downstream adapter.
>> However these algorithms are all identical, depending only on the
>> parent adapter.
>>
>> Avoid duplication by hoisting the i2c_algorithm from the adapters to
>> the i2c_mux_core object, and reuse it in all the adapters.
> 
> Ouch, while I like the concept of having one i2c_algorithm per mux,
> this patch is not working. Various i2c-mux drivers set the
> muxc->mux_locked variable *after* the i2c_mux_alloc call, and this
> patch breaks such use.
> 
> So, the patch needs some reworking. Sorry for not noticing this
> earlier.

Thanks for the heads up.

3 drivers have the issue you mentioned, and two of them are not trivial
to fix. Ok, as soon as I can spend a little time on this I'll have a
look and hopefully send a fixed patch.

Regards,
-- 
Luca

Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm

[Luca Ceresoli]

Hi Peter,

On 10/10/18 17:48, Luca Ceresoli wrote:
> Hi Peter,
> 
> On 08/10/2018 23:43, Peter Rosin wrote:
>> On 2018-10-03 17:19, Luca Ceresoli wrote:
>>> From: Luca Ceresoli <luca@lucaceresoli.net>
>>>
>>> i2c-mux instantiates one i2c_algorithm for each downstream adapter.
>>> However these algorithms are all identical, depending only on the
>>> parent adapter.
>>>
>>> Avoid duplication by hoisting the i2c_algorithm from the adapters to
>>> the i2c_mux_core object, and reuse it in all the adapters.
>>
>> Ouch, while I like the concept of having one i2c_algorithm per mux,
>> this patch is not working. Various i2c-mux drivers set the
>> muxc->mux_locked variable *after* the i2c_mux_alloc call, and this
>> patch breaks such use.

I finally had a look into this issue. Three drivers are setting
mux_locked after i2c_mux_alloc: i2c-mux-gpmux, i2c-mux-gpio and
i2c-mux-pinctrl.

i2c-mux-gpmux is trivial to fix.

The other two are not trivial because:

 1. they compute mux_locked from other variables
 2. those variables are stored in the drivers "private" data
 3. their private data is stored inside struct i2c_mux_core
    (muxc->priv) which exists only after i2c_mux_alloc()

In those cases computing mux_locked before i2c_mux_alloc() involves
quite invasive changes. It took 3 non-trivial commits just for
i2c-mux-gpio, and I still have to look into i2c-mux-pinctrl.

So the question is: do we really want to do this?

Using the private storage provided by i2c_mux_alloc() is a handy
feature, at least for simpler drivers which know in advance the flags
they need to set. OTOH I don't like individual drivers to manipulate
mux_core flags that look very much like internal data. It makes any
change to the i2c mux core harder, since every changed line could have
side effects in some drivers, which is what's happening here.

What's your opinion about this issue?

Thanks,
-- 
Luca

Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm

[Peter Rosin]

On 2018-11-18 12:13, Luca Ceresoli wrote:
> Hi Peter,
> 
> On 10/10/18 17:48, Luca Ceresoli wrote:
>> Hi Peter,
>>
>> On 08/10/2018 23:43, Peter Rosin wrote:
>>> On 2018-10-03 17:19, Luca Ceresoli wrote:
>>>> From: Luca Ceresoli <luca@lucaceresoli.net>
>>>>
>>>> i2c-mux instantiates one i2c_algorithm for each downstream adapter.
>>>> However these algorithms are all identical, depending only on the
>>>> parent adapter.
>>>>
>>>> Avoid duplication by hoisting the i2c_algorithm from the adapters to
>>>> the i2c_mux_core object, and reuse it in all the adapters.
>>>
>>> Ouch, while I like the concept of having one i2c_algorithm per mux,
>>> this patch is not working. Various i2c-mux drivers set the
>>> muxc->mux_locked variable *after* the i2c_mux_alloc call, and this
>>> patch breaks such use.
> 
> I finally had a look into this issue. Three drivers are setting
> mux_locked after i2c_mux_alloc: i2c-mux-gpmux, i2c-mux-gpio and
> i2c-mux-pinctrl.
> 
> i2c-mux-gpmux is trivial to fix.
> 
> The other two are not trivial because:
> 
>  1. they compute mux_locked from other variables
>  2. those variables are stored in the drivers "private" data
>  3. their private data is stored inside struct i2c_mux_core
>     (muxc->priv) which exists only after i2c_mux_alloc()
> 
> In those cases computing mux_locked before i2c_mux_alloc() involves
> quite invasive changes. It took 3 non-trivial commits just for
> i2c-mux-gpio, and I still have to look into i2c-mux-pinctrl.
> 
> So the question is: do we really want to do this?
> 
> Using the private storage provided by i2c_mux_alloc() is a handy
> feature, at least for simpler drivers which know in advance the flags
> they need to set. OTOH I don't like individual drivers to manipulate
> mux_core flags that look very much like internal data. It makes any
> change to the i2c mux core harder, since every changed line could have
> side effects in some drivers, which is what's happening here.
> 
> What's your opinion about this issue?

I obviously don't like that drivers are poking around in struct
i2c_mux_core.

But, your description sounds precisely how I remembered it. The
underlying problem is of course that i2c-mux-gpio and
i2c-mux-pinctrl do really nasty digs into internal parts of the
gpio and the pinctrl subsystems as they *try* to figure out if
they should be mux-locked or parent-locked. The result of that
digging is not completely reliable, but it solves the issue
without help from device-tree properties in at least one case
that I know about. However, for that case I also know that there
is no risk of regression since I control the distribution of
both kernel and .dtb for any upgrade. Anyway, it was done like
it was since I at the time did not dare to question the feedback
from the device-tree camp, and actually thought it was a good
thing, and thus did not push for a device-tree property when
Rob complained about the property not describing HW and instead
was just working around kernel issues [1]. The mux-locked vs.
parent-locked property has been added since. In retrospect, the
whole attempt to auto-detect mux-locked or parent-locked was a
mistake, and everything would have been so much easier if the
device-tree could always just state what the requirement is. At
least that's my current thoughts on the matter. Maybe we should
attempt to remove the ugly auto-detect code and see if anyone
complains?

But of course, another aspect is that not everything is DT, so
perhaps there is no clean solution?

Cheers,
Peter

[1] https://lkml.org/lkml/2016/1/6/437

Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm

[Luca Ceresoli]

Hi Peter,

after some pondering, find below my reply.

On 27/11/18 19:51, Peter Rosin wrote:
> On 2018-11-18 12:13, Luca Ceresoli wrote:
>> Hi Peter,
>>
>> On 10/10/18 17:48, Luca Ceresoli wrote:
>>> Hi Peter,
>>>
>>> On 08/10/2018 23:43, Peter Rosin wrote:
>>>> On 2018-10-03 17:19, Luca Ceresoli wrote:
>>>>> From: Luca Ceresoli <luca@lucaceresoli.net>
>>>>>
>>>>> i2c-mux instantiates one i2c_algorithm for each downstream adapter.
>>>>> However these algorithms are all identical, depending only on the
>>>>> parent adapter.
>>>>>
>>>>> Avoid duplication by hoisting the i2c_algorithm from the adapters to
>>>>> the i2c_mux_core object, and reuse it in all the adapters.
>>>>
>>>> Ouch, while I like the concept of having one i2c_algorithm per mux,
>>>> this patch is not working. Various i2c-mux drivers set the
>>>> muxc->mux_locked variable *after* the i2c_mux_alloc call, and this
>>>> patch breaks such use.
>>
>> I finally had a look into this issue. Three drivers are setting
>> mux_locked after i2c_mux_alloc: i2c-mux-gpmux, i2c-mux-gpio and
>> i2c-mux-pinctrl.
>>
>> i2c-mux-gpmux is trivial to fix.
>>
>> The other two are not trivial because:
>>
>>  1. they compute mux_locked from other variables
>>  2. those variables are stored in the drivers "private" data
>>  3. their private data is stored inside struct i2c_mux_core
>>     (muxc->priv) which exists only after i2c_mux_alloc()
>>
>> In those cases computing mux_locked before i2c_mux_alloc() involves
>> quite invasive changes. It took 3 non-trivial commits just for
>> i2c-mux-gpio, and I still have to look into i2c-mux-pinctrl.
>>
>> So the question is: do we really want to do this?
>>
>> Using the private storage provided by i2c_mux_alloc() is a handy
>> feature, at least for simpler drivers which know in advance the flags
>> they need to set. OTOH I don't like individual drivers to manipulate
>> mux_core flags that look very much like internal data. It makes any
>> change to the i2c mux core harder, since every changed line could have
>> side effects in some drivers, which is what's happening here.
>>
>> What's your opinion about this issue?
> 
> I obviously don't like that drivers are poking around in struct
> i2c_mux_core.
> 
> But, your description sounds precisely how I remembered it. The
> underlying problem is of course that i2c-mux-gpio and
> i2c-mux-pinctrl do really nasty digs into internal parts of the
> gpio and the pinctrl subsystems as they *try* to figure out if
> they should be mux-locked or parent-locked. The result of that
> digging is not completely reliable, but it solves the issue
> without help from device-tree properties in at least one case
> that I know about. However, for that case I also know that there
> is no risk of regression since I control the distribution of
> both kernel and .dtb for any upgrade. Anyway, it was done like
> it was since I at the time did not dare to question the feedback
> from the device-tree camp, and actually thought it was a good
> thing, and thus did not push for a device-tree property when
> Rob complained about the property not describing HW and instead
> was just working around kernel issues [1]. The mux-locked vs.
> parent-locked property has been added since. In retrospect, the
> whole attempt to auto-detect mux-locked or parent-locked was a
> mistake, and everything would have been so much easier if the
> device-tree could always just state what the requirement is. At
> least that's my current thoughts on the matter. Maybe we should
> attempt to remove the ugly auto-detect code and see if anyone
> complains?

I'd love to remove that code, but this would change existing behavior,
and the change would go unnoticed until something breaks. Scary.

What about requiring that either "mux-locked" or "parent-locked" is
specified in DT? If none is specified, the driver would fail loudly at
probe time and users would stop and fix their DT. Then of course they
need to _upgrade_ their DT in existing products.

> But of course, another aspect is that not everything is DT, so
> perhaps there is no clean solution?

Well, platform code would have to be changed like DT, adding 2 flags:
mux-locked and parent-locked, and exactly one must be set.

This seems to me the safest path. Do you think it is a good idea,
especially with respect to the device tree camp?

Thanks,
-- 
Luca