[PATCH v3 0/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller

[PATCH v3 0/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller

[Jayachandran C]

Here is v3 of the driver for XLP9xx I2C controller. We have udpated
the driver with changes for the review from Uwe.

Please let me know if there are any comments or suggestions.

Thanks,
JC.

Changes v2->v3:
* Implement changes from Uwe Kleine-König's review:
  - add documentation link
  - remove unnecessary #includes
  - remove DATARDY interrupt
  - clean up alignment and whitespace
  - set CMD_STOP flag only for the last transfer
  - correct freq assignment in xlp9xx_i2c_get_frequency
  - remove printing out adap.nr in case of failure
  - update writing to control register to make the logic clear
  - clean up the #define ordering and naming a bit more
* use correct type in handling return of wait_for_completion_timeout

Changes v1->v2:
* Implement changes from Ray Jui's review
  - Support for 0 length transfers
  - remove .owner assignment in platform_driver and .data assignment in
    of_device_id table
  - add synchronize_irq and disable interrupt in .remove
  - Move most prints to dev_dbg
  - add COMPILE_TEST
  - align wrapped line in function definitions
  - IRQ_NONE return if interrupt is not ours.
  - error check frequency setting in DTB
  - fix check of return of devm_ioremap_resource() and remove
    unnecessary prints
  - fix incorrect irq check
  - include file ordering fixed
  - few unneeded variables taken out
  - use u32 consistently for unsigned 32 bit int
  - add comment for prescale value calculation

* Change clock-frequency parameter to take the bus frequency
  as expected in i2c subsystem

* Rearrage register definitions to group them better

* Add I2C_FUNC_10BIT_ADDR to xlp9xx_i2c_functionality()

* use reinit_completion before each transfer

* use i2c_add_adapter instead of doing i2c_add_numbered_adapter

Subhendu Sekhar Behera (2):
  of: Add vendor prefix 'netlogic'
  i2c: Support for Netlogic XLP9XX/5XX I2C controller.

 .../devicetree/bindings/i2c/i2c-xlp9xx.txt         |  22 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-xlp9xx.c                    | 446 +++++++++++++++++++++
 5 files changed, 480 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
 create mode 100644 drivers/i2c/busses/i2c-xlp9xx.c

-- 
1.9.1

[PATCH v3 1/2] of: Add vendor prefix 'netlogic'

[Jayachandran C]

From: Subhendu Sekhar Behera <sbehera-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

Add vendor name "netlogic" in vendor-prefixes.txt, which will be used for
the Netlogic XLP and XLPII MIPS SoCs. These processors were from NetLogic
Microsystems which is now part of Broadcom Corporation.

Signed-off-by: Subhendu Sekhar Behera <sbehera-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jayachandran C <jchandra-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 389ca13..a718eb1 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -123,6 +123,7 @@ mxicy	Macronix International Co., Ltd.
 national	National Semiconductor
 neonode		Neonode Inc.
 netgear	NETGEAR
+netlogic	Broadcom Corporation (formerly NetLogic Microsystems)
 newhaven	Newhaven Display International
 nintendo	Nintendo
 nokia	Nokia
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.

[Jayachandran C]

From: Subhendu Sekhar Behera <sbehera-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

Add an I2C bus driver i2c-xlp9xx.c to support the I2C block in the
XLP9xx/XLP5xx MIPS SoC. Update Kconfig and Makefile to add the
CONFIG_I2C_XLP9XX option.

Also add document Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
for DT compatible string "netlogic,xlp9xx-i2c".

Signed-off-by: Subhendu Sekhar Behera <sbehera-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jayachandran C <jchandra-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Ray Jui <rjui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 .../devicetree/bindings/i2c/i2c-xlp9xx.txt         |  22 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-xlp9xx.c                    | 446 +++++++++++++++++++++
 4 files changed, 479 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
 create mode 100644 drivers/i2c/busses/i2c-xlp9xx.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt b/Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
new file mode 100644
index 00000000..f23ae87
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt
@@ -0,0 +1,22 @@
+Device tree configuration for the I2C controller on the XLP9xx/5xx SoC
+
+Required properties:
+- compatible      : should be "netlogic,xlp9xx-i2c"
+- reg             : bus address start and address range size of device
+- interrupts      : interrupt number
+
+Optional properties:
+- clock-frequency : frequency of bus clock in Hz
+                    Defaults to 100 KHz when the property is not specified
+
+Example:
+
+i2c0: i2c@113100 {
+	compatible = "netlogic,xlp9xx-i2c";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0 0x113100 0x100>;
+	clock-frequency = <400000>;
+	interrupts = <30>;
+	interrupt-parent = <&pic>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 22da9c2..dde4648 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -898,6 +898,16 @@ config I2C_XLR
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-xlr.
 
+config I2C_XLP9XX
+	tristate "XLP9XX I2C support"
+	depends on CPU_XLP || COMPILE_TEST
+	help
+	  This driver enables support for the on-chip I2C interface of
+	  the Broadcom XLP9xx/XLP5xx MIPS processors.
+
+	  This driver can also be built as a module.  If so, the module will
+	  be called i2c-xlp9xx.
+
 config I2C_RCAR
 	tristate "Renesas R-Car I2C Controller"
 	depends on ARCH_SHMOBILE || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 3638feb..f8e0f0e 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_I2C_WMT)		+= i2c-wmt.o
 obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
 obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
 obj-$(CONFIG_I2C_XLR)		+= i2c-xlr.o
+obj-$(CONFIG_I2C_XLP9XX)	+= i2c-xlp9xx.o
 obj-$(CONFIG_I2C_RCAR)		+= i2c-rcar.o
 
 # External I2C/SMBus adapter drivers
diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
new file mode 100644
index 00000000..2f303ad
--- /dev/null
+++ b/drivers/i2c/busses/i2c-xlp9xx.c
@@ -0,0 +1,446 @@
+/*
+ * Copyright (c) 2003-2015 Broadcom Corporation
+ *
+ * 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.
+ */
+
+/*
+ * see Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt for device
+ * tree bindings documentation
+ */
+
+#include <linux/completion.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define XLP9XX_I2C_DIV			0x0
+#define XLP9XX_I2C_CTRL			0x1
+#define XLP9XX_I2C_CMD			0x2
+#define XLP9XX_I2C_STATUS		0x3
+#define XLP9XX_I2C_MTXFIFO		0x4
+#define XLP9XX_I2C_MRXFIFO		0x5
+#define XLP9XX_I2C_MFIFOCTRL		0x6
+#define XLP9XX_I2C_STXFIFO		0x7
+#define XLP9XX_I2C_SRXFIFO		0x8
+#define XLP9XX_I2C_SFIFOCTRL		0x9
+#define XLP9XX_I2C_SLAVEADDR		0xA
+#define XLP9XX_I2C_OWNADDR		0xB
+#define XLP9XX_I2C_FIFOWCNT		0xC
+#define XLP9XX_I2C_INTEN		0xD
+#define XLP9XX_I2C_INTST		0xE
+#define XLP9XX_I2C_WAITCNT		0xF
+#define XLP9XX_I2C_TIMEOUT		0X10
+#define XLP9XX_I2C_GENCALLADDR		0x11
+
+#define XLP9XX_I2C_CMD_START		BIT(7)
+#define XLP9XX_I2C_CMD_STOP		BIT(6)
+#define XLP9XX_I2C_CMD_READ		BIT(5)
+#define XLP9XX_I2C_CMD_WRITE		BIT(4)
+#define XLP9XX_I2C_CMD_ACK		BIT(3)
+
+#define XLP9XX_I2C_CTRL_MCTLEN_SHIFT	16
+#define XLP9XX_I2C_CTRL_MCTLEN_MASK	0xffff0000
+#define XLP9XX_I2C_CTRL_RST		BIT(8)
+#define XLP9XX_I2C_CTRL_EN		BIT(6)
+#define XLP9XX_I2C_CTRL_MASTER		BIT(4)
+#define XLP9XX_I2C_CTRL_FIFORD		BIT(1)
+#define XLP9XX_I2C_CTRL_ADDMODE		BIT(0)
+
+#define XLP9XX_I2C_INTEN_NACKADDR	BIT(25)
+#define XLP9XX_I2C_INTEN_SADDR		BIT(13)
+#define XLP9XX_I2C_INTEN_DATADONE	BIT(12)
+#define XLP9XX_I2C_INTEN_ARLOST		BIT(11)
+#define XLP9XX_I2C_INTEN_MFIFOFULL	BIT(4)
+#define XLP9XX_I2C_INTEN_MFIFOEMTY	BIT(3)
+#define XLP9XX_I2C_INTEN_MFIFOHI	BIT(2)
+#define XLP9XX_I2C_INTEN_BUSERR		BIT(0)
+
+#define XLP9XX_I2C_MFIFOCTRL_HITH_SHIFT		8
+#define XLP9XX_I2C_MFIFOCTRL_LOTH_SHIFT		0
+#define XLP9XX_I2C_MFIFOCTRL_RST		BIT(16)
+
+#define XLP9XX_I2C_SLAVEADDR_RW			BIT(0)
+#define XLP9XX_I2C_SLAVEADDR_ADDR_SHIFT		1
+
+#define XLP9XX_I2C_IP_CLK_FREQ		133000000UL
+#define XLP9XX_I2C_DEFAULT_FREQ		100000
+#define XLP9XX_I2C_HIGH_FREQ		400000
+#define XLP9XX_I2C_FIFO_SIZE		0x80U
+#define XLP9XX_I2C_TIMEOUT_MS		1000
+
+#define XLP9XX_I2C_FIFO_WCNT_MASK	0xff
+#define XLP9XX_I2C_STATUS_ERRMASK	(XLP9XX_I2C_INTEN_ARLOST | \
+			XLP9XX_I2C_INTEN_NACKADDR | XLP9XX_I2C_INTEN_BUSERR)
+
+struct xlp9xx_i2c_dev {
+	struct device *dev;
+	struct i2c_adapter adapter;
+	struct completion msg_complete;
+	int irq;
+	bool msg_read;
+	u32 __iomem *base;
+	u32 msg_buf_remaining;
+	u32 msg_len;
+	u32 clk_hz;
+	u32 msg_err;
+	u8 *msg_buf;
+};
+
+static inline void xlp9xx_write_i2c_reg(struct xlp9xx_i2c_dev *priv,
+					unsigned long reg, u32 val)
+{
+	writel(val, priv->base + reg);
+}
+
+static inline u32 xlp9xx_read_i2c_reg(struct xlp9xx_i2c_dev *priv,
+				      unsigned long reg)
+{
+	return readl(priv->base + reg);
+}
+
+static void xlp9xx_i2c_mask_irq(struct xlp9xx_i2c_dev *priv, u32 mask)
+{
+	mask = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTEN) & ~mask;
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, mask);
+}
+
+static void xlp9xx_i2c_unmask_irq(struct xlp9xx_i2c_dev *priv, u32 mask)
+{
+	mask = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTEN) | mask;
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, mask);
+}
+
+static void xlp9xx_i2c_set_rx_fifo_thres(struct xlp9xx_i2c_dev *priv,
+					 u32 th)
+{
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_MFIFOCTRL,
+			     (th << XLP9XX_I2C_MFIFOCTRL_HITH_SHIFT));
+}
+
+static void xlp9xx_i2c_fill_tx_fifo(struct xlp9xx_i2c_dev *priv)
+{
+	u32 len, i;
+	u8 *buf = priv->msg_buf;
+
+	len = min(priv->msg_buf_remaining, XLP9XX_I2C_FIFO_SIZE);
+	for (i = 0; i < len; i++)
+		xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_MTXFIFO, buf[i]);
+	priv->msg_buf_remaining -= len;
+	priv->msg_buf += len;
+}
+
+static void xlp9xx_i2c_drain_rx_fifo(struct xlp9xx_i2c_dev *priv)
+{
+	u32 len, i;
+	u8 *buf = priv->msg_buf;
+
+	len = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_FIFOWCNT) &
+				  XLP9XX_I2C_FIFO_WCNT_MASK;
+	len = min(priv->msg_buf_remaining, len);
+	for (i = 0; i < len; i++, buf++)
+		*buf = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_MRXFIFO);
+
+	priv->msg_buf_remaining -= len;
+	priv->msg_buf = buf;
+
+	if (priv->msg_buf_remaining)
+		xlp9xx_i2c_set_rx_fifo_thres(priv,
+			min(priv->msg_buf_remaining, XLP9XX_I2C_FIFO_SIZE));
+}
+
+static irqreturn_t xlp9xx_i2c_isr(int irq, void *dev_id)
+{
+	struct xlp9xx_i2c_dev *priv = dev_id;
+	u32 status;
+
+	status = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTST);
+	if (status == 0)
+		return IRQ_NONE;
+
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTST, status);
+	if (status & XLP9XX_I2C_STATUS_ERRMASK) {
+		priv->msg_err = status;
+		goto xfer_done;
+	}
+
+	/* SADDR ACK for SMBUS_QUICK */
+	if ((status & XLP9XX_I2C_INTEN_SADDR) && (priv->msg_len == 0))
+		goto xfer_done;
+
+	if (!priv->msg_read) {
+		if (status & XLP9XX_I2C_INTEN_MFIFOEMTY) {
+			/* TX FIFO got empty, fill it up again */
+			if (priv->msg_buf_remaining)
+				xlp9xx_i2c_fill_tx_fifo(priv);
+			else
+				xlp9xx_i2c_mask_irq(priv,
+						    XLP9XX_I2C_INTEN_MFIFOEMTY);
+		}
+	} else {
+		if (status & (XLP9XX_I2C_INTEN_DATADONE |
+			      XLP9XX_I2C_INTEN_MFIFOHI)) {
+			/* data is in FIFO, read it */
+			if (priv->msg_buf_remaining)
+				xlp9xx_i2c_drain_rx_fifo(priv);
+		}
+	}
+
+	/* Transfer complete */
+	if (status & XLP9XX_I2C_INTEN_DATADONE)
+		goto xfer_done;
+
+	return IRQ_HANDLED;
+
+xfer_done:
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, 0);
+	complete(&priv->msg_complete);
+	return IRQ_HANDLED;
+}
+
+static int xlp9xx_i2c_init(struct xlp9xx_i2c_dev *priv)
+{
+	u32 prescale;
+
+	/*
+	 * The controller uses 5 * SCL clock internally.
+	 * So prescale value should be divided by 5.
+	 */
+	prescale = (((XLP9XX_I2C_IP_CLK_FREQ / priv->clk_hz) - 8) / 5) - 1;
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_RST);
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_EN |
+			     XLP9XX_I2C_CTRL_MASTER);
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_DIV, prescale);
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, 0);
+
+	return 0;
+}
+
+static int xlp9xx_i2c_xfer_msg(struct xlp9xx_i2c_dev *priv, struct i2c_msg *msg,
+			       int last_msg)
+{
+	unsigned long timeleft;
+	u32 intr_mask, cmd, val;
+
+	priv->msg_buf = msg->buf;
+	priv->msg_buf_remaining = priv->msg_len = msg->len;
+	priv->msg_err = 0;
+	priv->msg_read = (msg->flags & I2C_M_RD);
+	reinit_completion(&priv->msg_complete);
+
+	/* Reset FIFO */
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_MFIFOCTRL,
+			     XLP9XX_I2C_MFIFOCTRL_RST);
+
+	/* set FIFO threshold if reading */
+	if (priv->msg_read) {
+		val = min(priv->msg_buf_remaining, XLP9XX_I2C_FIFO_SIZE);
+		xlp9xx_i2c_set_rx_fifo_thres(priv, val);
+	}
+
+	/* set slave addr */
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_SLAVEADDR,
+			     (msg->addr << XLP9XX_I2C_SLAVEADDR_ADDR_SHIFT) |
+			     (priv->msg_read ? XLP9XX_I2C_SLAVEADDR_RW : 0));
+
+	/* Build control word for transfer */
+	val = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_CTRL);
+	if (!priv->msg_read)
+		val &= ~XLP9XX_I2C_CTRL_FIFORD;
+	else
+		val |= XLP9XX_I2C_CTRL_FIFORD;	/* read */
+
+	if (msg->flags & I2C_M_TEN)
+		val |= XLP9XX_I2C_CTRL_ADDMODE;	/* 10-bit address mode*/
+	else
+		val &= ~XLP9XX_I2C_CTRL_ADDMODE;
+
+	/* set data length to be transferred */
+	val = (val & ~XLP9XX_I2C_CTRL_MCTLEN_MASK) |
+	      (msg->len << XLP9XX_I2C_CTRL_MCTLEN_SHIFT);
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, val);
+
+	/* fill fifo during tx */
+	if (!priv->msg_read)
+		xlp9xx_i2c_fill_tx_fifo(priv);
+
+	/* set interrupt mask */
+	intr_mask = (XLP9XX_I2C_INTEN_ARLOST | XLP9XX_I2C_INTEN_BUSERR |
+		     XLP9XX_I2C_INTEN_NACKADDR | XLP9XX_I2C_INTEN_DATADONE);
+
+	if (priv->msg_read) {
+		intr_mask |= XLP9XX_I2C_INTEN_MFIFOHI;
+		if (msg->len == 0)
+			intr_mask |= XLP9XX_I2C_INTEN_SADDR;
+	} else {
+		if (msg->len == 0)
+			intr_mask |= XLP9XX_I2C_INTEN_SADDR;
+		else
+			intr_mask |= XLP9XX_I2C_INTEN_MFIFOEMTY;
+	}
+	xlp9xx_i2c_unmask_irq(priv, intr_mask);
+
+	/* set cmd reg */
+	cmd = XLP9XX_I2C_CMD_START;
+	cmd |= (priv->msg_read ? XLP9XX_I2C_CMD_READ : XLP9XX_I2C_CMD_WRITE);
+	if (last_msg)
+		cmd |= XLP9XX_I2C_CMD_STOP;
+
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CMD, cmd);
+
+	timeleft = msecs_to_jiffies(XLP9XX_I2C_TIMEOUT_MS);
+	timeleft = wait_for_completion_timeout(&priv->msg_complete, timeleft);
+
+	if (priv->msg_err) {
+		dev_dbg(priv->dev, "transfer error %x!\n", priv->msg_err);
+		if (priv->msg_err & XLP9XX_I2C_INTEN_BUSERR)
+			xlp9xx_i2c_init(priv);
+		return -EAGAIN;
+	}
+
+	if (timeleft == 0) {
+		dev_dbg(priv->dev, "i2c transfer timed out!\n");
+		xlp9xx_i2c_init(priv);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int xlp9xx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			   int num)
+{
+	int i, ret;
+	struct xlp9xx_i2c_dev *priv = i2c_get_adapdata(adap);
+
+	for (i = 0; i < num; i++) {
+		ret = xlp9xx_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
+		if (ret != 0)
+			return ret;
+	}
+
+	return num;
+}
+
+static u32 xlp9xx_i2c_functionality(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C |
+		I2C_FUNC_10BIT_ADDR;
+}
+
+static struct i2c_algorithm xlp9xx_i2c_algo = {
+	.master_xfer = xlp9xx_i2c_xfer,
+	.functionality = xlp9xx_i2c_functionality,
+};
+
+static int xlp9xx_i2c_get_frequency(struct platform_device *pdev,
+				    struct xlp9xx_i2c_dev *priv)
+{
+	struct device_node *np = pdev->dev.of_node;
+	u32 freq;
+	int err;
+
+	err = of_property_read_u32(np, "clock-frequency", &freq);
+	if (err) {
+		freq = XLP9XX_I2C_DEFAULT_FREQ;
+		dev_dbg(&pdev->dev, "using default frequency %u\n", freq);
+	} else if (freq == 0 || freq > XLP9XX_I2C_HIGH_FREQ) {
+		dev_warn(&pdev->dev, "invalid frequency %u, using default\n",
+			 freq);
+		freq = XLP9XX_I2C_DEFAULT_FREQ;
+	}
+	priv->clk_hz = freq;
+
+	return 0;
+}
+
+static int xlp9xx_i2c_probe(struct platform_device *pdev)
+{
+	struct xlp9xx_i2c_dev *priv;
+	struct resource *res;
+	int err = 0;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq <= 0) {
+		dev_err(&pdev->dev, "invalid irq!\n");
+		return priv->irq;
+	}
+
+	xlp9xx_i2c_get_frequency(pdev, priv);
+	xlp9xx_i2c_init(priv);
+
+	err = devm_request_irq(&pdev->dev, priv->irq, xlp9xx_i2c_isr, 0,
+			       pdev->name, priv);
+	if (err) {
+		dev_err(&pdev->dev, "IRQ request failed!\n");
+		return err;
+	}
+
+	init_completion(&priv->msg_complete);
+	priv->adapter.dev.parent = &pdev->dev;
+	priv->adapter.algo = &xlp9xx_i2c_algo;
+	priv->adapter.dev.of_node = pdev->dev.of_node;
+	priv->dev = &pdev->dev;
+
+	snprintf(priv->adapter.name, sizeof(priv->adapter.name), "xlp9xx-i2c");
+	i2c_set_adapdata(&priv->adapter, priv);
+
+	err = i2c_add_adapter(&priv->adapter);
+	if (err) {
+		dev_err(&pdev->dev, "failed to add I2C adapter!\n");
+		return err;
+	}
+
+	platform_set_drvdata(pdev, priv);
+	dev_dbg(&pdev->dev, "I2C bus:%d added\n", priv->adapter.nr);
+
+	return 0;
+}
+
+static int xlp9xx_i2c_remove(struct platform_device *pdev)
+{
+	struct xlp9xx_i2c_dev *priv;
+
+	priv = platform_get_drvdata(pdev);
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, 0);
+	synchronize_irq(priv->irq);
+	i2c_del_adapter(&priv->adapter);
+	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, 0);
+
+	return 0;
+}
+
+static const struct of_device_id xlp9xx_i2c_of_match[] = {
+	{ .compatible = "netlogic,xlp9xx-i2c", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver xlp9xx_i2c_driver = {
+	.probe = xlp9xx_i2c_probe,
+	.remove = xlp9xx_i2c_remove,
+	.driver = {
+		.name = "xlp9xx-i2c",
+		.of_match_table = xlp9xx_i2c_of_match,
+	},
+};
+
+module_platform_driver(xlp9xx_i2c_driver);
+
+MODULE_AUTHOR("Subhendu Sekhar Behera <sbehera-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>");
+MODULE_DESCRIPTION("XLP9XX/5XX I2C Bus Controller Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v3 2/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.

[Uwe Kleine-König]

hello,

On Fri, Mar 13, 2015 at 11:59:58AM +0530, Jayachandran C wrote:
> diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
> new file mode 100644
> index 00000000..2f303ad
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-xlp9xx.c
> @@ -0,0 +1,446 @@
> +/*
> + * Copyright (c) 2003-2015 Broadcom Corporation
> + *
> + * 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.
> + */
> +
> +/*
> + * see Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt for device
> + * tree bindings documentation
> + */
When I asked for documentation here, I didn't meant the device tree
binding, but the hardware reference manual.

> [...]
> +static void xlp9xx_i2c_mask_irq(struct xlp9xx_i2c_dev *priv, u32 mask)
> +{
> +	mask = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTEN) & ~mask;
> +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, mask);
You didn't comment on my suggestion to add a variable here for improved
readability:

	u32 inten = xlp9xx_read_i2c_reg(...);
	inten &= ~mask;
	xlp9xx_write_i2c_reg(...);

> +}
> +
> +static void xlp9xx_i2c_unmask_irq(struct xlp9xx_i2c_dev *priv, u32 mask)
> +{
> +	mask = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTEN) | mask;
> +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, mask);
> +}
> +
> +static void xlp9xx_i2c_set_rx_fifo_thres(struct xlp9xx_i2c_dev *priv,
> +					 u32 th)
> +{
> +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_MFIFOCTRL,
> +			     (th << XLP9XX_I2C_MFIFOCTRL_HITH_SHIFT));
parenthesis are not needed here.

