Re: [PATCH v4 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family

[Andy Shevchenko]

On Mon, Feb 6, 2017 at 5:26 AM, Baoyou Xie  wrote:
> This patch adds i2c controller driver for ZTE's zx2967 family.

> +#define I2C_STOP   0
> +#define I2C_MASTER BIT(0)
> +#define I2C_ADDR_MODE_TEN  BIT(1)
> +#define I2C_IRQ_MSK_ENABLE BIT(3)
> +#define I2C_RW_READBIT(4)
> +#define I2C_CMB_RW_EN  BIT(5)
> +#define I2C_START  BIT(6)

> +#define I2C_ADDR_MODE_TEN  BIT(1)

I'm not sure you have to repeat this.

> +#define I2C_WFIFO_RESETBIT(7)
> +#define I2C_RFIFO_RESETBIT(7)

Hmm... Are they applied to the same register?

> +struct zx2967_i2c_info {
> +   spinlock_t  lock;

> +   struct device   *dev;
> +   struct i2c_adapter  adap;

I'm pretty sure you may access *dev from adap. Or they are different devices?

> +   struct clk  *clk;
> +   struct completion   complete;
> +   u32 clk_freq;
> +   void __iomem*reg_base;
> +   size_t  residue;
> +   int irq;
> +   int msg_rd;
> +   u8  *buf;
> +   u8  access_cnt;
> +   boolis_suspended;
> +};


> +static void zx2967_i2c_flush_fifos(struct zx2967_i2c_info *zx_i2c)
> +{

> +   u32 val;
> +   u32 offset;

Reversed tree?

> +
> +   if (zx_i2c->msg_rd) {
> +   offset = REG_RDCONF;
> +   val = I2C_RFIFO_RESET;
> +   } else {
> +   offset = REG_WRCONF;
> +   val = I2C_WFIFO_RESET;
> +   }
> +
> +   val |= zx2967_i2c_readl(zx_i2c, offset);
> +   zx2967_i2c_writel(zx_i2c, val, offset);
> +}

> +   zx2967_i2c_readsb(zx_i2c, val, REG_DATA, size);
> +   for (i = 0; i < size; i++) {

> +   *(zx_i2c->buf++) = val[i];

Do you need parens? I guess *p++ = x; is quite understandable pattern.

> +   zx_i2c->residue--;
> +   if (zx_i2c->residue <= 0)
> +   break;
> +   }
> +
> +   barrier();
> +
> +   return 0;
> +}

