All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver
@ 2018-03-19 11:38 ` Ken Chen
  0 siblings, 0 replies; 8+ messages in thread
From: Ken Chen @ 2018-03-19 11:38 UTC (permalink / raw)
  To: linux, wsa, peda, linux-i2c, linux-kernel; +Cc: Ken Chen, joel

Initial PCA9641 2 chennel I2C bus master arbiter
with separate implementation. It has probe issue
and difference device hehavior between PCA9541
and PCA9641 in original PCA9641 patch.

Signed-off-by: Ken Chen <chen.kenyy@inventec.com>
---
 drivers/i2c/muxes/Kconfig           |   9 +
 drivers/i2c/muxes/Makefile          |   1 +
 drivers/i2c/muxes/i2c-mux-pca9641.c | 360 ++++++++++++++++++++++++++++++++++++
 3 files changed, 370 insertions(+)
 create mode 100644 drivers/i2c/muxes/i2c-mux-pca9641.c

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 52a4a92..f9c51b8 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -73,6 +73,15 @@ config I2C_MUX_PCA954x
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca954x.
 
+config I2C_MUX_PCA9641
+	tristate "NXP PCA9641 I2C Master demultiplexer"
+	help
+	  If you say yes here you get support for the NXP PCA9641
+	  I2C Master demultiplexer with an arbiter function.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-mux-pca9641.
+
 config I2C_MUX_PINCTRL
 	tristate "pinctrl-based I2C multiplexer"
 	depends on PINCTRL
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 6d9d865..a229a1a 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_I2C_MUX_LTC4306)	+= i2c-mux-ltc4306.o
 obj-$(CONFIG_I2C_MUX_MLXCPLD)	+= i2c-mux-mlxcpld.o
 obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
 obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
+obj-$(CONFIG_I2C_MUX_PCA9641)	+= i2c-mux-pca9641.o
 obj-$(CONFIG_I2C_MUX_PINCTRL)	+= i2c-mux-pinctrl.o
 obj-$(CONFIG_I2C_MUX_REG)	+= i2c-mux-reg.o
 
diff --git a/drivers/i2c/muxes/i2c-mux-pca9641.c b/drivers/i2c/muxes/i2c-mux-pca9641.c
new file mode 100644
index 0000000..bcf45c8
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-mux-pca9641.c
@@ -0,0 +1,360 @@
+/*
+ * I2C demultiplexer driver for PCA9641 bus master demultiplexer
+ *
+ * Copyright (c) 2010 Ericsson AB.
+ *
+ * Author: Ken Chen <chen.kenyy@inventec.com>
+ *
+ * Derived from:
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/module.h>
+#include <linux/jiffies.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+
+#include <linux/i2c/pca954x.h>
+
+/*
+ * The PCA9641 is two I2C bus masters demultiplexer. It supports two I2C masters
+ * connected to a single slave bus.
+ *
+ * It is designed for high reliability dual master I2C bus applications where
+ * correct system operation is required, even when two I2C master issue their
+ * commands at the same time. The arbiter will select a winner and let it work
+ * uninterrupted, and the losing master will take control of the I2C bus after
+ * the winnter has finished. The arbiter also allows for queued requests where
+ * a master requests the downstream bus while the other master has control.
+ *
+ */
+
+#define PCA9641_ID		0x01
+#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 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 */
+
+/* arbitration retry delays, in us */
+#define SELECT_DELAY_SHORT	50
+#define SELECT_DELAY_LONG	1000
+
+struct pca9641 {
+	struct i2c_client *client;
+	unsigned long select_timeout;
+	unsigned long arb_timeout;
+};
+
+static const struct i2c_device_id pca9641_id[] = {
+	{"pca9641", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, pca9641_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id pca9641_of_match[] = {
+	{ .compatible = "nxp,pca9641" },
+	{}
+};
+#endif
+
+/*
+ * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
+ * as they will try to lock the adapter a second time.
+ */
+static int pca9641_reg_write(struct i2c_client *client, u8 command, u8 val)
+{
+	struct i2c_adapter *adap = client->adapter;
+	int ret;
+
+	if (adap->algo->master_xfer) {
+		struct i2c_msg msg;
+		char buf[2];
+
+		msg.addr = client->addr;
+		msg.flags = 0;
+		msg.len = 2;
+		buf[0] = command;
+		buf[1] = val;
+		msg.buf = buf;
+		ret = __i2c_transfer(adap, &msg, 1);
+	} else {
+		union i2c_smbus_data data;
+
+		data.byte = val;
+		ret = adap->algo->smbus_xfer(adap, client->addr,
+					     client->flags,
+					     I2C_SMBUS_WRITE,
+					     command,
+					     I2C_SMBUS_BYTE_DATA, &data);
+	}
+
+	return ret;
+}
+
+/*
+ * Read from chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
+ * as they will try to lock adapter a second time.
+ */
+static int pca9641_reg_read(struct i2c_client *client, u8 command)
+{
+	struct i2c_adapter *adap = client->adapter;
+	int ret;
+	u8 val;
+
+	if (adap->algo->master_xfer) {
+		struct i2c_msg msg[2] = {
+			{
+				.addr = client->addr,
+				.flags = 0,
+				.len = 1,
+				.buf = &command
+			},
+			{
+				.addr = client->addr,
+				.flags = I2C_M_RD,
+				.len = 1,
+				.buf = &val
+			}
+		};
+		ret = __i2c_transfer(adap, msg, 2);
+		if (ret == 2)
+			ret = val;
+		else if (ret >= 0)
+			ret = -EIO;
+	} else {
+		union i2c_smbus_data data;
+
+		ret = adap->algo->smbus_xfer(adap, client->addr,
+					     client->flags,
+					     I2C_SMBUS_READ,
+					     command,
+					     I2C_SMBUS_BYTE_DATA, &data);
+		if (!ret)
+			ret = data.byte;
+	}
+	return ret;
+}
+
+/*
+ * Arbitration management functions
+ */
+static void pca9641_release_bus(struct i2c_client *client)
+{
+	pca9641_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 pca9641 *data = i2c_mux_priv(muxc);
+	int reg_ctl, reg_sts;
+
+	reg_ctl = pca9641_reg_read(client, PCA9641_CONTROL);
+	if (reg_ctl < 0)
+		return reg_ctl;
+	reg_sts = pca9641_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;
+		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
+		reg_ctl = pca9641_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;
+			pca9641_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;
+		pca9641_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;
+		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
+	}
+	return 0;
+}
+
+static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan)
+{
+	struct pca9641 *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 pca9641 *data = i2c_mux_priv(muxc);
+	struct i2c_client *client = data->client;
+
+	pca9641_release_bus(client);
+	return 0;
+}
+
+/*
+ * I2C init/probing/exit functions
+ */
+static int pca9641_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adap = client->adapter;
+	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct i2c_mux_core *muxc;
+	struct pca9641 *data;
+	int force;
+	int ret;
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	/*
+	 * I2C accesses are unprotected here.
+	 * We have to lock the adapter before releasing the bus.
+	 */
+	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, 8, 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) {
+		dev_err(&client->dev, "failed to register master demultiplexer\n");
+		return ret;
+	}
+
+	dev_info(&client->dev, "registered master demultiplexer for I2C %s\n",
+		 client->name);
+
+	return 0;
+}
+
+static int pca9641_remove(struct i2c_client *client)
+{
+	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+
+	i2c_mux_del_adapters(muxc);
+	return 0;
+}
+
+static struct i2c_driver pca9641_driver = {
+	.driver = {
+		   .name = "pca9641",
+		   .of_match_table = of_match_ptr(pca9641_of_match),
+		   },
+	.probe = pca9641_probe,
+	.remove = pca9641_remove,
+	.id_table = pca9641_id,
+};
+
+module_i2c_driver(pca9641_driver);
+
+MODULE_AUTHOR("Ken Chen <chen.kenyy@inventec.com>");
+MODULE_DESCRIPTION("PCA9641 I2C master demultiplexer driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver
@ 2018-03-19 11:38 ` Ken Chen
  0 siblings, 0 replies; 8+ messages in thread
From: Ken Chen @ 2018-03-19 11:38 UTC (permalink / raw)
  To: linux, wsa, peda, linux-i2c, linux-kernel; +Cc: Ken Chen, joel

Initial PCA9641 2 chennel I2C bus master arbiter
with separate implementation. It has probe issue
and difference device hehavior between PCA9541
and PCA9641 in original PCA9641 patch.