I suggested to move the min calculation that you have to do before each
call to this function into it. You replied:

	The xlp9xx_i2c_... functions were written to do the
	hardware/register operations, so it is better to have this logic
	here.

...

> +}
> +
> +static void xlp9xx_i2c_fill_tx_fifo(struct xlp9xx_i2c_dev *priv)
> +{
> +	u32 len, i;
> +	u8 *buf = priv->msg_buf;
> +
> +	len = min(priv->msg_buf_remaining, XLP9XX_I2C_FIFO_SIZE);
... this didn't stop you here though :-) So I'm still convinced that
having the min function in xlp9xx_i2c_set_rx_fifo_thres is a good idea.

> +	for (i = 0; i < len; i++)
> +		xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_MTXFIFO, buf[i]);
> +	priv->msg_buf_remaining -= len;
> +	priv->msg_buf += len;
> +}
> [...]
> +static int xlp9xx_i2c_init(struct xlp9xx_i2c_dev *priv)
> +{
> +	u32 prescale;
> +
> +	/*
> +	 * The controller uses 5 * SCL clock internally.
> +	 * So prescale value should be divided by 5.
> +	 */
> +	prescale = (((XLP9XX_I2C_IP_CLK_FREQ / priv->clk_hz) - 8) / 5) - 1;
I still wonder if you should round differently here.

> +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_RST);
> +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_EN |
> +			     XLP9XX_I2C_CTRL_MASTER);
> +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_DIV, prescale);
> +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, 0);
> +
> +	return 0;
> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 2/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.

