* [PATCH v3] i2c: mux: remove duplicated i2c_algorithm @ 2018-10-03 15:19 Luca Ceresoli 2018-10-08 21:43 ` Peter Rosin 0 siblings, 1 reply; 6+ messages in thread From: Luca Ceresoli @ 2018-10-03 15:19 UTC (permalink / raw) To: linux-i2c; +Cc: linux-kernel, Luca Ceresoli, Wolfram Sang, Peter Rosin 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm 2018-10-03 15:19 [PATCH v3] i2c: mux: remove duplicated i2c_algorithm Luca Ceresoli @ 2018-10-08 21:43 ` Peter Rosin 2018-10-10 15:48 ` Luca Ceresoli 0 siblings, 1 reply; 6+ messages in thread From: Peter Rosin @ 2018-10-08 21:43 UTC (permalink / raw) To: Luca Ceresoli, linux-i2c; +Cc: linux-kernel, Luca Ceresoli, Wolfram Sang 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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm 2018-10-08 21:43 ` Peter Rosin @ 2018-10-10 15:48 ` Luca Ceresoli 2018-11-18 11:13 ` Luca Ceresoli 0 siblings, 1 reply; 6+ messages in thread From: Luca Ceresoli @ 2018-10-10 15:48 UTC (permalink / raw) To: Peter Rosin, linux-i2c; +Cc: linux-kernel, Luca Ceresoli, Wolfram Sang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm 2018-10-10 15:48 ` Luca Ceresoli @ 2018-11-18 11:13 ` Luca Ceresoli 2018-11-27 18:51 ` Peter Rosin 0 siblings, 1 reply; 6+ messages in thread From: Luca Ceresoli @ 2018-11-18 11:13 UTC (permalink / raw) To: Luca Ceresoli, Peter Rosin, linux-i2c; +Cc: linux-kernel, Wolfram Sang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm 2018-11-18 11:13 ` Luca Ceresoli @ 2018-11-27 18:51 ` Peter Rosin 2018-12-09 21:46 ` Luca Ceresoli 0 siblings, 1 reply; 6+ messages in thread From: Peter Rosin @ 2018-11-27 18:51 UTC (permalink / raw) To: Luca Ceresoli, Luca Ceresoli, linux-i2c; +Cc: linux-kernel, Wolfram Sang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] i2c: mux: remove duplicated i2c_algorithm 2018-11-27 18:51 ` Peter Rosin @ 2018-12-09 21:46 ` Luca Ceresoli 0 siblings, 0 replies; 6+ messages in thread From: Luca Ceresoli @ 2018-12-09 21:46 UTC (permalink / raw) To: Peter Rosin, Luca Ceresoli, linux-i2c; +Cc: linux-kernel, Wolfram Sang 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-09 21:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-03 15:19 [PATCH v3] i2c: mux: remove duplicated i2c_algorithm Luca Ceresoli 2018-10-08 21:43 ` Peter Rosin 2018-10-10 15:48 ` Luca Ceresoli 2018-11-18 11:13 ` Luca Ceresoli 2018-11-27 18:51 ` Peter Rosin 2018-12-09 21:46 ` Luca Ceresoli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).