[PATCH v7 0/7] i2c: Add FSI-attached I2C master algorithm

[PATCH v7 0/7] i2c: Add FSI-attached I2C master algorithm

[Eddie James]

This series adds an algorithm for an I2C master physically located on an FSI
slave device. The I2C master has multiple ports, each of which may be connected
to an I2C slave. Access to the I2C master registers is achieved over FSI bus.

Due to the multi-port nature of the I2C master, the driver instantiates a new
I2C adapter for each port connected to a slave. The connected ports should be
defined in the device tree under the I2C master device.

Changes since v6
 - Remove spinlock for reset functionality; it's unecessary and doesn't work
   with the latest FSI core.
 - Use a mutex instead of a semaphore, and don't wait for timeout to get the
   lock.
 - Use usleeps instead of schedule_timeout; it's not worth the overhead when
   the wait should be very short in between sending the command and receiving
   the response.

Changes since v5
 - Fix reset functionality and do a reset after every transfer failure

Edward A. James (7):
  dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation
  drivers/i2c: Add FSI-attached I2C master algorithm
  drivers/i2c: Add port structure to FSI algorithm
  drivers/i2c: Add abort and hardware reset procedures
  drivers/i2c: Add transfer implementation for FSI algorithm
  drivers/i2c: Add I2C master locking to FSI algorithm
  drivers/i2c: Add bus recovery for FSI algorithm

 Documentation/devicetree/bindings/i2c/i2c-fsi.txt |  40 ++
 drivers/i2c/busses/Kconfig                        |  11 +
 drivers/i2c/busses/Makefile                       |   1 +
 drivers/i2c/busses/i2c-fsi.c                      | 738 ++++++++++++++++++++++
 4 files changed, 790 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt
 create mode 100644 drivers/i2c/busses/i2c-fsi.c

-- 
1.8.3.1

[PATCH v7 1/7] dt-bindings: i2c: Add FSI-attached I2C master dt binding documentation

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Document the bindings.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/i2c/i2c-fsi.txt | 40 +++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-fsi.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-fsi.txt b/Documentation/devicetree/bindings/i2c/i2c-fsi.txt
new file mode 100644
index 0000000..b1be2ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-fsi.txt
@@ -0,0 +1,40 @@
+Device-tree bindings for FSI-attached I2C master and busses
+-----------------------------------------------------------
+
+Required properties:
+ - compatible = "ibm,i2c-fsi";
+ - reg = < address size >;		: The FSI CFAM address and address
+					  space size.
+ - #address-cells = <1>;		: Number of address cells in child
+					  nodes.
+ - #size-cells = <0>;			: Number of size cells in child nodes.
+ - child nodes				: Nodes to describe busses off the I2C
+					  master.
+
+Child node required properties:
+ - reg = < port number >		: The port number on the I2C master.
+
+Child node optional properties:
+ - child nodes				: Nodes to describe devices on the I2C
+					  bus.
+
+Examples:
+
+    i2c@1800 {
+        compatible = "ibm,i2c-fsi";
+        reg = < 0x1800 0x400 >;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        i2c-bus@0 {
+            reg = <0>;
+        };
+
+        i2c-bus@1 {
+            reg = <1>;
+
+            eeprom@50 {
+                compatible = "vendor,dev-name";
+            };
+        };
+    };
-- 
1.8.3.1

[PATCH v7 2/7] drivers/i2c: Add FSI-attached I2C master algorithm

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Add register definitions for FSI-attached I2C master and functions to
access those registers over FSI. Add an FSI driver so that our I2C bus
is probed up during an FSI scan.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/i2c/busses/Kconfig   |  11 ++
 drivers/i2c/busses/Makefile  |   1 +
 drivers/i2c/busses/i2c-fsi.c | 244 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-fsi.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 4f8df2e..ebba369 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1330,4 +1330,15 @@ config I2C_ZX2967
 	  This driver can also be built as a module. If so, the module will be
 	  called i2c-zx2967.
 
+config I2C_FSI
+	tristate "FSI I2C driver"
+	depends on FSI
+	help
+	  Driver for FSI bus attached I2C masters. These are I2C masters that
+	  are connected to the system over a FSI bus, instead of the more
+	  common PCI or MMIO interface.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called as i2c-fsi.
+
 endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 5a86914..4909fd6 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -137,5 +137,6 @@ obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
 obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
 obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
+obj-$(CONFIG_I2C_FSI)		+= i2c-fsi.o
 
 ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