[Uwe Kleine-König]

Hello,

On Sat, Mar 14, 2015 at 05:18:37PM +0530, Jayachandran C. wrote:
> On Fri, Mar 13, 2015 at 11:24:06AM +0100, Uwe Kleine-König wrote:
> > On Fri, Mar 13, 2015 at 11:59:58AM +0530, Jayachandran C wrote:
> > > diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
> > > new file mode 100644
> > > index 00000000..2f303ad
> > > --- /dev/null
> > > +++ b/drivers/i2c/busses/i2c-xlp9xx.c
> > > @@ -0,0 +1,446 @@
> > > [...]
> > > +
> > > +/*
> > > + * see Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt for device
> > > + * tree bindings documentation
> > > + */
> > When I asked for documentation here, I didn't meant the device tree
> > binding, but the hardware reference manual.
> 
> Unfortunately there is no standalone documentation for the I2C controller,
> this block is used in XLP9xx and XLP5xx SoCs and you can get the documentation
> for the whole SoC from support.broadcom.com if you have an account there.
I don't. According to the instructions to get such an account I have to
contact my "Sales/Engineering contacts at Broadcom or its
Distributors/Manufacturer's representatives directly". Who would that be
for me? You?

> > > [...]
> > > +static void xlp9xx_i2c_mask_irq(struct xlp9xx_i2c_dev *priv, u32 mask)
> > > +{
> > > +	mask = xlp9xx_read_i2c_reg(priv, XLP9XX_I2C_INTEN) & ~mask;
> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, mask);
> > You didn't comment on my suggestion to add a variable here for improved
> > readability:
> > 
> > 	u32 inten = xlp9xx_read_i2c_reg(...);
> > 	inten &= ~mask;
> > 	xlp9xx_write_i2c_reg(...);
> > 
> 
> Thought it was not really needed, but it is a little better than the
> current code, will update.
Yeah, for the generated code it probably doesn't make much difference---if
at all. Readability and so maintainability is a good reason to update
code if there is no overhead in the compiled result. (And sometimes even
although there is overhead.)

