LinuxLists.cc - [PATCH linux dev-4.16 v2] i2c: muxes: pca9641: new driver

[permalink] [raw]

Subject: Re: [PATCH linux dev-4.16 v2] i2c: muxes: pca9641: new driver

On 2018-03-20 07:19, Ken Chen wrote:
> Signed-off-by: Ken Chen <[email protected]>

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

2018-03-20 09:34:22

[permalink] [raw]

Subject: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

In preparation for PCA9641 support, convert the mybus and busoff macros
to functions, and in the process prefix them with pca9541_. Also prefix
remaining chip specific macros with PCA9541_.

Signed-off-by: Peter Rosin <[email protected]>
---
drivers/i2c/muxes/i2c-mux-pca9541.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index ad168125d23d..47685eb4e0e9 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -59,10 +59,8 @@
#define PCA9541_ISTAT_MYTEST BIT(6)
#define PCA9541_ISTAT_NMYTEST BIT(7)

-#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 PCA9541_BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
+#define PCA9541_MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)

/* arbitration timeouts, in jiffies */
#define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */
@@ -93,6 +91,20 @@ static const struct of_device_id pca9541_of_match[] = {
MODULE_DEVICE_TABLE(of, pca9541_of_match);
#endif

+static int pca9541_mybus(int ctl)
+{
+ if (!(ctl & PCA9541_MYBUS))
+ return 1;
+ return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
+}
+
+static int pca9541_busoff(int ctl)
+{
+ if (!(ctl & PCA9541_BUSON))
+ return 1;
+ return (ctl & PCA9541_BUSON) == PCA9541_BUSON;
+}
+
/*
* Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
* as they will try to lock the adapter a second time.
@@ -181,7 +193,7 @@ static void pca9541_release_bus(struct i2c_client *client)
int reg;

reg = pca9541_reg_read(client, PCA9541_CONTROL);
- if (reg >= 0 && !busoff(reg) && mybus(reg))
+ if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
pca9541_reg_write(client, PCA9541_CONTROL,
(reg & PCA9541_CTL_NBUSON) >> 1);
}
@@ -233,7 +245,7 @@ static int pca9541_arbitrate(struct i2c_client *client)
if (reg < 0)
return reg;

- if (busoff(reg)) {
+ if (pca9541_busoff(reg)) {
int istat;
/*
* Bus is off. Request ownership or turn it on unless
@@ -258,7 +270,7 @@ static int pca9541_arbitrate(struct i2c_client *client)
*/
data->select_timeout = SELECT_DELAY_LONG * 2;
}
- } else if (mybus(reg)) {
+ } else if (pca9541_mybus(reg)) {
/*
* Bus is on, and we own it. We are done with acquisition.
* Reset NTESTON and BUSINIT, then return success.
--
2.11.0

2018-03-20 09:34:39

[permalink] [raw]

Subject: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

Make the arbitrate and release_bus implementation chip specific.

Signed-off-by: Peter Rosin <[email protected]>
---
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);
--
2.11.0

2018-03-20 09:34:48

[permalink] [raw]

Subject: [PATCH 1/3] i2c: mux: pca9541: use the BIT macro

Because it looks nice!

Signed-off-by: Peter Rosin <[email protected]>
---
drivers/i2c/muxes/i2c-mux-pca9541.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 6a39adaf433f..ad168125d23d 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -16,6 +16,7 @@
* warranty of any kind, whether express or implied.
*/

+#include <linux/bitops.h>
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/i2c.h>
@@ -43,20 +44,20 @@
#define PCA9541_CONTROL 0x01
#define PCA9541_ISTAT 0x02

-#define PCA9541_CTL_MYBUS (1 << 0)
-#define PCA9541_CTL_NMYBUS (1 << 1)
-#define PCA9541_CTL_BUSON (1 << 2)
-#define PCA9541_CTL_NBUSON (1 << 3)
-#define PCA9541_CTL_BUSINIT (1 << 4)
-#define PCA9541_CTL_TESTON (1 << 6)
-#define PCA9541_CTL_NTESTON (1 << 7)
-
-#define PCA9541_ISTAT_INTIN (1 << 0)
-#define PCA9541_ISTAT_BUSINIT (1 << 1)
-#define PCA9541_ISTAT_BUSOK (1 << 2)
-#define PCA9541_ISTAT_BUSLOST (1 << 3)
-#define PCA9541_ISTAT_MYTEST (1 << 6)
-#define PCA9541_ISTAT_NMYTEST (1 << 7)
+#define PCA9541_CTL_MYBUS BIT(0)
+#define PCA9541_CTL_NMYBUS BIT(1)
+#define PCA9541_CTL_BUSON BIT(2)
+#define PCA9541_CTL_NBUSON BIT(3)
+#define PCA9541_CTL_BUSINIT BIT(4)
+#define PCA9541_CTL_TESTON BIT(6)
+#define PCA9541_CTL_NTESTON BIT(7)
+
+#define PCA9541_ISTAT_INTIN BIT(0)
+#define PCA9541_ISTAT_BUSINIT BIT(1)
+#define PCA9541_ISTAT_BUSOK BIT(2)
+#define PCA9541_ISTAT_BUSLOST BIT(3)
+#define PCA9541_ISTAT_MYTEST BIT(6)
+#define PCA9541_ISTAT_NMYTEST BIT(7)

#define BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
#define MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
--
2.11.0

2018-03-20 13:20:04

[permalink] [raw]

Subject: Re: [PATCH 1/3] i2c: mux: pca9541: use the BIT macro

On 03/20/2018 02:31 AM, Peter Rosin wrote:
> Because it looks nice!
>
> Signed-off-by: Peter Rosin <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> drivers/i2c/muxes/i2c-mux-pca9541.c | 29 +++++++++++++++--------------
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 6a39adaf433f..ad168125d23d 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -16,6 +16,7 @@
> * warranty of any kind, whether express or implied.
> */
>
> +#include <linux/bitops.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/i2c.h>
> @@ -43,20 +44,20 @@
> #define PCA9541_CONTROL 0x01
> #define PCA9541_ISTAT 0x02
>
> -#define PCA9541_CTL_MYBUS (1 << 0)
> -#define PCA9541_CTL_NMYBUS (1 << 1)
> -#define PCA9541_CTL_BUSON (1 << 2)
> -#define PCA9541_CTL_NBUSON (1 << 3)
> -#define PCA9541_CTL_BUSINIT (1 << 4)
> -#define PCA9541_CTL_TESTON (1 << 6)
> -#define PCA9541_CTL_NTESTON (1 << 7)
> -
> -#define PCA9541_ISTAT_INTIN (1 << 0)
> -#define PCA9541_ISTAT_BUSINIT (1 << 1)
> -#define PCA9541_ISTAT_BUSOK (1 << 2)
> -#define PCA9541_ISTAT_BUSLOST (1 << 3)
> -#define PCA9541_ISTAT_MYTEST (1 << 6)
> -#define PCA9541_ISTAT_NMYTEST (1 << 7)
> +#define PCA9541_CTL_MYBUS BIT(0)
> +#define PCA9541_CTL_NMYBUS BIT(1)
> +#define PCA9541_CTL_BUSON BIT(2)
> +#define PCA9541_CTL_NBUSON BIT(3)
> +#define PCA9541_CTL_BUSINIT BIT(4)
> +#define PCA9541_CTL_TESTON BIT(6)
> +#define PCA9541_CTL_NTESTON BIT(7)
> +
> +#define PCA9541_ISTAT_INTIN BIT(0)
> +#define PCA9541_ISTAT_BUSINIT BIT(1)
> +#define PCA9541_ISTAT_BUSOK BIT(2)
> +#define PCA9541_ISTAT_BUSLOST BIT(3)
> +#define PCA9541_ISTAT_MYTEST BIT(6)
> +#define PCA9541_ISTAT_NMYTEST BIT(7)
>
> #define BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
> #define MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
>

2018-03-20 13:21:51

[permalink] [raw]

Subject: Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

On 03/20/2018 02:31 AM, Peter Rosin wrote:
> In preparation for PCA9641 support, convert the mybus and busoff macros
> to functions, and in the process prefix them with pca9541_. Also prefix
> remaining chip specific macros with PCA9541_.
>
> Signed-off-by: Peter Rosin <[email protected]>
> ---
> drivers/i2c/muxes/i2c-mux-pca9541.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index ad168125d23d..47685eb4e0e9 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -59,10 +59,8 @@
> #define PCA9541_ISTAT_MYTEST BIT(6)
> #define PCA9541_ISTAT_NMYTEST BIT(7)
>
> -#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 PCA9541_BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
> +#define PCA9541_MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
>
> /* arbitration timeouts, in jiffies */
> #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */
> @@ -93,6 +91,20 @@ static const struct of_device_id pca9541_of_match[] = {
> MODULE_DEVICE_TABLE(of, pca9541_of_match);
> #endif
>
> +static int pca9541_mybus(int ctl)

bool ?

> +{
> + if (!(ctl & PCA9541_MYBUS))
> + return 1;

true ?

> + return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
> +}
> +
> +static int pca9541_busoff(int ctl)

bool ?

> +{
> + if (!(ctl & PCA9541_BUSON))
> + return 1;

true ?

> + return (ctl & PCA9541_BUSON) == PCA9541_BUSON;
> +}
> +
> /*
> * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
> * as they will try to lock the adapter a second time.
> @@ -181,7 +193,7 @@ static void pca9541_release_bus(struct i2c_client *client)
> int reg;
>
> reg = pca9541_reg_read(client, PCA9541_CONTROL);
> - if (reg >= 0 && !busoff(reg) && mybus(reg))
> + if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
> pca9541_reg_write(client, PCA9541_CONTROL,
> (reg & PCA9541_CTL_NBUSON) >> 1);
> }
> @@ -233,7 +245,7 @@ static int pca9541_arbitrate(struct i2c_client *client)
> if (reg < 0)
> return reg;
>
> - if (busoff(reg)) {
> + if (pca9541_busoff(reg)) {
> int istat;
> /*
> * Bus is off. Request ownership or turn it on unless
> @@ -258,7 +270,7 @@ static int pca9541_arbitrate(struct i2c_client *client)
> */
> data->select_timeout = SELECT_DELAY_LONG * 2;
> }
> - } else if (mybus(reg)) {
> + } else if (pca9541_mybus(reg)) {
> /*
> * Bus is on, and we own it. We are done with acquisition.
> * Reset NTESTON and BUSINIT, then return success.
>

2018-03-20 13:24:10

[permalink] [raw]

Subject: Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

On 03/20/2018 02:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
>
> Signed-off-by: Peter Rosin <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
> 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);
>

2018-03-20 21:50:47

[permalink] [raw]

Subject: Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

On 2018-03-20 14:20, Guenter Roeck wrote:
> On 03/20/2018 02:31 AM, Peter Rosin wrote:
>> +static int pca9541_mybus(int ctl)
>
> bool ?
>
>> +{
>> + if (!(ctl & PCA9541_MYBUS))
>> + return 1;
>
> true ?
>
>> + return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
>> +}
>> +
>> +static int pca9541_busoff(int ctl)
>
> bool ?
>
>> +{
>> + if (!(ctl & PCA9541_BUSON))
>> + return 1;
>
> true ?

bool? true? Isn't that C++? Sigh, but you're right, and old habits
die hard...

Would it be ok to add your reviewed-by tag as I fix that?

Cheers,
Peter

2018-03-20 21:59:03

[permalink] [raw]

Subject: Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

On Tue, Mar 20, 2018 at 10:48:40PM +0100, Peter Rosin wrote:
> On 2018-03-20 14:20, Guenter Roeck wrote:
> > On 03/20/2018 02:31 AM, Peter Rosin wrote:
> >> +static int pca9541_mybus(int ctl)
> >
> > bool ?
> >
> >> +{
> >> + if (!(ctl & PCA9541_MYBUS))
> >> + return 1;
> >
> > true ?
> >
> >> + return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
> >> +}
> >> +
> >> +static int pca9541_busoff(int ctl)
> >
> > bool ?
> >
> >> +{
> >> + if (!(ctl & PCA9541_BUSON))
> >> + return 1;
> >
> > true ?
>
> bool? true? Isn't that C++? Sigh, but you're right, and old habits
> die hard...
>
> Would it be ok to add your reviewed-by tag as I fix that?
>
Sure.

Guenter

> Cheers,
> Peter

2018-03-20 23:19:01

[permalink] [raw]

Subject: Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

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 <[email protected]>
> ---
> 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

2018-03-20 23:26:06

[permalink] [raw]

Subject: Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

Hi Peter,

On 03/20/2018 11:31 AM, Peter Rosin wrote:
> In preparation for PCA9641 support, convert the mybus and busoff macros
> to functions, and in the process prefix them with pca9541_. Also prefix
> remaining chip specific macros with PCA9541_.
>
> Signed-off-by: Peter Rosin <[email protected]>
> ---
> drivers/i2c/muxes/i2c-mux-pca9541.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index ad168125d23d..47685eb4e0e9 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -59,10 +59,8 @@
> #define PCA9541_ISTAT_MYTEST BIT(6)
> #define PCA9541_ISTAT_NMYTEST BIT(7)
>
> -#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 PCA9541_BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
> +#define PCA9541_MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
>
> /* arbitration timeouts, in jiffies */
> #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */
> @@ -93,6 +91,20 @@ static const struct of_device_id pca9541_of_match[] = {
> MODULE_DEVICE_TABLE(of, pca9541_of_match);
> #endif
>
> +static int pca9541_mybus(int ctl)

static inline?

> +{
> + if (!(ctl & PCA9541_MYBUS))
> + return 1;
> + return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
> +}
> +
> +static int pca9541_busoff(int ctl)

static inline?

> +{
> + if (!(ctl & PCA9541_BUSON))
> + return 1;
> + return (ctl & PCA9541_BUSON) == PCA9541_BUSON;
> +}

Reviewed-by: Vladimir Zapolskiy <[email protected]>

--
With best wishes,
Vladimir

2018-03-20 23:26:39

[permalink] [raw]

Subject: Re: [PATCH 1/3] i2c: mux: pca9541: use the BIT macro

On 03/20/2018 11:31 AM, Peter Rosin wrote:
> Because it looks nice!
>
> Signed-off-by: Peter Rosin <[email protected]>

Reviewed-by: Vladimir Zapolskiy <[email protected]>

--
With best wishes,
Vladimir

2018-03-21 01:20:31

[permalink] [raw]

Subject: Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

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 <[email protected]>
>> ---
>> 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
>

2018-03-21 05:54:38

[permalink] [raw]

Subject: Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

On 2018-03-21 00:24, Vladimir Zapolskiy wrote:
> Hi Peter,
>
> On 03/20/2018 11:31 AM, Peter Rosin wrote:
>> In preparation for PCA9641 support, convert the mybus and busoff macros
>> to functions, and in the process prefix them with pca9541_. Also prefix
>> remaining chip specific macros with PCA9541_.
>>
>> Signed-off-by: Peter Rosin <[email protected]>
>> ---
>> drivers/i2c/muxes/i2c-mux-pca9541.c | 26 +++++++++++++++++++-------
>> 1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
>> index ad168125d23d..47685eb4e0e9 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
>> @@ -59,10 +59,8 @@
>> #define PCA9541_ISTAT_MYTEST BIT(6)
>> #define PCA9541_ISTAT_NMYTEST BIT(7)
>>
>> -#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 PCA9541_BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
>> +#define PCA9541_MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
>>
>> /* arbitration timeouts, in jiffies */
>> #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */
>> @@ -93,6 +91,20 @@ static const struct of_device_id pca9541_of_match[] = {
>> MODULE_DEVICE_TABLE(of, pca9541_of_match);
>> #endif
>>
>> +static int pca9541_mybus(int ctl)
>
> static inline?

No, "inline" is only used in header files in the kernel. The compiler
is free to inline whatever function it likes anyway, and in this case
we do not know better than the compiler. We don't care either. At least,
that is my understanding of the situation regarding the "inline"
keyword.

>
>> +{
>> + if (!(ctl & PCA9541_MYBUS))
>> + return 1;
>> + return (ctl & PCA9541_MYBUS) == PCA9541_MYBUS;
>> +}
>> +
>> +static int pca9541_busoff(int ctl)
>
> static inline?
>
>> +{
>> + if (!(ctl & PCA9541_BUSON))
>> + return 1;
>> + return (ctl & PCA9541_BUSON) == PCA9541_BUSON;
>> +}
>
> Reviewed-by: Vladimir Zapolskiy <[email protected]>

Thanks!

Cheers,
Peter

>
> --
> With best wishes,
> Vladimir
>

2018-03-21 06:55:24

[permalink] [raw]

Subject: Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

Hi Peter,

On 03/21/2018 07:53 AM, Peter Rosin wrote:
> On 2018-03-21 00:24, Vladimir Zapolskiy wrote:
>> Hi Peter,
>>
>> On 03/20/2018 11:31 AM, Peter Rosin wrote:
>>> In preparation for PCA9641 support, convert the mybus and busoff macros
>>> to functions, and in the process prefix them with pca9541_. Also prefix
>>> remaining chip specific macros with PCA9541_.
>>>
>>> Signed-off-by: Peter Rosin <[email protected]>
>>> ---
>>> drivers/i2c/muxes/i2c-mux-pca9541.c | 26 +++++++++++++++++++-------
>>> 1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
>>> index ad168125d23d..47685eb4e0e9 100644
>>> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
>>> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
>>> @@ -59,10 +59,8 @@
>>> #define PCA9541_ISTAT_MYTEST BIT(6)
>>> #define PCA9541_ISTAT_NMYTEST BIT(7)
>>>
>>> -#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 PCA9541_BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
>>> +#define PCA9541_MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
>>>
>>> /* arbitration timeouts, in jiffies */
>>> #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */
>>> @@ -93,6 +91,20 @@ static const struct of_device_id pca9541_of_match[] = {
>>> MODULE_DEVICE_TABLE(of, pca9541_of_match);
>>> #endif
>>>
>>> +static int pca9541_mybus(int ctl)
>>
>> static inline?
>
> No, "inline" is only used in header files in the kernel.

No, it is an incorrect statement, you should be aware of that.

> The compiler is free to inline whatever function it likes anyway, and
> in this case we do not know better than the compiler. We don't care

That's a candidate case, when we could know better than the compiler.

But "don't care" argument is still valid :)

--
With best wishes,
Vladimir

2018-03-21 07:02:44

[permalink] [raw]

Subject: Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

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

2018-03-21 07:06:19

[permalink] [raw]

Subject: Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

On 03/20/2018 11:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
>
> Signed-off-by: Peter Rosin <[email protected]>

Reviewed-by: Vladimir Zapolskiy <[email protected]>

The change is really good and correct, it is just too extended IMHO.

--
With best wishes,
Vladimir

2018-03-21 07:37:16

[permalink] [raw]

Subject: Re: [PATCH 2/3] i2c: mux: pca9541: namespace cleanup

On 2018-03-21 07:54, Vladimir Zapolskiy wrote:
> Hi Peter,
>
> On 03/21/2018 07:53 AM, Peter Rosin wrote:
>> On 2018-03-21 00:24, Vladimir Zapolskiy wrote:
>>> Hi Peter,
>>>
>>> On 03/20/2018 11:31 AM, Peter Rosin wrote:
>>>> In preparation for PCA9641 support, convert the mybus and busoff macros
>>>> to functions, and in the process prefix them with pca9541_. Also prefix
>>>> remaining chip specific macros with PCA9541_.
>>>>
>>>> Signed-off-by: Peter Rosin <[email protected]>
>>>> ---
>>>> drivers/i2c/muxes/i2c-mux-pca9541.c | 26 +++++++++++++++++++-------
>>>> 1 file changed, 19 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
>>>> index ad168125d23d..47685eb4e0e9 100644
>>>> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
>>>> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
>>>> @@ -59,10 +59,8 @@
>>>> #define PCA9541_ISTAT_MYTEST BIT(6)
>>>> #define PCA9541_ISTAT_NMYTEST BIT(7)
>>>>
>>>> -#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 PCA9541_BUSON (PCA9541_CTL_BUSON | PCA9541_CTL_NBUSON)
>>>> +#define PCA9541_MYBUS (PCA9541_CTL_MYBUS | PCA9541_CTL_NMYBUS)
>>>>
>>>> /* arbitration timeouts, in jiffies */
>>>> #define ARB_TIMEOUT (HZ / 8) /* 125 ms until forcing bus ownership */
>>>> @@ -93,6 +91,20 @@ static const struct of_device_id pca9541_of_match[] = {
>>>> MODULE_DEVICE_TABLE(of, pca9541_of_match);
>>>> #endif
>>>>
>>>> +static int pca9541_mybus(int ctl)
>>>
>>> static inline?
>>
>> No, "inline" is only used in header files in the kernel.
>
> No, it is an incorrect statement, you should be aware of that.

Yeah, that was sloppy wording on my part. Let's say I meant useful
instead of used. My point is that inline is quite useless (in a C
file), the compiler will do its thing anyway. Rhetorical question:
what is the point of having both noinline and __always_inline?
Because plain old inline is overridden by the compiler, just like
the register keyword.

>> The compiler is free to inline whatever function it likes anyway, and
>> in this case we do not know better than the compiler. We don't care
>
> That's a candidate case, when we could know better than the compiler.

Could we? Maybe for specific compilers and architectures, but
probably not for all cases. And the future is in the cards etc. And
we don't actually know even for current compilers. Also, quoting
Documentation/process/4.Coding

More recent compilers take an increasingly active role in deciding
whether a given function should actually be inlined or not. So the
liberal placement of "inline" keywords may not just be excessive; it
could also be irrelevant.

> But "don't care" argument is still valid :)

Yes :-)

Cheers,
Peter

2018-03-21 08:11:54

[permalink] [raw]

Subject: Re: [PATCH 3/3] i2c: mux: pca9541: prepare for PCA9641 support

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

2018-04-11 09:41:40

[permalink] [raw]

Subject: Re: [PATCH linux dev-4.16 v2] i2c: muxes: pca9641: new driver

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 <[email protected]>
>
> 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
>

2018-04-13 07:01:16

by ChenKenYY 陳永營 TAO

[permalink] [raw]

Subject: Re: [PATCH linux dev-4.16 v2] i2c: muxes: pca9641: new driver

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 <[email protected]>:
> 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 <[email protected]>
>>
>> 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
>>
>

2018-04-13 08:11:54