new file mode 100644
index 0000000..79475f8
--- /dev/null
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -0,0 +1,244 @@
+/*
+ * Copyright 2017 IBM Corporation
+ *
+ * Eddie James <eajames@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/fsi.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define FSI_ENGID_I2C		0x7
+
+/* Find left shift from first set bit in m */
+#define MASK_TO_LSH(m)		(__builtin_ffsll(m) - 1ULL)
+
+/* Extract field m from v */
+#define GETFIELD(m, v)		(((v) & (m)) >> MASK_TO_LSH(m))
+
+/* Set field m of v to val */
+#define SETFIELD(m, v, val)	\
+	(((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
+
+#define I2C_DEFAULT_CLK_DIV	6
+
+/* i2c registers */
+#define I2C_FSI_FIFO		0x00
+#define I2C_FSI_CMD		0x04
+#define I2C_FSI_MODE		0x08
+#define I2C_FSI_WATER_MARK	0x0C
+#define I2C_FSI_INT_MASK	0x10
+#define I2C_FSI_INT_COND	0x14
+#define I2C_FSI_OR_INT_MASK	0x14
+#define I2C_FSI_INTS		0x18
+#define I2C_FSI_AND_INT_MASK	0x18
+#define I2C_FSI_STAT		0x1C
+#define I2C_FSI_RESET_I2C	0x1C
+#define I2C_FSI_ESTAT		0x20
+#define I2C_FSI_RESET_ERR	0x20
+#define I2C_FSI_RESID_LEN	0x24
+#define I2C_FSI_SET_SCL		0x24
+#define I2C_FSI_PORT_BUSY	0x28
+#define I2C_FSI_RESET_SCL	0x2C
+#define I2C_FSI_SET_SDA		0x30
+#define I2C_FSI_RESET_SDA	0x34
+
+/* cmd register */
+#define I2C_CMD_WITH_START	0x80000000
+#define I2C_CMD_WITH_ADDR	0x40000000
+#define I2C_CMD_RD_CONT		0x20000000
+#define I2C_CMD_WITH_STOP	0x10000000
+#define I2C_CMD_FORCELAUNCH	0x08000000
+#define I2C_CMD_ADDR		0x00fe0000
+#define I2C_CMD_READ		0x00010000
+#define I2C_CMD_LEN		0x0000ffff
+
+/* mode register */
+#define I2C_MODE_CLKDIV		0xffff0000
+#define I2C_MODE_PORT		0x0000fc00
+#define I2C_MODE_ENHANCED	0x00000008
+#define I2C_MODE_DIAG		0x00000004
+#define I2C_MODE_PACE_ALLOW	0x00000002
+#define I2C_MODE_WRAP		0x00000001
+
+/* watermark register */
+#define I2C_WATERMARK_HI	0x0000f000
+#define I2C_WATERMARK_LO	0x000000f0
+
+#define I2C_FIFO_HI_LVL		4
+#define I2C_FIFO_LO_LVL		4
+
+/* interrupt register */
+#define I2C_INT_INV_CMD		0x00008000
+#define I2C_INT_PARITY		0x00004000
+#define I2C_INT_BE_OVERRUN	0x00002000
+#define I2C_INT_BE_ACCESS	0x00001000
+#define I2C_INT_LOST_ARB	0x00000800
+#define I2C_INT_NACK		0x00000400
+#define I2C_INT_DAT_REQ		0x00000200
+#define I2C_INT_CMD_COMP	0x00000100
+#define I2C_INT_STOP_ERR	0x00000080
+#define I2C_INT_BUSY		0x00000040
+#define I2C_INT_IDLE		0x00000020
+
+#define I2C_INT_ENABLE		0x0000ff80
+#define I2C_INT_ERR		0x0000fcc0
+
+/* status register */
+#define I2C_STAT_INV_CMD	0x80000000
+#define I2C_STAT_PARITY		0x40000000
+#define I2C_STAT_BE_OVERRUN	0x20000000
+#define I2C_STAT_BE_ACCESS	0x10000000
+#define I2C_STAT_LOST_ARB	0x08000000
+#define I2C_STAT_NACK		0x04000000
+#define I2C_STAT_DAT_REQ	0x02000000
+#define I2C_STAT_CMD_COMP	0x01000000
+#define I2C_STAT_STOP_ERR	0x00800000
+#define I2C_STAT_MAX_PORT	0x000f0000
+#define I2C_STAT_ANY_INT	0x00008000
+#define I2C_STAT_SCL_IN		0x00000800
+#define I2C_STAT_SDA_IN		0x00000400
+#define I2C_STAT_PORT_BUSY	0x00000200
+#define I2C_STAT_SELF_BUSY	0x00000100
+#define I2C_STAT_FIFO_COUNT	0x000000ff
+
+#define I2C_STAT_ERR		0xfc800000
+#define I2C_STAT_ANY_RESP	0xff800000
+
+/* extended status register */
+#define I2C_ESTAT_FIFO_SZ	0xff000000
+#define I2C_ESTAT_SCL_IN_SY	0x00008000
+#define I2C_ESTAT_SDA_IN_SY	0x00004000
+#define I2C_ESTAT_S_SCL		0x00002000
+#define I2C_ESTAT_S_SDA		0x00001000
+#define I2C_ESTAT_M_SCL		0x00000800
+#define I2C_ESTAT_M_SDA		0x00000400
+#define I2C_ESTAT_HI_WATER	0x00000200
+#define I2C_ESTAT_LO_WATER	0x00000100
+#define I2C_ESTAT_PORT_BUSY	0x00000080
+#define I2C_ESTAT_SELF_BUSY	0x00000040
+#define I2C_ESTAT_VERSION	0x0000001f
+
+struct fsi_i2c_master {
+	struct fsi_device	*fsi;
+	u8			fifo_size;
+};
+
+static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
+			    u32 *data)
+{
+	int rc;
+	__be32 data_be;
+
+	rc = fsi_device_read(fsi, reg, &data_be, sizeof(data_be));
+	if (rc)
+		return rc;
+
+	*data = be32_to_cpu(data_be);
+
+	return 0;
+}
+
+static int fsi_i2c_write_reg(struct fsi_device *fsi, unsigned int reg,
+			     u32 *data)
+{
+	__be32 data_be = cpu_to_be32(*data);
+
+	return fsi_device_write(fsi, reg, &data_be, sizeof(data_be));
+}
+
+static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
+{
+	int rc;
+	u32 mode = I2C_MODE_ENHANCED, extended_status, watermark = 0;
+	u32 interrupt = 0;
+
+	/* since we use polling, disable interrupts */
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_INT_MASK, &interrupt);
+	if (rc)
+		return rc;
+
+	mode = SETFIELD(I2C_MODE_CLKDIV, mode, I2C_DEFAULT_CLK_DIV);
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_ESTAT, &extended_status);
+	if (rc)
+		return rc;
+
+	i2c->fifo_size = GETFIELD(I2C_ESTAT_FIFO_SZ, extended_status);
+	watermark = SETFIELD(I2C_WATERMARK_HI, watermark,
+			     i2c->fifo_size - I2C_FIFO_HI_LVL);
+	watermark = SETFIELD(I2C_WATERMARK_LO, watermark,
+			     I2C_FIFO_LO_LVL);
+
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_WATER_MARK, &watermark);
+
+	return rc;
+}
+
+static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			int num)
+{
+	return -EOPNOTSUPP;
+}
+
+static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_PROTOCOL_MANGLING | I2C_FUNC_10BIT_ADDR
+		| I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm fsi_i2c_algorithm = {
+	.master_xfer = fsi_i2c_xfer,
+	.functionality = fsi_i2c_functionality,
+};
+
+static int fsi_i2c_probe(struct device *dev)
+{
+	struct fsi_i2c_master *i2c;
+	int rc;
+
+	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->fsi = to_fsi_dev(dev);
+
+	rc = fsi_i2c_dev_init(i2c);
+	if (rc)
+		return rc;
+
+	dev_set_drvdata(dev, i2c);
+
+	return 0;
+}
+
+static const struct fsi_device_id fsi_i2c_ids[] = {
+	{ FSI_ENGID_I2C, FSI_VERSION_ANY },
+	{ 0 }
+};
+
+static struct fsi_driver fsi_i2c_driver = {
+	.id_table = fsi_i2c_ids,
+	.drv = {
+		.name = "i2c-fsi",
+		.bus = &fsi_bus_type,
+		.probe = fsi_i2c_probe,
+	},
+};
+
+module_fsi_driver(fsi_i2c_driver);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("FSI attached I2C master");
+MODULE_LICENSE("GPL");
-- 
1.8.3.1

[PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Add and initialize I2C adapters for each port on the FSI-attached I2C
master. Ports for each master are defined in the devicetree.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 96 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 79475f8..4ad5d68 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -14,7 +14,9 @@
 #include <linux/fsi.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 #define FSI_ENGID_I2C		0x7
 
@@ -130,6 +132,14 @@
 struct fsi_i2c_master {
 	struct fsi_device	*fsi;
 	u8			fifo_size;
+	struct list_head	ports;
+};
+
+struct fsi_i2c_port {
+	struct list_head	list;
+	struct i2c_adapter	adapter;
+	struct fsi_i2c_master	*master;
+	u16			port;
 };
 
 static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -186,9 +196,44 @@ static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
 	return rc;
 }
 
+static int fsi_i2c_set_port(struct fsi_i2c_port *port)
+{
+	int rc;
+	struct fsi_device *fsi = port->master->fsi;
+	u32 mode, dummy = 0;
+	u16 old_port;
+
+	rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	old_port = GETFIELD(I2C_MODE_PORT, mode);
+
+	if (old_port != port->port) {
+		mode = SETFIELD(I2C_MODE_PORT, mode, port->port);
+		rc = fsi_i2c_write_reg(fsi, I2C_FSI_MODE, &mode);
+		if (rc)
+			return rc;
+
+		/* reset engine when port is changed */
+		rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
+		if (rc)
+			return rc;
+	}
+
+	return rc;
+}
+
 static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			int num)
 {
+	int rc;
+	struct fsi_i2c_port *port = adap->algo_data;
+
+	rc = fsi_i2c_set_port(port);
+	if (rc)
+		return rc;
+
 	return -EOPNOTSUPP;
 }
 
@@ -206,23 +251,73 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
 static int fsi_i2c_probe(struct device *dev)
 {
 	struct fsi_i2c_master *i2c;
+	struct fsi_i2c_port *port;
+	struct device_node *np;
 	int rc;
+	u32 port_no;
 
 	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
 	i2c->fsi = to_fsi_dev(dev);
+	INIT_LIST_HEAD(&i2c->ports);
 
 	rc = fsi_i2c_dev_init(i2c);
 	if (rc)
 		return rc;
 
+	/* Add adapter for each i2c port of the master. */
+	for_each_available_child_of_node(dev->of_node, np) {
+		rc = of_property_read_u32(np, "reg", &port_no);
+		if (rc || port_no > USHRT_MAX)
+			continue;
+
+		port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+		if (!port)
+			break;
+
+		port->master = i2c;
+		port->port = port_no;
+
+		port->adapter.owner = THIS_MODULE;
+		port->adapter.dev.of_node = np;
+		port->adapter.dev.parent = dev;
+		port->adapter.algo = &fsi_i2c_algorithm;
+		port->adapter.algo_data = port;
+
+		snprintf(port->adapter.name, sizeof(port->adapter.name),
+			 "i2c_bus-%u", port_no);
+
+		rc = i2c_add_adapter(&port->adapter);
+		if (rc < 0) {
+			dev_err(dev, "Failed to register adapter: %d\n", rc);
+			devm_kfree(dev, port);
+			continue;
+		}
+
+		list_add(&port->list, &i2c->ports);
+	}
+
 	dev_set_drvdata(dev, i2c);
 
 	return 0;
 }
 