> > > [...]
> > > +static int xlp9xx_i2c_init(struct xlp9xx_i2c_dev *priv)
> > > +{
> > > +	u32 prescale;
> > > +
> > > +	/*
> > > +	 * The controller uses 5 * SCL clock internally.
> > > +	 * So prescale value should be divided by 5.
> > > +	 */
> > > +	prescale = (((XLP9XX_I2C_IP_CLK_FREQ / priv->clk_hz) - 8) / 5) - 1;
> > I still wonder if you should round differently here.
> 
> Are you thinking of making sure that we don't exceed the given clock freq
> because of rounding? I don't think this is an issue.
Right, that and also in the other direction not to give up speed because
the calculation is not optimal.

> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_RST);
> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_CTRL, XLP9XX_I2C_CTRL_EN |
> > > +			     XLP9XX_I2C_CTRL_MASTER);
> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_DIV, prescale);
> > > +	xlp9xx_write_i2c_reg(priv, XLP9XX_I2C_INTEN, 0);
> > > +
> > > +	return 0;
> > > +}
> 
> I will post a v4 (which takes care of another comment I have seen in this
> thread as well).
BTW, I fully agree to Arnd's request to use the full SoC's name in the
compatible string. Pick the first SoC to fix the name, and for the later
SoCs use both strings. This method proved to be flexible enough to work
even if Broadcom might release a SoC next year with a name that
matches 9xx but an incompatible i2c device.
For Freescale devices it works exactly like this. There are (to the best
of our knowledge) 3 similar implementations. On i.MX51 the device is
used that first appeared in the i.MX21 and imx51.dtsi has:

	compatible = "fsl,imx51-i2c", "fsl,imx21-i2c";