> +static int zx2967_i2c_fill_tx_fifo(struct zx2967_i2c_info *zx_i2c)
> +{

> +   u8 *buf = zx_i2c->buf;
> +   size_t residue = zx_i2c->residue;

Reversed tree?

> +
> +   if (residue == 0) {
> +   dev_err(zx_i2c->dev, "residue is %d\n", (int)residue);
> +   return -EINVAL;
> +   }

> +static void zx2967_enable_tenbit(struct zx2967_i2c_info *zx_i2c, __u16 addr)
> +{
> +   u16 val = (addr >> 7) & 0x7;

Magic values.

> +   if (val > 0) {

It can't be negative ->

if (val) {

> +   zx2967_i2c_writel(zx_i2c, val, REG_DEVADDR_H);
> +   val = (zx2967_i2c_readl(zx_i2c, REG_CMD)) | I2C_ADDR_MODE_TEN;
> +   zx2967_i2c_writel(zx_i2c, val, REG_CMD);
> +   }
> +}

> +static int zx2967_i2c_xfer(struct i2c_adapter *adap,
> +  struct i2c_msg *msgs, int num)
> +{
> +   struct zx2967_i2c_info *zx_i2c = i2c_get_adapdata(adap);
> +   int ret;
> +   int i;
> +
> +   if (zx_i2c->is_suspended)
> +   return -EBUSY;
> +
> +   zx2967_i2c_writel(zx_i2c, (msgs->addr & 0x7f), REG_DEVADDR_L);
> +   zx2967_i2c_writel(zx_i2c, (msgs->addr >> 7) & 0x7, REG_DEVADDR_H);
> +   if (zx2967_i2c_readl(zx_i2c, REG_DEVADDR_H) > 0)
> +   zx2967_enable_tenbit(zx_i2c, msgs->addr);
> +
> +   for (i = 0; i < num; i++) {
> +   ret = zx2967_i2c_xfer_msg(zx_i2c, [i]);
> +   if (ret)
> +   return ret;

> +   if (num > 1)

Would it be drastic performance impact if you remove this condition
and do sleep unconditionally?

> +   usleep_range(1000, 2000);

Why do you need this in any case? Comment, please. Do this for every
non-commented *sleep() call in this driver.
(You may define minimum sleep range, put comment there and use it in
those *sleep() calls)

> +   }
> +
> +   return num;
> +}

> +#ifdef CONFIG_PM
> +static const struct dev_pm_ops zx2967_i2c_dev_pm_ops = {
> +   .suspend= zx2967_i2c_suspend,
> +   .resume = zx2967_i2c_resume,
> +};
> +#define ZX2967_I2C_DEV_PM_OPS  (_i2c_dev_pm_ops)
> +#else
> +#defineZX2967_I2C_DEV_PM_OPS   NULL
> +#endif

Remove these ugly #ifdef:s There are suitable macros are available in
pm.h. Like SIMPLE_PM_OPS().

> +static int zx2967_i2c_probe(struct platform_device *pdev)
> +{
> +   struct zx2967_i2c_info *zx_i2c;
> +   void __iomem *reg_base;
> +   struct resource *res;
> +   struct clk *clk;
> +   int ret;

> +   ret = device_property_read_u32(>dev, "clock-frequency",
> +  _i2c->clk_freq);
> +   if (ret) {
> +   dev_err(>dev, "missing 

Re: [PATCH v4 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family

[Andy Shevchenko]

On Mon, Feb 6, 2017 at 5:26 AM, Baoyou Xie  wrote:
> This patch adds i2c controller driver for ZTE's zx2967 family.

> +#define I2C_STOP   0
> +#define I2C_MASTER BIT(0)
> +#define I2C_ADDR_MODE_TEN  BIT(1)
> +#define I2C_IRQ_MSK_ENABLE BIT(3)
> +#define I2C_RW_READBIT(4)
> +#define I2C_CMB_RW_EN  BIT(5)
> +#define I2C_START  BIT(6)

> +#define I2C_ADDR_MODE_TEN  BIT(1)

I'm not sure you have to repeat this.

> +#define I2C_WFIFO_RESETBIT(7)
> +#define I2C_RFIFO_RESETBIT(7)

Hmm... Are they applied to the same register?

> +struct zx2967_i2c_info {
> +   spinlock_t  lock;

> +   struct device   *dev;
> +   struct i2c_adapter  adap;

I'm pretty sure you may access *dev from adap. Or they are different devices?

> +   struct clk  *clk;
> +   struct completion   complete;
> +   u32 clk_freq;
> +   void __iomem*reg_base;
> +   size_t  residue;
> +   int irq;
> +   int msg_rd;
> +   u8  *buf;
> +   u8  access_cnt;
> +   boolis_suspended;
> +};


> +static void zx2967_i2c_flush_fifos(struct zx2967_i2c_info *zx_i2c)
> +{

> +   u32 val;
> +   u32 offset;

Reversed tree?

> +
> +   if (zx_i2c->msg_rd) {
> +   offset = REG_RDCONF;
> +   val = I2C_RFIFO_RESET;
> +   } else {
> +   offset = REG_WRCONF;
> +   val = I2C_WFIFO_RESET;
> +   }
> +
> +   val |= zx2967_i2c_readl(zx_i2c, offset);
> +   zx2967_i2c_writel(zx_i2c, val, offset);
> +}

> +   zx2967_i2c_readsb(zx_i2c, val, REG_DATA, size);
> +   for (i = 0; i < size; i++) {

> +   *(zx_i2c->buf++) = val[i];

Do you need parens? I guess *p++ = x; is quite understandable pattern.

> +   zx_i2c->residue--;
> +   if (zx_i2c->residue <= 0)
> +   break;
> +   }
> +
> +   barrier();
> +
> +   return 0;
> +}

> +static int zx2967_i2c_fill_tx_fifo(struct zx2967_i2c_info *zx_i2c)
> +{

> +   u8 *buf = zx_i2c->buf;
> +   size_t residue = zx_i2c->residue;

Reversed tree?

> +
> +   if (residue == 0) {
> +   dev_err(zx_i2c->dev, "residue is %d\n", (int)residue);
> +   return -EINVAL;
> +   }

> +static void zx2967_enable_tenbit(struct zx2967_i2c_info *zx_i2c, __u16 addr)
> +{
> +   u16 val = (addr >> 7) & 0x7;

Magic values.

> +   if (val > 0) {

It can't be negative ->

if (val) {

> +   zx2967_i2c_writel(zx_i2c, val, REG_DEVADDR_H);
> +   val = (zx2967_i2c_readl(zx_i2c, REG_CMD)) | I2C_ADDR_MODE_TEN;
> +   zx2967_i2c_writel(zx_i2c, val, REG_CMD);
> +   }
> +}

> +static int zx2967_i2c_xfer(struct i2c_adapter *adap,
> +  struct i2c_msg *msgs, int num)
> +{
> +   struct zx2967_i2c_info *zx_i2c = i2c_get_adapdata(adap);
> +   int ret;
> +   int i;
> +
> +   if (zx_i2c->is_suspended)
> +   return -EBUSY;
> +
> +   zx2967_i2c_writel(zx_i2c, (msgs->addr & 0x7f), REG_DEVADDR_L);
> +   zx2967_i2c_writel(zx_i2c, (msgs->addr >> 7) & 0x7, REG_DEVADDR_H);
> +   if (zx2967_i2c_readl(zx_i2c, REG_DEVADDR_H) > 0)
> +   zx2967_enable_tenbit(zx_i2c, msgs->addr);
> +
> +   for (i = 0; i < num; i++) {
> +   ret = zx2967_i2c_xfer_msg(zx_i2c, [i]);
> +   if (ret)
> +   return ret;

> +   if (num > 1)

Would it be drastic performance impact if you remove this condition
and do sleep unconditionally?

> +   usleep_range(1000, 2000);

Why do you need this in any case? Comment, please. Do this for every
non-commented *sleep() call in this driver.
(You may define minimum sleep range, put comment there and use it in
those *sleep() calls)

> +   }
> +
> +   return num;
> +}

> +#ifdef CONFIG_PM
> +static const struct dev_pm_ops zx2967_i2c_dev_pm_ops = {
> +   .suspend= zx2967_i2c_suspend,
> +   .resume = zx2967_i2c_resume,
> +};
> +#define ZX2967_I2C_DEV_PM_OPS  (_i2c_dev_pm_ops)
> +#else
> +#defineZX2967_I2C_DEV_PM_OPS   NULL
> +#endif

Remove these ugly #ifdef:s There are suitable macros are available in
pm.h. Like SIMPLE_PM_OPS().

> +static int zx2967_i2c_probe(struct platform_device *pdev)
> +{
> +   struct zx2967_i2c_info *zx_i2c;
> +   void __iomem *reg_base;
> +   struct resource *res;
> +   struct clk *clk;
> +   int ret;

> +   ret = device_property_read_u32(>dev, "clock-frequency",
> +  _i2c->clk_freq);
> +   if (ret) {
> +   dev_err(>dev, "missing clock-frequency");
> +

Re: [PATCH v4 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family

[Shawn Guo]

On Mon, Feb 06, 2017 at 11:28:20AM +0800, Baoyou Xie wrote:
> + Shawn
> 
> On 6 February 2017 at 11:26, Baoyou Xie  wrote:
> 
> > This patch adds i2c controller driver for ZTE's zx2967 family.
> >
> > Signed-off-by: Baoyou Xie 

Reviewed-by: Shawn Guo 

Re: [PATCH v4 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family

[Shawn Guo]

On Mon, Feb 06, 2017 at 11:28:20AM +0800, Baoyou Xie wrote:
> + Shawn
> 
> On 6 February 2017 at 11:26, Baoyou Xie  wrote:
> 
> > This patch adds i2c controller driver for ZTE's zx2967 family.
> >
> > Signed-off-by: Baoyou Xie 

Reviewed-by: Shawn Guo 

[PATCH v4 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family

[Baoyou Xie]

This patch adds i2c controller driver for ZTE's zx2967 family.

Signed-off-by: Baoyou Xie 
---
 drivers/i2c/busses/Kconfig  |   9 +
 drivers/i2c/busses/Makefile |   1 +
 drivers/i2c/busses/i2c-zx2967.c | 688 
 3 files changed, 698 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-zx2967.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e4a603e..d983e36 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1246,4 +1246,13 @@ config I2C_OPAL
  This driver can also be built as a module. If so, the module will be
  called as i2c-opal.
 
+config I2C_ZX2967
+   tristate "ZTE zx2967 I2C support"
+   depends on ARCH_ZX
+   default y
+   help
+ Selecting this option will add ZX2967 I2C driver.
+ This driver can also be built as a module. If so, the module will be
+ called i2c-zx2967.
+
 endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index beb4809..16b2901 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -101,6 +101,7 @@ 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
+obj-$(CONFIG_I2C_ZX2967)   += i2c-zx2967.o
 
 # External I2C/SMBus adapter drivers
 obj-$(CONFIG_I2C_DIOLAN_U2C)   += i2c-diolan-u2c.o
diff --git a/drivers/i2c/busses/i2c-zx2967.c b/drivers/i2c/busses/i2c-zx2967.c
new file mode 100644
index 000..7b3471a
--- /dev/null
+++ b/drivers/i2c/busses/i2c-zx2967.c
@@ -0,0 +1,688 @@
+/*
+ * ZTE's zx2967 family i2c bus controller driver
+ *
+ * Copyright (C) 2017 ZTE Ltd.
+ *
+ * Author: Baoyou Xie 
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define REG_CMD0x04
+#define REG_DEVADDR_H  0x0C
+#define REG_DEVADDR_L  0x10
+#define REG_CLK_DIV_FS 0x14
+#define REG_CLK_DIV_HS 0x18
+#define REG_WRCONF 0x1C
+#define REG_RDCONF 0x20
+#define REG_DATA   0x24
+#define REG_STAT   0x28
+
+#define I2C_STOP   0
+#define I2C_MASTER BIT(0)
+#define I2C_ADDR_MODE_TEN  BIT(1)
+#define I2C_IRQ_MSK_ENABLE BIT(3)
+#define I2C_RW_READBIT(4)
+#define I2C_CMB_RW_EN  BIT(5)
+#define I2C_START  BIT(6)
+#define I2C_ADDR_MODE_TEN  BIT(1)
+
+#define I2C_WFIFO_RESETBIT(7)
+#define I2C_RFIFO_RESETBIT(7)
+
+#define I2C_IRQ_ACK_CLEAR  BIT(7)
+#define I2C_INT_MASK   GENMASK(6, 0)
+
+#define I2C_TRANS_DONE BIT(0)
+#define I2C_ERROR_DEVICE   BIT(1)
+#define I2C_ERROR_DATA BIT(2)
+#define I2C_ERROR_MASK GENMASK(2, 1)
+
+#define I2C_SR_BUSYBIT(6)
+
+#define I2C_SR_EDEVICE BIT(1)
+#define I2C_SR_EDATA   BIT(2)
+
+#define I2C_FIFO_MAX   16
+
+#define I2C_TIMEOUTmsecs_to_jiffies(1000)
+
+struct zx2967_i2c_info {
+   spinlock_t  lock;
+   struct device   *dev;
+   struct i2c_adapter  adap;
+   struct clk  *clk;
+   struct completion   complete;
+   u32 clk_freq;
+   void __iomem*reg_base;
+   size_t  residue;
+   int irq;
+   int msg_rd;
+   u8  *buf;
+   u8  access_cnt;
+   boolis_suspended;
+};
+
+static void zx2967_i2c_writel(struct zx2967_i2c_info *zx_i2c,
+ u32 val, unsigned long reg)
+{
+   writel_relaxed(val, zx_i2c->reg_base + reg);
+}
+
+static u32 zx2967_i2c_readl(struct zx2967_i2c_info *zx_i2c, unsigned long reg)
+{
+   return readl_relaxed(zx_i2c->reg_base + reg);
+}
+
+static void zx2967_i2c_writesb(struct zx2967_i2c_info *zx_i2c,
+  void *data, unsigned long reg, int len)
+{
+   writesb(zx_i2c->reg_base + reg, data, len);
+}
+
+static void zx2967_i2c_readsb(struct zx2967_i2c_info *zx_i2c,
+ void *data, unsigned long reg, int len)
+{
+   readsb(zx_i2c->reg_base + reg, data, len);
+}
+
+static void zx2967_i2c_start_ctrl(struct zx2967_i2c_info *zx_i2c)
+{
+   u32 status;
+   u32 ctl;
+
+   status = zx2967_i2c_readl(zx_i2c, REG_STAT);
+   status |= I2C_IRQ_ACK_CLEAR;
+   zx2967_i2c_writel(zx_i2c, status, REG_STAT);
+
+   ctl = zx2967_i2c_readl(zx_i2c, 

[PATCH v4 3/3] i2c: zx2967: add i2c controller driver for ZTE's zx2967 family

[Baoyou Xie]

This patch adds i2c controller driver for ZTE's zx2967 family.

Signed-off-by: Baoyou Xie 
---
 drivers/i2c/busses/Kconfig  |   9 +
 drivers/i2c/busses/Makefile |   1 +
 drivers/i2c/busses/i2c-zx2967.c | 688 
 3 files changed, 698 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-zx2967.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e4a603e..d983e36 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1246,4 +1246,13 @@ config I2C_OPAL
  This driver can also be built as a module. If so, the module will be
  called as i2c-opal.
 
+config I2C_ZX2967
+   tristate "ZTE zx2967 I2C support"
+   depends on ARCH_ZX
+   default y
+   help
+ Selecting this option will add ZX2967 I2C driver.
+ This driver can also be built as a module. If so, the module will be
+ called i2c-zx2967.
+
 endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index beb4809..16b2901 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -101,6 +101,7 @@ 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
+obj-$(CONFIG_I2C_ZX2967)   += i2c-zx2967.o
 
 # External I2C/SMBus adapter drivers
 obj-$(CONFIG_I2C_DIOLAN_U2C)   += i2c-diolan-u2c.o
diff --git a/drivers/i2c/busses/i2c-zx2967.c b/drivers/i2c/busses/i2c-zx2967.c
new file mode 100644
index 000..7b3471a
--- /dev/null
+++ b/drivers/i2c/busses/i2c-zx2967.c
@@ -0,0 +1,688 @@
+/*
+ * ZTE's zx2967 family i2c bus controller driver
+ *
+ * Copyright (C) 2017 ZTE Ltd.
+ *
+ * Author: Baoyou Xie 
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define REG_CMD0x04
+#define REG_DEVADDR_H  0x0C
+#define REG_DEVADDR_L  0x10
+#define REG_CLK_DIV_FS 0x14
+#define REG_CLK_DIV_HS 0x18
+#define REG_WRCONF 0x1C
+#define REG_RDCONF 0x20
+#define REG_DATA   0x24
+#define REG_STAT   0x28
+
+#define I2C_STOP   0
+#define I2C_MASTER BIT(0)
+#define I2C_ADDR_MODE_TEN  BIT(1)
+#define I2C_IRQ_MSK_ENABLE BIT(3)
+#define I2C_RW_READBIT(4)
+#define I2C_CMB_RW_EN  BIT(5)
+#define I2C_START  BIT(6)
+#define I2C_ADDR_MODE_TEN  BIT(1)
+
+#define I2C_WFIFO_RESETBIT(7)
+#define I2C_RFIFO_RESETBIT(7)
+
+#define I2C_IRQ_ACK_CLEAR  BIT(7)
+#define I2C_INT_MASK   GENMASK(6, 0)
+
+#define I2C_TRANS_DONE BIT(0)
+#define I2C_ERROR_DEVICE   BIT(1)
+#define I2C_ERROR_DATA BIT(2)
+#define I2C_ERROR_MASK GENMASK(2, 1)
+
+#define I2C_SR_BUSYBIT(6)
+
+#define I2C_SR_EDEVICE BIT(1)
+#define I2C_SR_EDATA   BIT(2)
+
+#define I2C_FIFO_MAX   16
+
+#define I2C_TIMEOUTmsecs_to_jiffies(1000)
+
+struct zx2967_i2c_info {
+   spinlock_t  lock;
+   struct device   *dev;
+   struct i2c_adapter  adap;
+   struct clk  *clk;
+   struct completion   complete;
+   u32 clk_freq;
+   void __iomem*reg_base;
+   size_t  residue;
+   int irq;
+   int msg_rd;
+   u8  *buf;
+   u8  access_cnt;
+   boolis_suspended;
+};
+
+static void zx2967_i2c_writel(struct zx2967_i2c_info *zx_i2c,
+ u32 val, unsigned long reg)
+{
+   writel_relaxed(val, zx_i2c->reg_base + reg);
+}
+
+static u32 zx2967_i2c_readl(struct zx2967_i2c_info *zx_i2c, unsigned long reg)
+{
+   return readl_relaxed(zx_i2c->reg_base + reg);
+}
+
+static void zx2967_i2c_writesb(struct zx2967_i2c_info *zx_i2c,
+  void *data, unsigned long reg, int len)
+{
+   writesb(zx_i2c->reg_base + reg, data, len);
+}
+
+static void zx2967_i2c_readsb(struct zx2967_i2c_info *zx_i2c,
+ void *data, unsigned long reg, int len)
+{
+   readsb(zx_i2c->reg_base + reg, data, len);
+}
+
+static void zx2967_i2c_start_ctrl(struct zx2967_i2c_info *zx_i2c)
+{
+   u32 status;
+   u32 ctl;
+
+   status = zx2967_i2c_readl(zx_i2c, REG_STAT);
+   status |= I2C_IRQ_ACK_CLEAR;
+   zx2967_i2c_writel(zx_i2c, status, REG_STAT);
+
+   ctl = zx2967_i2c_readl(zx_i2c, REG_CMD);
+   if (zx_i2c->msg_rd)
+