+static int fsi_i2c_remove(struct device *dev)
+{
+	struct fsi_i2c_master *i2c = dev_get_drvdata(dev);
+	struct fsi_i2c_port *port;
+
+	if (!list_empty(&i2c->ports)) {
+		list_for_each_entry(port, &i2c->ports, list) {
+			i2c_del_adapter(&port->adapter);
+		}
+	}
+
+	return 0;
+}
+
 static const struct fsi_device_id fsi_i2c_ids[] = {
 	{ FSI_ENGID_I2C, FSI_VERSION_ANY },
 	{ 0 }
@@ -234,6 +329,7 @@ static int fsi_i2c_probe(struct device *dev)
 		.name = "i2c-fsi",
 		.bus = &fsi_bus_type,
 		.probe = fsi_i2c_probe,
+		.remove = fsi_i2c_remove,
 	},
 };
 
-- 
1.8.3.1

[PATCH v7 4/7] drivers/i2c: Add abort and hardware reset procedures

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Add abort procedure for failed transfers. Add engine and bus reset
procedures to recover from as many faults as possible.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 172 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 172 insertions(+)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 4ad5d68..c0b17df 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -9,10 +9,12 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/fsi.h>
 #include <linux/i2c.h>
+#include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
@@ -129,6 +131,14 @@
 #define I2C_ESTAT_SELF_BUSY	0x00000040
 #define I2C_ESTAT_VERSION	0x0000001f
 
+#define I2C_PORT_BUSY_RESET	0x80000000
+
+#define I2C_LOCAL_SLEEP_MAX_US	500
+#define I2C_LOCAL_SLEEP_MIN_US	50
+
+/* choose timeout length from legacy driver; it's well tested */
+#define I2C_ABORT_TIMEOUT	msecs_to_jiffies(100)
+
 struct fsi_i2c_master {
 	struct fsi_device	*fsi;
 	u8			fifo_size;
@@ -224,6 +234,168 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
 	return rc;
 }
 
+static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
+{
+	int i, rc;
+	u32 mode, stat, ext, dummy = 0;
+
+	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	mode |= I2C_MODE_DIAG;
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	for (i = 0; i < 9; ++i) {
+		rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
+		if (rc)
+			return rc;
+
+		rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
+		if (rc)
+			return rc;
+	}
+
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
+	if (rc)
+		return rc;
+
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SDA, &dummy);
+	if (rc)
+		return rc;
+
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
+	if (rc)
+		return rc;
+
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SDA, &dummy);
+	if (rc)
+		return rc;
+
+	mode &= ~I2C_MODE_DIAG;
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
+	if (rc)
+		return rc;
+
+	/* check for hardware fault */
+	if (!(stat & I2C_STAT_SCL_IN) || !(stat & I2C_STAT_SDA_IN)) {
+		rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_ESTAT, &ext);
+		if (rc)
+			return rc;
+
+		dev_err(&i2c->fsi->dev, "bus stuck status[%08X] ext[%08X]\n",
+			stat, ext);
+	}
+
+	return 0;
+}
+
+static int fsi_i2c_reset(struct fsi_i2c_master *i2c, u16 port)
+{
+	int rc;
+	u32 mode, stat, dummy = 0;
+
+	/* reset engine */
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
+	if (rc)
+		return rc;
+
+	/* re-init engine */
+	rc = fsi_i2c_dev_init(i2c);
+	if (rc)
+		return rc;
+
+	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+	if (rc)
+		return rc;
+
+	/* set port; default after reset is 0 */
+	if (port) {
+		mode = SETFIELD(I2C_MODE_PORT, mode, port);
+		rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_MODE, &mode);
+		if (rc)
+			return rc;
+	}
+
+	/* reset busy register; hw workaround */
+	dummy = I2C_PORT_BUSY_RESET;
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_PORT_BUSY, &dummy);
+	if (rc)
+		return rc;
+
+	/* force bus reset */
+	rc = fsi_i2c_reset_bus(i2c);
+	if (rc)
+		return rc;
+
+	/* reset errors */
+	dummy = 0;
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
+	if (rc)
+		return rc;
+
+	/* wait for command complete; time from legacy driver */
+	msleep(1);
+
+	rc = fsi_i2c_read_reg(i2c->fsi, I2C_FSI_STAT, &stat);
+	if (rc)
+		return rc;
+
+	if (stat & I2C_STAT_CMD_COMP)
+		return rc;
+
+	/* failed to get command complete; reset engine again */
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_I2C, &dummy);
+	if (rc)
+		return rc;
+
+	/* re-init engine again */
+	return fsi_i2c_dev_init(i2c);
+}
+
+static int fsi_i2c_abort(struct fsi_i2c_port *port, u32 status)
+{
+	int rc;
+	unsigned long start;
+	u32 cmd = I2C_CMD_WITH_STOP;
+	struct fsi_device *fsi = port->master->fsi;
+
+	rc = fsi_i2c_reset(port->master, port->port);
+	if (rc)
+		return rc;
+
+	/* skip final stop command for these errors */
+	if (status & (I2C_STAT_PARITY | I2C_STAT_LOST_ARB | I2C_STAT_STOP_ERR))
+		return 0;
+
+	/* write stop command */
+	rc = fsi_i2c_write_reg(fsi, I2C_FSI_CMD, &cmd);
+	if (rc)
+		return rc;
+
+	/* wait until we see command complete in the master */
+	start = jiffies;
+
+	do {
+		rc = fsi_i2c_read_reg(fsi, I2C_FSI_STAT, &status);
+		if (rc)
+			return rc;
+
+		if (status & I2C_STAT_CMD_COMP)
+			return 0;
+
+		usleep_range(I2C_LOCAL_SLEEP_MIN_US, I2C_LOCAL_SLEEP_MAX_US);
+	} while (time_after(start + I2C_ABORT_TIMEOUT, jiffies));
+
+	return -ETIME;
+}
+
 static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			int num)
 {
-- 
1.8.3.1

[PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Execute I2C transfers from the FSI-attached I2C master. Use polling
instead of interrupts as we have no hardware IRQ over FSI.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 203 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 201 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index c0b17df..78e3586 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -150,6 +150,7 @@ struct fsi_i2c_port {
 	struct i2c_adapter	adapter;
 	struct fsi_i2c_master	*master;
 	u16			port;
+	u16			xfrd;
 };
 
 static int fsi_i2c_read_reg(struct fsi_device *fsi, unsigned int reg,
@@ -234,6 +235,103 @@ static int fsi_i2c_set_port(struct fsi_i2c_port *port)
 	return rc;
 }
 
+static int fsi_i2c_start(struct fsi_i2c_port *port, struct i2c_msg *msg,
+			 bool stop)
+{
+	int rc;
+	struct fsi_i2c_master *i2c = port->master;
+	u32 cmd = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR;
+
+	port->xfrd = 0;
+
+	if (msg->flags & I2C_M_RD)
+		cmd |= I2C_CMD_READ;
+
+	if (stop || msg->flags & I2C_M_STOP)
+		cmd |= I2C_CMD_WITH_STOP;
+
+	cmd = SETFIELD(I2C_CMD_ADDR, cmd, msg->addr >> 1);
+	cmd = SETFIELD(I2C_CMD_LEN, cmd, msg->len);
+
+	rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_CMD, &cmd);
+
+	return rc;
+}
+
+static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+			      u8 fifo_count)
+{
+	int write;
+	int rc = 0;
+	struct fsi_i2c_master *i2c = port->master;
+	int bytes_to_write = i2c->fifo_size - fifo_count;
+	int bytes_remaining = msg->len - port->xfrd;
+
+	if (bytes_to_write > bytes_remaining)
+		bytes_to_write = bytes_remaining;
+
+	while (bytes_to_write > 0) {
+		write = bytes_to_write;
+		/* fsi limited to max 4 byte aligned ops */
+		if (bytes_to_write > 4)
+			write = 4;
+		else if (write == 3)
+			write = 2;
+
+		rc = fsi_device_write(i2c->fsi, I2C_FSI_FIFO,
+				      &msg->buf[port->xfrd], write);
+		if (rc)
+			return rc;
+
+		port->xfrd += write;
+		bytes_to_write -= write;
+	}
+
+	return rc;
+}
+
+static int fsi_i2c_read_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
+			     u8 fifo_count)
+{
+	int read;
+	int rc = 0;
+	struct fsi_i2c_master *i2c = port->master;
+	int xfr_remaining = msg->len - port->xfrd;
+	u32 dummy;
+
+	while (fifo_count) {
+		read = fifo_count;
+		/* fsi limited to max 4 byte aligned ops */
+		if (fifo_count > 4)
+			read = 4;
+		else if (read == 3)
+			read = 2;
+
+		if (xfr_remaining) {
+			if (xfr_remaining < read)
+				read = xfr_remaining;
+
+			rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO,
+					     &msg->buf[port->xfrd], read);
+			if (rc)
+				return rc;
+
+			port->xfrd += read;
+			xfr_remaining -= read;
+		} else {
+			/* no more buffer but data in fifo, need to clear it */
+			rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO, &dummy,
+					     read);
+			if (rc)
+				return rc;
+		}
+
+		fifo_count -= read;
+	}
+
+	return rc;
+}
+
 static int fsi_i2c_reset_bus(struct fsi_i2c_master *i2c)
 {
 	int i, rc;
@@ -396,17 +494,118 @@ static int fsi_i2c_abort(struct fsi_i2c_port *port, u32 status)
 	return -ETIME;
 }
 