Signed-off-by: Ken Chen <chen.kenyy@inventec.com>
---
 drivers/i2c/muxes/Kconfig           |   9 +
 drivers/i2c/muxes/Makefile          |   1 +
 drivers/i2c/muxes/i2c-mux-pca9641.c | 360 ++++++++++++++++++++++++++++++++++++
 3 files changed, 370 insertions(+)
 create mode 100644 drivers/i2c/muxes/i2c-mux-pca9641.c

diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index 52a4a92..f9c51b8 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -73,6 +73,15 @@ config I2C_MUX_PCA954x
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mux-pca954x.
 
+config I2C_MUX_PCA9641
+	tristate "NXP PCA9641 I2C Master demultiplexer"
+	help
+	  If you say yes here you get support for the NXP PCA9641
+	  I2C Master demultiplexer with an arbiter function.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-mux-pca9641.
+
 config I2C_MUX_PINCTRL
 	tristate "pinctrl-based I2C multiplexer"
 	depends on PINCTRL
diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
index 6d9d865..a229a1a 100644
--- a/drivers/i2c/muxes/Makefile
+++ b/drivers/i2c/muxes/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_I2C_MUX_LTC4306)	+= i2c-mux-ltc4306.o
 obj-$(CONFIG_I2C_MUX_MLXCPLD)	+= i2c-mux-mlxcpld.o
 obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
 obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
+obj-$(CONFIG_I2C_MUX_PCA9641)	+= i2c-mux-pca9641.o
 obj-$(CONFIG_I2C_MUX_PINCTRL)	+= i2c-mux-pinctrl.o
 obj-$(CONFIG_I2C_MUX_REG)	+= i2c-mux-reg.o
 
