| Message ID | 20180320093200.19179-4-peda@axentia.se |
|---|---|
| State | Superseded |
| Delegated to: | Peter Rosin |
| Headers | show |
| Series | [1/3] i2c: mux: pca9541: use the BIT macro | expand |
On 03/20/2018 02:32 AM, Peter Rosin wrote: > Make the arbitrate and release_bus implementation chip specific. > > Signed-off-by: Peter Rosin <peda@axentia.se> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++++++++++++++++++++++++++---------- > 1 file changed, 45 insertions(+), 17 deletions(-) > > diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c > index 47685eb4e0e9..cac629e36bf8 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca9541.c > +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c > @@ -23,6 +23,7 @@ > #include <linux/i2c-mux.h> > #include <linux/jiffies.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/platform_data/pca954x.h> > #include <linux/slab.h> > > @@ -70,26 +71,22 @@ > #define SELECT_DELAY_SHORT 50 > #define SELECT_DELAY_LONG 1000 > > -struct pca9541 { > - struct i2c_client *client; > - unsigned long select_timeout; > - unsigned long arb_timeout; > +enum chip_name { > + pca9541, > }; > > -static const struct i2c_device_id pca9541_id[] = { > - {"pca9541", 0}, > - {} > +struct chip_desc { > + int (*arbitrate)(struct i2c_client *client); > + void (*release_bus)(struct i2c_client *client); > }; > > -MODULE_DEVICE_TABLE(i2c, pca9541_id); > +struct pca9541 { > + const struct chip_desc *chip; > > -#ifdef CONFIG_OF > -static const struct of_device_id pca9541_of_match[] = { > - { .compatible = "nxp,pca9541" }, > - {} > + struct i2c_client *client; > + unsigned long select_timeout; > + unsigned long arb_timeout; > }; > -MODULE_DEVICE_TABLE(of, pca9541_of_match); > -#endif > > static int pca9541_mybus(int ctl) > { > @@ -318,7 +315,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan) > /* force bus ownership after this time */ > > do { > - ret = pca9541_arbitrate(client); > + ret = data->chip->arbitrate(client); > if (ret) > return ret < 0 ? ret : 0; > > @@ -336,10 +333,32 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) > struct pca9541 *data = i2c_mux_priv(muxc); > struct i2c_client *client = data->client; > > - pca9541_release_bus(client); > + data->chip->release_bus(client); > return 0; > } > > +static const struct chip_desc chips[] = { > + [pca9541] = { > + .arbitrate = pca9541_arbitrate, > + .release_bus = pca9541_release_bus, > + }, > +}; > + > +static const struct i2c_device_id pca9541_id[] = { > + { "pca9541", pca9541 }, > + {} > +}; > + > +MODULE_DEVICE_TABLE(i2c, pca9541_id); > + > +#ifdef CONFIG_OF > +static const struct of_device_id pca9541_of_match[] = { > + { .compatible = "nxp,pca9541", .data = &chips[pca9541] }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, pca9541_of_match); > +#endif > + > /* > * I2C init/probing/exit functions > */ > @@ -348,6 +367,8 @@ static int pca9541_probe(struct i2c_client *client, > { > struct i2c_adapter *adap = client->adapter; > struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev); > + const struct of_device_id *match; > + const struct chip_desc *chip; > struct i2c_mux_core *muxc; > struct pca9541 *data; > int force; > @@ -356,12 +377,18 @@ static int pca9541_probe(struct i2c_client *client, > if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA)) > return -ENODEV; > > + match = of_match_device(of_match_ptr(pca9541_of_match), &client->dev); > + if (match) > + chip = of_device_get_match_data(&client->dev); > + else > + chip = &chips[id->driver_data]; > + > /* > * I2C accesses are unprotected here. > * We have to lock the adapter before releasing the bus. > */ > i2c_lock_adapter(adap); > - pca9541_release_bus(client); > + chip->release_bus(client); > i2c_unlock_adapter(adap); > > /* Create mux adapter */ > @@ -376,6 +403,7 @@ static int pca9541_probe(struct i2c_client *client, > return -ENOMEM; > > data = i2c_mux_priv(muxc); > + data->chip = chip; > data->client = client; > > i2c_set_clientdata(client, muxc); >
Hi Peter, Ken, On 03/20/2018 11:32 AM, Peter Rosin wrote: > Make the arbitrate and release_bus implementation chip specific. > by chance I took a look at the original implementation done by Ken, and I would say that this 3/3 change is an overkill as a too generic one. Is there any next observable extension? And do two abstracted (*arbitrate) and (*release_bus) cover it well? Probably no. At first it would be simpler to add a new chip id field into struct pca9541 (struct rename would be needed of course), and do a selection of specific pca9x41_arbitrate() and pca9x41_release_bus() depending on it: ----8<---- diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c index 47685eb..a40e6d8 100644 --- a/drivers/i2c/muxes/i2c-mux-pca9541.c +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c @@ -70,8 +70,13 @@ #define SELECT_DELAY_SHORT 50 #define SELECT_DELAY_LONG 1000 +enum chip_id { + pca9541, +}; + struct pca9541 { struct i2c_client *client; + enum chip_id id; unsigned long select_timeout; unsigned long arb_timeout; }; @@ -188,10 +193,13 @@ static int pca9541_reg_read(struct i2c_client *client, u8 command) */ /* Release bus. Also reset NTESTON and BUSINIT if it was set. */ -static void pca9541_release_bus(struct i2c_client *client) +static void pca9541_release_bus(struct i2c_client *client, enum chip_id id) { int reg; + if (id != pca9541) + return; + reg = pca9541_reg_read(client, PCA9541_CONTROL); if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg)) pca9541_reg_write(client, PCA9541_CONTROL, @@ -235,12 +243,15 @@ static const u8 pca9541_control[16] = { * 0 : bus not acquired * 1 : bus acquired */ -static int pca9541_arbitrate(struct i2c_client *client) +static int pca9541_arbitrate(struct i2c_client *client, enum chip_id id) { struct i2c_mux_core *muxc = i2c_get_clientdata(client); struct pca9541 *data = i2c_mux_priv(muxc); int reg; + if (id != pca9541) + return -EOPNOTSUPP; + reg = pca9541_reg_read(client, PCA9541_CONTROL); if (reg < 0) return reg; @@ -318,7 +329,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan) /* force bus ownership after this time */ do { - ret = pca9541_arbitrate(client); + ret = pca9541_arbitrate(client, data->id); if (ret) return ret < 0 ? ret : 0; @@ -336,7 +347,7 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) struct pca9541 *data = i2c_mux_priv(muxc); struct i2c_client *client = data->client; - pca9541_release_bus(client); + pca9541_release_bus(client, data->id); return 0; } @@ -361,7 +372,7 @@ static int pca9541_probe(struct i2c_client *client, * We have to lock the adapter before releasing the bus. */ i2c_lock_adapter(adap); - pca9541_release_bus(client); + pca9541_release_bus(client, pca9541); i2c_unlock_adapter(adap); /* Create mux adapter */ @@ -377,6 +388,7 @@ static int pca9541_probe(struct i2c_client *client, data = i2c_mux_priv(muxc); data->client = client; + data->id = pca9541; i2c_set_clientdata(client, muxc); ----8<---- > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++++++++++++++++++++++++++---------- > 1 file changed, 45 insertions(+), 17 deletions(-) > The change above is trivial and it does not cancel any further extensions similar to your idea, the open question is if there is a demand right at the moment. > diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c > index 47685eb4e0e9..cac629e36bf8 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca9541.c > +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c > @@ -23,6 +23,7 @@ > #include <linux/i2c-mux.h> > #include <linux/jiffies.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/platform_data/pca954x.h> > #include <linux/slab.h> > > @@ -70,26 +71,22 @@ > #define SELECT_DELAY_SHORT 50 > #define SELECT_DELAY_LONG 1000 > > -struct pca9541 { > - struct i2c_client *client; > - unsigned long select_timeout; > - unsigned long arb_timeout; > +enum chip_name { chip_name sound like a string storage, chip_id might be better here. > + pca9541, > }; > -- With best wishes, Vladimir
On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote: > Hi Peter, Ken, > > On 03/20/2018 11:32 AM, Peter Rosin wrote: >> Make the arbitrate and release_bus implementation chip specific. >> > > by chance I took a look at the original implementation done by Ken, and > I would say that this 3/3 change is an overkill as a too generic one. > Is there any next observable extension? And do two abstracted (*arbitrate) > and (*release_bus) cover it well? Probably no. > > At first it would be simpler to add a new chip id field into struct pca9541 > (struct rename would be needed of course), and do a selection of specific > pca9x41_arbitrate() and pca9x41_release_bus() depending on it: > FWIW, I very much prefer Peter's code. I think it is much cleaner. Guenter > ----8<---- > diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c > index 47685eb..a40e6d8 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca9541.c > +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c > @@ -70,8 +70,13 @@ > #define SELECT_DELAY_SHORT 50 > #define SELECT_DELAY_LONG 1000 > > +enum chip_id { > + pca9541, > +}; > + > struct pca9541 { > struct i2c_client *client; > + enum chip_id id; > unsigned long select_timeout; > unsigned long arb_timeout; > }; > @@ -188,10 +193,13 @@ static int pca9541_reg_read(struct i2c_client *client, u8 command) > */ > > /* Release bus. Also reset NTESTON and BUSINIT if it was set. */ > -static void pca9541_release_bus(struct i2c_client *client) > +static void pca9541_release_bus(struct i2c_client *client, enum chip_id id) > { > int reg; > > + if (id != pca9541) > + return; > + > reg = pca9541_reg_read(client, PCA9541_CONTROL); > if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg)) > pca9541_reg_write(client, PCA9541_CONTROL, > @@ -235,12 +243,15 @@ static const u8 pca9541_control[16] = { > * 0 : bus not acquired > * 1 : bus acquired > */ > -static int pca9541_arbitrate(struct i2c_client *client) > +static int pca9541_arbitrate(struct i2c_client *client, enum chip_id id) > { > struct i2c_mux_core *muxc = i2c_get_clientdata(client); > struct pca9541 *data = i2c_mux_priv(muxc); > int reg; > > + if (id != pca9541) > + return -EOPNOTSUPP; > + > reg = pca9541_reg_read(client, PCA9541_CONTROL); > if (reg < 0) > return reg; > @@ -318,7 +329,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan) > /* force bus ownership after this time */ > > do { > - ret = pca9541_arbitrate(client); > + ret = pca9541_arbitrate(client, data->id); > if (ret) > return ret < 0 ? ret : 0; > > @@ -336,7 +347,7 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) > struct pca9541 *data = i2c_mux_priv(muxc); > struct i2c_client *client = data->client; > > - pca9541_release_bus(client); > + pca9541_release_bus(client, data->id); > return 0; > } > > @@ -361,7 +372,7 @@ static int pca9541_probe(struct i2c_client *client, > * We have to lock the adapter before releasing the bus. > */ > i2c_lock_adapter(adap); > - pca9541_release_bus(client); > + pca9541_release_bus(client, pca9541); > i2c_unlock_adapter(adap); > > /* Create mux adapter */ > @@ -377,6 +388,7 @@ static int pca9541_probe(struct i2c_client *client, > > data = i2c_mux_priv(muxc); > data->client = client; > + data->id = pca9541; > > i2c_set_clientdata(client, muxc); > > ----8<---- > > >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++++++++++++++++++++++++++---------- >> 1 file changed, 45 insertions(+), 17 deletions(-) >> > > The change above is trivial and it does not cancel any further > extensions similar to your idea, the open question is if there > is a demand right at the moment. > >> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c >> index 47685eb4e0e9..cac629e36bf8 100644 >> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c >> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c >> @@ -23,6 +23,7 @@ >> #include <linux/i2c-mux.h> >> #include <linux/jiffies.h> >> #include <linux/module.h> >> +#include <linux/of_device.h> >> #include <linux/platform_data/pca954x.h> >> #include <linux/slab.h> >> >> @@ -70,26 +71,22 @@ >> #define SELECT_DELAY_SHORT 50 >> #define SELECT_DELAY_LONG 1000 >> >> -struct pca9541 { >> - struct i2c_client *client; >> - unsigned long select_timeout; >> - unsigned long arb_timeout; >> +enum chip_name { > > chip_name sound like a string storage, chip_id might be better here. > >> + pca9541, >> }; >> > > -- > With best wishes, > Vladimir >
On 03/21/2018 03:19 AM, Guenter Roeck wrote: > On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote: >> Hi Peter, Ken, >> >> On 03/20/2018 11:32 AM, Peter Rosin wrote: >>> Make the arbitrate and release_bus implementation chip specific. >>> >> >> by chance I took a look at the original implementation done by Ken, and >> I would say that this 3/3 change is an overkill as a too generic one. >> Is there any next observable extension? And do two abstracted (*arbitrate) >> and (*release_bus) cover it well? Probably no. >> >> At first it would be simpler to add a new chip id field into struct pca9541 >> (struct rename would be needed of course), and do a selection of specific >> pca9x41_arbitrate() and pca9x41_release_bus() depending on it: >> > > FWIW, I very much prefer Peter's code. I think it is much cleaner. Peter's code is generic, and it makes the change about 3 times longer in lines of code, and the following pca9641 change on top of it will be larger as well, because generalization requires service. My main concern is that if such generalization is really needed in the driver. -- With best wishes, Vladimir
On 03/20/2018 11:32 AM, Peter Rosin wrote: > Make the arbitrate and release_bus implementation chip specific. > > Signed-off-by: Peter Rosin <peda@axentia.se> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> The change is really good and correct, it is just too extended IMHO. -- With best wishes, Vladimir
On 2018-03-21 08:01, Vladimir Zapolskiy wrote: > On 03/21/2018 03:19 AM, Guenter Roeck wrote: >> On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote: >>> Hi Peter, Ken, >>> >>> On 03/20/2018 11:32 AM, Peter Rosin wrote: >>>> Make the arbitrate and release_bus implementation chip specific. >>>> >>> >>> by chance I took a look at the original implementation done by Ken, and >>> I would say that this 3/3 change is an overkill as a too generic one. >>> Is there any next observable extension? And do two abstracted (*arbitrate) >>> and (*release_bus) cover it well? Probably no. >>> >>> At first it would be simpler to add a new chip id field into struct pca9541 >>> (struct rename would be needed of course), and do a selection of specific >>> pca9x41_arbitrate() and pca9x41_release_bus() depending on it: >>> >> >> FWIW, I very much prefer Peter's code. I think it is much cleaner. > > Peter's code is generic, and it makes the change about 3 times longer in lines > of code, and the following pca9641 change on top of it will be larger as well, > because generalization requires service. > > My main concern is that if such generalization is really needed in the driver. I just did a comparison of what would happen if I took the same shortcuts you did, and I got 18 new lines and 3 changed lines (and some moved lines that could have been a separate patch). You have 12 new lines and 5 changed lines. So, the big difference is that I add the of_match_device call while you do not. So, it looks like you are comparing apples and oranges. Do you have a reason for not calling of_match_device? Or were you punting that for the patch adding PCA9641 support? That's odd, because the point of the patch is to prepare for smooth addition of that support. Also, I think my code allows adding support for PCA9641 with only new lines, while your version requires changing of code. So, I'm rejecting your arguments that your patch is significantly simpler. And while I'm obviously a tad bit biased, I do agree with Guenter that my structure is cleaner. Cheers, Peter
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c index 47685eb4e0e9..cac629e36bf8 100644 --- a/drivers/i2c/muxes/i2c-mux-pca9541.c +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c @@ -23,6 +23,7 @@ #include <linux/i2c-mux.h> #include <linux/jiffies.h> #include <linux/module.h> +#include <linux/of_device.h> #include <linux/platform_data/pca954x.h> #include <linux/slab.h> @@ -70,26 +71,22 @@ #define SELECT_DELAY_SHORT 50 #define SELECT_DELAY_LONG 1000 -struct pca9541 { - struct i2c_client *client; - unsigned long select_timeout; - unsigned long arb_timeout; +enum chip_name { + pca9541, }; -static const struct i2c_device_id pca9541_id[] = { - {"pca9541", 0}, - {} +struct chip_desc { + int (*arbitrate)(struct i2c_client *client); + void (*release_bus)(struct i2c_client *client); }; -MODULE_DEVICE_TABLE(i2c, pca9541_id); +struct pca9541 { + const struct chip_desc *chip; -#ifdef CONFIG_OF -static const struct of_device_id pca9541_of_match[] = { - { .compatible = "nxp,pca9541" }, - {} + struct i2c_client *client; + unsigned long select_timeout; + unsigned long arb_timeout; }; -MODULE_DEVICE_TABLE(of, pca9541_of_match); -#endif static int pca9541_mybus(int ctl) { @@ -318,7 +315,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan) /* force bus ownership after this time */ do { - ret = pca9541_arbitrate(client); + ret = data->chip->arbitrate(client); if (ret) return ret < 0 ? ret : 0; @@ -336,10 +333,32 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) struct pca9541 *data = i2c_mux_priv(muxc); struct i2c_client *client = data->client; - pca9541_release_bus(client); + data->chip->release_bus(client); return 0; } +static const struct chip_desc chips[] = { + [pca9541] = { + .arbitrate = pca9541_arbitrate, + .release_bus = pca9541_release_bus, + }, +}; + +static const struct i2c_device_id pca9541_id[] = { + { "pca9541", pca9541 }, + {} +}; + +MODULE_DEVICE_TABLE(i2c, pca9541_id); + +#ifdef CONFIG_OF +static const struct of_device_id pca9541_of_match[] = { + { .compatible = "nxp,pca9541", .data = &chips[pca9541] }, + {} +}; +MODULE_DEVICE_TABLE(of, pca9541_of_match); +#endif + /* * I2C init/probing/exit functions */ @@ -348,6 +367,8 @@ static int pca9541_probe(struct i2c_client *client, { struct i2c_adapter *adap = client->adapter; struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev); + const struct of_device_id *match; + const struct chip_desc *chip; struct i2c_mux_core *muxc; struct pca9541 *data; int force; @@ -356,12 +377,18 @@ static int pca9541_probe(struct i2c_client *client, if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA)) return -ENODEV; + match = of_match_device(of_match_ptr(pca9541_of_match), &client->dev); + if (match) + chip = of_device_get_match_data(&client->dev); + else + chip = &chips[id->driver_data]; + /* * I2C accesses are unprotected here. * We have to lock the adapter before releasing the bus. */ i2c_lock_adapter(adap); - pca9541_release_bus(client); + chip->release_bus(client); i2c_unlock_adapter(adap); /* Create mux adapter */ @@ -376,6 +403,7 @@ static int pca9541_probe(struct i2c_client *client, return -ENOMEM; data = i2c_mux_priv(muxc); + data->chip = chip; data->client = client; i2c_set_clientdata(client, muxc);
Make the arbitrate and release_bus implementation chip specific. Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++++++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 17 deletions(-)