+static int fsi_i2c_handle_status(struct fsi_i2c_port *port,
+				 struct i2c_msg *msg, u32 status)
+{
+	int rc;
+	u8 fifo_count;
+
+	if (status & I2C_STAT_ERR) {
+		rc = fsi_i2c_abort(port, status);
+		if (rc)
+			return rc;
+
+		if (status & I2C_STAT_INV_CMD)
+			return -EINVAL;
+
+		if (status & (I2C_STAT_PARITY | I2C_STAT_BE_OVERRUN |
+		    I2C_STAT_BE_ACCESS))
+			return -EPROTO;
+
+		if (status & I2C_STAT_NACK)
+			return -ENXIO;
+
+		if (status & I2C_STAT_LOST_ARB)
+			return -EAGAIN;
+
+		if (status & I2C_STAT_STOP_ERR)
+			return -EBADMSG;
+
+		return -EIO;
+	}
+
+	if (status & I2C_STAT_DAT_REQ) {
+		fifo_count = GETFIELD(I2C_STAT_FIFO_COUNT, status);
+
+		if (msg->flags & I2C_M_RD)
+			rc = fsi_i2c_read_fifo(port, msg, fifo_count);
+		else
+			rc = fsi_i2c_write_fifo(port, msg, fifo_count);
+
+		return rc;
+	}
+
+	if (status & I2C_STAT_CMD_COMP) {
+		if (port->xfrd < msg->len)
+			rc = -ENODATA;
+		else
+			rc = msg->len;
+
+		return rc;
+	}
+
+	return 0;
+}
+
+static int fsi_i2c_wait(struct fsi_i2c_port *port, struct i2c_msg *msg,
+			unsigned long timeout)
+{
+	u32 status = 0;
+	int rc;
+	unsigned long start = jiffies;
+
+	do {
+		rc = fsi_i2c_read_reg(port->master->fsi, I2C_FSI_STAT,
+				      &status);
+		if (rc)
+			return rc;
+
+		if (status & I2C_STAT_ANY_RESP) {
+			rc = fsi_i2c_handle_status(port, msg, status);
+			if (rc < 0)
+				return rc;
+
+			/* cmd complete and all data xfrd */
+			if (rc == msg->len)
+				return 0;
+
+			/* need to xfr more data, but maybe don't need wait */
+			continue;
+		}
+
+		usleep_range(I2C_LOCAL_SLEEP_MIN_US, I2C_LOCAL_SLEEP_MAX_US);
+	} while (time_after(start + timeout, jiffies));
+
+	return -ETIME;
+}
+
 static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			int num)
 {
-	int rc;
+	int i, rc;
+	unsigned long start_time;
 	struct fsi_i2c_port *port = adap->algo_data;
+	struct i2c_msg *msg;
 
 	rc = fsi_i2c_set_port(port);
 	if (rc)
 		return rc;
 
-	return -EOPNOTSUPP;
+	for (i = 0; i < num; ++i) {
+		msg = msgs + i;
+		start_time = jiffies;
+
+		rc = fsi_i2c_start(port, msg, i == num - 1);
+		if (rc)
+			return rc;
+
+		rc = fsi_i2c_wait(port, msg,
+				  adap->timeout - (jiffies - start_time));
+		if (rc)
+			return rc;
+	}
+
+	return 0;
 }
 
 static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
-- 
1.8.3.1

[PATCH v7 6/7] drivers/i2c: Add I2C master locking to FSI algorithm

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Since there are many ports per master, each with it's own adapter and
chardev, we need some locking to prevent transfers from changing the
master state while other transfers are in progress.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index 78e3586..f309267 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 
 #define FSI_ENGID_I2C		0x7
@@ -143,6 +144,7 @@ struct fsi_i2c_master {
 	struct fsi_device	*fsi;
 	u8			fifo_size;
 	struct list_head	ports;
+	struct mutex		lock;
 };
 
 struct fsi_i2c_port {
@@ -585,11 +587,14 @@ static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	int i, rc;
 	unsigned long start_time;
 	struct fsi_i2c_port *port = adap->algo_data;
+	struct fsi_i2c_master *master = port->master;
 	struct i2c_msg *msg;
 
+	mutex_lock(&master->lock);
+
 	rc = fsi_i2c_set_port(port);
 	if (rc)
-		return rc;
+		goto unlock;
 
 	for (i = 0; i < num; ++i) {
 		msg = msgs + i;
@@ -597,15 +602,17 @@ static int fsi_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 
 		rc = fsi_i2c_start(port, msg, i == num - 1);
 		if (rc)
-			return rc;
+			goto unlock;
 
 		rc = fsi_i2c_wait(port, msg,
 				  adap->timeout - (jiffies - start_time));
 		if (rc)
-			return rc;
+			goto unlock;
 	}
 
-	return 0;
+unlock:
+	mutex_unlock(&master->lock);
+	return rc;
 }
 
 static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
@@ -631,6 +638,7 @@ static int fsi_i2c_probe(struct device *dev)
 	if (!i2c)
 		return -ENOMEM;
 
+	mutex_init(&i2c->lock);
 	i2c->fsi = to_fsi_dev(dev);
 	INIT_LIST_HEAD(&i2c->ports);
 
-- 
1.8.3.1

[PATCH v7 7/7] drivers/i2c: Add bus recovery for FSI algorithm

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Bus recovery should reset the engine and force clock the bus 9 times
to recover most situations.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/i2c/busses/i2c-fsi.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/i2c/busses/i2c-fsi.c b/drivers/i2c/busses/i2c-fsi.c
index f309267..52a662c 100644
--- a/drivers/i2c/busses/i2c-fsi.c
+++ b/drivers/i2c/busses/i2c-fsi.c
@@ -621,6 +621,24 @@ static u32 fsi_i2c_functionality(struct i2c_adapter *adap)
 		| I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 