.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 2/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.

[Uwe Kleine-König]

Hello,

On Tue, Mar 17, 2015 at 08:00:40PM +0530, Jayachandran C. wrote:
> On Sat, Mar 14, 2015 at 09:01:24PM +0100, Uwe Kleine-König wrote:
> > On Sat, Mar 14, 2015 at 05:18:37PM +0530, Jayachandran C. wrote:
> > > On Fri, Mar 13, 2015 at 11:24:06AM +0100, Uwe Kleine-König wrote:
> > > > On Fri, Mar 13, 2015 at 11:59:58AM +0530, Jayachandran C wrote:
> > > > > diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
> > > > > new file mode 100644
> > > > > index 00000000..2f303ad
> > > > > --- /dev/null
> > > > > +++ b/drivers/i2c/busses/i2c-xlp9xx.c
> > > > > @@ -0,0 +1,446 @@
> > > > > [...]
> > > > > +
> > > > > +/*
> > > > > + * see Documentation/devicetree/bindings/i2c/i2c-xlp9xx.txt for device
> > > > > + * tree bindings documentation
> > > > > + */
> > > > When I asked for documentation here, I didn't meant the device tree
> > > > binding, but the hardware reference manual.
> > > 
> > > Unfortunately there is no standalone documentation for the I2C controller,
> > > this block is used in XLP9xx and XLP5xx SoCs and you can get the documentation
> > > for the whole SoC from support.broadcom.com if you have an account there.
> > I don't. According to the instructions to get such an account I have to
> > contact my "Sales/Engineering contacts at Broadcom or its
> > Distributors/Manufacturer's representatives directly". Who would that be
> > for me? You?
> 
> Not really. If you want to develop drivers for a XLP9xx board you
> have, then you should have an account at support site and the
> instructions apply.
I don't want to develop drivers for such a board, among other reasons
because I don't own one. But I want to make a review of such a driver.
And therefor a datasheet would be helpful.
 
> The rationale given in device tree guidelines is that the silicon vendor
> may release future chips which break the wildcard. In this specific case
> that issue will not happen, because the driver is from the silicon vendor.
> Anyway, for consistency, we will take out the wildcard in v4.
I honestly hope the high trust in your employer is justified. It didn't
work for me (for a different employer of course).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3 2/2] i2c: Support for Netlogic XLP9XX/5XX I2C controller.

[Arnd Bergmann]

On Friday 13 March 2015 11:59:58 Jayachandran C wrote:
> +- compatible      : should be "netlogic,xlp9xx-i2c"
> 

Use a real model number here, not a wildcard.

Also, please send the binding as a separate patch from the driver.

	Arnd