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

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

[Ken Chen]

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

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

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

[Ken Chen]

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

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

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

[Joel Stanley]

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
>

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

[Peter Rosin]

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

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

[Peter Rosin]

Because it looks nice!

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 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

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

[Peter Rosin]

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 <peda@axentia.se>
---
 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

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

[Peter Rosin]

Make the arbitrate and release_bus implementation chip specific.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 17 deletions(-)

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

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

[Guenter Roeck]

On 03/20/2018 02:31 AM, Peter Rosin wrote:
> Because it looks nice!
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

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

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

[Guenter Roeck]

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 <peda@axentia.se>
> ---
>   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.
> 

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

[Guenter Roeck]

On 03/20/2018 02:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++++++++++++++++++++++++++----------
>   1 file changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 47685eb4e0e9..cac629e36bf8 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -23,6 +23,7 @@
>   #include <linux/i2c-mux.h>
>   #include <linux/jiffies.h>
>   #include <linux/module.h>
> +#include <linux/of_device.h>
>   #include <linux/platform_data/pca954x.h>
>   #include <linux/slab.h>
>   
> @@ -70,26 +71,22 @@
>   #define SELECT_DELAY_SHORT	50
>   #define SELECT_DELAY_LONG	1000
>   
> -struct pca9541 {
> -	struct i2c_client *client;
> -	unsigned long select_timeout;
> -	unsigned long arb_timeout;
> +enum chip_name {
> +	pca9541,
>   };
>   
> -static const struct i2c_device_id pca9541_id[] = {
> -	{"pca9541", 0},
> -	{}
> +struct chip_desc {
> +	int (*arbitrate)(struct i2c_client *client);
> +	void (*release_bus)(struct i2c_client *client);
>   };
>   
> -MODULE_DEVICE_TABLE(i2c, pca9541_id);
> +struct pca9541 {
> +	const struct chip_desc *chip;
>   
> -#ifdef CONFIG_OF
> -static const struct of_device_id pca9541_of_match[] = {
> -	{ .compatible = "nxp,pca9541" },
> -	{}
> +	struct i2c_client *client;
> +	unsigned long select_timeout;
> +	unsigned long arb_timeout;
>   };
> -MODULE_DEVICE_TABLE(of, pca9541_of_match);
> -#endif
>   
>   static int pca9541_mybus(int ctl)
>   {
> @@ -318,7 +315,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan)
>   		/* force bus ownership after this time */
>   
>   	do {
> -		ret = pca9541_arbitrate(client);
> +		ret = data->chip->arbitrate(client);
>   		if (ret)
>   			return ret < 0 ? ret : 0;
>   
> @@ -336,10 +333,32 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)
>   	struct pca9541 *data = i2c_mux_priv(muxc);
>   	struct i2c_client *client = data->client;
>   
> -	pca9541_release_bus(client);
> +	data->chip->release_bus(client);
>   	return 0;
>   }
>   
> +static const struct chip_desc chips[] = {
> +	[pca9541] = {
> +		.arbitrate = pca9541_arbitrate,
> +		.release_bus = pca9541_release_bus,
> +	},
> +};
> +
> +static const struct i2c_device_id pca9541_id[] = {
> +	{ "pca9541", pca9541 },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, pca9541_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id pca9541_of_match[] = {
> +	{ .compatible = "nxp,pca9541", .data = &chips[pca9541] },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, pca9541_of_match);
> +#endif
> +
>   /*
>    * I2C init/probing/exit functions
>    */
> @@ -348,6 +367,8 @@ static int pca9541_probe(struct i2c_client *client,
>   {
>   	struct i2c_adapter *adap = client->adapter;
>   	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	const struct of_device_id *match;
> +	const struct chip_desc *chip;
>   	struct i2c_mux_core *muxc;
>   	struct pca9541 *data;
>   	int force;
> @@ -356,12 +377,18 @@ static int pca9541_probe(struct i2c_client *client,
>   	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
>   		return -ENODEV;
>   
> +	match = of_match_device(of_match_ptr(pca9541_of_match), &client->dev);
> +	if (match)
> +		chip = of_device_get_match_data(&client->dev);
> +	else
> +		chip = &chips[id->driver_data];
> +
>   	/*
>   	 * I2C accesses are unprotected here.
>   	 * We have to lock the adapter before releasing the bus.
>   	 */
>   	i2c_lock_adapter(adap);
> -	pca9541_release_bus(client);
> +	chip->release_bus(client);
>   	i2c_unlock_adapter(adap);
>   
>   	/* Create mux adapter */
> @@ -376,6 +403,7 @@ static int pca9541_probe(struct i2c_client *client,
>   		return -ENOMEM;
>   
>   	data = i2c_mux_priv(muxc);
> +	data->chip = chip;
>   	data->client = client;
>   
>   	i2c_set_clientdata(client, muxc);
> 

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

[Peter Rosin]

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

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

[Guenter Roeck]

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

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

[Vladimir Zapolskiy]

Hi Peter, Ken,

On 03/20/2018 11:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
> 

by chance I took a look at the original implementation done by Ken, and
I would say that this 3/3 change is an overkill as a too generic one.
Is there any next observable extension? And do two abstracted (*arbitrate)
and (*release_bus) cover it well? Probably no.

At first it would be simpler to add a new chip id field into struct pca9541
(struct rename would be needed of course), and do a selection of specific
pca9x41_arbitrate() and pca9x41_release_bus() depending on it:

----8<----
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 47685eb..a40e6d8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -70,8 +70,13 @@
 #define SELECT_DELAY_SHORT	50
 #define SELECT_DELAY_LONG	1000
 
+enum chip_id {
+	pca9541,
+};
+
 struct pca9541 {
 	struct i2c_client *client;
+	enum chip_id id;
 	unsigned long select_timeout;
 	unsigned long arb_timeout;
 };
@@ -188,10 +193,13 @@ static int pca9541_reg_read(struct i2c_client *client, u8 command)
  */
 
 /* Release bus. Also reset NTESTON and BUSINIT if it was set. */
-static void pca9541_release_bus(struct i2c_client *client)
+static void pca9541_release_bus(struct i2c_client *client, enum chip_id id)
 {
 	int reg;
 
+	if (id != pca9541)
+		return;
+
 	reg = pca9541_reg_read(client, PCA9541_CONTROL);
 	if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
 		pca9541_reg_write(client, PCA9541_CONTROL,
@@ -235,12 +243,15 @@ static const u8 pca9541_control[16] = {
  *  0 : bus not acquired
  *  1 : bus acquired
  */
-static int pca9541_arbitrate(struct i2c_client *client)
+static int pca9541_arbitrate(struct i2c_client *client, enum chip_id id)
 {
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
 	struct pca9541 *data = i2c_mux_priv(muxc);
 	int reg;
 
+	if (id != pca9541)
+		return -EOPNOTSUPP;
+
 	reg = pca9541_reg_read(client, PCA9541_CONTROL);
 	if (reg < 0)
 		return reg;
@@ -318,7 +329,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan)
 		/* force bus ownership after this time */
 
 	do {
-		ret = pca9541_arbitrate(client);
+		ret = pca9541_arbitrate(client, data->id);
 		if (ret)
 			return ret < 0 ? ret : 0;
 
@@ -336,7 +347,7 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)
 	struct pca9541 *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 
-	pca9541_release_bus(client);
+	pca9541_release_bus(client, data->id);
 	return 0;
 }
 
@@ -361,7 +372,7 @@ static int pca9541_probe(struct i2c_client *client,
 	 * We have to lock the adapter before releasing the bus.
 	 */
 	i2c_lock_adapter(adap);
-	pca9541_release_bus(client);
+	pca9541_release_bus(client, pca9541);
 	i2c_unlock_adapter(adap);
 
 	/* Create mux adapter */
@@ -377,6 +388,7 @@ static int pca9541_probe(struct i2c_client *client,
 
 	data = i2c_mux_priv(muxc);
 	data->client = client;
+	data->id = pca9541;
 
 	i2c_set_clientdata(client, muxc);
 
----8<----


> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++++++++++++++++++++++++++----------
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 

The change above is trivial and it does not cancel any further
extensions similar to your idea, the open question is if there
is a demand right at the moment.

> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 47685eb4e0e9..cac629e36bf8 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -23,6 +23,7 @@
>  #include <linux/i2c-mux.h>
>  #include <linux/jiffies.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_data/pca954x.h>
>  #include <linux/slab.h>
>  
> @@ -70,26 +71,22 @@
>  #define SELECT_DELAY_SHORT	50
>  #define SELECT_DELAY_LONG	1000
>  
> -struct pca9541 {
> -	struct i2c_client *client;
> -	unsigned long select_timeout;
> -	unsigned long arb_timeout;
> +enum chip_name {

chip_name sound like a string storage, chip_id might be better here.

> +	pca9541,
>  };
>  

--
With best wishes,
Vladimir

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

[Vladimir Zapolskiy]

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 <peda@axentia.se>
> ---
>  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 <vz@mleia.com>

--
With best wishes,
Vladimir

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

[Vladimir Zapolskiy]

On 03/20/2018 11:31 AM, Peter Rosin wrote:
> Because it looks nice!
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>

--
With best wishes,
Vladimir

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

[Guenter Roeck]

On 03/20/2018 04:17 PM, Vladimir Zapolskiy wrote:
> Hi Peter, Ken,
> 
> On 03/20/2018 11:32 AM, Peter Rosin wrote:
>> Make the arbitrate and release_bus implementation chip specific.
>>
> 
> by chance I took a look at the original implementation done by Ken, and
> I would say that this 3/3 change is an overkill as a too generic one.
> Is there any next observable extension? And do two abstracted (*arbitrate)
> and (*release_bus) cover it well? Probably no.
> 
> At first it would be simpler to add a new chip id field into struct pca9541
> (struct rename would be needed of course), and do a selection of specific
> pca9x41_arbitrate() and pca9x41_release_bus() depending on it:
> 

FWIW, I very much prefer Peter's code. I think it is much cleaner.

Guenter

> ----8<----
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
> index 47685eb..a40e6d8 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
> @@ -70,8 +70,13 @@
>   #define SELECT_DELAY_SHORT	50
>   #define SELECT_DELAY_LONG	1000
>   
> +enum chip_id {
> +	pca9541,
> +};
> +
>   struct pca9541 {
>   	struct i2c_client *client;
> +	enum chip_id id;
>   	unsigned long select_timeout;
>   	unsigned long arb_timeout;
>   };
> @@ -188,10 +193,13 @@ static int pca9541_reg_read(struct i2c_client *client, u8 command)
>    */
>   
>   /* Release bus. Also reset NTESTON and BUSINIT if it was set. */
> -static void pca9541_release_bus(struct i2c_client *client)
> +static void pca9541_release_bus(struct i2c_client *client, enum chip_id id)
>   {
>   	int reg;
>   
> +	if (id != pca9541)
> +		return;
> +
>   	reg = pca9541_reg_read(client, PCA9541_CONTROL);
>   	if (reg >= 0 && !pca9541_busoff(reg) && pca9541_mybus(reg))
>   		pca9541_reg_write(client, PCA9541_CONTROL,
> @@ -235,12 +243,15 @@ static const u8 pca9541_control[16] = {
>    *  0 : bus not acquired
>    *  1 : bus acquired
>    */
> -static int pca9541_arbitrate(struct i2c_client *client)
> +static int pca9541_arbitrate(struct i2c_client *client, enum chip_id id)
>   {
>   	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>   	struct pca9541 *data = i2c_mux_priv(muxc);
>   	int reg;
>   
> +	if (id != pca9541)
> +		return -EOPNOTSUPP;
> +
>   	reg = pca9541_reg_read(client, PCA9541_CONTROL);
>   	if (reg < 0)
>   		return reg;
> @@ -318,7 +329,7 @@ static int pca9541_select_chan(struct i2c_mux_core *muxc, u32 chan)
>   		/* force bus ownership after this time */
>   
>   	do {
> -		ret = pca9541_arbitrate(client);
> +		ret = pca9541_arbitrate(client, data->id);
>   		if (ret)
>   			return ret < 0 ? ret : 0;
>   
> @@ -336,7 +347,7 @@ static int pca9541_release_chan(struct i2c_mux_core *muxc, u32 chan)
>   	struct pca9541 *data = i2c_mux_priv(muxc);
>   	struct i2c_client *client = data->client;
>   
> -	pca9541_release_bus(client);
> +	pca9541_release_bus(client, data->id);
>   	return 0;
>   }
>   
> @@ -361,7 +372,7 @@ static int pca9541_probe(struct i2c_client *client,
>   	 * We have to lock the adapter before releasing the bus.
>   	 */
>   	i2c_lock_adapter(adap);
> -	pca9541_release_bus(client);
> +	pca9541_release_bus(client, pca9541);
>   	i2c_unlock_adapter(adap);
>   
>   	/* Create mux adapter */
> @@ -377,6 +388,7 @@ static int pca9541_probe(struct i2c_client *client,
>   
>   	data = i2c_mux_priv(muxc);
>   	data->client = client;
> +	data->id = pca9541;
>   
>   	i2c_set_clientdata(client, muxc);
>   
> ----8<----
> 
> 
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>   drivers/i2c/muxes/i2c-mux-pca9541.c | 62 +++++++++++++++++++++++++++----------
>>   1 file changed, 45 insertions(+), 17 deletions(-)
>>
> 
> The change above is trivial and it does not cancel any further
> extensions similar to your idea, the open question is if there
> is a demand right at the moment.
> 
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
>> index 47685eb4e0e9..cac629e36bf8 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca9541.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/i2c-mux.h>
>>   #include <linux/jiffies.h>
>>   #include <linux/module.h>
>> +#include <linux/of_device.h>
>>   #include <linux/platform_data/pca954x.h>
>>   #include <linux/slab.h>
>>   
>> @@ -70,26 +71,22 @@
>>   #define SELECT_DELAY_SHORT	50
>>   #define SELECT_DELAY_LONG	1000
>>   
>> -struct pca9541 {
>> -	struct i2c_client *client;
>> -	unsigned long select_timeout;
>> -	unsigned long arb_timeout;
>> +enum chip_name {
> 
> chip_name sound like a string storage, chip_id might be better here.
> 
>> +	pca9541,
>>   };
>>   
> 
> --
> With best wishes,
> Vladimir
> 

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

[Peter Rosin]

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 <peda@axentia.se>
>> ---
>>  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 <vz@mleia.com>

Thanks!

Cheers,
Peter

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

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

[Vladimir Zapolskiy]

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 <peda@axentia.se>
>>> ---
>>>  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

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

[Vladimir Zapolskiy]

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

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

[Vladimir Zapolskiy]

On 03/20/2018 11:32 AM, Peter Rosin wrote:
> Make the arbitrate and release_bus implementation chip specific.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>


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

--
With best wishes,
Vladimir

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

[Peter Rosin]

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 <peda@axentia.se>
>>>> ---
>>>>  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

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

[Peter Rosin]

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

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

[Peter Rosin]

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
> 

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

[ChenKenYY 陳永營 TAO]

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

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

[ChenKenYY 陳永營 TAO]

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

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

[Peter Rosin]

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

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

[ChenKenYY 陳永營 TAO]

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

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

[ChenKenYY 陳永營 TAO]

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