+static int fsi_i2c_recover_bus(struct i2c_adapter *adap)
+{
+	int rc;
+	struct fsi_i2c_port *port = adap->algo_data;
+	struct fsi_i2c_master *master = port->master;
+
+	mutex_lock(&master->lock);
+
+	rc = fsi_i2c_reset(master, port->port);
+
+	mutex_unlock(&master->lock);
+	return rc;
+}
+
+static struct i2c_bus_recovery_info fsi_i2c_bus_recovery_info = {
+	.recover_bus = fsi_i2c_recover_bus,
+};
+
 static const struct i2c_algorithm fsi_i2c_algorithm = {
 	.master_xfer = fsi_i2c_xfer,
 	.functionality = fsi_i2c_functionality,
@@ -663,6 +681,7 @@ static int fsi_i2c_probe(struct device *dev)
 		port->adapter.dev.of_node = np;
 		port->adapter.dev.parent = dev;
 		port->adapter.algo = &fsi_i2c_algorithm;
+		port->adapter.bus_recovery_info = &fsi_i2c_bus_recovery_info;
 		port->adapter.algo_data = port;
 
 		snprintf(port->adapter.name, sizeof(port->adapter.name),
-- 
1.8.3.1

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Andy Shevchenko]

On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add and initialize I2C adapters for each port on the FSI-attached I2C
> master. Ports for each master are defined in the devicetree.

> +#include <linux/of.h>


> +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
> +{
> +       int rc;
> +       struct fsi_device *fsi = port->master->fsi;
> +       u32 mode, dummy = 0;
> +       u16 old_port;
> +
> +       rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
> +       if (rc)
> +               return rc;
> +

> +       old_port = GETFIELD(I2C_MODE_PORT, mode);
> +
> +       if (old_port != port->port) {

Why not simple

if (port->port == GETFIELD())
  return 0;

?

> +               /* reset engine when port is changed */
> +               rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
> +               if (rc)
> +                       return rc;
> +       }

> +       return rc;

It's hardly would be non-zero, right?

> +}

>  static int fsi_i2c_probe(struct device *dev)
>  {

Isn't below somehow repeats of_i2c_register_devices() ?
Why not to use it?

> +       /* Add adapter for each i2c port of the master. */
> +       for_each_available_child_of_node(dev->of_node, np) {
> +               rc = of_property_read_u32(np, "reg", &port_no);
> +               if (rc || port_no > USHRT_MAX)
> +                       continue;
> +
> +               port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +               if (!port)
> +                       break;
> +
> +               port->master = i2c;
> +               port->port = port_no;
> +
> +               port->adapter.owner = THIS_MODULE;
> +               port->adapter.dev.of_node = np;
> +               port->adapter.dev.parent = dev;
> +               port->adapter.algo = &fsi_i2c_algorithm;
> +               port->adapter.algo_data = port;
> +
> +               snprintf(port->adapter.name, sizeof(port->adapter.name),
> +                        "i2c_bus-%u", port_no);
> +
> +               rc = i2c_add_adapter(&port->adapter);
> +               if (rc < 0) {

> +                       dev_err(dev, "Failed to register adapter: %d\n", rc);

> +                       devm_kfree(dev, port);

This hurts my eyes. Why?!

> +                       continue;
> +               }
> +
> +               list_add(&port->list, &i2c->ports);
> +       }
> +
>         dev_set_drvdata(dev, i2c);
>
>         return 0;
>  }

> +       if (!list_empty(&i2c->ports)) {

My gosh, this is done already in list_for_each*()

> +               list_for_each_entry(port, &i2c->ports, list) {
> +                       i2c_del_adapter(&port->adapter);
> +               }

> +       }



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 2/7] drivers/i2c: Add FSI-attached I2C master algorithm

[Randy Dunlap]

On 05/29/2018 03:24 PM, Eddie James wrote:
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 4f8df2e..ebba369 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1330,4 +1330,15 @@ config I2C_ZX2967
>  	  This driver can also be built as a module. If so, the module will be
>  	  called i2c-zx2967.
>  
> +config I2C_FSI
> +	tristate "FSI I2C driver"
> +	depends on FSI
> +	help
> +	  Driver for FSI bus attached I2C masters. These are I2C masters that
> +	  are connected to the system over a FSI bus, instead of the more

	                              over an FSI bus,

> +	  common PCI or MMIO interface.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called as i2c-fsi.
> +
>  endmenu


-- 
~Randy

Re: [PATCH v7 2/7] drivers/i2c: Add FSI-attached I2C master algorithm

[Andy Shevchenko]

On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add register definitions for FSI-attached I2C master and functions to
> access those registers over FSI. Add an FSI driver so that our I2C bus
> is probed up during an FSI scan.

This looks like reinventing a wheel in some ways.

See my comments below.

> +/*
> + * Copyright 2017 IBM Corporation
> + *
> + * Eddie James <eajames@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */

We are using SPDX identifiers. Can you?

> +/* Find left shift from first set bit in m */
> +#define MASK_TO_LSH(m)         (__builtin_ffsll(m) - 1ULL)

Oh. What about GENMASK()?

> +/* Extract field m from v */
> +#define GETFIELD(m, v)         (((v) & (m)) >> MASK_TO_LSH(m))
> +
> +/* Set field m of v to val */
> +#define SETFIELD(m, v, val)    \
> +       (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))

Oh, what about https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h
?

> +#define I2C_CMD_WITH_START     0x80000000
> +#define I2C_CMD_WITH_ADDR      0x40000000
> +#define I2C_CMD_RD_CONT                0x20000000
> +#define I2C_CMD_WITH_STOP      0x10000000
> +#define I2C_CMD_FORCELAUNCH    0x08000000

BIT() ?

> +#define I2C_CMD_ADDR           0x00fe0000
> +#define I2C_CMD_READ           0x00010000

GENMASK()? Though precisely here it might be good to leave explicit values.

> +#define I2C_CMD_LEN            0x0000ffff

> +#define I2C_MODE_CLKDIV                0xffff0000
> +#define I2C_MODE_PORT          0x0000fc00
> +#define I2C_MODE_ENHANCED      0x00000008
> +#define I2C_MODE_DIAG          0x00000004
> +#define I2C_MODE_PACE_ALLOW    0x00000002
> +#define I2C_MODE_WRAP          0x00000001

What are they? Masks? Bit fields? Just plain numbers?

> +#define I2C_WATERMARK_HI       0x0000f000
> +#define I2C_WATERMARK_LO       0x000000f0

GENMASK() ?

> +#define I2C_INT_INV_CMD                0x00008000
> +#define I2C_INT_PARITY         0x00004000
> +#define I2C_INT_BE_OVERRUN     0x00002000
> +#define I2C_INT_BE_ACCESS      0x00001000
> +#define I2C_INT_LOST_ARB       0x00000800
> +#define I2C_INT_NACK           0x00000400
> +#define I2C_INT_DAT_REQ                0x00000200
> +#define I2C_INT_CMD_COMP       0x00000100
> +#define I2C_INT_STOP_ERR       0x00000080
> +#define I2C_INT_BUSY           0x00000040
> +#define I2C_INT_IDLE           0x00000020

BIT()

> +#define I2C_INT_ENABLE         0x0000ff80
> +#define I2C_INT_ERR            0x0000fcc0

> +#define I2C_STAT_INV_CMD       0x80000000
> +#define I2C_STAT_PARITY                0x40000000
> +#define I2C_STAT_BE_OVERRUN    0x20000000
> +#define I2C_STAT_BE_ACCESS     0x10000000
> +#define I2C_STAT_LOST_ARB      0x08000000
> +#define I2C_STAT_NACK          0x04000000
> +#define I2C_STAT_DAT_REQ       0x02000000
> +#define I2C_STAT_CMD_COMP      0x01000000
> +#define I2C_STAT_STOP_ERR      0x00800000
> +#define I2C_STAT_MAX_PORT      0x000f0000
> +#define I2C_STAT_ANY_INT       0x00008000
> +#define I2C_STAT_SCL_IN                0x00000800
> +#define I2C_STAT_SDA_IN                0x00000400
> +#define I2C_STAT_PORT_BUSY     0x00000200
> +#define I2C_STAT_SELF_BUSY     0x00000100

BIT()

> +#define I2C_STAT_FIFO_COUNT    0x000000ff

GENMASK()

> +
> +#define I2C_STAT_ERR           0xfc800000
> +#define I2C_STAT_ANY_RESP      0xff800000

> +#define I2C_ESTAT_FIFO_SZ      0xff000000

GENMASK()

> +#define I2C_ESTAT_SCL_IN_SY    0x00008000
> +#define I2C_ESTAT_SDA_IN_SY    0x00004000
> +#define I2C_ESTAT_S_SCL                0x00002000
> +#define I2C_ESTAT_S_SDA                0x00001000
> +#define I2C_ESTAT_M_SCL                0x00000800
> +#define I2C_ESTAT_M_SDA                0x00000400
> +#define I2C_ESTAT_HI_WATER     0x00000200
> +#define I2C_ESTAT_LO_WATER     0x00000100
> +#define I2C_ESTAT_PORT_BUSY    0x00000080
> +#define I2C_ESTAT_SELF_BUSY    0x00000040

BIT()

> +#define I2C_ESTAT_VERSION      0x0000001f

GENMASK()

> +       __be32 data_be;

No need to have a suffix. If anything can go wrong we have a tool,
it's called sparse. It will catch out inappropriate use of __bitwise
types.

> +       __be32 data_be = cpu_to_be32(*data);

cpu_to_be32p()  IIUC?

