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@xxxxxxxxxxxx>
>
> 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
>
