| Message ID | 20180320061909.5775-1-chen.kenyy@inventec.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Peter Rosin |
| Headers | show |
| Series | [linux,dev-4.16,v2] i2c: muxes: pca9641: new driver | expand |
Hi Ken, A note on your subject line: we use the "linux dev-4.16" style tags in OpenBMC to indicate which branch you're targetting, but in upstream Linux we always target the next release, so you don't need to use --subject-prefix at all. On Tue, Mar 20, 2018 at 4:49 PM, Ken Chen <chen.kenyy@inventec.com> wrote: > Signed-off-by: Ken Chen <chen.kenyy@inventec.com> Try to add some words to the commit message describing why you're making the change. I'll leave it to Peter and Guneter to review the implementation. Cheers, Joel > > --- > v1->v2 > - Merged PCA9641 code into i2c-mux-pca9541.c > - Modified title > - Add PCA9641 detect function > --- > drivers/i2c/muxes/i2c-mux-pca9541.c | 184 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 174 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c > index 6a39ada..493f947 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca9541.c > +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c > @@ -1,5 +1,5 @@ > /* > - * I2C multiplexer driver for PCA9541 bus master selector > + * I2C multiplexer driver for PCA9541/PCA9641 bus master selector > * > * Copyright (c) 2010 Ericsson AB. > * > @@ -26,8 +26,8 @@ > #include <linux/slab.h> > > /* > - * The PCA9541 is a bus master selector. It supports two I2C masters connected > - * to a single slave bus. > + * The PCA9541/PCA9641 is a bus master selector. It supports two I2C masters > + * connected to a single slave bus. > * > * Before each bus transaction, a master has to acquire bus ownership. After the > * transaction is complete, bus ownership has to be released. This fits well > @@ -58,11 +58,43 @@ > #define PCA9541_ISTAT_MYTEST (1 << 6) > #define PCA9541_ISTAT_NMYTEST (1 << 7) > > +#define PCA9641_ID 0x00 > +#define PCA9641_ID_MAGIC 0x38 > + > +#define PCA9641_CONTROL 0x01 > +#define PCA9641_STATUS 0x02 > +#define PCA9641_TIME 0x03 > + > +#define PCA9641_CTL_LOCK_REQ BIT(0) > +#define PCA9641_CTL_LOCK_GRANT BIT(1) > +#define PCA9641_CTL_BUS_CONNECT BIT(2) > +#define PCA9641_CTL_BUS_INIT BIT(3) > +#define PCA9641_CTL_SMBUS_SWRST BIT(4) > +#define PCA9641_CTL_IDLE_TIMER_DIS BIT(5) > +#define PCA9641_CTL_SMBUS_DIS BIT(6) > +#define PCA9641_CTL_PRIORITY BIT(7) > + > +#define PCA9641_STS_OTHER_LOCK BIT(0) > +#define PCA9641_STS_BUS_INIT_FAIL BIT(1) > +#define PCA9641_STS_BUS_HUNG BIT(2) > +#define PCA9641_STS_MBOX_EMPTY BIT(3) > +#define PCA9641_STS_MBOX_FULL BIT(4) > +#define PCA9641_STS_TEST_INT BIT(5) > +#define PCA9641_STS_SCL_IO BIT(6) > +#define PCA9641_STS_SDA_IO BIT(7) > + > +#define PCA9641_RES_TIME 0x03 > + > #define BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON) > #define MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS) > #define mybus(x) (!((x) & MYBUS) || ((x) & MYBUS) == MYBUS) > #define busoff(x) (!((x) & BUSON) || ((x) & BUSON) == BUSON) > > +#define BUSOFF(x, y) (!((x) & PCA9641_CTL_LOCK_GRANT) && \ > + !((y) & PCA9641_STS_OTHER_LOCK)) > +#define other_lock(x) ((x) & PCA9641_STS_OTHER_LOCK) > +#define lock_grant(x) ((x) & PCA9641_CTL_LOCK_GRANT) > + > /* arbitration timeouts, in jiffies */ > #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */ > #define ARB2_TIMEOUT (HZ / 4) /* 250 ms until acquisition failure */ > @@ -79,6 +111,7 @@ struct pca9541 { > > static const struct i2c_device_id pca9541_id[] = { > {"pca9541", 0}, > + {"pca9641", 1}, > {} > }; > > @@ -87,6 +120,7 @@ MODULE_DEVICE_TABLE(i2c, pca9541_id); > #ifdef CONFIG_OF > static const struct of_device_id pca9541_of_match[] = { > { .compatible = "nxp,pca9541" }, > + { .compatible = "nxp,pca9641" }, > {} > }; > MODULE_DEVICE_TABLE(of, pca9541_of_match); > @@ -328,6 +362,125 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) > } > > /* > + * Arbitration management functions > + */ > +static void pca9641_release_bus(struct i2c_client *client) > +{ > + pca9541_reg_write(client, PCA9641_CONTROL, 0); > +} > + > +/* > + * Channel arbitration > + * > + * Return values: > + * <0: error > + * 0 : bus not acquired > + * 1 : bus acquired > + */ > +static int pca9641_arbitrate(struct i2c_client *client) > +{ > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + struct pca9541 *data = i2c_mux_priv(muxc); > + int reg_ctl, reg_sts; > + > + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); > + if (reg_ctl < 0) > + return reg_ctl; > + reg_sts = pca9541_reg_read(client, PCA9641_STATUS); > + > + if (BUSOFF(reg_ctl, reg_sts)) { > + /* > + * Bus is off. Request ownership or turn it on unless > + * other master requested ownership. > + */ > + reg_ctl |= PCA9641_CTL_LOCK_REQ; > + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); > + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); > + > + if (lock_grant(reg_ctl)) { > + /* > + * Other master did not request ownership, > + * or arbitration timeout expired. Take the bus. > + */ > + reg_ctl |= PCA9641_CTL_BUS_CONNECT > + | PCA9641_CTL_LOCK_REQ; > + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); > + data->select_timeout = SELECT_DELAY_SHORT; > + > + return 1; > + } else { > + /* > + * Other master requested ownership. > + * Set extra long timeout to give it time to acquire it. > + */ > + data->select_timeout = SELECT_DELAY_LONG * 2; > + } > + } else if (lock_grant(reg_ctl)) { > + /* > + * Bus is on, and we own it. We are done with acquisition. > + */ > + reg_ctl |= PCA9641_CTL_BUS_CONNECT | PCA9641_CTL_LOCK_REQ; > + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); > + > + return 1; > + } else if (other_lock(reg_sts)) { > + /* > + * Other master owns the bus. > + * If arbitration timeout has expired, force ownership. > + * Otherwise request it. > + */ > + data->select_timeout = SELECT_DELAY_LONG; > + reg_ctl |= PCA9641_CTL_LOCK_REQ; > + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); > + } > + return 0; > +} > + > +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan) > +{ > + struct pca9541 *data = i2c_mux_priv(muxc); > + struct i2c_client *client = data->client; > + int ret; > + unsigned long timeout = jiffies + ARB2_TIMEOUT; > + /* give up after this time */ > + > + data->arb_timeout = jiffies + ARB_TIMEOUT; > + /* force bus ownership after this time */ > + > + do { > + ret = pca9641_arbitrate(client); > + if (ret) > + return ret < 0 ? ret : 0; > + > + if (data->select_timeout == SELECT_DELAY_SHORT) > + udelay(data->select_timeout); > + else > + msleep(data->select_timeout / 1000); > + } while (time_is_after_eq_jiffies(timeout)); > + > + return -ETIMEDOUT; > +} > + > +static int pca9641_release_chan(struct i2c_mux_core *muxc, u32 chan) > +{ > + struct pca9541 *data = i2c_mux_priv(muxc); > + struct i2c_client *client = data->client; > + > + pca9641_release_bus(client); > + return 0; > +} > + > +static int pca9641_detect_id(struct i2c_client *client) > +{ > + int reg; > + > + reg = pca9541_reg_read(client, PCA9641_ID); > + if (reg == PCA9641_ID_MAGIC) > + return 1; > + else > + return 0; > +} > +/* > * I2C init/probing/exit functions > */ > static int pca9541_probe(struct i2c_client *client, > @@ -339,34 +492,45 @@ static int pca9541_probe(struct i2c_client *client, > struct pca9541 *data; > int force; > int ret; > + int detect_id; > > if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA)) > return -ENODEV; > > + detect_id = pca9641_detect_id(client); > /* > * I2C accesses are unprotected here. > * We have to lock the adapter before releasing the bus. > */ > - i2c_lock_adapter(adap); > - pca9541_release_bus(client); > - i2c_unlock_adapter(adap); > - > + if (detect_id == 0) { > + i2c_lock_adapter(adap); > + pca9541_release_bus(client); > + i2c_unlock_adapter(adap); > + } else { > + i2c_lock_adapter(adap); > + pca9641_release_bus(client); > + i2c_unlock_adapter(adap); > + } > /* Create mux adapter */ > > force = 0; > if (pdata) > force = pdata->modes[0].adap_id; > - muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), > + if (detect_id == 0) { > + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), > I2C_MUX_ARBITRATOR, > pca9541_select_chan, pca9541_release_chan); > + } else { > + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), > + I2C_MUX_ARBITRATOR, > + pca9641_select_chan, pca9641_release_chan); > + } > if (!muxc) > return -ENOMEM; > > data = i2c_mux_priv(muxc); > data->client = client; > - > i2c_set_clientdata(client, muxc); > - You can leave the whitespace as it is here. > ret = i2c_mux_add_adapter(muxc, force, 0, 0); > if (ret) > return ret; > -- > 2.9.3 >
On 2018-03-20 07:19, Ken Chen wrote: > Signed-off-by: Ken Chen <chen.kenyy@inventec.com> Ok, now that you are not adding a new driver, but instead modify an existing driver, the subject I requested in no longer relevant. Now I would like to see: i2c: mux: pca9541: add support for PCA9641 chips Or something like that. > --- > v1->v2 > - Merged PCA9641 code into i2c-mux-pca9541.c > - Modified title > - Add PCA9641 detect function > --- > drivers/i2c/muxes/i2c-mux-pca9541.c | 184 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 174 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c > index 6a39ada..493f947 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca9541.c > +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c > @@ -1,5 +1,5 @@ > /* > - * I2C multiplexer driver for PCA9541 bus master selector > + * I2C multiplexer driver for PCA9541/PCA9641 bus master selector > * > * Copyright (c) 2010 Ericsson AB. > * > @@ -26,8 +26,8 @@ > #include <linux/slab.h> > > /* > - * The PCA9541 is a bus master selector. It supports two I2C masters connected > - * to a single slave bus. > + * The PCA9541/PCA9641 is a bus master selector. It supports two I2C masters PCA9541 and PCA9641 are bus master selectors. They support two I2C masters And make sure to lose the trailing space. > + * connected to a single slave bus. > * > * Before each bus transaction, a master has to acquire bus ownership. After the > * transaction is complete, bus ownership has to be released. This fits well > @@ -58,11 +58,43 @@ > #define PCA9541_ISTAT_MYTEST (1 << 6) > #define PCA9541_ISTAT_NMYTEST (1 << 7) > > +#define PCA9641_ID 0x00 > +#define PCA9641_ID_MAGIC 0x38 > + > +#define PCA9641_CONTROL 0x01 > +#define PCA9641_STATUS 0x02 > +#define PCA9641_TIME 0x03 > + > +#define PCA9641_CTL_LOCK_REQ BIT(0) > +#define PCA9641_CTL_LOCK_GRANT BIT(1) > +#define PCA9641_CTL_BUS_CONNECT BIT(2) > +#define PCA9641_CTL_BUS_INIT BIT(3) > +#define PCA9641_CTL_SMBUS_SWRST BIT(4) > +#define PCA9641_CTL_IDLE_TIMER_DIS BIT(5) > +#define PCA9641_CTL_SMBUS_DIS BIT(6) > +#define PCA9641_CTL_PRIORITY BIT(7) > + > +#define PCA9641_STS_OTHER_LOCK BIT(0) > +#define PCA9641_STS_BUS_INIT_FAIL BIT(1) > +#define PCA9641_STS_BUS_HUNG BIT(2) > +#define PCA9641_STS_MBOX_EMPTY BIT(3) > +#define PCA9641_STS_MBOX_FULL BIT(4) > +#define PCA9641_STS_TEST_INT BIT(5) > +#define PCA9641_STS_SCL_IO BIT(6) > +#define PCA9641_STS_SDA_IO BIT(7) > + > +#define PCA9641_RES_TIME 0x03 > + > #define BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON) > #define MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS) > #define mybus(x) (!((x) & MYBUS) || ((x) & MYBUS) == MYBUS) > #define busoff(x) (!((x) & BUSON) || ((x) & BUSON) == BUSON) > > +#define BUSOFF(x, y) (!((x) & PCA9641_CTL_LOCK_GRANT) && \ > + !((y) & PCA9641_STS_OTHER_LOCK)) > +#define other_lock(x) ((x) & PCA9641_STS_OTHER_LOCK) > +#define lock_grant(x) ((x) & PCA9641_CTL_LOCK_GRANT) These macro names are now completely hideous. They were bad before, but this is just too much for me. So, instead of adding BUSOFF etc, I would like to see all the macros with a chip prefix. But I think they will get overly long, so I think you should just write trivial pca9541_mybus, pca9541_busoff, pca9641_busoff etc functions. The compiler should inline them just fine. The rename of the existing macros and their conversion to functions should be in the first preparatory patch that I mention below. The new functions should be in the second patch. > + > /* arbitration timeouts, in jiffies */ > #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */ > #define ARB2_TIMEOUT (HZ / 4) /* 250 ms until acquisition failure */ > @@ -79,6 +111,7 @@ struct pca9541 { > > static const struct i2c_device_id pca9541_id[] = { > {"pca9541", 0}, > + {"pca9641", 1}, You are actually not using this 0/1 difference. Have a look at e.g. how the i2c-mux-pca954x driver uses this as an index into a chip description array. I would like to see something similar here... > {} > }; > > @@ -87,6 +120,7 @@ MODULE_DEVICE_TABLE(i2c, pca9541_id); > #ifdef CONFIG_OF > static const struct of_device_id pca9541_of_match[] = { > { .compatible = "nxp,pca9541" }, > + { .compatible = "nxp,pca9641" }, ...including pointers to the above chip descriptions here, just like the pca954x driver. > {} > }; > MODULE_DEVICE_TABLE(of, pca9541_of_match); > @@ -328,6 +362,125 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) > } > > /* > + * Arbitration management functions > + */ > +static void pca9641_release_bus(struct i2c_client *client) > +{ > + pca9541_reg_write(client, PCA9641_CONTROL, 0); > +} > + > +/* > + * Channel arbitration > + * > + * Return values: > + * <0: error > + * 0 : bus not acquired > + * 1 : bus acquired > + */ > +static int pca9641_arbitrate(struct i2c_client *client) > +{ > + struct i2c_mux_core *muxc = i2c_get_clientdata(client); > + struct pca9541 *data = i2c_mux_priv(muxc); > + int reg_ctl, reg_sts; > + > + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); > + if (reg_ctl < 0) > + return reg_ctl; > + reg_sts = pca9541_reg_read(client, PCA9641_STATUS); > + > + if (BUSOFF(reg_ctl, reg_sts)) { > + /* > + * Bus is off. Request ownership or turn it on unless > + * other master requested ownership. > + */ > + reg_ctl |= PCA9641_CTL_LOCK_REQ; > + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); > + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); > + > + if (lock_grant(reg_ctl)) { > + /* > + * Other master did not request ownership, > + * or arbitration timeout expired. Take the bus. > + */ > + reg_ctl |= PCA9641_CTL_BUS_CONNECT > + | PCA9641_CTL_LOCK_REQ; > + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); > + data->select_timeout = SELECT_DELAY_SHORT; > + > + return 1; > + } else { > + /* > + * Other master requested ownership. > + * Set extra long timeout to give it time to acquire it. > + */ > + data->select_timeout = SELECT_DELAY_LONG * 2; > + } > + } else if (lock_grant(reg_ctl)) { > + /* > + * Bus is on, and we own it. We are done with acquisition. > + */ > + reg_ctl |= PCA9641_CTL_BUS_CONNECT | PCA9641_CTL_LOCK_REQ; > + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); > + > + return 1; > + } else if (other_lock(reg_sts)) { > + /* > + * Other master owns the bus. > + * If arbitration timeout has expired, force ownership. > + * Otherwise request it. > + */ > + data->select_timeout = SELECT_DELAY_LONG; > + reg_ctl |= PCA9641_CTL_LOCK_REQ; > + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); > + } > + return 0; > +} > + > +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan) > +{ > + struct pca9541 *data = i2c_mux_priv(muxc); > + struct i2c_client *client = data->client; > + int ret; > + unsigned long timeout = jiffies + ARB2_TIMEOUT; > + /* give up after this time */ > + > + data->arb_timeout = jiffies + ARB_TIMEOUT; > + /* force bus ownership after this time */ > + > + do { > + ret = pca9641_arbitrate(client); > + if (ret) > + return ret < 0 ? ret : 0; > + > + if (data->select_timeout == SELECT_DELAY_SHORT) > + udelay(data->select_timeout); > + else > + msleep(data->select_timeout / 1000); > + } while (time_is_after_eq_jiffies(timeout)); > + > + return -ETIMEDOUT; > +} > + > +static int pca9641_release_chan(struct i2c_mux_core *muxc, u32 chan) > +{ > + struct pca9541 *data = i2c_mux_priv(muxc); > + struct i2c_client *client = data->client; > + > + pca9641_release_bus(client); > + return 0; > +} The pca9641_select_chan and pca9641_release_chan functions are exact copies of the pca9541 counterparts, with the exception of which functions they ultimately call. So, instead of using different function pointers in the i2c_mux_alloc calls below, add a couple of function pointers to the above mentioned chip description struct. Then change pca9541_release_chan to something like this: 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; data->chip->release_bus(client); return 0; } Similarly for the *_select_chan "wrapper". Now, these changes will somewhat affect the pca9541 side of the driver, so I would like to see more than one patch. There should be patches that prepares the driver that should be kind of easy to verify that they are equivalent but that makes adding a new chip easier, and then one patch at then end that adds the new chip. Hmm, it will probably be easier if I write those patches instead of reviewing them. I will followup with them. But note that I can only compile test them, so I would like to see tags for them. > + > +static int pca9641_detect_id(struct i2c_client *client) > +{ > + int reg; > + > + reg = pca9541_reg_read(client, PCA9641_ID); > + if (reg == PCA9641_ID_MAGIC) > + return 1; > + else > + return 0; > +} This was not what I had in mind. If you do dig out the id, I think you should only use it to verify that the input to the probe function is correct and error out otherwise. But maybe I'm conservative? Anyway, with the above patches you will not need this. > +/* > * I2C init/probing/exit functions > */ > static int pca9541_probe(struct i2c_client *client, > @@ -339,34 +492,45 @@ static int pca9541_probe(struct i2c_client *client, > struct pca9541 *data; > int force; > int ret; > + int detect_id; > > if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA)) > return -ENODEV; > > + detect_id = pca9641_detect_id(client); > /* > * I2C accesses are unprotected here. > * We have to lock the adapter before releasing the bus. > */ > - i2c_lock_adapter(adap); > - pca9541_release_bus(client); > - i2c_unlock_adapter(adap); > - > + if (detect_id == 0) { > + i2c_lock_adapter(adap); > + pca9541_release_bus(client); > + i2c_unlock_adapter(adap); > + } else { > + i2c_lock_adapter(adap); > + pca9641_release_bus(client); > + i2c_unlock_adapter(adap); > + } > /* Create mux adapter */ > > force = 0; > if (pdata) > force = pdata->modes[0].adap_id; > - muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), > + if (detect_id == 0) { > + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), > I2C_MUX_ARBITRATOR, > pca9541_select_chan, pca9541_release_chan); > + } else { > + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), > + I2C_MUX_ARBITRATOR, > + pca9641_select_chan, pca9641_release_chan); > + } > if (!muxc) > return -ENOMEM; > > data = i2c_mux_priv(muxc); > data->client = client; > - > i2c_set_clientdata(client, muxc); > - Please don't do spurious whitespace changes like this as part of a functional change. > ret = i2c_mux_add_adapter(muxc, force, 0, 0); > if (ret) > return ret; > You should change the Kconfig file to mention the new chip and you are still missing a devicetree binding. Cheers, Peter
Hi Ken, It's been a couple of weeks and I wondered if you are making any progress? Simple lack of time perhaps, or are you stuck and need help? Cheers, Peter On 2018-03-20 10:31, Peter Rosin wrote: > On 2018-03-20 07:19, Ken Chen wrote: >> Signed-off-by: Ken Chen <chen.kenyy@inventec.com> > > Ok, now that you are not adding a new driver, but instead > modify an existing driver, the subject I requested in no > longer relevant. Now I would like to see: > > i2c: mux: pca9541: add support for PCA9641 chips > > Or something like that. > >> --- >> v1->v2 >> - Merged PCA9641 code into i2c-mux-pca9541.c >> - Modified title >> - Add PCA9641 detect function >> --- >> drivers/i2c/muxes/i2c-mux-pca9541.c | 184 ++++++++++++++++++++++++++++++++++-- >> 1 file changed, 174 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c >> index 6a39ada..493f947 100644 >> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c >> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c >> @@ -1,5 +1,5 @@ >> /* >> - * I2C multiplexer driver for PCA9541 bus master selector >> + * I2C multiplexer driver for PCA9541/PCA9641 bus master selector >> * >> * Copyright (c) 2010 Ericsson AB. >> * >> @@ -26,8 +26,8 @@ >> #include <linux/slab.h> >> >> /* >> - * The PCA9541 is a bus master selector. It supports two I2C masters connected >> - * to a single slave bus. >> + * The PCA9541/PCA9641 is a bus master selector. It supports two I2C masters > > PCA9541 and PCA9641 are bus master selectors. They support two I2C masters > > And make sure to lose the trailing space. > >> + * connected to a single slave bus. >> * >> * Before each bus transaction, a master has to acquire bus ownership. After the >> * transaction is complete, bus ownership has to be released. This fits well >> @@ -58,11 +58,43 @@ >> #define PCA9541_ISTAT_MYTEST (1 << 6) >> #define PCA9541_ISTAT_NMYTEST (1 << 7) >> >> +#define PCA9641_ID 0x00 >> +#define PCA9641_ID_MAGIC 0x38 >> + >> +#define PCA9641_CONTROL 0x01 >> +#define PCA9641_STATUS 0x02 >> +#define PCA9641_TIME 0x03 >> + >> +#define PCA9641_CTL_LOCK_REQ BIT(0) >> +#define PCA9641_CTL_LOCK_GRANT BIT(1) >> +#define PCA9641_CTL_BUS_CONNECT BIT(2) >> +#define PCA9641_CTL_BUS_INIT BIT(3) >> +#define PCA9641_CTL_SMBUS_SWRST BIT(4) >> +#define PCA9641_CTL_IDLE_TIMER_DIS BIT(5) >> +#define PCA9641_CTL_SMBUS_DIS BIT(6) >> +#define PCA9641_CTL_PRIORITY BIT(7) >> + >> +#define PCA9641_STS_OTHER_LOCK BIT(0) >> +#define PCA9641_STS_BUS_INIT_FAIL BIT(1) >> +#define PCA9641_STS_BUS_HUNG BIT(2) >> +#define PCA9641_STS_MBOX_EMPTY BIT(3) >> +#define PCA9641_STS_MBOX_FULL BIT(4) >> +#define PCA9641_STS_TEST_INT BIT(5) >> +#define PCA9641_STS_SCL_IO BIT(6) >> +#define PCA9641_STS_SDA_IO BIT(7) >> + >> +#define PCA9641_RES_TIME 0x03 >> + >> #define BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON) >> #define MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS) >> #define mybus(x) (!((x) & MYBUS) || ((x) & MYBUS) == MYBUS) >> #define busoff(x) (!((x) & BUSON) || ((x) & BUSON) == BUSON) >> >> +#define BUSOFF(x, y) (!((x) & PCA9641_CTL_LOCK_GRANT) && \ >> + !((y) & PCA9641_STS_OTHER_LOCK)) >> +#define other_lock(x) ((x) & PCA9641_STS_OTHER_LOCK) >> +#define lock_grant(x) ((x) & PCA9641_CTL_LOCK_GRANT) > These macro names are now completely hideous. They were bad before, > but this is just too much for me. So, instead of adding BUSOFF etc, > I would like to see all the macros with a chip prefix. But I think > they will get overly long, so I think you should just write trivial > pca9541_mybus, pca9541_busoff, pca9641_busoff etc functions. The > compiler should inline them just fine. > > The rename of the existing macros and their conversion to functions > should be in the first preparatory patch that I mention below. The > new functions should be in the second patch. > >> + >> /* arbitration timeouts, in jiffies */ >> #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */ >> #define ARB2_TIMEOUT (HZ / 4) /* 250 ms until acquisition failure */ >> @@ -79,6 +111,7 @@ struct pca9541 { >> >> static const struct i2c_device_id pca9541_id[] = { >> {"pca9541", 0}, >> + {"pca9641", 1}, > > You are actually not using this 0/1 difference. Have a look at > e.g. how the i2c-mux-pca954x driver uses this as an index into > a chip description array. I would like to see something similar > here... > >> {} >> }; >> >> @@ -87,6 +120,7 @@ MODULE_DEVICE_TABLE(i2c, pca9541_id); >> #ifdef CONFIG_OF >> static const struct of_device_id pca9541_of_match[] = { >> { .compatible = "nxp,pca9541" }, >> + { .compatible = "nxp,pca9641" }, > > ...including pointers to the above chip descriptions here, just > like the pca954x driver. > >> {} >> }; >> MODULE_DEVICE_TABLE(of, pca9541_of_match); >> @@ -328,6 +362,125 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) >> } >> >> /* >> + * Arbitration management functions >> + */ >> +static void pca9641_release_bus(struct i2c_client *client) >> +{ >> + pca9541_reg_write(client, PCA9641_CONTROL, 0); >> +} >> + >> +/* >> + * Channel arbitration >> + * >> + * Return values: >> + * <0: error >> + * 0 : bus not acquired >> + * 1 : bus acquired >> + */ >> +static int pca9641_arbitrate(struct i2c_client *client) >> +{ >> + struct i2c_mux_core *muxc = i2c_get_clientdata(client); >> + struct pca9541 *data = i2c_mux_priv(muxc); >> + int reg_ctl, reg_sts; >> + >> + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); >> + if (reg_ctl < 0) >> + return reg_ctl; >> + reg_sts = pca9541_reg_read(client, PCA9641_STATUS); >> + >> + if (BUSOFF(reg_ctl, reg_sts)) { >> + /* >> + * Bus is off. Request ownership or turn it on unless >> + * other master requested ownership. >> + */ >> + reg_ctl |= PCA9641_CTL_LOCK_REQ; >> + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); >> + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); >> + >> + if (lock_grant(reg_ctl)) { >> + /* >> + * Other master did not request ownership, >> + * or arbitration timeout expired. Take the bus. >> + */ >> + reg_ctl |= PCA9641_CTL_BUS_CONNECT >> + | PCA9641_CTL_LOCK_REQ; >> + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); >> + data->select_timeout = SELECT_DELAY_SHORT; >> + >> + return 1; >> + } else { >> + /* >> + * Other master requested ownership. >> + * Set extra long timeout to give it time to acquire it. >> + */ >> + data->select_timeout = SELECT_DELAY_LONG * 2; >> + } >> + } else if (lock_grant(reg_ctl)) { >> + /* >> + * Bus is on, and we own it. We are done with acquisition. >> + */ >> + reg_ctl |= PCA9641_CTL_BUS_CONNECT | PCA9641_CTL_LOCK_REQ; >> + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); >> + >> + return 1; >> + } else if (other_lock(reg_sts)) { >> + /* >> + * Other master owns the bus. >> + * If arbitration timeout has expired, force ownership. >> + * Otherwise request it. >> + */ >> + data->select_timeout = SELECT_DELAY_LONG; >> + reg_ctl |= PCA9641_CTL_LOCK_REQ; >> + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); >> + } >> + return 0; >> +} >> + >> +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan) >> +{ >> + struct pca9541 *data = i2c_mux_priv(muxc); >> + struct i2c_client *client = data->client; >> + int ret; >> + unsigned long timeout = jiffies + ARB2_TIMEOUT; >> + /* give up after this time */ >> + >> + data->arb_timeout = jiffies + ARB_TIMEOUT; >> + /* force bus ownership after this time */ >> + >> + do { >> + ret = pca9641_arbitrate(client); >> + if (ret) >> + return ret < 0 ? ret : 0; >> + >> + if (data->select_timeout == SELECT_DELAY_SHORT) >> + udelay(data->select_timeout); >> + else >> + msleep(data->select_timeout / 1000); >> + } while (time_is_after_eq_jiffies(timeout)); >> + >> + return -ETIMEDOUT; >> +} >> + >> +static int pca9641_release_chan(struct i2c_mux_core *muxc, u32 chan) >> +{ >> + struct pca9541 *data = i2c_mux_priv(muxc); >> + struct i2c_client *client = data->client; >> + >> + pca9641_release_bus(client); >> + return 0; >> +} > > The pca9641_select_chan and pca9641_release_chan functions are exact > copies of the pca9541 counterparts, with the exception of which > functions they ultimately call. So, instead of using different > function pointers in the i2c_mux_alloc calls below, add a couple of > function pointers to the above mentioned chip description struct. > > Then change pca9541_release_chan to something like this: > > 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; > > data->chip->release_bus(client); > return 0; > } > > Similarly for the *_select_chan "wrapper". > > Now, these changes will somewhat affect the pca9541 side of the > driver, so I would like to see more than one patch. There should be > patches that prepares the driver that should be kind of easy to > verify that they are equivalent but that makes adding a new chip > easier, and then one patch at then end that adds the new chip. Hmm, > it will probably be easier if I write those patches instead of > reviewing them. I will followup with them. But note that I can > only compile test them, so I would like to see tags for them. > >> + >> +static int pca9641_detect_id(struct i2c_client *client) >> +{ >> + int reg; >> + >> + reg = pca9541_reg_read(client, PCA9641_ID); >> + if (reg == PCA9641_ID_MAGIC) >> + return 1; >> + else >> + return 0; >> +} > > This was not what I had in mind. If you do dig out the id, I think > you should only use it to verify that the input to the probe function > is correct and error out otherwise. But maybe I'm conservative? > Anyway, with the above patches you will not need this. > >> +/* >> * I2C init/probing/exit functions >> */ >> static int pca9541_probe(struct i2c_client *client, >> @@ -339,34 +492,45 @@ static int pca9541_probe(struct i2c_client *client, >> struct pca9541 *data; >> int force; >> int ret; >> + int detect_id; >> >> if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA)) >> return -ENODEV; >> >> + detect_id = pca9641_detect_id(client); >> /* >> * I2C accesses are unprotected here. >> * We have to lock the adapter before releasing the bus. >> */ >> - i2c_lock_adapter(adap); >> - pca9541_release_bus(client); >> - i2c_unlock_adapter(adap); >> - >> + if (detect_id == 0) { >> + i2c_lock_adapter(adap); >> + pca9541_release_bus(client); >> + i2c_unlock_adapter(adap); >> + } else { >> + i2c_lock_adapter(adap); >> + pca9641_release_bus(client); >> + i2c_unlock_adapter(adap); >> + } >> /* Create mux adapter */ >> >> force = 0; >> if (pdata) >> force = pdata->modes[0].adap_id; >> - muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), >> + if (detect_id == 0) { >> + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), >> I2C_MUX_ARBITRATOR, >> pca9541_select_chan, pca9541_release_chan); >> + } else { >> + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), >> + I2C_MUX_ARBITRATOR, >> + pca9641_select_chan, pca9641_release_chan); >> + } >> if (!muxc) >> return -ENOMEM; >> >> data = i2c_mux_priv(muxc); >> data->client = client; >> - >> i2c_set_clientdata(client, muxc); >> - > > Please don't do spurious whitespace changes like this as part of a > functional change. > >> ret = i2c_mux_add_adapter(muxc, force, 0, 0); >> if (ret) >> return ret; >> > > You should change the Kconfig file to mention the new chip and you are > still missing a devicetree binding. > > Cheers, > Peter >
Hi Peter, Sorry for late. Here has some event at my company which needs to pause this work. If the status changed, I will update my patch. Thanks. Ken 2018-04-11 17:37 GMT+08:00 Peter Rosin <peda@axentia.se>: > Hi Ken, > > It's been a couple of weeks and I wondered if you are making any > progress? Simple lack of time perhaps, or are you stuck and need > help? > > Cheers, > Peter > > On 2018-03-20 10:31, Peter Rosin wrote: >> On 2018-03-20 07:19, Ken Chen wrote: >>> Signed-off-by: Ken Chen <chen.kenyy@inventec.com> >> >> Ok, now that you are not adding a new driver, but instead >> modify an existing driver, the subject I requested in no >> longer relevant. Now I would like to see: >> >> i2c: mux: pca9541: add support for PCA9641 chips >> >> Or something like that. >> >>> --- >>> v1->v2 >>> - Merged PCA9641 code into i2c-mux-pca9541.c >>> - Modified title >>> - Add PCA9641 detect function >>> --- >>> drivers/i2c/muxes/i2c-mux-pca9541.c | 184 ++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 174 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c >>> index 6a39ada..493f947 100644 >>> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c >>> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c >>> @@ -1,5 +1,5 @@ >>> /* >>> - * I2C multiplexer driver for PCA9541 bus master selector >>> + * I2C multiplexer driver for PCA9541/PCA9641 bus master selector >>> * >>> * Copyright (c) 2010 Ericsson AB. >>> * >>> @@ -26,8 +26,8 @@ >>> #include <linux/slab.h> >>> >>> /* >>> - * The PCA9541 is a bus master selector. It supports two I2C masters connected >>> - * to a single slave bus. >>> + * The PCA9541/PCA9641 is a bus master selector. It supports two I2C masters >> >> PCA9541 and PCA9641 are bus master selectors. They support two I2C masters >> >> And make sure to lose the trailing space. >> >>> + * connected to a single slave bus. >>> * >>> * Before each bus transaction, a master has to acquire bus ownership. After the >>> * transaction is complete, bus ownership has to be released. This fits well >>> @@ -58,11 +58,43 @@ >>> #define PCA9541_ISTAT_MYTEST (1 << 6) >>> #define PCA9541_ISTAT_NMYTEST (1 << 7) >>> >>> +#define PCA9641_ID 0x00 >>> +#define PCA9641_ID_MAGIC 0x38 >>> + >>> +#define PCA9641_CONTROL 0x01 >>> +#define PCA9641_STATUS 0x02 >>> +#define PCA9641_TIME 0x03 >>> + >>> +#define PCA9641_CTL_LOCK_REQ BIT(0) >>> +#define PCA9641_CTL_LOCK_GRANT BIT(1) >>> +#define PCA9641_CTL_BUS_CONNECT BIT(2) >>> +#define PCA9641_CTL_BUS_INIT BIT(3) >>> +#define PCA9641_CTL_SMBUS_SWRST BIT(4) >>> +#define PCA9641_CTL_IDLE_TIMER_DIS BIT(5) >>> +#define PCA9641_CTL_SMBUS_DIS BIT(6) >>> +#define PCA9641_CTL_PRIORITY BIT(7) >>> + >>> +#define PCA9641_STS_OTHER_LOCK BIT(0) >>> +#define PCA9641_STS_BUS_INIT_FAIL BIT(1) >>> +#define PCA9641_STS_BUS_HUNG BIT(2) >>> +#define PCA9641_STS_MBOX_EMPTY BIT(3) >>> +#define PCA9641_STS_MBOX_FULL BIT(4) >>> +#define PCA9641_STS_TEST_INT BIT(5) >>> +#define PCA9641_STS_SCL_IO BIT(6) >>> +#define PCA9641_STS_SDA_IO BIT(7) >>> + >>> +#define PCA9641_RES_TIME 0x03 >>> + >>> #define BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON) >>> #define MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS) >>> #define mybus(x) (!((x) & MYBUS) || ((x) & MYBUS) == MYBUS) >>> #define busoff(x) (!((x) & BUSON) || ((x) & BUSON) == BUSON) >>> >>> +#define BUSOFF(x, y) (!((x) & PCA9641_CTL_LOCK_GRANT) && \ >>> + !((y) & PCA9641_STS_OTHER_LOCK)) >>> +#define other_lock(x) ((x) & PCA9641_STS_OTHER_LOCK) >>> +#define lock_grant(x) ((x) & PCA9641_CTL_LOCK_GRANT) >> These macro names are now completely hideous. They were bad before, >> but this is just too much for me. So, instead of adding BUSOFF etc, >> I would like to see all the macros with a chip prefix. But I think >> they will get overly long, so I think you should just write trivial >> pca9541_mybus, pca9541_busoff, pca9641_busoff etc functions. The >> compiler should inline them just fine. >> >> The rename of the existing macros and their conversion to functions >> should be in the first preparatory patch that I mention below. The >> new functions should be in the second patch. >> >>> + >>> /* arbitration timeouts, in jiffies */ >>> #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */ >>> #define ARB2_TIMEOUT (HZ / 4) /* 250 ms until acquisition failure */ >>> @@ -79,6 +111,7 @@ struct pca9541 { >>> >>> static const struct i2c_device_id pca9541_id[] = { >>> {"pca9541", 0}, >>> + {"pca9641", 1}, >> >> You are actually not using this 0/1 difference. Have a look at >> e.g. how the i2c-mux-pca954x driver uses this as an index into >> a chip description array. I would like to see something similar >> here... >> >>> {} >>> }; >>> >>> @@ -87,6 +120,7 @@ MODULE_DEVICE_TABLE(i2c, pca9541_id); >>> #ifdef CONFIG_OF >>> static const struct of_device_id pca9541_of_match[] = { >>> { .compatible = "nxp,pca9541" }, >>> + { .compatible = "nxp,pca9641" }, >> >> ...including pointers to the above chip descriptions here, just >> like the pca954x driver. >> >>> {} >>> }; >>> MODULE_DEVICE_TABLE(of, pca9541_of_match); >>> @@ -328,6 +362,125 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) >>> } >>> >>> /* >>> + * Arbitration management functions >>> + */ >>> +static void pca9641_release_bus(struct i2c_client *client) >>> +{ >>> + pca9541_reg_write(client, PCA9641_CONTROL, 0); >>> +} >>> + >>> +/* >>> + * Channel arbitration >>> + * >>> + * Return values: >>> + * <0: error >>> + * 0 : bus not acquired >>> + * 1 : bus acquired >>> + */ >>> +static int pca9641_arbitrate(struct i2c_client *client) >>> +{ >>> + struct i2c_mux_core *muxc = i2c_get_clientdata(client); >>> + struct pca9541 *data = i2c_mux_priv(muxc); >>> + int reg_ctl, reg_sts; >>> + >>> + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); >>> + if (reg_ctl < 0) >>> + return reg_ctl; >>> + reg_sts = pca9541_reg_read(client, PCA9641_STATUS); >>> + >>> + if (BUSOFF(reg_ctl, reg_sts)) { >>> + /* >>> + * Bus is off. Request ownership or turn it on unless >>> + * other master requested ownership. >>> + */ >>> + reg_ctl |= PCA9641_CTL_LOCK_REQ; >>> + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); >>> + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); >>> + >>> + if (lock_grant(reg_ctl)) { >>> + /* >>> + * Other master did not request ownership, >>> + * or arbitration timeout expired. Take the bus. >>> + */ >>> + reg_ctl |= PCA9641_CTL_BUS_CONNECT >>> + | PCA9641_CTL_LOCK_REQ; >>> + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); >>> + data->select_timeout = SELECT_DELAY_SHORT; >>> + >>> + return 1; >>> + } else { >>> + /* >>> + * Other master requested ownership. >>> + * Set extra long timeout to give it time to acquire it. >>> + */ >>> + data->select_timeout = SELECT_DELAY_LONG * 2; >>> + } >>> + } else if (lock_grant(reg_ctl)) { >>> + /* >>> + * Bus is on, and we own it. We are done with acquisition. >>> + */ >>> + reg_ctl |= PCA9641_CTL_BUS_CONNECT | PCA9641_CTL_LOCK_REQ; >>> + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); >>> + >>> + return 1; >>> + } else if (other_lock(reg_sts)) { >>> + /* >>> + * Other master owns the bus. >>> + * If arbitration timeout has expired, force ownership. >>> + * Otherwise request it. >>> + */ >>> + data->select_timeout = SELECT_DELAY_LONG; >>> + reg_ctl |= PCA9641_CTL_LOCK_REQ; >>> + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); >>> + } >>> + return 0; >>> +} >>> + >>> +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan) >>> +{ >>> + struct pca9541 *data = i2c_mux_priv(muxc); >>> + struct i2c_client *client = data->client; >>> + int ret; >>> + unsigned long timeout = jiffies + ARB2_TIMEOUT; >>> + /* give up after this time */ >>> + >>> + data->arb_timeout = jiffies + ARB_TIMEOUT; >>> + /* force bus ownership after this time */ >>> + >>> + do { >>> + ret = pca9641_arbitrate(client); >>> + if (ret) >>> + return ret < 0 ? ret : 0; >>> + >>> + if (data->select_timeout == SELECT_DELAY_SHORT) >>> + udelay(data->select_timeout); >>> + else >>> + msleep(data->select_timeout / 1000); >>> + } while (time_is_after_eq_jiffies(timeout)); >>> + >>> + return -ETIMEDOUT; >>> +} >>> + >>> +static int pca9641_release_chan(struct i2c_mux_core *muxc, u32 chan) >>> +{ >>> + struct pca9541 *data = i2c_mux_priv(muxc); >>> + struct i2c_client *client = data->client; >>> + >>> + pca9641_release_bus(client); >>> + return 0; >>> +} >> >> The pca9641_select_chan and pca9641_release_chan functions are exact >> copies of the pca9541 counterparts, with the exception of which >> functions they ultimately call. So, instead of using different >> function pointers in the i2c_mux_alloc calls below, add a couple of >> function pointers to the above mentioned chip description struct. >> >> Then change pca9541_release_chan to something like this: >> >> 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; >> >> data->chip->release_bus(client); >> return 0; >> } >> >> Similarly for the *_select_chan "wrapper". >> >> Now, these changes will somewhat affect the pca9541 side of the >> driver, so I would like to see more than one patch. There should be >> patches that prepares the driver that should be kind of easy to >> verify that they are equivalent but that makes adding a new chip >> easier, and then one patch at then end that adds the new chip. Hmm, >> it will probably be easier if I write those patches instead of >> reviewing them. I will followup with them. But note that I can >> only compile test them, so I would like to see tags for them. >> >>> + >>> +static int pca9641_detect_id(struct i2c_client *client) >>> +{ >>> + int reg; >>> + >>> + reg = pca9541_reg_read(client, PCA9641_ID); >>> + if (reg == PCA9641_ID_MAGIC) >>> + return 1; >>> + else >>> + return 0; >>> +} >> >> This was not what I had in mind. If you do dig out the id, I think >> you should only use it to verify that the input to the probe function >> is correct and error out otherwise. But maybe I'm conservative? >> Anyway, with the above patches you will not need this. >> >>> +/* >>> * I2C init/probing/exit functions >>> */ >>> static int pca9541_probe(struct i2c_client *client, >>> @@ -339,34 +492,45 @@ static int pca9541_probe(struct i2c_client *client, >>> struct pca9541 *data; >>> int force; >>> int ret; >>> + int detect_id; >>> >>> if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA)) >>> return -ENODEV; >>> >>> + detect_id = pca9641_detect_id(client); >>> /* >>> * I2C accesses are unprotected here. >>> * We have to lock the adapter before releasing the bus. >>> */ >>> - i2c_lock_adapter(adap); >>> - pca9541_release_bus(client); >>> - i2c_unlock_adapter(adap); >>> - >>> + if (detect_id == 0) { >>> + i2c_lock_adapter(adap); >>> + pca9541_release_bus(client); >>> + i2c_unlock_adapter(adap); >>> + } else { >>> + i2c_lock_adapter(adap); >>> + pca9641_release_bus(client); >>> + i2c_unlock_adapter(adap); >>> + } >>> /* Create mux adapter */ >>> >>> force = 0; >>> if (pdata) >>> force = pdata->modes[0].adap_id; >>> - muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), >>> + if (detect_id == 0) { >>> + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), >>> I2C_MUX_ARBITRATOR, >>> pca9541_select_chan, pca9541_release_chan); >>> + } else { >>> + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), >>> + I2C_MUX_ARBITRATOR, >>> + pca9641_select_chan, pca9641_release_chan); >>> + } >>> if (!muxc) >>> return -ENOMEM; >>> >>> data = i2c_mux_priv(muxc); >>> data->client = client; >>> - >>> i2c_set_clientdata(client, muxc); >>> - >> >> Please don't do spurious whitespace changes like this as part of a >> functional change. >> >>> ret = i2c_mux_add_adapter(muxc, force, 0, 0); >>> if (ret) >>> return ret; >>> >> >> You should change the Kconfig file to mention the new chip and you are >> still missing a devicetree binding. >> >> Cheers, >> Peter >> >
On 2018-04-13 08:59, ChenKenYY 陳永營 TAO wrote: > Hi Peter, > > Sorry for late. Here has some event at my company which needs to pause > this work. > > If the status changed, I will update my patch. No worries, I just want to know how to handle my preparatory patches. If nothing changes, I think I'll push the first two anyway, but will hold off the third until there is some real need from it. Or can you perhaps make time to test a patch adding pca9641 if I adapt your code to my 3/3 patch? Then I can put these patches behind me. Cheers, Peter
2018-04-13 15:27 GMT+08:00 Peter Rosin <peda@axentia.se>: > On 2018-04-13 08:59, ChenKenYY 陳永營 TAO wrote: >> Hi Peter, >> >> Sorry for late. Here has some event at my company which needs to pause >> this work. >> >> If the status changed, I will update my patch. > > No worries, I just want to know how to handle my preparatory patches. If > nothing changes, I think I'll push the first two anyway, but will hold > off the third until there is some real need from it. > > Or can you perhaps make time to test a patch adding pca9641 if I adapt > your code to my 3/3 patch? Then I can put these patches behind me. > I think you can push code first, I will test and update when I ready. Thanks, Ken > Cheers, > Peter
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c index 6a39ada..493f947 100644 --- a/drivers/i2c/muxes/i2c-mux-pca9541.c +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c @@ -1,5 +1,5 @@ /* - * I2C multiplexer driver for PCA9541 bus master selector + * I2C multiplexer driver for PCA9541/PCA9641 bus master selector * * Copyright (c) 2010 Ericsson AB. * @@ -26,8 +26,8 @@ #include <linux/slab.h> /* - * The PCA9541 is a bus master selector. It supports two I2C masters connected - * to a single slave bus. + * The PCA9541/PCA9641 is a bus master selector. It supports two I2C masters + * connected to a single slave bus. * * Before each bus transaction, a master has to acquire bus ownership. After the * transaction is complete, bus ownership has to be released. This fits well @@ -58,11 +58,43 @@ #define PCA9541_ISTAT_MYTEST (1 << 6) #define PCA9541_ISTAT_NMYTEST (1 << 7) +#define PCA9641_ID 0x00 +#define PCA9641_ID_MAGIC 0x38 + +#define PCA9641_CONTROL 0x01 +#define PCA9641_STATUS 0x02 +#define PCA9641_TIME 0x03 + +#define PCA9641_CTL_LOCK_REQ BIT(0) +#define PCA9641_CTL_LOCK_GRANT BIT(1) +#define PCA9641_CTL_BUS_CONNECT BIT(2) +#define PCA9641_CTL_BUS_INIT BIT(3) +#define PCA9641_CTL_SMBUS_SWRST BIT(4) +#define PCA9641_CTL_IDLE_TIMER_DIS BIT(5) +#define PCA9641_CTL_SMBUS_DIS BIT(6) +#define PCA9641_CTL_PRIORITY BIT(7) + +#define PCA9641_STS_OTHER_LOCK BIT(0) +#define PCA9641_STS_BUS_INIT_FAIL BIT(1) +#define PCA9641_STS_BUS_HUNG BIT(2) +#define PCA9641_STS_MBOX_EMPTY BIT(3) +#define PCA9641_STS_MBOX_FULL BIT(4) +#define PCA9641_STS_TEST_INT BIT(5) +#define PCA9641_STS_SCL_IO BIT(6) +#define PCA9641_STS_SDA_IO BIT(7) + +#define PCA9641_RES_TIME 0x03 + #define BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON) #define MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS) #define mybus(x) (!((x) & MYBUS) || ((x) & MYBUS) == MYBUS) #define busoff(x) (!((x) & BUSON) || ((x) & BUSON) == BUSON) +#define BUSOFF(x, y) (!((x) & PCA9641_CTL_LOCK_GRANT) && \ + !((y) & PCA9641_STS_OTHER_LOCK)) +#define other_lock(x) ((x) & PCA9641_STS_OTHER_LOCK) +#define lock_grant(x) ((x) & PCA9641_CTL_LOCK_GRANT) + /* arbitration timeouts, in jiffies */ #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */ #define ARB2_TIMEOUT (HZ / 4) /* 250 ms until acquisition failure */ @@ -79,6 +111,7 @@ struct pca9541 { static const struct i2c_device_id pca9541_id[] = { {"pca9541", 0}, + {"pca9641", 1}, {} }; @@ -87,6 +120,7 @@ MODULE_DEVICE_TABLE(i2c, pca9541_id); #ifdef CONFIG_OF static const struct of_device_id pca9541_of_match[] = { { .compatible = "nxp,pca9541" }, + { .compatible = "nxp,pca9641" }, {} }; MODULE_DEVICE_TABLE(of, pca9541_of_match); @@ -328,6 +362,125 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan) } /* + * Arbitration management functions + */ +static void pca9641_release_bus(struct i2c_client *client) +{ + pca9541_reg_write(client, PCA9641_CONTROL, 0); +} + +/* + * Channel arbitration + * + * Return values: + * <0: error + * 0 : bus not acquired + * 1 : bus acquired + */ +static int pca9641_arbitrate(struct i2c_client *client) +{ + struct i2c_mux_core *muxc = i2c_get_clientdata(client); + struct pca9541 *data = i2c_mux_priv(muxc); + int reg_ctl, reg_sts; + + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); + if (reg_ctl < 0) + return reg_ctl; + reg_sts = pca9541_reg_read(client, PCA9641_STATUS); + + if (BUSOFF(reg_ctl, reg_sts)) { + /* + * Bus is off. Request ownership or turn it on unless + * other master requested ownership. + */ + reg_ctl |= PCA9641_CTL_LOCK_REQ; + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); + reg_ctl = pca9541_reg_read(client, PCA9641_CONTROL); + + if (lock_grant(reg_ctl)) { + /* + * Other master did not request ownership, + * or arbitration timeout expired. Take the bus. + */ + reg_ctl |= PCA9641_CTL_BUS_CONNECT + | PCA9641_CTL_LOCK_REQ; + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); + data->select_timeout = SELECT_DELAY_SHORT; + + return 1; + } else { + /* + * Other master requested ownership. + * Set extra long timeout to give it time to acquire it. + */ + data->select_timeout = SELECT_DELAY_LONG * 2; + } + } else if (lock_grant(reg_ctl)) { + /* + * Bus is on, and we own it. We are done with acquisition. + */ + reg_ctl |= PCA9641_CTL_BUS_CONNECT | PCA9641_CTL_LOCK_REQ; + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); + + return 1; + } else if (other_lock(reg_sts)) { + /* + * Other master owns the bus. + * If arbitration timeout has expired, force ownership. + * Otherwise request it. + */ + data->select_timeout = SELECT_DELAY_LONG; + reg_ctl |= PCA9641_CTL_LOCK_REQ; + pca9541_reg_write(client, PCA9641_CONTROL, reg_ctl); + } + return 0; +} + +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan) +{ + struct pca9541 *data = i2c_mux_priv(muxc); + struct i2c_client *client = data->client; + int ret; + unsigned long timeout = jiffies + ARB2_TIMEOUT; + /* give up after this time */ + + data->arb_timeout = jiffies + ARB_TIMEOUT; + /* force bus ownership after this time */ + + do { + ret = pca9641_arbitrate(client); + if (ret) + return ret < 0 ? ret : 0; + + if (data->select_timeout == SELECT_DELAY_SHORT) + udelay(data->select_timeout); + else + msleep(data->select_timeout / 1000); + } while (time_is_after_eq_jiffies(timeout)); + + return -ETIMEDOUT; +} + +static int pca9641_release_chan(struct i2c_mux_core *muxc, u32 chan) +{ + struct pca9541 *data = i2c_mux_priv(muxc); + struct i2c_client *client = data->client; + + pca9641_release_bus(client); + return 0; +} + +static int pca9641_detect_id(struct i2c_client *client) +{ + int reg; + + reg = pca9541_reg_read(client, PCA9641_ID); + if (reg == PCA9641_ID_MAGIC) + return 1; + else + return 0; +} +/* * I2C init/probing/exit functions */ static int pca9541_probe(struct i2c_client *client, @@ -339,34 +492,45 @@ static int pca9541_probe(struct i2c_client *client, struct pca9541 *data; int force; int ret; + int detect_id; if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA)) return -ENODEV; + detect_id = pca9641_detect_id(client); /* * I2C accesses are unprotected here. * We have to lock the adapter before releasing the bus. */ - i2c_lock_adapter(adap); - pca9541_release_bus(client); - i2c_unlock_adapter(adap); - + if (detect_id == 0) { + i2c_lock_adapter(adap); + pca9541_release_bus(client); + i2c_unlock_adapter(adap); + } else { + i2c_lock_adapter(adap); + pca9641_release_bus(client); + i2c_unlock_adapter(adap); + } /* Create mux adapter */ force = 0; if (pdata) force = pdata->modes[0].adap_id; - muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), + if (detect_id == 0) { + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), I2C_MUX_ARBITRATOR, pca9541_select_chan, pca9541_release_chan); + } else { + muxc = i2c_mux_alloc(adap, &client->dev, 1, sizeof(*data), + I2C_MUX_ARBITRATOR, + pca9641_select_chan, pca9641_release_chan); + } if (!muxc) return -ENOMEM; data = i2c_mux_priv(muxc); data->client = client; - i2c_set_clientdata(client, muxc); - ret = i2c_mux_add_adapter(muxc, force, 0, 0); if (ret) return ret;
Signed-off-by: Ken Chen <chen.kenyy@inventec.com> --- v1->v2 - Merged PCA9641 code into i2c-mux-pca9541.c - Modified title - Add PCA9641 detect function --- drivers/i2c/muxes/i2c-mux-pca9541.c | 184 ++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 10 deletions(-)