> +static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
> +{
> +       int rc;
> +       u32 mode = I2C_MODE_ENHANCED, extended_status, watermark = 0;

> +       u32 interrupt = 0;

Redundant assignment.

> +
> +       /* since we use polling, disable interrupts */
> +       rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_INT_MASK, &interrupt);
> +       if (rc)
> +               return rc;

> +       return rc;

Would be non-zero?

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm

[Andy Shevchenko]

On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Execute I2C transfers from the FSI-attached I2C master. Use polling
> instead of interrupts as we have no hardware IRQ over FSI.

> +       if (msg->flags & I2C_M_RD)
> +               cmd |= I2C_CMD_READ;

I think we have a helper for this, though not sure.

> +static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
> +                             u8 fifo_count)
> +{
> +       int write;

> +       int rc = 0;

Redundant assignment.

> +       struct fsi_i2c_master *i2c = port->master;
> +       int bytes_to_write = i2c->fifo_size - fifo_count;
> +       int bytes_remaining = msg->len - port->xfrd;

> +       if (bytes_to_write > bytes_remaining)
> +               bytes_to_write = bytes_remaining;

_write = min(_write, _remaining);

> +       while (bytes_to_write > 0) {

> +               write = bytes_to_write;
> +               /* fsi limited to max 4 byte aligned ops */
> +               if (bytes_to_write > 4)
> +                       write = 4;
> +               else if (write == 3)
> +                       write = 2;

write = min_t(int, 4, rounddown_pow_of_two(bytes_to_write));

Also check it carefully, it might be optimized even more, though I
didn't think much.

> +       }

> +       return rc;

How it can be non-zero?

> +}

> +
> +static int fsi_i2c_read_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
> +                            u8 fifo_count)
> +{
> +       int read;

> +       int rc = 0;

Redundant assignment.

> +       struct fsi_i2c_master *i2c = port->master;
> +       int xfr_remaining = msg->len - port->xfrd;
> +       u32 dummy;
> +
> +       while (fifo_count) {

> +               read = fifo_count;
> +               /* fsi limited to max 4 byte aligned ops */
> +               if (fifo_count > 4)
> +                       read = 4;
> +               else if (read == 3)
> +                       read = 2;

See above for write case and do in similar way.

> +
> +               if (xfr_remaining) {

> +                       if (xfr_remaining < read)
> +                               read = xfr_remaining;

read = min(read, _remaining);

> +
> +                       rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO,
> +                                            &msg->buf[port->xfrd], read);
> +                       if (rc)
> +                               return rc;
> +
> +                       port->xfrd += read;
> +                       xfr_remaining -= read;
> +               } else {
> +                       /* no more buffer but data in fifo, need to clear it */
> +                       rc = fsi_device_read(i2c->fsi, I2C_FSI_FIFO, &dummy,
> +                                            read);
> +                       if (rc)
> +                               return rc;
> +               }
> +
> +               fifo_count -= read;
> +       }
> +

> +       return rc;

How non-zero possible here?

> +}

> +               if (port->xfrd < msg->len)
> +                       rc = -ENODATA;
> +               else
> +                       rc = msg->len;
> +
> +               return rc;

if (...)
  return -ENODATA;
return msg->len;
 ?

> +       u32 status = 0;

Redundant assignment.

> +       return -ETIME;

ETIMEDOUT ?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 4/7] drivers/i2c: Add abort and hardware reset procedures

[Andy Shevchenko]

On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add abort procedure for failed transfers. Add engine and bus reset
> procedures to recover from as many faults as possible.

> +#define I2C_PORT_BUSY_RESET    0x80000000

BIT() ?

> +       for (i = 0; i < 9; ++i) {

i++ would work as well.

> +               rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_SCL, &dummy);
> +               if (rc)
> +                       return rc;
> +
> +               rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_SET_SCL, &dummy);
> +               if (rc)
> +                       return rc;
> +       }

> +       /* reset errors */
> +       dummy = 0;
> +       rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
> +       if (rc)
> +               return rc;

Ah, okay, discard my previous comments on dummy assignments. It seems
input and output.

> +       msleep(1);

msleep(1) usually a bad idea.
usleep_range() I guess what you need here.

> +}

> +       return -ETIME;

ETIMEDOUT ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 2/7] drivers/i2c: Add FSI-attached I2C master algorithm

[Eddie James]

On 05/29/2018 06:42 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Add register definitions for FSI-attached I2C master and functions to
>> access those registers over FSI. Add an FSI driver so that our I2C bus
>> is probed up during an FSI scan.
> This looks like reinventing a wheel in some ways.
>
> See my comments below.
>
>> +/*
>> + * Copyright 2017 IBM Corporation
>> + *
>> + * Eddie James <eajames@us.ibm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
> We are using SPDX identifiers. Can you?

Sure.

>
>> +/* Find left shift from first set bit in m */
>> +#define MASK_TO_LSH(m)         (__builtin_ffsll(m) - 1ULL)
> Oh. What about GENMASK()?
>
>> +/* Extract field m from v */
>> +#define GETFIELD(m, v)         (((v) & (m)) >> MASK_TO_LSH(m))
>> +
>> +/* Set field m of v to val */
>> +#define SETFIELD(m, v, val)    \
>> +       (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
> Oh, what about https://elixir.bootlin.com/linux/latest/source/include/linux/bitfield.h
> ?

Good idea, thanks.

>
>> +#define I2C_CMD_WITH_START     0x80000000
>> +#define I2C_CMD_WITH_ADDR      0x40000000
>> +#define I2C_CMD_RD_CONT                0x20000000
>> +#define I2C_CMD_WITH_STOP      0x10000000
>> +#define I2C_CMD_FORCELAUNCH    0x08000000
> BIT() ?
>
>> +#define I2C_CMD_ADDR           0x00fe0000
>> +#define I2C_CMD_READ           0x00010000
> GENMASK()? Though precisely here it might be good to leave explicit values.
>
>> +#define I2C_CMD_LEN            0x0000ffff
>> +#define I2C_MODE_CLKDIV                0xffff0000
>> +#define I2C_MODE_PORT          0x0000fc00
>> +#define I2C_MODE_ENHANCED      0x00000008
>> +#define I2C_MODE_DIAG          0x00000004
>> +#define I2C_MODE_PACE_ALLOW    0x00000002
>> +#define I2C_MODE_WRAP          0x00000001
> What are they? Masks? Bit fields? Just plain numbers?
>
>> +#define I2C_WATERMARK_HI       0x0000f000
>> +#define I2C_WATERMARK_LO       0x000000f0
> GENMASK() ?
>
>> +#define I2C_INT_INV_CMD                0x00008000
>> +#define I2C_INT_PARITY         0x00004000
>> +#define I2C_INT_BE_OVERRUN     0x00002000
>> +#define I2C_INT_BE_ACCESS      0x00001000
>> +#define I2C_INT_LOST_ARB       0x00000800
>> +#define I2C_INT_NACK           0x00000400
>> +#define I2C_INT_DAT_REQ                0x00000200
>> +#define I2C_INT_CMD_COMP       0x00000100
>> +#define I2C_INT_STOP_ERR       0x00000080
>> +#define I2C_INT_BUSY           0x00000040
>> +#define I2C_INT_IDLE           0x00000020
> BIT()
>
>> +#define I2C_INT_ENABLE         0x0000ff80
>> +#define I2C_INT_ERR            0x0000fcc0
>> +#define I2C_STAT_INV_CMD       0x80000000
>> +#define I2C_STAT_PARITY                0x40000000
>> +#define I2C_STAT_BE_OVERRUN    0x20000000
>> +#define I2C_STAT_BE_ACCESS     0x10000000
>> +#define I2C_STAT_LOST_ARB      0x08000000
>> +#define I2C_STAT_NACK          0x04000000
>> +#define I2C_STAT_DAT_REQ       0x02000000
>> +#define I2C_STAT_CMD_COMP      0x01000000
>> +#define I2C_STAT_STOP_ERR      0x00800000
>> +#define I2C_STAT_MAX_PORT      0x000f0000
>> +#define I2C_STAT_ANY_INT       0x00008000
>> +#define I2C_STAT_SCL_IN                0x00000800
>> +#define I2C_STAT_SDA_IN                0x00000400
>> +#define I2C_STAT_PORT_BUSY     0x00000200
>> +#define I2C_STAT_SELF_BUSY     0x00000100
> BIT()
>
>> +#define I2C_STAT_FIFO_COUNT    0x000000ff
> GENMASK()
>
>> +
>> +#define I2C_STAT_ERR           0xfc800000
>> +#define I2C_STAT_ANY_RESP      0xff800000
>> +#define I2C_ESTAT_FIFO_SZ      0xff000000
> GENMASK()
>
>> +#define I2C_ESTAT_SCL_IN_SY    0x00008000
>> +#define I2C_ESTAT_SDA_IN_SY    0x00004000
>> +#define I2C_ESTAT_S_SCL                0x00002000
>> +#define I2C_ESTAT_S_SDA                0x00001000
>> +#define I2C_ESTAT_M_SCL                0x00000800
>> +#define I2C_ESTAT_M_SDA                0x00000400
>> +#define I2C_ESTAT_HI_WATER     0x00000200
>> +#define I2C_ESTAT_LO_WATER     0x00000100
>> +#define I2C_ESTAT_PORT_BUSY    0x00000080
>> +#define I2C_ESTAT_SELF_BUSY    0x00000040
> BIT()
>
>> +#define I2C_ESTAT_VERSION      0x0000001f
> GENMASK()
>
>> +       __be32 data_be;
> No need to have a suffix. If anything can go wrong we have a tool,
> it's called sparse. It will catch out inappropriate use of __bitwise
> types.

I already have a variable called data...

>
>> +       __be32 data_be = cpu_to_be32(*data);
> cpu_to_be32p()  IIUC?

Sure.

>
>> +static int fsi_i2c_dev_init(struct fsi_i2c_master *i2c)
>> +{
>> +       int rc;
>> +       u32 mode = I2C_MODE_ENHANCED, extended_status, watermark = 0;
>> +       u32 interrupt = 0;
> Redundant assignment.

No, I need to set the interrupt register to 0, so I must set this.

>
>> +
>> +       /* since we use polling, disable interrupts */
>> +       rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_INT_MASK, &interrupt);
>> +       if (rc)
>> +               return rc;
>> +       return rc;
> Would be non-zero?

No, fsi_i2c_write_reg returns non-zero on error, zero on success. That 
is what I want.

Thanks,
Eddie

>
>> +}

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Eddie James]