diff --git a/drivers/i2c/muxes/i2c-mux-pca9641.c b/drivers/i2c/muxes/i2c-mux-pca9641.c
new file mode 100644
index 0000000..bcf45c8
--- /dev/null
+++ b/drivers/i2c/muxes/i2c-mux-pca9641.c
@@ -0,0 +1,360 @@
+/*
+ * I2C demultiplexer driver for PCA9641 bus master demultiplexer
+ *
+ * Copyright (c) 2010 Ericsson AB.
+ *
+ * Author: Ken Chen <chen.kenyy@inventec.com>
+ *
+ * Derived from:
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/module.h>
+#include <linux/jiffies.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
+
+#include <linux/i2c/pca954x.h>
+
+/*
+ * The PCA9641 is two I2C bus masters demultiplexer. It supports two I2C masters
+ * connected to a single slave bus.
+ *
+ * It is designed for high reliability dual master I2C bus applications where
+ * correct system operation is required, even when two I2C master issue their
+ * commands at the same time. The arbiter will select a winner and let it work
+ * uninterrupted, and the losing master will take control of the I2C bus after
+ * the winnter has finished. The arbiter also allows for queued requests where
+ * a master requests the downstream bus while the other master has control.
+ *
+ */
+
+#define PCA9641_ID		0x01
+#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 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 */
+
+/* arbitration retry delays, in us */
+#define SELECT_DELAY_SHORT	50
+#define SELECT_DELAY_LONG	1000
+
+struct pca9641 {
+	struct i2c_client *client;
+	unsigned long select_timeout;
+	unsigned long arb_timeout;
+};
+
+static const struct i2c_device_id pca9641_id[] = {
+	{"pca9641", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, pca9641_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id pca9641_of_match[] = {
+	{ .compatible = "nxp,pca9641" },
+	{}
+};
+#endif
+
+/*
+ * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
+ * as they will try to lock the adapter a second time.
+ */
+static int pca9641_reg_write(struct i2c_client *client, u8 command, u8 val)
+{
+	struct i2c_adapter *adap = client->adapter;
+	int ret;
+
+	if (adap->algo->master_xfer) {
+		struct i2c_msg msg;
+		char buf[2];
+
+		msg.addr = client->addr;
+		msg.flags = 0;
+		msg.len = 2;
+		buf[0] = command;
+		buf[1] = val;
+		msg.buf = buf;
+		ret = __i2c_transfer(adap, &msg, 1);
+	} else {
+		union i2c_smbus_data data;
+
+		data.byte = val;
+		ret = adap->algo->smbus_xfer(adap, client->addr,
+					     client->flags,
+					     I2C_SMBUS_WRITE,
+					     command,
+					     I2C_SMBUS_BYTE_DATA, &data);
+	}
+
+	return ret;
+}
+
+/*
+ * Read from chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
+ * as they will try to lock adapter a second time.
+ */
+static int pca9641_reg_read(struct i2c_client *client, u8 command)
+{
+	struct i2c_adapter *adap = client->adapter;
+	int ret;
+	u8 val;
+
+	if (adap->algo->master_xfer) {
+		struct i2c_msg msg[2] = {
+			{
+				.addr = client->addr,
+				.flags = 0,
+				.len = 1,
+				.buf = &command
+			},
+			{
+				.addr = client->addr,
+				.flags = I2C_M_RD,
+				.len = 1,
+				.buf = &val
+			}
+		};
+		ret = __i2c_transfer(adap, msg, 2);
+		if (ret == 2)
+			ret = val;
+		else if (ret >= 0)
+			ret = -EIO;
+	} else {
+		union i2c_smbus_data data;
+
+		ret = adap->algo->smbus_xfer(adap, client->addr,
+					     client->flags,
+					     I2C_SMBUS_READ,
+					     command,
+					     I2C_SMBUS_BYTE_DATA, &data);
+		if (!ret)
+			ret = data.byte;
+	}
+	return ret;
+}
+
+/*
+ * Arbitration management functions
+ */
+static void pca9641_release_bus(struct i2c_client *client)
+{
+	pca9641_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 pca9641 *data = i2c_mux_priv(muxc);
+	int reg_ctl, reg_sts;
+
+	reg_ctl = pca9641_reg_read(client, PCA9641_CONTROL);
+	if (reg_ctl < 0)
+		return reg_ctl;
+	reg_sts = pca9641_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;
+		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
+		reg_ctl = pca9641_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;
+			pca9641_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;
+		pca9641_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;
+		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
+	}
+	return 0;
+}
+
+static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan)
+{
+	struct pca9641 *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 pca9641 *data = i2c_mux_priv(muxc);
+	struct i2c_client *client = data->client;
+
+	pca9641_release_bus(client);
+	return 0;
+}
+
+/*
+ * I2C init/probing/exit functions
+ */
+static int pca9641_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct i2c_adapter *adap = client->adapter;
+	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct i2c_mux_core *muxc;
+	struct pca9641 *data;
+	int force;
+	int ret;
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	/*
+	 * I2C accesses are unprotected here.
+	 * We have to lock the adapter before releasing the bus.
+	 */
+	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, 8, 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) {
+		dev_err(&client->dev, "failed to register master demultiplexer\n");
+		return ret;
+	}
+
+	dev_info(&client->dev, "registered master demultiplexer for I2C %s\n",
+		 client->name);
+
+	return 0;
+}
+
+static int pca9641_remove(struct i2c_client *client)
+{
+	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+
+	i2c_mux_del_adapters(muxc);
+	return 0;
+}
+
+static struct i2c_driver pca9641_driver = {
+	.driver = {
+		   .name = "pca9641",
+		   .of_match_table = of_match_ptr(pca9641_of_match),
+		   },
+	.probe = pca9641_probe,
+	.remove = pca9641_remove,
+	.id_table = pca9641_id,
+};
+
+module_i2c_driver(pca9641_driver);
+
+MODULE_AUTHOR("Ken Chen <chen.kenyy@inventec.com>");
+MODULE_DESCRIPTION("PCA9641 I2C master demultiplexer driver");
+MODULE_LICENSE("GPL v2");
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver
  2018-03-19 11:38 ` Ken Chen
  (?)
@ 2018-03-19 12:45 ` Peter Rosin
  2018-03-19 13:44   ` Guenter Roeck
  -1 siblings, 1 reply; 8+ messages in thread
From: Peter Rosin @ 2018-03-19 12:45 UTC (permalink / raw)
  To: Ken Chen, linux, wsa, linux-i2c, linux-kernel; +Cc: joel

Hi Ken,

Thanks for the patch!

I would have liked this subject:
i2c: muxes: pca9641: new driver

On 2018-03-19 12:38, Ken Chen wrote:
> Initial PCA9641 2 chennel I2C bus master arbiter

channel

> with separate implementation. It has probe issue
> and difference device hehavior between PCA9541

behavior

> and PCA9641 in original PCA9641 patch.
> 
> Signed-off-by: Ken Chen <chen.kenyy@inventec.com>
> ---
>  drivers/i2c/muxes/Kconfig           |   9 +
>  drivers/i2c/muxes/Makefile          |   1 +
>  drivers/i2c/muxes/i2c-mux-pca9641.c | 360 ++++++++++++++++++++++++++++++++++++

Given the big similarities between this and the pca9541 driver,
I would very much prefer if you could extend that driver instead
of making an almost-copy like this.

I have added some comments below anyway, but most of them are
irrelevant given that I want this merged with the pca9541 driver.

But maybe Guenter feels differently about that merge?

>  3 files changed, 370 insertions(+)
>  create mode 100644 drivers/i2c/muxes/i2c-mux-pca9641.c
> 
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 52a4a92..f9c51b8 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -73,6 +73,15 @@ config I2C_MUX_PCA954x
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-mux-pca954x.
>  
> +config I2C_MUX_PCA9641
> +	tristate "NXP PCA9641 I2C Master demultiplexer"
> +	help
> +	  If you say yes here you get support for the NXP PCA9641
> +	  I2C Master demultiplexer with an arbiter function.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-mux-pca9641.
> +
>  config I2C_MUX_PINCTRL
>  	tristate "pinctrl-based I2C multiplexer"
>  	depends on PINCTRL
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 6d9d865..a229a1a 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_I2C_MUX_LTC4306)	+= i2c-mux-ltc4306.o
>  obj-$(CONFIG_I2C_MUX_MLXCPLD)	+= i2c-mux-mlxcpld.o
>  obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
>  obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
> +obj-$(CONFIG_I2C_MUX_PCA9641)	+= i2c-mux-pca9641.o
>  obj-$(CONFIG_I2C_MUX_PINCTRL)	+= i2c-mux-pinctrl.o
>  obj-$(CONFIG_I2C_MUX_REG)	+= i2c-mux-reg.o
>  
> diff --git a/drivers/i2c/muxes/i2c-mux-pca9641.c b/drivers/i2c/muxes/i2c-mux-pca9641.c
> new file mode 100644
> index 0000000..bcf45c8
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-pca9641.c
> @@ -0,0 +1,360 @@
> +/*
> + * I2C demultiplexer driver for PCA9641 bus master demultiplexer
> + *
> + * Copyright (c) 2010 Ericsson AB.
> + *
> + * Author: Ken Chen <chen.kenyy@inventec.com>

A bit rich to claim to be the sole author, when you clearly "just"
modified the pca9541 driver. Please state the history!

> + *
> + * Derived from:

Maybe you intended to add something here, but forgot?

> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/jiffies.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c-mux.h>
> +
> +#include <linux/i2c/pca954x.h>

What is this last include for? We have moved away from specifying platform
data in (new) code.

> +
> +/*
> + * The PCA9641 is two I2C bus masters demultiplexer. It supports two I2C masters

"is two I2C bus masters" -> "is a two I2C bus master"

> + * connected to a single slave bus.
> + *
> + * It is designed for high reliability dual master I2C bus applications where
> + * correct system operation is required, even when two I2C master issue their
> + * commands at the same time. The arbiter will select a winner and let it work
> + * uninterrupted, and the losing master will take control of the I2C bus after
> + * the winnter has finished. The arbiter also allows for queued requests where

winner

> + * a master requests the downstream bus while the other master has control.
> + *
> + */
> +
> +#define PCA9641_ID		0x01
> +#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 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 */
> +
> +/* arbitration retry delays, in us */
> +#define SELECT_DELAY_SHORT	50
> +#define SELECT_DELAY_LONG	1000
> +
> +struct pca9641 {
> +	struct i2c_client *client;
> +	unsigned long select_timeout;
> +	unsigned long arb_timeout;
> +};
> +
> +static const struct i2c_device_id pca9641_id[] = {
> +	{"pca9641", 0},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, pca9641_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id pca9641_of_match[] = {
> +	{ .compatible = "nxp,pca9641" },

You need to describe the DT binding in Documentation/devicetree/bindings/i2c
See nxp,pca9541.txt for inspiration.

> +	{}
> +};
> +#endif
> +
> +/*
> + * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
> + * as they will try to lock the adapter a second time.
> + */
> +static int pca9641_reg_write(struct i2c_client *client, u8 command, u8 val)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	int ret;
> +
> +	if (adap->algo->master_xfer) {
> +		struct i2c_msg msg;
> +		char buf[2];
> +
> +		msg.addr = client->addr;
> +		msg.flags = 0;
> +		msg.len = 2;
> +		buf[0] = command;
> +		buf[1] = val;
> +		msg.buf = buf;
> +		ret = __i2c_transfer(adap, &msg, 1);
> +	} else {
> +		union i2c_smbus_data data;
> +
> +		data.byte = val;
> +		ret = adap->algo->smbus_xfer(adap, client->addr,
> +					     client->flags,
> +					     I2C_SMBUS_WRITE,
> +					     command,
> +					     I2C_SMBUS_BYTE_DATA, &data);
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Read from chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
> + * as they will try to lock adapter a second time.
> + */
> +static int pca9641_reg_read(struct i2c_client *client, u8 command)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	int ret;
> +	u8 val;
> +
> +	if (adap->algo->master_xfer) {
> +		struct i2c_msg msg[2] = {
> +			{
> +				.addr = client->addr,
> +				.flags = 0,
> +				.len = 1,
> +				.buf = &command
> +			},
> +			{
> +				.addr = client->addr,
> +				.flags = I2C_M_RD,
> +				.len = 1,
> +				.buf = &val
> +			}
> +		};
> +		ret = __i2c_transfer(adap, msg, 2);
> +		if (ret == 2)
> +			ret = val;
> +		else if (ret >= 0)
> +			ret = -EIO;
> +	} else {
> +		union i2c_smbus_data data;
> +
> +		ret = adap->algo->smbus_xfer(adap, client->addr,
> +					     client->flags,
> +					     I2C_SMBUS_READ,
> +					     command,
> +					     I2C_SMBUS_BYTE_DATA, &data);
> +		if (!ret)
> +			ret = data.byte;
> +	}
> +	return ret;
> +}
> +
> +/*
> + * Arbitration management functions
> + */
> +static void pca9641_release_bus(struct i2c_client *client)
> +{
> +	pca9641_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 pca9641 *data = i2c_mux_priv(muxc);
> +	int reg_ctl, reg_sts;
> +
> +	reg_ctl = pca9641_reg_read(client, PCA9641_CONTROL);
> +	if (reg_ctl < 0)
> +		return reg_ctl;
> +	reg_sts = pca9641_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;
> +		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
> +		reg_ctl = pca9641_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;
> +			pca9641_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;
> +		pca9641_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;
> +		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
> +	}
> +	return 0;
> +}
> +
> +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan)
> +{
> +	struct pca9641 *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 pca9641 *data = i2c_mux_priv(muxc);
> +	struct i2c_client *client = data->client;
> +
> +	pca9641_release_bus(client);
> +	return 0;
> +}
> +
> +/*
> + * I2C init/probing/exit functions
> + */
> +static int pca9641_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
> +	struct i2c_mux_core *muxc;
> +	struct pca9641 *data;
> +	int force;
> +	int ret;
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;

Reading the data sheet, I noticed that the chip supports I2C device id.
There's a brand new function available in -next [1] that allows you to
check this. See the pca954x driver in -next for an example [2]. It checks
the I2C device id for the newer pca984x chips.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/i2c-core-base.c
i2c_get_device_id(), currently circa line 2000

[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/muxes/i2c-mux-pca954x.c

> +	/*
> +	 * I2C accesses are unprotected here.
> +	 * We have to lock the adapter before releasing the bus.
> +	 */
> +	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, 8, sizeof(*data),

Why 8? Should be 1, no?

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

Lose one of the empty lines here.

> +	ret = i2c_mux_add_adapter(muxc, force, 0, 0);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to register master demultiplexer\n");

This dev_err should go. i2c_mux_add_adapter provides a more
detailed error message on failure.

Cheers,
Peter

> +		return ret;
> +	}
> +
> +	dev_info(&client->dev, "registered master demultiplexer for I2C %s\n",
> +		 client->name);
> +
> +	return 0;
> +}
> +
> +static int pca9641_remove(struct i2c_client *client)
> +{
> +	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +
> +	i2c_mux_del_adapters(muxc);
> +	return 0;
> +}
> +
> +static struct i2c_driver pca9641_driver = {
> +	.driver = {
> +		   .name = "pca9641",
> +		   .of_match_table = of_match_ptr(pca9641_of_match),
> +		   },
> +	.probe = pca9641_probe,
> +	.remove = pca9641_remove,
> +	.id_table = pca9641_id,
> +};
> +
> +module_i2c_driver(pca9641_driver);
> +
> +MODULE_AUTHOR("Ken Chen <chen.kenyy@inventec.com>");
> +MODULE_DESCRIPTION("PCA9641 I2C master demultiplexer driver");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver
  2018-03-19 12:45 ` Peter Rosin