On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Add and initialize I2C adapters for each port on the FSI-attached I2C
>> master. Ports for each master are defined in the devicetree.
>> +#include <linux/of.h>
>
>> +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
>> +{
>> +       int rc;
>> +       struct fsi_device *fsi = port->master->fsi;
>> +       u32 mode, dummy = 0;
>> +       u16 old_port;
>> +
>> +       rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
>> +       if (rc)
>> +               return rc;
>> +
>> +       old_port = GETFIELD(I2C_MODE_PORT, mode);
>> +
>> +       if (old_port != port->port) {
> Why not simple
>
> if (port->port == GETFIELD())
>    return 0;
>
> ?

Sure.

>
>> +               /* reset engine when port is changed */
>> +               rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
>> +               if (rc)
>> +                       return rc;
>> +       }
>> +       return rc;
> It's hardly would be non-zero, right?

No, fsi_i2c_read_reg and fsi_i2c_write_reg both return 0 on success and 
non-zero on error. That is the desired behavior of this function also.

>
>> +}
>>   static int fsi_i2c_probe(struct device *dev)
>>   {
> Isn't below somehow repeats of_i2c_register_devices() ?
> Why not to use it?

Because I need to assign all these port structure fields. Also looks 
like of_i2c_register_devices creates new devices; I just want an adapter 
for each port.

>
>> +       /* Add adapter for each i2c port of the master. */
>> +       for_each_available_child_of_node(dev->of_node, np) {
>> +               rc = of_property_read_u32(np, "reg", &port_no);
>> +               if (rc || port_no > USHRT_MAX)
>> +                       continue;
>> +
>> +               port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> +               if (!port)
>> +                       break;
>> +
>> +               port->master = i2c;
>> +               port->port = port_no;
>> +
>> +               port->adapter.owner = THIS_MODULE;
>> +               port->adapter.dev.of_node = np;
>> +               port->adapter.dev.parent = dev;
>> +               port->adapter.algo = &fsi_i2c_algorithm;
>> +               port->adapter.algo_data = port;
>> +
>> +               snprintf(port->adapter.name, sizeof(port->adapter.name),
>> +                        "i2c_bus-%u", port_no);
>> +
>> +               rc = i2c_add_adapter(&port->adapter);
>> +               if (rc < 0) {
>> +                       dev_err(dev, "Failed to register adapter: %d\n", rc);
>> +                       devm_kfree(dev, port);
> This hurts my eyes. Why?!

What would you suggest instead?

>
>> +                       continue;
>> +               }
>> +
>> +               list_add(&port->list, &i2c->ports);
>> +       }
>> +
>>          dev_set_drvdata(dev, i2c);
>>
>>          return 0;
>>   }
>> +       if (!list_empty(&i2c->ports)) {
> My gosh, this is done already in list_for_each*()

No, list_for_each_entry does NOT check if the list is empty or if the 
first entry is NULL.

Thanks,
Eddie

>
>> +               list_for_each_entry(port, &i2c->ports, list) {
>> +                       i2c_del_adapter(&port->adapter);
>> +               }
>> +       }
>
>

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Eddie James]

On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Add and initialize I2C adapters for each port on the FSI-attached I2C
>> master. Ports for each master are defined in the devicetree.
>> +#include <linux/of.h>
>
>> +static int fsi_i2c_set_port(struct fsi_i2c_port *port)
>> +{
>> +       int rc;
>> +       struct fsi_device *fsi = port->master->fsi;
>> +       u32 mode, dummy = 0;
>> +       u16 old_port;
>> +
>> +       rc = fsi_i2c_read_reg(fsi, I2C_FSI_MODE, &mode);
>> +       if (rc)
>> +               return rc;
>> +
>> +       old_port = GETFIELD(I2C_MODE_PORT, mode);
>> +
>> +       if (old_port != port->port) {
> Why not simple
>
> if (port->port == GETFIELD())
>    return 0;
>
> ?
>
>> +               /* reset engine when port is changed */
>> +               rc = fsi_i2c_write_reg(fsi, I2C_FSI_RESET_ERR, &dummy);
>> +               if (rc)
>> +                       return rc;
>> +       }
>> +       return rc;
> It's hardly would be non-zero, right?

Sorry, misunderstood your comment here. You are correct, it can only be 
zero.

Thanks,
Eddie

>
>> +}
>>   static int fsi_i2c_probe(struct device *dev)
>>   {
> Isn't below somehow repeats of_i2c_register_devices() ?
> Why not to use it?
>
>> +       /* Add adapter for each i2c port of the master. */
>> +       for_each_available_child_of_node(dev->of_node, np) {
>> +               rc = of_property_read_u32(np, "reg", &port_no);
>> +               if (rc || port_no > USHRT_MAX)
>> +                       continue;
>> +
>> +               port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> +               if (!port)
>> +                       break;
>> +
>> +               port->master = i2c;
>> +               port->port = port_no;
>> +
>> +               port->adapter.owner = THIS_MODULE;
>> +               port->adapter.dev.of_node = np;
>> +               port->adapter.dev.parent = dev;
>> +               port->adapter.algo = &fsi_i2c_algorithm;
>> +               port->adapter.algo_data = port;
>> +
>> +               snprintf(port->adapter.name, sizeof(port->adapter.name),
>> +                        "i2c_bus-%u", port_no);
>> +
>> +               rc = i2c_add_adapter(&port->adapter);
>> +               if (rc < 0) {
>> +                       dev_err(dev, "Failed to register adapter: %d\n", rc);
>> +                       devm_kfree(dev, port);
> This hurts my eyes. Why?!
>
>> +                       continue;
>> +               }
>> +
>> +               list_add(&port->list, &i2c->ports);
>> +       }
>> +
>>          dev_set_drvdata(dev, i2c);
>>
>>          return 0;
>>   }
>> +       if (!list_empty(&i2c->ports)) {
> My gosh, this is done already in list_for_each*()
>
>> +               list_for_each_entry(port, &i2c->ports, list) {
>> +                       i2c_del_adapter(&port->adapter);
>> +               }
>> +       }
>
>

Re: [PATCH v7 5/7] drivers/i2c: Add transfer implementation for FSI algorithm

[Eddie James]

On 05/29/2018 07:08 PM, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Execute I2C transfers from the FSI-attached I2C master. Use polling
>> instead of interrupts as we have no hardware IRQ over FSI.
>> +       if (msg->flags & I2C_M_RD)
>> +               cmd |= I2C_CMD_READ;
> I think we have a helper for this, though not sure.

Didn't see any other I2C drivers using any helper for msg->flags.

>
>> +static int fsi_i2c_write_fifo(struct fsi_i2c_port *port, struct i2c_msg *msg,
>> +                             u8 fifo_count)
>> +{
>> +       int write;
>> +       int rc = 0;
> Redundant assignment.
>
>> +       struct fsi_i2c_master *i2c = port->master;
>> +       int bytes_to_write = i2c->fifo_size - fifo_count;
>> +       int bytes_remaining = msg->len - port->xfrd;
>> +       if (bytes_to_write > bytes_remaining)
>> +               bytes_to_write = bytes_remaining;
> _write = min(_write, _remaining);
>
>> +       while (bytes_to_write > 0) {
>> +               write = bytes_to_write;
>> +               /* fsi limited to max 4 byte aligned ops */
>> +               if (bytes_to_write > 4)
>> +                       write = 4;
>> +               else if (write == 3)
>> +                       write = 2;
> write = min_t(int, 4, rounddown_pow_of_two(bytes_to_write));
>
> Also check it carefully, it might be optimized even more, though I
> didn't think much.

I think it is more readable this way, and I'm not convinced the 
min(rounddown()) is faster. I did however add a common function to do 
this check since it's performed in both the read and write fifo 
functions. Let me know what you think on v8.

Thanks,
Eddie

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Benjamin Herrenschmidt]

On Wed, 2018-05-30 at 10:47 -0500, Eddie James wrote:
> > > +       if (!list_empty(&i2c->ports)) {
> > 
> > My gosh, this is done already in list_for_each*()
> 
> No, list_for_each_entry does NOT check if the list is empty or if the 
> first entry is NULL.

NULL is never valid for a list. It does however check for an empty list

It does it implicitely in the test part of the for () statement,
checking if the next pointer points back to the head.

Cheers,
Ben.

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Andy Shevchenko]

On Wed, May 30, 2018 at 6:47 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
>> On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com>
>> wrote:

>>>   static int fsi_i2c_probe(struct device *dev)
>>>   {
>>
>> Isn't below somehow repeats of_i2c_register_devices() ?
>> Why not to use it?
>
>
> Because I need to assign all these port structure fields. Also looks like
> of_i2c_register_devices creates new devices; I just want an adapter for each
> port.

Hmm... Wolfram, what is your opinion on this design?


>>> +                       devm_kfree(dev, port);
>>
>> This hurts my eyes. Why?!

> What would you suggest instead?

You even didn't wait for answer, why to ask then?
Moreover, you didn't answer to my question. Why are you doing that
call implicitly?

>>> +       if (!list_empty(&i2c->ports)) {
>>
>> My gosh, this is done already in list_for_each*()

> No, list_for_each_entry does NOT check if the list is empty or if the first
> entry is NULL.

Please, read the macro source code again.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Andy Shevchenko]

On Thu, May 31, 2018 at 12:27 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, May 30, 2018 at 6:47 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> On 05/29/2018 06:19 PM, Andy Shevchenko wrote:

>>>> +                       devm_kfree(dev, port);

>>> This hurts my eyes. Why?!
>
>> What would you suggest instead?

> Why are you doing that
> call implicitly?

s/implicitly/explicitly/


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Benjamin Herrenschmidt]

On Thu, 2018-05-31 at 00:27 +0300, Andy Shevchenko wrote:
> On Wed, May 30, 2018 at 6:47 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> > On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
> > > On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com>
> > > wrote:
> > > >   static int fsi_i2c_probe(struct device *dev)
> > > >   {
> > > 
> > > Isn't below somehow repeats of_i2c_register_devices() ?
> > > Why not to use it?
> > 
> > 
> > Because I need to assign all these port structure fields. Also looks like
> > of_i2c_register_devices creates new devices; I just want an adapter for each
> > port.
> 
> Hmm... Wolfram, what is your opinion on this design?

Andy, I don't understand your issue.

of_i2c_register_devices() is about discovering the i2c devices below a
given bus. This is not what is happening here.

This is a driver for a master that supports multiple busses, so it the
above loop creates all the busses.

> > > > +                       devm_kfree(dev, port);
> > > 
> > > This hurts my eyes. Why?!
> > What would you suggest instead?
> 
> You even didn't wait for answer, why to ask then?

Please stop being so rude.

> Moreover, you didn't answer to my question. Why are you doing that
> call implicitly?

"implicitly" ? What's implicit here ? This is just pretty standard
cleanup after failure, you are being very cryptic here.

Please state precisely what it is you dislike with that code instead of
expecting us to guess and being nasty about it. Eddie was a genuine
question, he doesn't see what you think is "hurtful to the eyes" in the
code you quoted.

> > > > +       if (!list_empty(&i2c->ports)) {
> > > 
> > > My gosh, this is done already in list_for_each*()
> > No, list_for_each_entry does NOT check if the list is empty or if the first
> > entry is NULL.
> 
> Please, read the macro source code again.

Cheers,
Ben.

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Andy Shevchenko]

On Thu, May 31, 2018 at 1:34 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Thu, 2018-05-31 at 00:27 +0300, Andy Shevchenko wrote:
>> On Wed, May 30, 2018 at 6:47 PM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
>> > On 05/29/2018 06:19 PM, Andy Shevchenko wrote:
>> > > On Wed, May 30, 2018 at 1:24 AM, Eddie James <eajames@linux.vnet.ibm.com>
>> > > wrote:

>> > > Isn't below somehow repeats of_i2c_register_devices() ?
>> > > Why not to use it?

>> > Because I need to assign all these port structure fields. Also looks like
>> > of_i2c_register_devices creates new devices; I just want an adapter for each
>> > port.
>>
>> Hmm... Wolfram, what is your opinion on this design?
>
> Andy, I don't understand your issue.
>
> of_i2c_register_devices() is about discovering the i2c devices below a
> given bus. This is not what is happening here.
>
> This is a driver for a master that supports multiple busses, so it the
> above loop creates all the busses.

My issue here, that it feels like a lot of duplication with existing approaches.
Though, it might be a right thing to do at the end. So, let's just
assume maintainer will give their point of view.

>> > > > +                       devm_kfree(dev, port);
>> > >
>> > > This hurts my eyes. Why?!
>> > What would you suggest instead?
>>
>> You even didn't wait for answer, why to ask then?
>
> Please stop being so rude.

OK.

>> Moreover, you didn't answer to my question. Why are you doing that
>> call implicitly?
>
> "implicitly" ? What's implicit here ? This is just pretty standard
> cleanup after failure, you are being very cryptic here.
>
> Please state precisely what it is you dislike with that code instead of
> expecting us to guess and being nasty about it. Eddie was a genuine
> question, he doesn't see what you think is "hurtful to the eyes" in the
> code you quoted.

In 99% cases when someone calls devm_kfree() it means wrong choice of
devm_k*alloc() in the first place.
So, with explanation given why it's done in this way I would rather
suggest to switch to plain k*alloc() / kfree().

Or do we really care about few hundreds of bytes wasted?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 3/7] drivers/i2c: Add port structure to FSI algorithm

[Eddie James]

On 05/30/2018 04:16 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2018-05-30 at 10:47 -0500, Eddie James wrote:
>>>> +       if (!list_empty(&i2c->ports)) {
>>> My gosh, this is done already in list_for_each*()
>> No, list_for_each_entry does NOT check if the list is empty or if the
>> first entry is NULL.
> NULL is never valid for a list. It does however check for an empty list
>
> It does it implicitely in the test part of the for () statement,
> checking if the next pointer points back to the head.

Thanks Ben. My mistake on this one; I misread the macro. I will remove 
the check in v9... going to wait a little for any further comments now.

Thanks,
Eddie

>
> Cheers,
> Ben.
>
>