@ 2018-03-19 13:44   ` Guenter Roeck
  2018-03-20  6:14       ` ChenKenYY 陳永營 TAO
  0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2018-03-19 13:44 UTC (permalink / raw)
  To: Peter Rosin, Ken Chen, wsa, linux-i2c, linux-kernel; +Cc: joel

On 03/19/2018 05:45 AM, Peter Rosin wrote:
> Hi Ken,
> 
> Thanks for the patch!
> 
> I would have liked this subject:
> i2c: muxes: pca9641: new driver
> 
> On 2018-03-19 12:38, Ken Chen wrote:
>> Initial PCA9641 2 chennel I2C bus master arbiter
> 
> channel
> 
>> with separate implementation. It has probe issue
>> and difference device hehavior between PCA9541
> 
> behavior
> 
>> and PCA9641 in original PCA9641 patch.
>>
>> Signed-off-by: Ken Chen <chen.kenyy@inventec.com>
>> ---
>>   drivers/i2c/muxes/Kconfig           |   9 +
>>   drivers/i2c/muxes/Makefile          |   1 +
>>   drivers/i2c/muxes/i2c-mux-pca9641.c | 360 ++++++++++++++++++++++++++++++++++++
> 
> Given the big similarities between this and the pca9541 driver,
> I would very much prefer if you could extend that driver instead
> of making an almost-copy like this.
> 
> I have added some comments below anyway, but most of them are
> irrelevant given that I want this merged with the pca9541 driver.
> 
> But maybe Guenter feels differently about that merge?
> 

Same here. I didn't do a line-by-line comparison, but the code looks very similar.
I did look into the probe function searching for differences after noticing
"It has probe issue ...", but the difference there must be quite subtle since
I didn't find it. The only real difference seems to be arbitration, but that can
easily be added to the original driver as chip specific functions.

>>   3 files changed, 370 insertions(+)
>>   create mode 100644 drivers/i2c/muxes/i2c-mux-pca9641.c
>>
>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>> index 52a4a92..f9c51b8 100644
>> --- a/drivers/i2c/muxes/Kconfig
>> +++ b/drivers/i2c/muxes/Kconfig
>> @@ -73,6 +73,15 @@ config I2C_MUX_PCA954x
>>   	  This driver can also be built as a module.  If so, the module
>>   	  will be called i2c-mux-pca954x.
>>   
>> +config I2C_MUX_PCA9641
>> +	tristate "NXP PCA9641 I2C Master demultiplexer"
>> +	help
>> +	  If you say yes here you get support for the NXP PCA9641
>> +	  I2C Master demultiplexer with an arbiter function.
>> +
>> +	  This driver can also be built as a module.  If so, the module
>> +	  will be called i2c-mux-pca9641.
>> +
>>   config I2C_MUX_PINCTRL
>>   	tristate "pinctrl-based I2C multiplexer"
>>   	depends on PINCTRL
>> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
>> index 6d9d865..a229a1a 100644
>> --- a/drivers/i2c/muxes/Makefile
>> +++ b/drivers/i2c/muxes/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_I2C_MUX_LTC4306)	+= i2c-mux-ltc4306.o
>>   obj-$(CONFIG_I2C_MUX_MLXCPLD)	+= i2c-mux-mlxcpld.o
>>   obj-$(CONFIG_I2C_MUX_PCA9541)	+= i2c-mux-pca9541.o
>>   obj-$(CONFIG_I2C_MUX_PCA954x)	+= i2c-mux-pca954x.o
>> +obj-$(CONFIG_I2C_MUX_PCA9641)	+= i2c-mux-pca9641.o
>>   obj-$(CONFIG_I2C_MUX_PINCTRL)	+= i2c-mux-pinctrl.o
>>   obj-$(CONFIG_I2C_MUX_REG)	+= i2c-mux-reg.o
>>   
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca9641.c b/drivers/i2c/muxes/i2c-mux-pca9641.c
>> new file mode 100644
>> index 0000000..bcf45c8
>> --- /dev/null
>> +++ b/drivers/i2c/muxes/i2c-mux-pca9641.c
>> @@ -0,0 +1,360 @@
>> +/*
>> + * I2C demultiplexer driver for PCA9641 bus master demultiplexer
>> + *
>> + * Copyright (c) 2010 Ericsson AB.
>> + *
>> + * Author: Ken Chen <chen.kenyy@inventec.com>
> 
> A bit rich to claim to be the sole author, when you clearly "just"
> modified the pca9541 driver. Please state the history!
> 

And while retaining the original copyright.

>> + *
>> + * Derived from:
> 
> Maybe you intended to add something here, but forgot?
> 
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */

New files should use the SPDX license identifier.

>> +
>> +#include <linux/module.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/delay.h>
>> +#include <linux/slab.h>
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/i2c-mux.h>
>> +
>> +#include <linux/i2c/pca954x.h>
> 
> What is this last include for? We have moved away from specifying platform
> data in (new) code.
> 
>> +
>> +/*
>> + * The PCA9641 is two I2C bus masters demultiplexer. It supports two I2C masters
> 
> "is two I2C bus masters" -> "is a two I2C bus master"
> 
>> + * connected to a single slave bus.
>> + *
>> + * It is designed for high reliability dual master I2C bus applications where
>> + * correct system operation is required, even when two I2C master issue their
>> + * commands at the same time. The arbiter will select a winner and let it work
>> + * uninterrupted, and the losing master will take control of the I2C bus after
>> + * the winnter has finished. The arbiter also allows for queued requests where
> 
> winner
> 
>> + * a master requests the downstream bus while the other master has control.
>> + *
>> + */
>> +
>> +#define PCA9641_ID		0x01
>> +#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 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 */
>> +
>> +/* arbitration retry delays, in us */
>> +#define SELECT_DELAY_SHORT	50
>> +#define SELECT_DELAY_LONG	1000
>> +
>> +struct pca9641 {
>> +	struct i2c_client *client;
>> +	unsigned long select_timeout;
>> +	unsigned long arb_timeout;
>> +};
>> +
>> +static const struct i2c_device_id pca9641_id[] = {
>> +	{"pca9641", 0},
>> +	{}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, pca9641_id);
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id pca9641_of_match[] = {
>> +	{ .compatible = "nxp,pca9641" },
> 
> You need to describe the DT binding in Documentation/devicetree/bindings/i2c
> See nxp,pca9541.txt for inspiration.
> 
>> +	{}
>> +};
>> +#endif
>> +
>> +/*
>> + * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
>> + * as they will try to lock the adapter a second time.
>> + */
>> +static int pca9641_reg_write(struct i2c_client *client, u8 command, u8 val)
>> +{
>> +	struct i2c_adapter *adap = client->adapter;
>> +	int ret;
>> +
>> +	if (adap->algo->master_xfer) {
>> +		struct i2c_msg msg;
>> +		char buf[2];
>> +
>> +		msg.addr = client->addr;
>> +		msg.flags = 0;
>> +		msg.len = 2;
>> +		buf[0] = command;
>> +		buf[1] = val;
>> +		msg.buf = buf;
>> +		ret = __i2c_transfer(adap, &msg, 1);
>> +	} else {
>> +		union i2c_smbus_data data;
>> +
>> +		data.byte = val;
>> +		ret = adap->algo->smbus_xfer(adap, client->addr,
>> +					     client->flags,
>> +					     I2C_SMBUS_WRITE,
>> +					     command,
>> +					     I2C_SMBUS_BYTE_DATA, &data);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Read from chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
>> + * as they will try to lock adapter a second time.
>> + */
>> +static int pca9641_reg_read(struct i2c_client *client, u8 command)
>> +{
>> +	struct i2c_adapter *adap = client->adapter;
>> +	int ret;
>> +	u8 val;
>> +
>> +	if (adap->algo->master_xfer) {
>> +		struct i2c_msg msg[2] = {
>> +			{
>> +				.addr = client->addr,
>> +				.flags = 0,
>> +				.len = 1,
>> +				.buf = &command
>> +			},
>> +			{
>> +				.addr = client->addr,
>> +				.flags = I2C_M_RD,
>> +				.len = 1,
>> +				.buf = &val
>> +			}
>> +		};
>> +		ret = __i2c_transfer(adap, msg, 2);
>> +		if (ret == 2)
>> +			ret = val;
>> +		else if (ret >= 0)
>> +			ret = -EIO;
>> +	} else {
>> +		union i2c_smbus_data data;
>> +
>> +		ret = adap->algo->smbus_xfer(adap, client->addr,
>> +					     client->flags,
>> +					     I2C_SMBUS_READ,
>> +					     command,
>> +					     I2C_SMBUS_BYTE_DATA, &data);
>> +		if (!ret)
>> +			ret = data.byte;
>> +	}
>> +	return ret;
>> +}
>> +
>> +/*
>> + * Arbitration management functions
>> + */
>> +static void pca9641_release_bus(struct i2c_client *client)
>> +{
>> +	pca9641_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 pca9641 *data = i2c_mux_priv(muxc);
>> +	int reg_ctl, reg_sts;
>> +
>> +	reg_ctl = pca9641_reg_read(client, PCA9641_CONTROL);
>> +	if (reg_ctl < 0)
>> +		return reg_ctl;
>> +	reg_sts = pca9641_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;
>> +		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
>> +		reg_ctl = pca9641_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;
>> +			pca9641_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;
>> +		pca9641_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;
>> +		pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan)
>> +{
>> +	struct pca9641 *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 pca9641 *data = i2c_mux_priv(muxc);
>> +	struct i2c_client *client = data->client;
>> +
>> +	pca9641_release_bus(client);
>> +	return 0;
>> +}
>> +
>> +/*
>> + * I2C init/probing/exit functions
>> + */
>> +static int pca9641_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	struct i2c_adapter *adap = client->adapter;
>> +	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
>> +	struct i2c_mux_core *muxc;
>> +	struct pca9641 *data;
>> +	int force;
>> +	int ret;
>> +
>> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
>> +		return -ENODEV;
> 
> Reading the data sheet, I noticed that the chip supports I2C device id.
> There's a brand new function available in -next [1] that allows you to
> check this. See the pca954x driver in -next for an example [2]. It checks
> the I2C device id for the newer pca984x chips.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/i2c-core-base.c
> i2c_get_device_id(), currently circa line 2000
> 
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/muxes/i2c-mux-pca954x.c
> 
>> +	/*
>> +	 * I2C accesses are unprotected here.
>> +	 * We have to lock the adapter before releasing the bus.
>> +	 */
>> +	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, 8, sizeof(*data),
> 
> Why 8? Should be 1, no?
> 
>> +			     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);
>> +
>> +
> 
> Lose one of the empty lines here.
> 
>> +	ret = i2c_mux_add_adapter(muxc, force, 0, 0);
>> +	if (ret) {
>> +		dev_err(&client->dev, "failed to register master demultiplexer\n");
> 
> This dev_err should go. i2c_mux_add_adapter provides a more
> detailed error message on failure.
> 
> Cheers,
> Peter
> 
>> +		return ret;
>> +	}
>> +
>> +	dev_info(&client->dev, "registered master demultiplexer for I2C %s\n",
>> +		 client->name);
>> +
>> +	return 0;
>> +}
>> +
>> +static int pca9641_remove(struct i2c_client *client)
>> +{
>> +	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>> +
>> +	i2c_mux_del_adapters(muxc);
>> +	return 0;
>> +}
>> +
>> +static struct i2c_driver pca9641_driver = {
>> +	.driver = {
>> +		   .name = "pca9641",
>> +		   .of_match_table = of_match_ptr(pca9641_of_match),
>> +		   },
>> +	.probe = pca9641_probe,
>> +	.remove = pca9641_remove,
>> +	.id_table = pca9641_id,
>> +};
>> +
>> +module_i2c_driver(pca9641_driver);
>> +
>> +MODULE_AUTHOR("Ken Chen <chen.kenyy@inventec.com>");
>> +MODULE_DESCRIPTION("PCA9641 I2C master demultiplexer driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver
  2018-03-19 13:44   ` Guenter Roeck
@ 2018-03-20  6:14       ` ChenKenYY 陳永營 TAO
  0 siblings, 0 replies; 8+ messages in thread
From: ChenKenYY 陳永營 TAO @ 2018-03-20  6:14 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Peter Rosin, wsa, linux-i2c, linux-kernel, Joel Stanley

Hi all,
I had merged into i2c-mux-pca9541.c
It will be send out later.

2018-03-19 21:44 GMT+08:00 Guenter Roeck <linux@roeck-us.net>:
> On 03/19/2018 05:45 AM, Peter Rosin wrote:
>>
>> Hi Ken,
>>
>> Thanks for the patch!
>>
>> I would have liked this subject:
>> i2c: muxes: pca9641: new driver
>>
>> On 2018-03-19 12:38, Ken Chen wrote:
>>>
>>> Initial PCA9641 2 chennel I2C bus master arbiter
>>
>>
>> channel
>>
>>> with separate implementation. It has probe issue
>>> and difference device hehavior between PCA9541
>>
>>
>> behavior
>>
>>> and PCA9641 in original PCA9641 patch.
>>>
>>> Signed-off-by: Ken Chen <chen.kenyy@inventec.com>
>>> ---
>>>   drivers/i2c/muxes/Kconfig           |   9 +
>>>   drivers/i2c/muxes/Makefile          |   1 +
>>>   drivers/i2c/muxes/i2c-mux-pca9641.c | 360
>>> ++++++++++++++++++++++++++++++++++++
>>
>>
>> Given the big similarities between this and the pca9541 driver,
>> I would very much prefer if you could extend that driver instead
>> of making an almost-copy like this.
>>
>> I have added some comments below anyway, but most of them are
>> irrelevant given that I want this merged with the pca9541 driver.
>>
>> But maybe Guenter feels differently about that merge?
>>
>
> Same here. I didn't do a line-by-line comparison, but the code looks very
> similar.
> I did look into the probe function searching for differences after noticing
> "It has probe issue ...", but the difference there must be quite subtle
> since
> I didn't find it. The only real difference seems to be arbitration, but that
> can
> easily be added to the original driver as chip specific functions.
>
>
>>>   3 files changed, 370 insertions(+)
>>>   create mode 100644 drivers/i2c/muxes/i2c-mux-pca9641.c
>>>
>>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>>> index 52a4a92..f9c51b8 100644
>>> --- a/drivers/i2c/muxes/Kconfig
>>> +++ b/drivers/i2c/muxes/Kconfig
>>> @@ -73,6 +73,15 @@ config I2C_MUX_PCA954x
>>>           This driver can also be built as a module.  If so, the module
>>>           will be called i2c-mux-pca954x.
>>>   +config I2C_MUX_PCA9641
>>> +       tristate "NXP PCA9641 I2C Master demultiplexer"
>>> +       help
>>> +         If you say yes here you get support for the NXP PCA9641
>>> +         I2C Master demultiplexer with an arbiter function.
>>> +
>>> +         This driver can also be built as a module.  If so, the module
>>> +         will be called i2c-mux-pca9641.
>>> +
>>>   config I2C_MUX_PINCTRL
>>>         tristate "pinctrl-based I2C multiplexer"
>>>         depends on PINCTRL
>>> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
>>> index 6d9d865..a229a1a 100644
>>> --- a/drivers/i2c/muxes/Makefile
>>> +++ b/drivers/i2c/muxes/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_I2C_MUX_LTC4306) += i2c-mux-ltc4306.o
>>>   obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o
>>>   obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
>>>   obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
>>> +obj-$(CONFIG_I2C_MUX_PCA9641)  += i2c-mux-pca9641.o
>>>   obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o
>>>   obj-$(CONFIG_I2C_MUX_REG)     += i2c-mux-reg.o
>>>   diff --git a/drivers/i2c/muxes/i2c-mux-pca9641.c
>>> b/drivers/i2c/muxes/i2c-mux-pca9641.c
>>> new file mode 100644
>>> index 0000000..bcf45c8
>>> --- /dev/null
>>> +++ b/drivers/i2c/muxes/i2c-mux-pca9641.c
>>> @@ -0,0 +1,360 @@
>>> +/*
>>> + * I2C demultiplexer driver for PCA9641 bus master demultiplexer
>>> + *
>>> + * Copyright (c) 2010 Ericsson AB.
>>> + *
>>> + * Author: Ken Chen <chen.kenyy@inventec.com>
>>
>>
>> A bit rich to claim to be the sole author, when you clearly "just"
>> modified the pca9541 driver. Please state the history!
>>
>
> And while retaining the original copyright.
>
>>> + *
>>> + * Derived from:
>>
>>
>> Maybe you intended to add something here, but forgot?
>>
>>> + *
>>> + * This file is licensed under the terms of the GNU General Public
>>> + * License version 2. This program is licensed "as is" without any
>>> + * warranty of any kind, whether express or implied.
>>> + */
>
>
> New files should use the SPDX license identifier.
>
How can I use the SPDX license?
The previous license is GNU at i2c-mux-pca9541.c
>
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/jiffies.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/device.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/i2c-mux.h>
>>> +
>>> +#include <linux/i2c/pca954x.h>
>>
>>
>> What is this last include for? We have moved away from specifying platform
>> data in (new) code.
Fixed
>>
>>> +
>>> +/*
>>> + * The PCA9641 is two I2C bus masters demultiplexer. It supports two I2C
>>> masters
>>
>>
>> "is two I2C bus masters" -> "is a two I2C bus master"
>>
>>> + * connected to a single slave bus.
>>> + *
>>> + * It is designed for high reliability dual master I2C bus applications
>>> where
>>> + * correct system operation is required, even when two I2C master issue
>>> their
>>> + * commands at the same time. The arbiter will select a winner and let
>>> it work
>>> + * uninterrupted, and the losing master will take control of the I2C bus
>>> after
>>> + * the winnter has finished. The arbiter also allows for queued requests
>>> where
>>
>>
>> winner
>>
>>> + * a master requests the downstream bus while the other master has
>>> control.
>>> + *
>>> + */
>>> +
>>> +#define PCA9641_ID             0x01
>>> +#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 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 */
>>> +
>>> +/* arbitration retry delays, in us */
>>> +#define SELECT_DELAY_SHORT     50
>>> +#define SELECT_DELAY_LONG      1000
>>> +
>>> +struct pca9641 {
>>> +       struct i2c_client *client;
>>> +       unsigned long select_timeout;
>>> +       unsigned long arb_timeout;
>>> +};
>>> +
>>> +static const struct i2c_device_id pca9641_id[] = {
>>> +       {"pca9641", 0},
>>> +       {}
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(i2c, pca9641_id);
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id pca9641_of_match[] = {
>>> +       { .compatible = "nxp,pca9641" },
>>
>>
>> You need to describe the DT binding in
>> Documentation/devicetree/bindings/i2c
>> See nxp,pca9541.txt for inspiration.
>>
>>> +       {}
>>> +};
>>> +#endif
>>> +
>>> +/*
>>> + * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
>>> + * as they will try to lock the adapter a second time.
>>> + */
>>> +static int pca9641_reg_write(struct i2c_client *client, u8 command, u8
>>> val)
>>> +{
>>> +       struct i2c_adapter *adap = client->adapter;
>>> +       int ret;
>>> +
>>> +       if (adap->algo->master_xfer) {
>>> +               struct i2c_msg msg;
>>> +               char buf[2];
>>> +
>>> +               msg.addr = client->addr;
>>> +               msg.flags = 0;
>>> +               msg.len = 2;
>>> +               buf[0] = command;
>>> +               buf[1] = val;
>>> +               msg.buf = buf;
>>> +               ret = __i2c_transfer(adap, &msg, 1);
>>> +       } else {
>>> +               union i2c_smbus_data data;
>>> +
>>> +               data.byte = val;
>>> +               ret = adap->algo->smbus_xfer(adap, client->addr,
>>> +                                            client->flags,
>>> +                                            I2C_SMBUS_WRITE,
>>> +                                            command,
>>> +                                            I2C_SMBUS_BYTE_DATA, &data);
>>> +       }
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +/*
>>> + * Read from chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
>>> + * as they will try to lock adapter a second time.
>>> + */
>>> +static int pca9641_reg_read(struct i2c_client *client, u8 command)
>>> +{
>>> +       struct i2c_adapter *adap = client->adapter;
>>> +       int ret;
>>> +       u8 val;
>>> +
>>> +       if (adap->algo->master_xfer) {
>>> +               struct i2c_msg msg[2] = {
>>> +                       {
>>> +                               .addr = client->addr,
>>> +                               .flags = 0,
>>> +                               .len = 1,
>>> +                               .buf = &command
>>> +                       },
>>> +                       {
>>> +                               .addr = client->addr,
>>> +                               .flags = I2C_M_RD,
>>> +                               .len = 1,
>>> +                               .buf = &val
>>> +                       }
>>> +               };
>>> +               ret = __i2c_transfer(adap, msg, 2);
>>> +               if (ret == 2)
>>> +                       ret = val;
>>> +               else if (ret >= 0)
>>> +                       ret = -EIO;
>>> +       } else {
>>> +               union i2c_smbus_data data;
>>> +
>>> +               ret = adap->algo->smbus_xfer(adap, client->addr,
>>> +                                            client->flags,
>>> +                                            I2C_SMBUS_READ,
>>> +                                            command,
>>> +                                            I2C_SMBUS_BYTE_DATA, &data);
>>> +               if (!ret)
>>> +                       ret = data.byte;
>>> +       }
>>> +       return ret;
>>> +}
>>> +
>>> +/*
>>> + * Arbitration management functions
>>> + */
>>> +static void pca9641_release_bus(struct i2c_client *client)
>>> +{
>>> +       pca9641_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 pca9641 *data = i2c_mux_priv(muxc);
>>> +       int reg_ctl, reg_sts;
>>> +
>>> +       reg_ctl = pca9641_reg_read(client, PCA9641_CONTROL);
>>> +       if (reg_ctl < 0)
>>> +               return reg_ctl;
>>> +       reg_sts = pca9641_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;
>>> +               pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
>>> +               reg_ctl = pca9641_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;
>>> +                       pca9641_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;
>>> +               pca9641_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;
>>> +               pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
>>> +       }
>>> +       return 0;
>>> +}
>>> +
>>> +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan)
>>> +{
>>> +       struct pca9641 *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 pca9641 *data = i2c_mux_priv(muxc);
>>> +       struct i2c_client *client = data->client;
>>> +
>>> +       pca9641_release_bus(client);
>>> +       return 0;
>>> +}
>>> +
>>> +/*
>>> + * I2C init/probing/exit functions
>>> + */
>>> +static int pca9641_probe(struct i2c_client *client,
>>> +                        const struct i2c_device_id *id)
>>> +{
>>> +       struct i2c_adapter *adap = client->adapter;
>>> +       struct pca954x_platform_data *pdata =
>>> dev_get_platdata(&client->dev);
>>> +       struct i2c_mux_core *muxc;
>>> +       struct pca9641 *data;
>>> +       int force;
>>> +       int ret;
>>> +
>>> +       if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
>>> +               return -ENODEV;
>>
>>
>> Reading the data sheet, I noticed that the chip supports I2C device id.
>> There's a brand new function available in -next [1] that allows you to
>> check this. See the pca954x driver in -next for an example [2]. It checks
>> the I2C device id for the newer pca984x chips.
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/i2c-core-base.c
>> i2c_get_device_id(), currently circa line 2000
>>
>> [2]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/muxes/i2c-mux-pca954x.c
>>
Please help to review new patch the detect ID method.
>>> +       /*
>>> +        * I2C accesses are unprotected here.
>>> +        * We have to lock the adapter before releasing the bus.
>>> +        */
>>> +       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, 8, sizeof(*data),
>>
>>
>> Why 8? Should be 1, no?
Yes. should be 1.
>>
>>> +                            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);
>>> +
>>> +
>>
>>
>> Lose one of the empty lines here.
Fix
>>
>>> +       ret = i2c_mux_add_adapter(muxc, force, 0, 0);
>>> +       if (ret) {
>>> +               dev_err(&client->dev, "failed to register master
>>> demultiplexer\n");
>>
>>
>> This dev_err should go. i2c_mux_add_adapter provides a more
>> detailed error message on failure.
Removed and follow pca9541 probe code.
>>
>> Cheers,
>> Peter
>>
>>> +               return ret;
>>> +       }
>>> +
>>> +       dev_info(&client->dev, "registered master demultiplexer for I2C
>>> %s\n",
>>> +                client->name);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int pca9641_remove(struct i2c_client *client)
>>> +{
>>> +       struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>>> +
>>> +       i2c_mux_del_adapters(muxc);
>>> +       return 0;
>>> +}
>>> +
>>> +static struct i2c_driver pca9641_driver = {
>>> +       .driver = {
>>> +                  .name = "pca9641",
>>> +                  .of_match_table = of_match_ptr(pca9641_of_match),
>>> +                  },
>>> +       .probe = pca9641_probe,
>>> +       .remove = pca9641_remove,
>>> +       .id_table = pca9641_id,
>>> +};
>>> +
>>> +module_i2c_driver(pca9641_driver);
>>> +
>>> +MODULE_AUTHOR("Ken Chen <chen.kenyy@inventec.com>");
>>> +MODULE_DESCRIPTION("PCA9641 I2C master demultiplexer driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver
@ 2018-03-20  6:14       ` ChenKenYY 陳永營 TAO
  0 siblings, 0 replies; 8+ messages in thread
From: ChenKenYY 陳永營 TAO @ 2018-03-20  6:14 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Peter Rosin, wsa, linux-i2c, linux-kernel, Joel Stanley

Hi all,
I had merged into i2c-mux-pca9541.c
It will be send out later.

2018-03-19 21:44 GMT+08:00 Guenter Roeck <linux@roeck-us.net>:
> On 03/19/2018 05:45 AM, Peter Rosin wrote:
>>
>> Hi Ken,
>>
>> Thanks for the patch!
>>
>> I would have liked this subject:
>> i2c: muxes: pca9641: new driver
>>
>> On 2018-03-19 12:38, Ken Chen wrote:
>>>
>>> Initial PCA9641 2 chennel I2C bus master arbiter
>>
>>
>> channel
>>
>>> with separate implementation. It has probe issue
>>> and difference device hehavior between PCA9541
>>
>>
>> behavior
>>
>>> and PCA9641 in original PCA9641 patch.
>>>
>>> Signed-off-by: Ken Chen <chen.kenyy@inventec.com>
>>> ---
>>>   drivers/i2c/muxes/Kconfig           |   9 +
>>>   drivers/i2c/muxes/Makefile          |   1 +
>>>   drivers/i2c/muxes/i2c-mux-pca9641.c | 360
>>> ++++++++++++++++++++++++++++++++++++
>>
>>
>> Given the big similarities between this and the pca9541 driver,
>> I would very much prefer if you could extend that driver instead
>> of making an almost-copy like this.
>>
>> I have added some comments below anyway, but most of them are
>> irrelevant given that I want this merged with the pca9541 driver.
>>
>> But maybe Guenter feels differently about that merge?
>>
>
> Same here. I didn't do a line-by-line comparison, but the code looks very
> similar.
> I did look into the probe function searching for differences after noticing
> "It has probe issue ...", but the difference there must be quite subtle
> since
> I didn't find it. The only real difference seems to be arbitration, but that
> can
> easily be added to the original driver as chip specific functions.
>
>
>>>   3 files changed, 370 insertions(+)
>>>   create mode 100644 drivers/i2c/muxes/i2c-mux-pca9641.c
>>>
>>> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
>>> index 52a4a92..f9c51b8 100644
>>> --- a/drivers/i2c/muxes/Kconfig
>>> +++ b/drivers/i2c/muxes/Kconfig
>>> @@ -73,6 +73,15 @@ config I2C_MUX_PCA954x
>>>           This driver can also be built as a module.  If so, the module
>>>           will be called i2c-mux-pca954x.
>>>   +config I2C_MUX_PCA9641
>>> +       tristate "NXP PCA9641 I2C Master demultiplexer"
>>> +       help
>>> +         If you say yes here you get support for the NXP PCA9641
>>> +         I2C Master demultiplexer with an arbiter function.
>>> +
>>> +         This driver can also be built as a module.  If so, the module
>>> +         will be called i2c-mux-pca9641.
>>> +
>>>   config I2C_MUX_PINCTRL
>>>         tristate "pinctrl-based I2C multiplexer"
>>>         depends on PINCTRL
>>> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
>>> index 6d9d865..a229a1a 100644
>>> --- a/drivers/i2c/muxes/Makefile
>>> +++ b/drivers/i2c/muxes/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_I2C_MUX_LTC4306) += i2c-mux-ltc4306.o
>>>   obj-$(CONFIG_I2C_MUX_MLXCPLD) += i2c-mux-mlxcpld.o
>>>   obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
>>>   obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
>>> +obj-$(CONFIG_I2C_MUX_PCA9641)  += i2c-mux-pca9641.o
>>>   obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o
>>>   obj-$(CONFIG_I2C_MUX_REG)     += i2c-mux-reg.o
>>>   diff --git a/drivers/i2c/muxes/i2c-mux-pca9641.c
>>> b/drivers/i2c/muxes/i2c-mux-pca9641.c
>>> new file mode 100644
>>> index 0000000..bcf45c8
>>> --- /dev/null
>>> +++ b/drivers/i2c/muxes/i2c-mux-pca9641.c
>>> @@ -0,0 +1,360 @@
>>> +/*
>>> + * I2C demultiplexer driver for PCA9641 bus master demultiplexer
>>> + *
>>> + * Copyright (c) 2010 Ericsson AB.
>>> + *
>>> + * Author: Ken Chen <chen.kenyy@inventec.com>
>>
>>
>> A bit rich to claim to be the sole author, when you clearly "just"
>> modified the pca9541 driver. Please state the history!
>>
>
> And while retaining the original copyright.
>
>>> + *
>>> + * Derived from:
>>
>>
>> Maybe you intended to add something here, but forgot?
>>
>>> + *
>>> + * This file is licensed under the terms of the GNU General Public
>>> + * License version 2. This program is licensed "as is" without any
>>> + * warranty of any kind, whether express or implied.
>>> + */
>
>
> New files should use the SPDX license identifier.
>
How can I use the SPDX license?
The previous license is GNU at i2c-mux-pca9541.c
>
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/jiffies.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/device.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/i2c-mux.h>
>>> +
>>> +#include <linux/i2c/pca954x.h>
>>
>>
>> What is this last include for? We have moved away from specifying platform
>> data in (new) code.
Fixed
>>
>>> +
>>> +/*
>>> + * The PCA9641 is two I2C bus masters demultiplexer. It supports two I2C
>>> masters
>>
>>
>> "is two I2C bus masters" -> "is a two I2C bus master"
>>
>>> + * connected to a single slave bus.
>>> + *
>>> + * It is designed for high reliability dual master I2C bus applications
>>> where
>>> + * correct system operation is required, even when two I2C master issue
>>> their
>>> + * commands at the same time. The arbiter will select a winner and let
>>> it work
>>> + * uninterrupted, and the losing master will take control of the I2C bus
>>> after
>>> + * the winnter has finished. The arbiter also allows for queued requests
>>> where
>>
>>
>> winner
>>
>>> + * a master requests the downstream bus while the other master has
>>> control.
>>> + *
>>> + */
>>> +
>>> +#define PCA9641_ID             0x01
>>> +#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 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 */
>>> +
>>> +/* arbitration retry delays, in us */
>>> +#define SELECT_DELAY_SHORT     50
>>> +#define SELECT_DELAY_LONG      1000
>>> +
>>> +struct pca9641 {
>>> +       struct i2c_client *client;
>>> +       unsigned long select_timeout;
>>> +       unsigned long arb_timeout;
>>> +};
>>> +
>>> +static const struct i2c_device_id pca9641_id[] = {
>>> +       {"pca9641", 0},
>>> +       {}
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(i2c, pca9641_id);
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id pca9641_of_match[] = {
>>> +       { .compatible = "nxp,pca9641" },
>>
>>
>> You need to describe the DT binding in
>> Documentation/devicetree/bindings/i2c
>> See nxp,pca9541.txt for inspiration.
>>
>>> +       {}
>>> +};
>>> +#endif
>>> +
>>> +/*
>>> + * Write to chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
>>> + * as they will try to lock the adapter a second time.
>>> + */
>>> +static int pca9641_reg_write(struct i2c_client *client, u8 command, u8
>>> val)
>>> +{
>>> +       struct i2c_adapter *adap = client->adapter;
>>> +       int ret;
>>> +
>>> +       if (adap->algo->master_xfer) {
>>> +               struct i2c_msg msg;
>>> +               char buf[2];
>>> +
>>> +               msg.addr = client->addr;
>>> +               msg.flags = 0;
>>> +               msg.len = 2;
>>> +               buf[0] = command;
>>> +               buf[1] = val;
>>> +               msg.buf = buf;
>>> +               ret = __i2c_transfer(adap, &msg, 1);
>>> +       } else {
>>> +               union i2c_smbus_data data;
>>> +
>>> +               data.byte = val;
>>> +               ret = adap->algo->smbus_xfer(adap, client->addr,
>>> +                                            client->flags,
>>> +                                            I2C_SMBUS_WRITE,
>>> +                                            command,
>>> +                                            I2C_SMBUS_BYTE_DATA, &data);
>>> +       }
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +/*
>>> + * Read from chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
>>> + * as they will try to lock adapter a second time.
>>> + */
>>> +static int pca9641_reg_read(struct i2c_client *client, u8 command)
>>> +{
>>> +       struct i2c_adapter *adap = client->adapter;
>>> +       int ret;
>>> +       u8 val;
>>> +
>>> +       if (adap->algo->master_xfer) {
>>> +               struct i2c_msg msg[2] = {
>>> +                       {
>>> +                               .addr = client->addr,
>>> +                               .flags = 0,
>>> +                               .len = 1,
>>> +                               .buf = &command
>>> +                       },
>>> +                       {
>>> +                               .addr = client->addr,
>>> +                               .flags = I2C_M_RD,
>>> +                               .len = 1,
>>> +                               .buf = &val
>>> +                       }
>>> +               };
>>> +               ret = __i2c_transfer(adap, msg, 2);
>>> +               if (ret == 2)
>>> +                       ret = val;
>>> +               else if (ret >= 0)
>>> +                       ret = -EIO;
>>> +       } else {
>>> +               union i2c_smbus_data data;
>>> +
>>> +               ret = adap->algo->smbus_xfer(adap, client->addr,
>>> +                                            client->flags,
>>> +                                            I2C_SMBUS_READ,
>>> +                                            command,
>>> +                                            I2C_SMBUS_BYTE_DATA, &data);
>>> +               if (!ret)
>>> +                       ret = data.byte;
>>> +       }
>>> +       return ret;
>>> +}
>>> +
>>> +/*
>>> + * Arbitration management functions
>>> + */
>>> +static void pca9641_release_bus(struct i2c_client *client)
>>> +{
>>> +       pca9641_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 pca9641 *data = i2c_mux_priv(muxc);
>>> +       int reg_ctl, reg_sts;
>>> +
>>> +       reg_ctl = pca9641_reg_read(client, PCA9641_CONTROL);
>>> +       if (reg_ctl < 0)
>>> +               return reg_ctl;
>>> +       reg_sts = pca9641_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;
>>> +               pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
>>> +               reg_ctl = pca9641_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;
>>> +                       pca9641_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;
>>> +               pca9641_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;
>>> +               pca9641_reg_write(client, PCA9641_CONTROL, reg_ctl);
>>> +       }
>>> +       return 0;
>>> +}
>>> +
>>> +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan)
>>> +{
>>> +       struct pca9641 *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 pca9641 *data = i2c_mux_priv(muxc);
>>> +       struct i2c_client *client = data->client;
>>> +
>>> +       pca9641_release_bus(client);
>>> +       return 0;
>>> +}
>>> +
>>> +/*
>>> + * I2C init/probing/exit functions
>>> + */
>>> +static int pca9641_probe(struct i2c_client *client,
>>> +                        const struct i2c_device_id *id)
>>> +{
>>> +       struct i2c_adapter *adap = client->adapter;
>>> +       struct pca954x_platform_data *pdata =
>>> dev_get_platdata(&client->dev);
>>> +       struct i2c_mux_core *muxc;
>>> +       struct pca9641 *data;
>>> +       int force;
>>> +       int ret;
>>> +
>>> +       if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_BYTE_DATA))
>>> +               return -ENODEV;
>>
>>
>> Reading the data sheet, I noticed that the chip supports I2C device id.
>> There's a brand new function available in -next [1] that allows you to
>> check this. See the pca954x driver in -next for an example [2]. It checks
>> the I2C device id for the newer pca984x chips.
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/i2c-core-base.c
>> i2c_get_device_id(), currently circa line 2000
>>
>> [2]
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/i2c/muxes/i2c-mux-pca954x.c
>>
Please help to review new patch the detect ID method.
>>> +       /*
>>> +        * I2C accesses are unprotected here.
>>> +        * We have to lock the adapter before releasing the bus.
>>> +        */
>>> +       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, 8, sizeof(*data),
>>
>>
>> Why 8? Should be 1, no?
Yes. should be 1.
>>
>>> +                            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);
>>> +
>>> +
>>
>>
>> Lose one of the empty lines here.
Fix
>>
>>> +       ret = i2c_mux_add_adapter(muxc, force, 0, 0);
>>> +       if (ret) {
>>> +               dev_err(&client->dev, "failed to register master
>>> demultiplexer\n");
>>
>>
>> This dev_err should go. i2c_mux_add_adapter provides a more
>> detailed error message on failure.
Removed and follow pca9541 probe code.
>>
>> Cheers,
>> Peter
>>
>>> +               return ret;
>>> +       }
>>> +
>>> +       dev_info(&client->dev, "registered master demultiplexer for I2C
>>> %s\n",
>>> +                client->name);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int pca9641_remove(struct i2c_client *client)
>>> +{
>>> +       struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>>> +
>>> +       i2c_mux_del_adapters(muxc);
>>> +       return 0;
>>> +}
>>> +
>>> +static struct i2c_driver pca9641_driver = {
>>> +       .driver = {
>>> +                  .name = "pca9641",
>>> +                  .of_match_table = of_match_ptr(pca9641_of_match),
>>> +                  },
>>> +       .probe = pca9641_probe,
>>> +       .remove = pca9641_remove,
>>> +       .id_table = pca9641_id,
>>> +};
>>> +
>>> +module_i2c_driver(pca9641_driver);
>>> +
>>> +MODULE_AUTHOR("Ken Chen <chen.kenyy@inventec.com>");
>>> +MODULE_DESCRIPTION("PCA9641 I2C master demultiplexer driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver
  2018-03-19 11:38 ` Ken Chen
@ 2018-03-20  7:00   ` kbuild test robot
  -1 siblings, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2018-03-20  7:00 UTC (permalink / raw)
  To: Ken Chen
  Cc: kbuild-all, linux, wsa, peda, linux-i2c, linux-kernel, Ken Chen, joel

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

Hi Ken,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180319]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ken-Chen/drivers-i2c-master-arbiter-create-driver/20180320-134925
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> drivers/i2c/muxes/i2c-mux-pca9641.c:23:10: fatal error: linux/i2c/pca954x.h: No such file or directory
    #include <linux/i2c/pca954x.h>
             ^~~~~~~~~~~~~~~~~~~~~
   compilation terminated.

vim +23 drivers/i2c/muxes/i2c-mux-pca9641.c

    22	
  > 23	#include <linux/i2c/pca954x.h>
    24	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 63088 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver
@ 2018-03-20  7:00   ` kbuild test robot
  0 siblings, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2018-03-20  7:00 UTC (permalink / raw)
  Cc: kbuild-all, linux, wsa, peda, linux-i2c, linux-kernel, Ken Chen, joel

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

Hi Ken,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180319]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ken-Chen/drivers-i2c-master-arbiter-create-driver/20180320-134925
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> drivers/i2c/muxes/i2c-mux-pca9641.c:23:10: fatal error: linux/i2c/pca954x.h: No such file or directory
    #include <linux/i2c/pca954x.h>
             ^~~~~~~~~~~~~~~~~~~~~
   compilation terminated.

vim +23 drivers/i2c/muxes/i2c-mux-pca9641.c

    22	
  > 23	#include <linux/i2c/pca954x.h>
    24	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 63088 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-03-20  7:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-19 11:38 [PATCH linux dev-4.16] drivers: i2c: master arbiter: create driver Ken Chen
2018-03-19 11:38 ` Ken Chen
2018-03-19 12:45 ` Peter Rosin
2018-03-19 13:44   ` Guenter Roeck
2018-03-20  6:14     ` ChenKenYY 陳永營 TAO
2018-03-20  6:14       ` ChenKenYY 陳永營 TAO
2018-03-20  7:00 ` kbuild test robot
2018-03-20  7:00   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.