linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/5] i2c: designware: Add slave support
@ 2016-12-07 17:55 Luis Oliveira
  2016-12-07 17:55 ` [PATCH v4 1/5] i2c: designware: Refactoring of the i2c-designware Luis Oliveira
                   ` (4 more replies)
  0 siblings, 5 replies; 22+ messages in thread
From: Luis Oliveira @ 2016-12-07 17:55 UTC (permalink / raw)
  To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

The purpose of this patch is to enable Linux to be a I2C slave by enabling the
slave functionality in the designware I2C controller. The patch refactors the
original i2c-designware-core and extracts all master functions to a
i2c-designware-master source file as suggested by Andy Shevchenko. It also 
creates a i2c-designware-slave source file and keeps the common functions in the
i2c-designware-src source file. For that changes also had to be made in the 
Makefile and Kconfig.
The driver instantiates in slave or master mode by checking the compatible string
of the device tree (see devicetree/bindings/i2c/i2c-designware.txt). ACPI is not
supported.
The functionality was tested using the hardware independent software backend 
slave-eeprom driver.

Luis Oliveira (5):
  i2c: designware: Refactoring of the i2c-designware
  i2c: designware: Master mode as separated driver
  i2c: designware: Add slave definitions
  i2c: designware: Add slave mode as separated driver
  i2c: designware: Cleaning comments and formatation

 .../devicetree/bindings/i2c/i2c-designware.txt     |   4 +
 drivers/i2c/busses/Kconfig                         |   1 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-designware-common.c         | 258 ++++++++++++
 drivers/i2c/busses/i2c-designware-core.h           | 158 ++++++++
 ...c-designware-core.c => i2c-designware-master.c} | 380 ++----------------
 drivers/i2c/busses/i2c-designware-platdrv.c        | 106 ++++-
 drivers/i2c/busses/i2c-designware-slave.c          | 435 +++++++++++++++++++++
 8 files changed, 973 insertions(+), 370 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-common.c
 rename drivers/i2c/busses/{i2c-designware-core.c => i2c-designware-master.c} (66%)
 create mode 100644 drivers/i2c/busses/i2c-designware-slave.c

-- 
2.11.0

^ permalink raw reply	[flat|nested] 22+ messages in thread
* [PATCH v4 1/5] i2c: designware: Refactoring of the i2c-designware
  2016-12-07 17:55 [PATCH v4 0/5] i2c: designware: Add slave support Luis Oliveira
@ 2016-12-07 17:55 ` Luis Oliveira
  2016-12-07 18:58   ` Andy Shevchenko
  2016-12-07 17:55 ` [PATCH v4 2/5] i2c: designware: Master mode as separated driver Luis Oliveira
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Luis Oliveira @ 2016-12-07 17:55 UTC (permalink / raw)
  To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

- Factor out all _master() part of code from i2c-designware-core
  and i2c-designware-platdrv to separate functions.
- Standardize all code related with MASTER modes.

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V3->V4: (Andy Shevchenko)
- I have to take off DW_IC_INTR_TX_EMPTY from DW_IC_INTR_DEFAULT_MASK
  because is not common to both operating modes 
- Comments were changed to be specific to "master"

 drivers/i2c/busses/i2c-designware-core.c    | 57 +++++++++++++++++++----------
 drivers/i2c/busses/i2c-designware-platdrv.c | 35 +++++++++++-------
 2 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6d81c56184d3..a51addfde565 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -87,10 +87,12 @@
 #define DW_IC_INTR_GEN_CALL	0x800
 
 #define DW_IC_INTR_DEFAULT_MASK		(DW_IC_INTR_RX_FULL | \
-					 DW_IC_INTR_TX_EMPTY | \
 					 DW_IC_INTR_TX_ABRT | \
 					 DW_IC_INTR_STOP_DET)
 
+#define DW_IC_INTR_MASTER_MASK		(DW_IC_INTR_DEFAULT_MASK | \
+					 DW_IC_INTR_TX_EMPTY)
+
 #define DW_IC_STATUS_ACTIVITY	0x1
 
 #define DW_IC_SDA_HOLD_RX_SHIFT		16
@@ -202,6 +204,16 @@ static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
 	}
 }
 
+static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
+{
+	/* Configure Tx/Rx FIFO threshold levels */
+	dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
+	dw_writel(dev, 0, DW_IC_RX_TL);
+
+	/* configure the i2c master */
+	dw_writel(dev, dev->master_cfg, DW_IC_CON);
+}
+
 static u32
 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 {
@@ -318,10 +330,10 @@ static void i2c_dw_release_lock(struct dw_i2c_dev *dev)
 }
 
 /**
- * i2c_dw_init() - initialize the designware i2c master hardware
+ * i2c_dw_init() - initialize the designware i2c hardware
  * @dev: device private data
  *
- * This functions configures and enables the I2C master.
+ * This functions configures and enables the I2C.
  * This function is called during I2C init function, and in case of timeout at
  * run time.
  */
@@ -440,12 +452,9 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 			"Hardware too old to adjust SDA hold time.\n");
 	}
 
-	/* Configure Tx/Rx FIFO threshold levels */
-	dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
-	dw_writel(dev, 0, DW_IC_RX_TL);
-
-	/* configure the i2c master */
-	dw_writel(dev, dev->master_cfg , DW_IC_CON);
+	if ((dev->master_cfg & DW_IC_CON_MASTER) &&
+		 (dev->master_cfg & DW_IC_CON_SLAVE_DISABLE))
+		i2c_dw_configure_fifo_master(dev);
 
 	i2c_dw_release_lock(dev);
 
@@ -513,7 +522,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 
 	/* Clear and enable interrupts */
 	dw_readl(dev, DW_IC_CLR_INTR);
-	dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
+	dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
 }
 
 /*
@@ -533,7 +542,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	u8 *buf = dev->tx_buf;
 	bool need_restart = false;
 
-	intr_mask = DW_IC_INTR_DEFAULT_MASK;
+	intr_mask = DW_IC_INTR_MASTER_MASK;
 
 	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		u32 flags = msgs[dev->msg_write_idx].flags;
@@ -886,16 +895,9 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
  * Interrupt service routine. This gets called whenever an I2C interrupt
  * occurs.
  */
-static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
+int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
 {
-	struct dw_i2c_dev *dev = dev_id;
-	u32 stat, enabled;
-
-	enabled = dw_readl(dev, DW_IC_ENABLE);
-	stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
-	dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, enabled, stat);
-	if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
-		return IRQ_NONE;
+	u32 stat;
 
 	stat = i2c_dw_read_clear_intrbits(dev);
 
@@ -932,7 +934,22 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 		i2c_dw_disable_int(dev);
 		dw_writel(dev, stat, DW_IC_INTR_MASK);
 	}
+	return 0;
+}
+
+static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
+{
+	struct dw_i2c_dev *dev = dev_id;
+	u32 stat, enabled, mode;
+
+	enabled = dw_readl(dev, DW_IC_ENABLE);
+	stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
+	dev_dbg(dev->dev, "%s: enabled=%#x stat=%#x\n", __func__, enabled, stat);
+	if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
+		return IRQ_NONE;
 
+	i2c_dw_irq_handler_master(dev);
+	complete(&dev->cmd_complete);
 	return IRQ_HANDLED;
 }
 
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 08153ea4d848..e55b5544c733 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -138,6 +138,27 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
 }
 #endif
 
+static void i2c_dw_configure_master(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+			  DW_IC_CON_RESTART_EN;
+
+	dev_info(&pdev->dev, "I am registed as a I2C Master!\n");
+
+	switch (dev->clk_freq) {
+	case 100000:
+		dev->master_cfg |= DW_IC_CON_SPEED_STD;
+		break;
+	case 3400000:
+		dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
+		break;
+	default:
+		dev->master_cfg |= DW_IC_CON_SPEED_FAST;
+	}
+}
+
 static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
 {
 	if (IS_ERR(i_dev->clk))
@@ -221,19 +242,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 
 	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
 
-	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
-			  DW_IC_CON_RESTART_EN;
-
-	switch (dev->clk_freq) {
-	case 100000:
-		dev->master_cfg |= DW_IC_CON_SPEED_STD;
-		break;
-	case 3400000:
-		dev->master_cfg |= DW_IC_CON_SPEED_HIGH;
-		break;
-	default:
-		dev->master_cfg |= DW_IC_CON_SPEED_FAST;
-	}
+	i2c_dw_configure_master(pdev);
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (!i2c_dw_plat_prepare_clk(dev, true)) {
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v4 2/5] i2c: designware: Master mode as separated driver
  2016-12-07 17:55 [PATCH v4 0/5] i2c: designware: Add slave support Luis Oliveira
  2016-12-07 17:55 ` [PATCH v4 1/5] i2c: designware: Refactoring of the i2c-designware Luis Oliveira
@ 2016-12-07 17:55 ` Luis Oliveira
  2016-12-07 19:03   ` Andy Shevchenko
  2016-12-07 17:55 ` [PATCH v4 3/5] i2c: designware: Add slave definitions Luis Oliveira
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Luis Oliveira @ 2016-12-07 17:55 UTC (permalink / raw)
  To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

- The functions related to I2C master mode of operation were moved
  to a single file: i2c-designware-master.c
- Common functions were moved into i2c-designware-common.c
- Common definitions were moved into i2c-designware-core.h (were in core.c)

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V3->V4: (Andy Shevchenko)
- The name of the i2c-designware-src.c was changed to i2c-designware-common.c
  as suggested by Andy.

 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-designware-common.c         | 252 +++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h           | 131 ++++++++
 ...c-designware-core.c => i2c-designware-master.c} | 347 +--------------------
 4 files changed, 390 insertions(+), 341 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-common.c
 rename drivers/i2c/busses/{i2c-designware-core.c => i2c-designware-master.c} (66%)

diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 1c1bac87a9db..4f8f6a2b9346 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_I2C_CBUS_GPIO)	+= i2c-cbus-gpio.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_DESIGNWARE_CORE)	+= i2c-designware-core.o
+i2c-designware-core-objs := i2c-designware-common.o i2c-designware-master.o
 obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
 i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
new file mode 100644
index 000000000000..6afd2ff5d73f
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -0,0 +1,252 @@
+/*
+ * Synopsys DesignWare I2C adapter driver (master only).
+ *
+ * Based on the TI DAVINCI I2C adapter driver.
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software Inc.
+ * Copyright (C) 2009 Provigent Ltd.
+ *
+ * ----------------------------------------------------------------------------
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * ----------------------------------------------------------------------------
+ *
+ */
+#include <linux/export.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include "i2c-designware-core.h"
+
+static char *abort_sources[] = {
+	[ABRT_7B_ADDR_NOACK] =
+		"slave address not acknowledged (7bit mode)",
+	[ABRT_10ADDR1_NOACK] =
+		"first address byte not acknowledged (10bit mode)",
+	[ABRT_10ADDR2_NOACK] =
+		"second address byte not acknowledged (10bit mode)",
+	[ABRT_TXDATA_NOACK] =
+		"data not acknowledged",
+	[ABRT_GCALL_NOACK] =
+		"no acknowledgement for a general call",
+	[ABRT_GCALL_READ] =
+		"read after general call",
+	[ABRT_SBYTE_ACKDET] =
+		"start byte acknowledged",
+	[ABRT_SBYTE_NORSTRT] =
+		"trying to send start byte when restart is disabled",
+	[ABRT_10B_RD_NORSTRT] =
+		"trying to read when restart is disabled (10bit mode)",
+	[ABRT_MASTER_DIS] =
+		"trying to use disabled adapter",
+	[ARB_LOST] =
+		"lost arbitration",
+};
+
+u32 dw_readl(struct dw_i2c_dev *dev, int offset)
+{
+	u32 value;
+
+	if (dev->accessor_flags & ACCESS_16BIT)
+		value = readw_relaxed(dev->base + offset) |
+			(readw_relaxed(dev->base + offset + 2) << 16);
+	else
+		value = readl_relaxed(dev->base + offset);
+
+	if (dev->accessor_flags & ACCESS_SWAP)
+		return swab32(value);
+	else
+		return value;
+}
+
+void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
+{
+	if (dev->accessor_flags & ACCESS_SWAP)
+		b = swab32(b);
+
+	if (dev->accessor_flags & ACCESS_16BIT) {
+		writew_relaxed((u16)b, dev->base + offset);
+		writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
+	} else {
+		writel_relaxed(b, dev->base + offset);
+	}
+}
+
+u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
+{
+	/*
+	 * DesignWare I2C core doesn't seem to have solid strategy to meet
+	 * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
+	 * will result in violation of the tHD;STA spec.
+	 */
+	if (cond)
+		/*
+		 * Conditional expression:
+		 *
+		 *   IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH
+		 *
+		 * This is based on the DW manuals, and represents an ideal
+		 * configuration.  The resulting I2C bus speed will be
+		 * faster than any of the others.
+		 *
+		 * If your hardware is free from tHD;STA issue, try this one.
+		 */
+		return (ic_clk * tSYMBOL + 500000) / 1000000 - 8 + offset;
+	else
+		/*
+		 * Conditional expression:
+		 *
+		 *   IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)
+		 *
+		 * This is just experimental rule; the tHD;STA period turned
+		 * out to be proportinal to (_HCNT + 3).  With this setting,
+		 * we could meet both tHIGH and tHD;STA timing specs.
+		 *
+		 * If unsure, you'd better to take this alternative.
+		 *
+		 * The reason why we need to take into account "tf" here,
+		 * is the same as described in i2c_dw_scl_lcnt().
+		 */
+		return (ic_clk * (tSYMBOL + tf) + 500000) / 1000000
+			- 3 + offset;
+}
+
+u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
+{
+	/*
+	 * Conditional expression:
+	 *
+	 *   IC_[FS]S_SCL_LCNT + 1 >= IC_CLK * (tLOW + tf)
+	 *
+	 * DW I2C core starts counting the SCL CNTs for the LOW period
+	 * of the SCL clock (tLOW) as soon as it pulls the SCL line.
+	 * In order to meet the tLOW timing spec, we need to take into
+	 * account the fall time of SCL signal (tf).  Default tf value
+	 * should be 0.3 us, for safety.
+	 */
+	return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 + offset;
+}
+
+void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
+{
+	dw_writel(dev, enable, DW_IC_ENABLE);
+}
+
+void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable)
+{
+	int timeout = 100;
+
+	do {
+		__i2c_dw_enable(dev, enable);
+		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
+			return;
+
+		/*
+		 * Wait 10 times the signaling period of the highest I2C
+		 * transfer supported by the driver (for 400KHz this is
+		 * 25us) as described in the DesignWare I2C databook.
+		 */
+		usleep_range(25, 250);
+	} while (timeout--);
+
+	dev_warn(dev->dev, "timeout in %sabling adapter\n",
+		 enable ? "en" : "dis");
+}
+
+unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
+{
+	/*
+	 * Clock is not necessary if we got LCNT/HCNT values directly from
+	 * the platform code.
+	 */
+	if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
+		return 0;
+	return dev->get_clk_rate_khz(dev);
+}
+
+int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
+{
+	int ret;
+
+	if (!dev->acquire_lock)
+		return 0;
+
+	ret = dev->acquire_lock(dev);
+	if (!ret)
+		return 0;
+
+	dev_err(dev->dev, "couldn't acquire bus ownership\n");
+
+	return ret;
+}
+
+void i2c_dw_release_lock(struct dw_i2c_dev *dev)
+{
+	if (dev->release_lock)
+		dev->release_lock(dev);
+}
+
+/*
+ * Waiting for bus not busy
+ */
+int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
+{
+	int timeout = TIMEOUT;
+
+	while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
+		if (timeout <= 0) {
+			dev_warn(dev->dev, "timeout waiting for bus ready\n");
+			return -ETIMEDOUT;
+		}
+		timeout--;
+		usleep_range(1000, 1100);
+	}
+
+	return 0;
+}
+
+int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
+{
+	unsigned long abort_source = dev->abort_source;
+	int i;
+
+	if (abort_source & DW_IC_TX_ABRT_NOACK) {
+		for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
+			dev_dbg(dev->dev,
+				"%s: %s\n", __func__, abort_sources[i]);
+		return -EREMOTEIO;
+	}
+
+	for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
+		dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
+
+	if (abort_source & DW_IC_TX_ARB_LOST)
+		return -EAGAIN;
+	else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
+		return -EINVAL; /* wrong msgs[] data */
+	else
+		return -EIO;
+}
+
+u32 i2c_dw_func(struct i2c_adapter *adap)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+	return dev->functionality;
+}
+
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
+MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 26250b425e2f..8bba7a37c3ce 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -40,6 +40,124 @@
 #define DW_IC_CON_RESTART_EN		0x20
 #define DW_IC_CON_SLAVE_DISABLE		0x40
 
+/*
+ * Registers offset
+ */
+#define DW_IC_CON		0x0
+#define DW_IC_TAR		0x4
+#define DW_IC_DATA_CMD		0x10
+#define DW_IC_SS_SCL_HCNT	0x14
+#define DW_IC_SS_SCL_LCNT	0x18
+#define DW_IC_FS_SCL_HCNT	0x1c
+#define DW_IC_FS_SCL_LCNT	0x20
+#define DW_IC_HS_SCL_HCNT	0x24
+#define DW_IC_HS_SCL_LCNT	0x28
+#define DW_IC_INTR_STAT		0x2c
+#define DW_IC_INTR_MASK		0x30
+#define DW_IC_RAW_INTR_STAT	0x34
+#define DW_IC_RX_TL		0x38
+#define DW_IC_TX_TL		0x3c
+#define DW_IC_CLR_INTR		0x40
+#define DW_IC_CLR_RX_UNDER	0x44
+#define DW_IC_CLR_RX_OVER	0x48
+#define DW_IC_CLR_TX_OVER	0x4c
+#define DW_IC_CLR_RD_REQ	0x50
+#define DW_IC_CLR_TX_ABRT	0x54
+#define DW_IC_CLR_RX_DONE	0x58
+#define DW_IC_CLR_ACTIVITY	0x5c
+#define DW_IC_CLR_STOP_DET	0x60
+#define DW_IC_CLR_START_DET	0x64
+#define DW_IC_CLR_GEN_CALL	0x68
+#define DW_IC_ENABLE		0x6c
+#define DW_IC_STATUS		0x70
+#define DW_IC_TXFLR		0x74
+#define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
+#define DW_IC_TX_ABRT_SOURCE	0x80
+#define DW_IC_ENABLE_STATUS	0x9c
+#define DW_IC_COMP_PARAM_1	0xf4
+#define DW_IC_COMP_VERSION	0xf8
+#define DW_IC_SDA_HOLD_MIN_VERS	0x3131312A
+#define DW_IC_COMP_TYPE		0xfc
+#define DW_IC_COMP_TYPE_VALUE	0x44570140
+
+#define DW_IC_INTR_RX_UNDER	0x001
+#define DW_IC_INTR_RX_OVER	0x002
+#define DW_IC_INTR_RX_FULL	0x004
+#define DW_IC_INTR_TX_OVER	0x008
+#define DW_IC_INTR_TX_EMPTY	0x010
+#define DW_IC_INTR_RD_REQ	0x020
+#define DW_IC_INTR_TX_ABRT	0x040
+#define DW_IC_INTR_RX_DONE	0x080
+#define DW_IC_INTR_ACTIVITY	0x100
+#define DW_IC_INTR_STOP_DET	0x200
+#define DW_IC_INTR_START_DET	0x400
+#define DW_IC_INTR_GEN_CALL	0x800
+
+#define DW_IC_INTR_DEFAULT_MASK		(DW_IC_INTR_RX_FULL | \
+					 DW_IC_INTR_TX_ABRT | \
+					 DW_IC_INTR_STOP_DET)
+#define DW_IC_INTR_MASTER_MASK		(DW_IC_INTR_DEFAULT_MASK | \
+					 DW_IC_INTR_TX_EMPTY)
+#define DW_IC_STATUS_ACTIVITY		0x1
+#define DW_IC_STATUS_TFE		BIT(2)
+#define DW_IC_STATUS_MASTER_ACTIVITY	BIT(5)
+
+#define DW_IC_SDA_HOLD_RX_SHIFT		16
+#define DW_IC_SDA_HOLD_RX_MASK		GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
+
+#define DW_IC_ERR_TX_ABRT	0x1
+
+#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
+
+#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH	(BIT(2) | BIT(3))
+#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK	GENMASK(3, 2)
+
+/*
+ * status codes
+ */
+#define STATUS_IDLE			0x0
+#define STATUS_WRITE_IN_PROGRESS	0x1
+#define STATUS_READ_IN_PROGRESS		0x2
+
+#define TIMEOUT			20 /* ms */
+
+/*
+ * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
+ *
+ * only expected abort codes are listed here
+ * refer to the datasheet for the full list
+ */
+#define ABRT_7B_ADDR_NOACK	0
+#define ABRT_10ADDR1_NOACK	1
+#define ABRT_10ADDR2_NOACK	2
+#define ABRT_TXDATA_NOACK	3
+#define ABRT_GCALL_NOACK	4
+#define ABRT_GCALL_READ		5
+#define ABRT_SBYTE_ACKDET	7
+#define ABRT_SBYTE_NORSTRT	9
+#define ABRT_10B_RD_NORSTRT	10
+#define ABRT_MASTER_DIS		11
+#define ARB_LOST		12
+
+#define DW_IC_TX_ABRT_7B_ADDR_NOACK	(1UL << ABRT_7B_ADDR_NOACK)
+#define DW_IC_TX_ABRT_10ADDR1_NOACK	(1UL << ABRT_10ADDR1_NOACK)
+#define DW_IC_TX_ABRT_10ADDR2_NOACK	(1UL << ABRT_10ADDR2_NOACK)
+#define DW_IC_TX_ABRT_TXDATA_NOACK	(1UL << ABRT_TXDATA_NOACK)
+#define DW_IC_TX_ABRT_GCALL_NOACK	(1UL << ABRT_GCALL_NOACK)
+#define DW_IC_TX_ABRT_GCALL_READ	(1UL << ABRT_GCALL_READ)
+#define DW_IC_TX_ABRT_SBYTE_ACKDET	(1UL << ABRT_SBYTE_ACKDET)
+#define DW_IC_TX_ABRT_SBYTE_NORSTRT	(1UL << ABRT_SBYTE_NORSTRT)
+#define DW_IC_TX_ABRT_10B_RD_NORSTRT	(1UL << ABRT_10B_RD_NORSTRT)
+#define DW_IC_TX_ABRT_MASTER_DIS	(1UL << ABRT_MASTER_DIS)
+#define DW_IC_TX_ARB_LOST		(1UL << ARB_LOST)
+
+#define DW_IC_TX_ABRT_NOACK		(DW_IC_TX_ABRT_7B_ADDR_NOACK | \
+					 DW_IC_TX_ABRT_10ADDR1_NOACK | \
+					 DW_IC_TX_ABRT_10ADDR2_NOACK | \
+					 DW_IC_TX_ABRT_TXDATA_NOACK | \
+					 DW_IC_TX_ABRT_GCALL_NOACK)
+
 
 /**
  * struct dw_i2c_dev - private i2c-designware data
@@ -132,6 +250,19 @@ struct dw_i2c_dev {
 #define ACCESS_16BIT		0x00000002
 #define ACCESS_INTR_MASK	0x00000004
 
+u32 dw_readl(struct dw_i2c_dev *dev, int offset);
+void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
+u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset);
+u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset);
+void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable);
+void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable);
+unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev);
+int i2c_dw_acquire_lock(struct dw_i2c_dev *dev);
+void i2c_dw_release_lock(struct dw_i2c_dev *dev);
+int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev);
+int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev);
+u32 i2c_dw_func(struct i2c_adapter *adap);
+
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-master.c
similarity index 66%
rename from drivers/i2c/busses/i2c-designware-core.c
rename to drivers/i2c/busses/i2c-designware-master.c
index a51addfde565..9943addac3d7 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -32,305 +32,18 @@
 #include <linux/module.h>
 #include "i2c-designware-core.h"
 
-/*
- * Registers offset
- */
-#define DW_IC_CON		0x0
-#define DW_IC_TAR		0x4
-#define DW_IC_DATA_CMD		0x10
-#define DW_IC_SS_SCL_HCNT	0x14
-#define DW_IC_SS_SCL_LCNT	0x18
-#define DW_IC_FS_SCL_HCNT	0x1c
-#define DW_IC_FS_SCL_LCNT	0x20
-#define DW_IC_HS_SCL_HCNT	0x24
-#define DW_IC_HS_SCL_LCNT	0x28
-#define DW_IC_INTR_STAT		0x2c
-#define DW_IC_INTR_MASK		0x30
-#define DW_IC_RAW_INTR_STAT	0x34
-#define DW_IC_RX_TL		0x38
-#define DW_IC_TX_TL		0x3c
-#define DW_IC_CLR_INTR		0x40
-#define DW_IC_CLR_RX_UNDER	0x44
-#define DW_IC_CLR_RX_OVER	0x48
-#define DW_IC_CLR_TX_OVER	0x4c
-#define DW_IC_CLR_RD_REQ	0x50
-#define DW_IC_CLR_TX_ABRT	0x54
-#define DW_IC_CLR_RX_DONE	0x58
-#define DW_IC_CLR_ACTIVITY	0x5c
-#define DW_IC_CLR_STOP_DET	0x60
-#define DW_IC_CLR_START_DET	0x64
-#define DW_IC_CLR_GEN_CALL	0x68
-#define DW_IC_ENABLE		0x6c
-#define DW_IC_STATUS		0x70
-#define DW_IC_TXFLR		0x74
-#define DW_IC_RXFLR		0x78
-#define DW_IC_SDA_HOLD		0x7c
-#define DW_IC_TX_ABRT_SOURCE	0x80
-#define DW_IC_ENABLE_STATUS	0x9c
-#define DW_IC_COMP_PARAM_1	0xf4
-#define DW_IC_COMP_VERSION	0xf8
-#define DW_IC_SDA_HOLD_MIN_VERS	0x3131312A
-#define DW_IC_COMP_TYPE		0xfc
-#define DW_IC_COMP_TYPE_VALUE	0x44570140
-
-#define DW_IC_INTR_RX_UNDER	0x001
-#define DW_IC_INTR_RX_OVER	0x002
-#define DW_IC_INTR_RX_FULL	0x004
-#define DW_IC_INTR_TX_OVER	0x008
-#define DW_IC_INTR_TX_EMPTY	0x010
-#define DW_IC_INTR_RD_REQ	0x020
-#define DW_IC_INTR_TX_ABRT	0x040
-#define DW_IC_INTR_RX_DONE	0x080
-#define DW_IC_INTR_ACTIVITY	0x100
-#define DW_IC_INTR_STOP_DET	0x200
-#define DW_IC_INTR_START_DET	0x400
-#define DW_IC_INTR_GEN_CALL	0x800
-
-#define DW_IC_INTR_DEFAULT_MASK		(DW_IC_INTR_RX_FULL | \
-					 DW_IC_INTR_TX_ABRT | \
-					 DW_IC_INTR_STOP_DET)
-
-#define DW_IC_INTR_MASTER_MASK		(DW_IC_INTR_DEFAULT_MASK | \
-					 DW_IC_INTR_TX_EMPTY)
-
-#define DW_IC_STATUS_ACTIVITY	0x1
-
-#define DW_IC_SDA_HOLD_RX_SHIFT		16
-#define DW_IC_SDA_HOLD_RX_MASK		GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
-
-#define DW_IC_ERR_TX_ABRT	0x1
-
-#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
-
-#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH	(BIT(2) | BIT(3))
-#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK	GENMASK(3, 2)
-
-/*
- * status codes
- */
-#define STATUS_IDLE			0x0
-#define STATUS_WRITE_IN_PROGRESS	0x1
-#define STATUS_READ_IN_PROGRESS		0x2
-
-#define TIMEOUT			20 /* ms */
-
-/*
- * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
- *
- * only expected abort codes are listed here
- * refer to the datasheet for the full list
- */
-#define ABRT_7B_ADDR_NOACK	0
-#define ABRT_10ADDR1_NOACK	1
-#define ABRT_10ADDR2_NOACK	2
-#define ABRT_TXDATA_NOACK	3
-#define ABRT_GCALL_NOACK	4
-#define ABRT_GCALL_READ		5
-#define ABRT_SBYTE_ACKDET	7
-#define ABRT_SBYTE_NORSTRT	9
-#define ABRT_10B_RD_NORSTRT	10
-#define ABRT_MASTER_DIS		11
-#define ARB_LOST		12
-
-#define DW_IC_TX_ABRT_7B_ADDR_NOACK	(1UL << ABRT_7B_ADDR_NOACK)
-#define DW_IC_TX_ABRT_10ADDR1_NOACK	(1UL << ABRT_10ADDR1_NOACK)
-#define DW_IC_TX_ABRT_10ADDR2_NOACK	(1UL << ABRT_10ADDR2_NOACK)
-#define DW_IC_TX_ABRT_TXDATA_NOACK	(1UL << ABRT_TXDATA_NOACK)
-#define DW_IC_TX_ABRT_GCALL_NOACK	(1UL << ABRT_GCALL_NOACK)
-#define DW_IC_TX_ABRT_GCALL_READ	(1UL << ABRT_GCALL_READ)
-#define DW_IC_TX_ABRT_SBYTE_ACKDET	(1UL << ABRT_SBYTE_ACKDET)
-#define DW_IC_TX_ABRT_SBYTE_NORSTRT	(1UL << ABRT_SBYTE_NORSTRT)
-#define DW_IC_TX_ABRT_10B_RD_NORSTRT	(1UL << ABRT_10B_RD_NORSTRT)
-#define DW_IC_TX_ABRT_MASTER_DIS	(1UL << ABRT_MASTER_DIS)
-#define DW_IC_TX_ARB_LOST		(1UL << ARB_LOST)
-
-#define DW_IC_TX_ABRT_NOACK		(DW_IC_TX_ABRT_7B_ADDR_NOACK | \
-					 DW_IC_TX_ABRT_10ADDR1_NOACK | \
-					 DW_IC_TX_ABRT_10ADDR2_NOACK | \
-					 DW_IC_TX_ABRT_TXDATA_NOACK | \
-					 DW_IC_TX_ABRT_GCALL_NOACK)
-
-static char *abort_sources[] = {
-	[ABRT_7B_ADDR_NOACK] =
-		"slave address not acknowledged (7bit mode)",
-	[ABRT_10ADDR1_NOACK] =
-		"first address byte not acknowledged (10bit mode)",
-	[ABRT_10ADDR2_NOACK] =
-		"second address byte not acknowledged (10bit mode)",
-	[ABRT_TXDATA_NOACK] =
-		"data not acknowledged",
-	[ABRT_GCALL_NOACK] =
-		"no acknowledgement for a general call",
-	[ABRT_GCALL_READ] =
-		"read after general call",
-	[ABRT_SBYTE_ACKDET] =
-		"start byte acknowledged",
-	[ABRT_SBYTE_NORSTRT] =
-		"trying to send start byte when restart is disabled",
-	[ABRT_10B_RD_NORSTRT] =
-		"trying to read when restart is disabled (10bit mode)",
-	[ABRT_MASTER_DIS] =
-		"trying to use disabled adapter",
-	[ARB_LOST] =
-		"lost arbitration",
-};
-
-static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
-{
-	u32 value;
-
-	if (dev->accessor_flags & ACCESS_16BIT)
-		value = readw_relaxed(dev->base + offset) |
-			(readw_relaxed(dev->base + offset + 2) << 16);
-	else
-		value = readl_relaxed(dev->base + offset);
-
-	if (dev->accessor_flags & ACCESS_SWAP)
-		return swab32(value);
-	else
-		return value;
-}
-
-static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
-{
-	if (dev->accessor_flags & ACCESS_SWAP)
-		b = swab32(b);
-
-	if (dev->accessor_flags & ACCESS_16BIT) {
-		writew_relaxed((u16)b, dev->base + offset);
-		writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
-	} else {
-		writel_relaxed(b, dev->base + offset);
-	}
-}
-
 static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
 {
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
 
-	/* configure the i2c master */
+	/* configure the I2C master */
 	dw_writel(dev, dev->master_cfg, DW_IC_CON);
 }
 
-static u32
-i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
-{
-	/*
-	 * DesignWare I2C core doesn't seem to have solid strategy to meet
-	 * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
-	 * will result in violation of the tHD;STA spec.
-	 */
-	if (cond)
-		/*
-		 * Conditional expression:
-		 *
-		 *   IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH
-		 *
-		 * This is based on the DW manuals, and represents an ideal
-		 * configuration.  The resulting I2C bus speed will be
-		 * faster than any of the others.
-		 *
-		 * If your hardware is free from tHD;STA issue, try this one.
-		 */
-		return (ic_clk * tSYMBOL + 500000) / 1000000 - 8 + offset;
-	else
-		/*
-		 * Conditional expression:
-		 *
-		 *   IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)
-		 *
-		 * This is just experimental rule; the tHD;STA period turned
-		 * out to be proportinal to (_HCNT + 3).  With this setting,
-		 * we could meet both tHIGH and tHD;STA timing specs.
-		 *
-		 * If unsure, you'd better to take this alternative.
-		 *
-		 * The reason why we need to take into account "tf" here,
-		 * is the same as described in i2c_dw_scl_lcnt().
-		 */
-		return (ic_clk * (tSYMBOL + tf) + 500000) / 1000000
-			- 3 + offset;
-}
-
-static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
-{
-	/*
-	 * Conditional expression:
-	 *
-	 *   IC_[FS]S_SCL_LCNT + 1 >= IC_CLK * (tLOW + tf)
-	 *
-	 * DW I2C core starts counting the SCL CNTs for the LOW period
-	 * of the SCL clock (tLOW) as soon as it pulls the SCL line.
-	 * In order to meet the tLOW timing spec, we need to take into
-	 * account the fall time of SCL signal (tf).  Default tf value
-	 * should be 0.3 us, for safety.
-	 */
-	return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 + offset;
-}
-
-static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
-{
-	dw_writel(dev, enable, DW_IC_ENABLE);
-}
-
-static void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable)
-{
-	int timeout = 100;
-
-	do {
-		__i2c_dw_enable(dev, enable);
-		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
-			return;
-
-		/*
-		 * Wait 10 times the signaling period of the highest I2C
-		 * transfer supported by the driver (for 400KHz this is
-		 * 25us) as described in the DesignWare I2C databook.
-		 */
-		usleep_range(25, 250);
-	} while (timeout--);
-
-	dev_warn(dev->dev, "timeout in %sabling adapter\n",
-		 enable ? "en" : "dis");
-}
-
-static unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
-{
-	/*
-	 * Clock is not necessary if we got LCNT/HCNT values directly from
-	 * the platform code.
-	 */
-	if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
-		return 0;
-	return dev->get_clk_rate_khz(dev);
-}
-
-static int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
-{
-	int ret;
-
-	if (!dev->acquire_lock)
-		return 0;
-
-	ret = dev->acquire_lock(dev);
-	if (!ret)
-		return 0;
-
-	dev_err(dev->dev, "couldn't acquire bus ownership\n");
-
-	return ret;
-}
-
-static void i2c_dw_release_lock(struct dw_i2c_dev *dev)
-{
-	if (dev->release_lock)
-		dev->release_lock(dev);
-}
-
 /**
- * i2c_dw_init() - initialize the designware i2c hardware
+ * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data
  *
  * This functions configures and enables the I2C.
@@ -462,25 +175,6 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_init);
 
-/*
- * Waiting for bus not busy
- */
-static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
-{
-	int timeout = TIMEOUT;
-
-	while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
-		if (timeout <= 0) {
-			dev_warn(dev->dev, "timeout waiting for bus ready\n");
-			return -ETIMEDOUT;
-		}
-		timeout--;
-		usleep_range(1000, 1100);
-	}
-
-	return 0;
-}
-
 static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
@@ -715,29 +409,6 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 	}
 }
 
-static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
-{
-	unsigned long abort_source = dev->abort_source;
-	int i;
-
-	if (abort_source & DW_IC_TX_ABRT_NOACK) {
-		for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
-			dev_dbg(dev->dev,
-				"%s: %s\n", __func__, abort_sources[i]);
-		return -EREMOTEIO;
-	}
-
-	for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
-		dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
-
-	if (abort_source & DW_IC_TX_ARB_LOST)
-		return -EAGAIN;
-	else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
-		return -EINVAL; /* wrong msgs[] data */
-	else
-		return -EIO;
-}
-
 /*
  * Prepare controller for a transaction and call i2c_dw_xfer_msg
  */
@@ -825,12 +496,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	return ret;
 }
 
-static u32 i2c_dw_func(struct i2c_adapter *adap)
-{
-	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
-	return dev->functionality;
-}
-
 static struct i2c_algorithm i2c_dw_algo = {
 	.master_xfer	= i2c_dw_xfer,
 	.functionality	= i2c_dw_func,
@@ -892,10 +557,10 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
 }
 
 /*
- * Interrupt service routine. This gets called whenever an I2C interrupt
+ * Interrupt service routine. This gets called whenever an I2C master interrupt
  * occurs.
  */
-int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
+static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
 {
 	u32 stat;
 
@@ -940,7 +605,7 @@ int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
 static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 {
 	struct dw_i2c_dev *dev = dev_id;
-	u32 stat, enabled, mode;
+	u32 stat, enabled;
 
 	enabled = dw_readl(dev, DW_IC_ENABLE);
 	stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
@@ -1041,5 +706,5 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_probe);
 
-MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter master");
 MODULE_LICENSE("GPL");
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-07 17:55 [PATCH v4 0/5] i2c: designware: Add slave support Luis Oliveira
  2016-12-07 17:55 ` [PATCH v4 1/5] i2c: designware: Refactoring of the i2c-designware Luis Oliveira
  2016-12-07 17:55 ` [PATCH v4 2/5] i2c: designware: Master mode as separated driver Luis Oliveira
@ 2016-12-07 17:55 ` Luis Oliveira
  2016-12-07 19:11   ` Andy Shevchenko
  2016-12-12 17:02   ` Rob Herring
  2016-12-07 17:55 ` [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver Luis Oliveira
  2016-12-07 17:55 ` [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation Luis Oliveira
  4 siblings, 2 replies; 22+ messages in thread
From: Luis Oliveira @ 2016-12-07 17:55 UTC (permalink / raw)
  To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

- Add slave definitions to i2c-designware-core
- Changes in Kconfig to auto-enable I2C_SLAVE when compiling the modules
- Add mode property to designware-core.txt that enable the "slave" selection:
  - "mode" is an optional property that could be "slave" or "master"
  - if "mode" is not set the block is considered master by default

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V3->V4: (Andy Shevchenko)
- created a common property for modes 
- placed the generic dependency first

 .../devicetree/bindings/i2c/i2c-designware.txt     |  4 ++++
 drivers/i2c/busses/Kconfig                         |  1 +
 drivers/i2c/busses/i2c-designware-common.c         |  6 +++++
 drivers/i2c/busses/i2c-designware-core.h           | 26 ++++++++++++++++++++++
 4 files changed, 37 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index fee26dc3e858..8ed2b532cd54 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -20,6 +20,9 @@ Optional properties :
  - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
    This value which is by default 300ns is used to compute the tHIGH period.
 
+ - mode : should be either:
+           - "master" to setup the hardware block as a I2C master
+           - "slave" to setup the hardware block as a I2C slave
 Example :
 
 	i2c@f0000 {
@@ -42,4 +45,5 @@ Example :
 		i2c-sda-hold-time-ns = <300>;
 		i2c-sda-falling-time-ns = <300>;
 		i2c-scl-falling-time-ns = <300>;
+		mode = "slave";
 	};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8e43914023df..0c917b1a4778 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -469,6 +469,7 @@ config I2C_DESIGNWARE_CORE
 
 config I2C_DESIGNWARE_PLATFORM
 	tristate "Synopsys DesignWare Platform"
+	select I2C_SLAVE
 	select I2C_DESIGNWARE_CORE
 	depends on (ACPI && COMMON_CLK) || !ACPI
 	help
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 6afd2ff5d73f..41b38d8b8732 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -55,6 +55,12 @@ static char *abort_sources[] = {
 		"trying to use disabled adapter",
 	[ARB_LOST] =
 		"lost arbitration",
+	[ABRT_SLAVE_FLUSH_TXFIFO] =
+		"read command so flush old data in the TX FIFO",
+	[ABRT_SLAVE_ARBLOST] =
+		"slave lost the bus while transmitting data to a remote master",
+	[ABRT_SLAVE_RD_INTX] =
+		"slave request for data to be transmitted and",
 };
 
 u32 dw_readl(struct dw_i2c_dev *dev, int offset)
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 8bba7a37c3ce..b7dcd134e208 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -36,15 +36,20 @@
 #define DW_IC_CON_SPEED_FAST		0x4
 #define DW_IC_CON_SPEED_HIGH		0x6
 #define DW_IC_CON_SPEED_MASK		0x6
+#define DW_IC_CON_10BITADDR_SLAVE		0x8
 #define DW_IC_CON_10BITADDR_MASTER	0x10
 #define DW_IC_CON_RESTART_EN		0x20
 #define DW_IC_CON_SLAVE_DISABLE		0x40
+#define DW_IC_CON_STOP_DET_IFADDRESSED		0x80
+#define DW_IC_CON_TX_EMPTY_CTRL		0x100
+#define DW_IC_CON_RX_FIFO_FULL_HLD_CTRL		0x200
 
 /*
  * Registers offset
  */
 #define DW_IC_CON		0x0
 #define DW_IC_TAR		0x4
+#define DW_IC_SAR		0x8
 #define DW_IC_DATA_CMD		0x10
 #define DW_IC_SS_SCL_HCNT	0x14
 #define DW_IC_SS_SCL_LCNT	0x18
@@ -75,6 +80,7 @@
 #define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_ENABLE_STATUS	0x9c
+#define DW_IC_CLR_RESTART_DET	0xa8
 #define DW_IC_COMP_PARAM_1	0xf4
 #define DW_IC_COMP_VERSION	0xf8
 #define DW_IC_SDA_HOLD_MIN_VERS	0x3131312A
@@ -93,15 +99,22 @@
 #define DW_IC_INTR_STOP_DET	0x200
 #define DW_IC_INTR_START_DET	0x400
 #define DW_IC_INTR_GEN_CALL	0x800
+#define DW_IC_INTR_RESTART_DET	0x1000
 
 #define DW_IC_INTR_DEFAULT_MASK		(DW_IC_INTR_RX_FULL | \
 					 DW_IC_INTR_TX_ABRT | \
 					 DW_IC_INTR_STOP_DET)
 #define DW_IC_INTR_MASTER_MASK		(DW_IC_INTR_DEFAULT_MASK | \
 					 DW_IC_INTR_TX_EMPTY)
+#define DW_IC_INTR_SLAVE_MASK		(DW_IC_INTR_DEFAULT_MASK | \
+					 DW_IC_INTR_RX_DONE | \
+					 DW_IC_INTR_RX_UNDER | \
+					 DW_IC_INTR_RD_REQ)
+
 #define DW_IC_STATUS_ACTIVITY		0x1
 #define DW_IC_STATUS_TFE		BIT(2)
 #define DW_IC_STATUS_MASTER_ACTIVITY	BIT(5)
+#define DW_IC_STATUS_SLAVE_ACTIVITY	BIT(6)
 
 #define DW_IC_SDA_HOLD_RX_SHIFT		16
 #define DW_IC_SDA_HOLD_RX_MASK		GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
@@ -139,6 +152,9 @@
 #define ABRT_10B_RD_NORSTRT	10
 #define ABRT_MASTER_DIS		11
 #define ARB_LOST		12
+#define ABRT_SLAVE_FLUSH_TXFIFO	13
+#define ABRT_SLAVE_ARBLOST	14
+#define ABRT_SLAVE_RD_INTX	15
 
 #define DW_IC_TX_ABRT_7B_ADDR_NOACK	(1UL << ABRT_7B_ADDR_NOACK)
 #define DW_IC_TX_ABRT_10ADDR1_NOACK	(1UL << ABRT_10ADDR1_NOACK)
@@ -151,6 +167,9 @@
 #define DW_IC_TX_ABRT_10B_RD_NORSTRT	(1UL << ABRT_10B_RD_NORSTRT)
 #define DW_IC_TX_ABRT_MASTER_DIS	(1UL << ABRT_MASTER_DIS)
 #define DW_IC_TX_ARB_LOST		(1UL << ARB_LOST)
+#define DW_IC_RX_ABRT_SLAVE_RD_INTX	(1UL << ABRT_SLAVE_RD_INTX)
+#define DW_IC_RX_ABRT_SLAVE_ARBLOST	(1UL << ABRT_SLAVE_ARBLOST)
+#define DW_IC_RX_ABRT_SLAVE_FLUSH_TXFIFO	(1UL << ABRT_SLAVE_FLUSH_TXFIFO)
 
 #define DW_IC_TX_ABRT_NOACK		(DW_IC_TX_ABRT_7B_ADDR_NOACK | \
 					 DW_IC_TX_ABRT_10ADDR1_NOACK | \
@@ -206,6 +225,7 @@ struct dw_i2c_dev {
 	void __iomem		*base;
 	struct completion	cmd_complete;
 	struct clk		*clk;
+	struct i2c_client		*slave;
 	u32			(*get_clk_rate_khz) (struct dw_i2c_dev *dev);
 	struct dw_pci_controller *controller;
 	int			cmd_err;
@@ -225,6 +245,7 @@ struct dw_i2c_dev {
 	struct i2c_adapter	adapter;
 	u32			functionality;
 	u32			master_cfg;
+	u32			slave_cfg;
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
 	int			rx_outstanding;
@@ -268,6 +289,11 @@ extern void i2c_dw_disable(struct dw_i2c_dev *dev);
 extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
 extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
 extern int i2c_dw_probe(struct dw_i2c_dev *dev);
+extern int i2c_dw_init_slave(struct dw_i2c_dev *dev);
+extern void i2c_dw_disable_slave(struct dw_i2c_dev *dev);
+extern void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev);
+extern u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev);
+extern int i2c_dw_probe_slave(struct dw_i2c_dev *dev);
 
 #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
 extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver
  2016-12-07 17:55 [PATCH v4 0/5] i2c: designware: Add slave support Luis Oliveira
                   ` (2 preceding siblings ...)
  2016-12-07 17:55 ` [PATCH v4 3/5] i2c: designware: Add slave definitions Luis Oliveira
@ 2016-12-07 17:55 ` Luis Oliveira
  2016-12-07 19:07   ` Mark Rutland
                     ` (3 more replies)
  2016-12-07 17:55 ` [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation Luis Oliveira
  4 siblings, 4 replies; 22+ messages in thread
From: Luis Oliveira @ 2016-12-07 17:55 UTC (permalink / raw)
  To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

- Slave mode selected by compatibility string in platform module
- Changes in Makefile to Kbuild successfully compile i2c-designware-core
  with slave functions

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V3->V4: (Andy Shevchenko)
- nothing changed

 drivers/i2c/busses/Makefile                 |   2 +-
 drivers/i2c/busses/i2c-designware-core.h    |   1 +
 drivers/i2c/busses/i2c-designware-platdrv.c |  73 ++++-
 drivers/i2c/busses/i2c-designware-slave.c   | 433 ++++++++++++++++++++++++++++
 4 files changed, 499 insertions(+), 10 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-slave.c

diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 4f8f6a2b9346..c2ed84a86f49 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -40,7 +40,7 @@ obj-$(CONFIG_I2C_CBUS_GPIO)	+= i2c-cbus-gpio.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_DESIGNWARE_CORE)	+= i2c-designware-core.o
-i2c-designware-core-objs := i2c-designware-common.o i2c-designware-master.o
+i2c-designware-core-objs := i2c-designware-common.o i2c-designware-slave.o i2c-designware-master.o
 obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
 i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-designware-baytrail.o
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index b7dcd134e208..a0e5c88858a9 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -265,6 +265,7 @@ struct dw_i2c_dev {
 	void			(*release_lock)(struct dw_i2c_dev *dev);
 	bool			pm_runtime_disabled;
 	bool			dynamic_tar_update_enabled;
+	bool			mode;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index e55b5544c733..300cf2de4ae3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -142,6 +142,8 @@ static void i2c_dw_configure_master(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 
+	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
+
 	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
 			  DW_IC_CON_RESTART_EN;
 
@@ -159,6 +161,30 @@ static void i2c_dw_configure_master(struct platform_device *pdev)
 	}
 }
 
+static void i2c_dw_configure_slave(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+
+	dev->functionality = I2C_FUNC_SLAVE | DW_IC_DEFAULT_FUNCTIONALITY;
+
+	dev->slave_cfg = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL |
+			 DW_IC_CON_RESTART_EN | DW_IC_CON_STOP_DET_IFADDRESSED |
+			 DW_IC_CON_SPEED_FAST;
+
+	dev_info(&pdev->dev, "I am registed as a I2C Slave!\n");
+
+	switch (dev->clk_freq) {
+	case 100000:
+		dev->slave_cfg |= DW_IC_CON_SPEED_STD;
+		break;
+	case 3400000:
+		dev->slave_cfg |= DW_IC_CON_SPEED_HIGH;
+		break;
+	default:
+		dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
+	}
+}
+
 static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
 {
 	if (IS_ERR(i_dev->clk))
@@ -240,9 +266,12 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	dev->functionality = I2C_FUNC_10BIT_ADDR | DW_IC_DEFAULT_FUNCTIONALITY;
-
-	i2c_dw_configure_master(pdev);
+#ifndef CONFIG_ACPI
+	if (!device_property_match_string(&pdev->dev, "mode", "slave"))
+		i2c_dw_configure_slave(pdev);
+	else
+#endif
+		i2c_dw_configure_master(pdev);
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (!i2c_dw_plat_prepare_clk(dev, true)) {
@@ -255,7 +284,14 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	if (!dev->tx_fifo_depth) {
-		u32 param1 = i2c_dw_read_comp_param(dev);
+		u32 param1;
+#ifndef CONFIG_ACPI
+		if (!device_property_match_string(&pdev->dev,
+			 "mode", "slave"))
+			param1 = i2c_dw_read_comp_param_slave(dev);
+		else
+#endif
+			param1 = i2c_dw_read_comp_param(dev);
 
 		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
 		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
@@ -276,8 +312,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		pm_runtime_set_active(&pdev->dev);
 		pm_runtime_enable(&pdev->dev);
 	}
+#ifndef CONFIG_ACPI
+	if (device_property_match_string(&pdev->dev, "mode", "slave") == 0)
+		r = i2c_dw_probe_slave(dev);
+	else
+#endif
+		r = i2c_dw_probe(dev);
 
-	r = i2c_dw_probe(dev);
 	if (r && !dev->pm_runtime_disabled)
 		pm_runtime_disable(&pdev->dev);
 
@@ -292,7 +333,12 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&dev->adapter);
 
-	i2c_dw_disable(dev);
+#ifndef CONFIG_ACPI
+	if (!device_property_match_string(&pdev->dev, "mode", "slave"))
+		i2c_dw_disable_slave(dev);
+	else
+#endif
+		i2c_dw_disable(dev);
 
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
@@ -332,7 +378,12 @@ static int dw_i2c_plat_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
-	i2c_dw_disable(i_dev);
+#ifndef CONFIG_ACPI
+	if (!device_property_match_string(&pdev->dev, "mode", "slave"))
+		i2c_dw_disable_slave(i_dev);
+	else
+#endif
+		i2c_dw_disable(i_dev);
 	i2c_dw_plat_prepare_clk(i_dev, false);
 
 	return 0;
@@ -345,8 +396,12 @@ static int dw_i2c_plat_resume(struct device *dev)
 
 	i2c_dw_plat_prepare_clk(i_dev, true);
 
-	if (!i_dev->pm_runtime_disabled)
-		i2c_dw_init(i_dev);
+	if (!i_dev->pm_runtime_disabled) {
+		if (!device_property_match_string(&pdev->dev, "mode", "slave"))
+			i2c_dw_init_slave(i_dev);
+		else
+			i2c_dw_init(i_dev);
+	}
 
 	return 0;
 }
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
new file mode 100644
index 000000000000..1c7f82bb2513
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -0,0 +1,433 @@
+/*
+ * Synopsys DesignWare I2C adapter driver (master only).
+ *
+ * Based on the TI DAVINCI I2C adapter driver.
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software Inc.
+ * Copyright (C) 2009 Provigent Ltd.
+ *
+ * ----------------------------------------------------------------------------
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * ----------------------------------------------------------------------------
+ *
+ */
+#include <linux/export.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/pm_runtime.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include "i2c-designware-core.h"
+
+static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev)
+{
+	/* Configure Tx/Rx FIFO threshold levels */
+	dw_writel(dev, 0, DW_IC_TX_TL);
+	dw_writel(dev, 0, DW_IC_RX_TL);
+
+	/* configure the I2C slave */
+	dw_writel(dev, dev->slave_cfg, DW_IC_CON);
+	dw_writel(dev, DW_IC_INTR_SLAVE_MASK, DW_IC_INTR_MASK);
+}
+
+/**
+ * i2c_dw_init_slave() - initialize the designware i2c slave hardware
+ * @dev: device private data
+ *
+ * This functions configures and enables the I2C.
+ * This function is called during I2C init function, and in case of timeout at
+ * run time.
+ */
+int i2c_dw_init_slave(struct dw_i2c_dev *dev)
+{
+	u32 hcnt, lcnt;
+	u32 reg, comp_param1;
+	u32 sda_falling_time, scl_falling_time;
+	int ret;
+
+	ret = i2c_dw_acquire_lock(dev);
+	if (ret)
+		return ret;
+
+	reg = dw_readl(dev, DW_IC_COMP_TYPE);
+	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
+		/* Configure register endianness access */
+		dev->accessor_flags |= ACCESS_SWAP;
+	} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
+		/* Configure register access mode 16bit */
+		dev->accessor_flags |= ACCESS_16BIT;
+	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
+		dev_err(dev->dev, "Unknown Synopsys component type: "
+			"0x%08x\n", reg);
+		i2c_dw_release_lock(dev);
+		return -ENODEV;
+	}
+
+	comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1);
+
+	/* Disable the adapter */
+	__i2c_dw_enable_and_wait(dev, false);
+
+	/* set standard and fast speed deviders for high/low periods */
+	sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
+	scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
+
+	/* Set SCL timing parameters for standard-mode */
+	if (dev->ss_hcnt && dev->ss_lcnt) {
+		hcnt = dev->ss_hcnt;
+		lcnt = dev->ss_lcnt;
+	} else {
+		hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+					4000,	/* tHD;STA = tHIGH = 4.0 us */
+					sda_falling_time,
+					0,	/* 0: DW default, 1: Ideal */
+					0);	/* No offset */
+		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+					4700,	/* tLOW = 4.7 us */
+					scl_falling_time,
+					0);	/* No offset */
+	}
+	dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
+	dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
+	dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
+
+	/* Set SCL timing parameters for fast-mode or fast-mode plus */
+	if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev->fp_lcnt) {
+		hcnt = dev->fp_hcnt;
+		lcnt = dev->fp_lcnt;
+	} else if (dev->fs_hcnt && dev->fs_lcnt) {
+		hcnt = dev->fs_hcnt;
+		lcnt = dev->fs_lcnt;
+	} else {
+		hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+					600,	/* tHD;STA = tHIGH = 0.6 us */
+					sda_falling_time,
+					0,	/* 0: DW default, 1: Ideal */
+					0);	/* No offset */
+		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+					1300,	/* tLOW = 1.3 us */
+					scl_falling_time,
+					0);	/* No offset */
+	}
+	dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
+	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
+	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
+
+	if ((dev->slave_cfg & DW_IC_CON_SPEED_MASK) ==
+		DW_IC_CON_SPEED_HIGH) {
+		if ((comp_param1 & DW_IC_COMP_PARAM_1_SPEED_MODE_MASK)
+			!= DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) {
+			dev_err(dev->dev, "High Speed not supported!\n");
+			dev->slave_cfg &= ~DW_IC_CON_SPEED_MASK;
+			dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
+		} else if (dev->hs_hcnt && dev->hs_lcnt) {
+			hcnt = dev->hs_hcnt;
+			lcnt = dev->hs_lcnt;
+			dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT);
+			dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT);
+			dev_dbg(dev->dev, "HighSpeed-mode HCNT:LCNT = %d:%d\n",
+				hcnt, lcnt);
+		}
+	}
+
+	/* Configure SDA Hold Time if required */
+	reg = dw_readl(dev, DW_IC_COMP_VERSION);
+	reg = dw_readl(dev, DW_IC_COMP_VERSION);
+	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
+		if (!dev->sda_hold_time) {
+			/* Keep previous hold time setting if no one set it */
+			dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
+		}
+		/*
+		 * Workaround for avoiding TX arbitration lost in case I2C
+		 * slave pulls SDA down "too quickly" after falling egde of
+		 * SCL by enabling non-zero SDA RX hold. Specification says it
+		 * extends incoming SDA low to high transition while SCL is
+		 * high but it apprears to help also above issue.
+		 */
+		if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
+			dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+	} else {
+		dev_warn(dev->dev,
+			"Hardware too old to adjust SDA hold time.\n");
+	}
+
+	i2c_dw_configure_fifo_slave(dev);
+	i2c_dw_release_lock(dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_init_slave);
+
+int i2c_dw_reg_slave(struct i2c_client *slave)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
+
+	if (dev->slave)
+		return -EBUSY;
+	if (slave->flags & I2C_CLIENT_TEN)
+		return -EAFNOSUPPORT;
+		/* set slave address in the IC_SAR register,
+		* the address to which the DW_apb_i2c responds */
+
+	__i2c_dw_enable(dev, false);
+	dw_writel(dev, slave->addr, DW_IC_SAR);
+	dev->slave = slave;
+
+	__i2c_dw_enable(dev, true);
+
+	dev->cmd_err = 0;
+	dev->msg_write_idx = 0;
+	dev->msg_read_idx = 0;
+	dev->msg_err = 0;
+	dev->status = STATUS_IDLE;
+	dev->abort_source = 0;
+	dev->rx_outstanding = 0;
+
+	return 0;
+}
+
+static int i2c_dw_unreg_slave(struct i2c_client *slave)
+{
+	struct dw_i2c_dev *dev =  i2c_get_adapdata(slave->adapter);
+
+	i2c_dw_disable_int_slave(dev);
+	i2c_dw_disable_slave(dev);
+	dev->slave =  NULL;
+
+	return 0;
+}
+
+static u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev)
+{
+	u32 stat;
+
+	/*
+	 * The IC_INTR_STAT register just indicates "enabled" interrupts.
+	 * Ths unmasked raw version of interrupt status bits are available
+	 * in the IC_RAW_INTR_STAT register.
+	 *
+	 * That is,
+	 *   stat = dw_readl(IC_INTR_STAT);
+	 * equals to,
+	 *   stat = dw_readl(IC_RAW_INTR_STAT) & dw_readl(IC_INTR_MASK);
+	 *
+	 * The raw version might be useful for debugging purposes.
+	 */
+	stat = dw_readl(dev, DW_IC_INTR_STAT);
+
+	/*
+	 * Do not use the IC_CLR_INTR register to clear interrupts, or
+	 * you'll miss some interrupts, triggered during the period from
+	 * dw_readl(IC_INTR_STAT) to dw_readl(IC_CLR_INTR).
+	 *
+	 * Instead, use the separately-prepared IC_CLR_* registers.
+	 */
+	if (stat & DW_IC_INTR_TX_ABRT)
+		dw_readl(dev, DW_IC_CLR_TX_ABRT);
+	if (stat & DW_IC_INTR_RX_UNDER)
+		dw_readl(dev, DW_IC_CLR_RX_UNDER);
+	if (stat & DW_IC_INTR_RX_OVER)
+		dw_readl(dev, DW_IC_CLR_RX_OVER);
+	if (stat & DW_IC_INTR_TX_OVER)
+		dw_readl(dev, DW_IC_CLR_TX_OVER);
+	if (stat & DW_IC_INTR_RX_DONE)
+		dw_readl(dev, DW_IC_CLR_RX_DONE);
+	if (stat & DW_IC_INTR_ACTIVITY)
+		dw_readl(dev, DW_IC_CLR_ACTIVITY);
+	if (stat & DW_IC_INTR_STOP_DET)
+		dw_readl(dev, DW_IC_CLR_STOP_DET);
+	if (stat & DW_IC_INTR_START_DET)
+		dw_readl(dev, DW_IC_CLR_START_DET);
+	if (stat & DW_IC_INTR_GEN_CALL)
+		dw_readl(dev, DW_IC_CLR_GEN_CALL);
+
+	return stat;
+}
+
+/*
+ * Interrupt service routine. This gets called whenever an I2C slave interrupt
+ * occurs.
+ */
+
+static bool i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
+{
+	u32 raw_stat, stat, enabled;
+	u8 val, slave_activity;
+
+	stat = dw_readl(dev, DW_IC_INTR_STAT);
+	enabled = dw_readl(dev, DW_IC_ENABLE);
+	raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
+	slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
+		 DW_IC_STATUS_SLAVE_ACTIVITY)>>6);
+
+	if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY))
+		return false;
+
+	dev_dbg(dev->dev,
+	 "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x : INTR_STAT=%#x\n",
+	 __func__, enabled, slave_activity, raw_stat, stat);
+
+	if (stat & DW_IC_INTR_RESTART_DET)
+		dw_readl(dev, DW_IC_CLR_RESTART_DET);
+	if (stat & DW_IC_INTR_START_DET)
+		dw_readl(dev, DW_IC_CLR_START_DET);
+	if (stat & DW_IC_INTR_ACTIVITY)
+		dw_readl(dev, DW_IC_CLR_ACTIVITY);
+	if (stat & DW_IC_INTR_RX_OVER)
+		dw_readl(dev, DW_IC_CLR_RX_OVER);
+	if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
+		i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
+
+	if (slave_activity) {
+		if (stat & DW_IC_INTR_RD_REQ) {
+			if (stat & DW_IC_INTR_RX_FULL) {
+				val = dw_readl(dev, DW_IC_DATA_CMD);
+				if (!i2c_slave_event(dev->slave,
+				 I2C_SLAVE_WRITE_RECEIVED, &val)) {
+					dev_dbg(dev->dev, "Byte %X acked!",
+					 val);
+				}
+				dw_readl(dev, DW_IC_CLR_RD_REQ);
+				stat = i2c_dw_read_clear_intrbits_slave(dev);
+			} else {
+				dw_readl(dev, DW_IC_CLR_RD_REQ);
+				dw_readl(dev, DW_IC_CLR_RX_UNDER);
+				stat = i2c_dw_read_clear_intrbits_slave(dev);
+			}
+			if (!i2c_slave_event(dev->slave,
+					 I2C_SLAVE_READ_REQUESTED, &val))
+				dw_writel(dev, val, DW_IC_DATA_CMD);
+		}
+	}
+
+	if (stat & DW_IC_INTR_RX_DONE) {
+		if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
+		 &val))
+			dw_readl(dev, DW_IC_CLR_RX_DONE);
+
+		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+		stat = i2c_dw_read_clear_intrbits_slave(dev);
+		return true;
+	}
+
+	if (stat & DW_IC_INTR_RX_FULL) {
+		val = dw_readl(dev, DW_IC_DATA_CMD);
+		if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
+		 &val))
+			dev_dbg(dev->dev, "Byte %X acked!", val);
+	} else {
+		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+		stat = i2c_dw_read_clear_intrbits_slave(dev);
+	}
+
+	if (stat & DW_IC_INTR_TX_OVER)
+		dw_readl(dev, DW_IC_CLR_TX_OVER);
+
+	return true;
+}
+
+static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
+{
+	struct dw_i2c_dev *dev = dev_id;
+
+	i2c_dw_read_clear_intrbits_slave(dev);
+	if (!i2c_dw_irq_handler_slave(dev))
+		return IRQ_NONE;
+
+	complete(&dev->cmd_complete);
+	return IRQ_HANDLED;
+}
+
+static struct i2c_algorithm i2c_dw_algo = {
+	.functionality	= i2c_dw_func,
+	.reg_slave	= i2c_dw_reg_slave,
+	.unreg_slave	= i2c_dw_unreg_slave,
+};
+
+void i2c_dw_disable_slave(struct dw_i2c_dev *dev)
+{
+	/* Disable controller */
+	__i2c_dw_enable_and_wait(dev, false);
+
+	/* Disable all interupts */
+	dw_writel(dev, 0, DW_IC_INTR_MASK);
+	dw_readl(dev, DW_IC_CLR_INTR);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_disable_slave);
+
+void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev)
+{
+	dw_writel(dev, 0, DW_IC_INTR_MASK);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_disable_int_slave);
+
+u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev)
+{
+	return dw_readl(dev, DW_IC_COMP_PARAM_1);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param_slave);
+
+int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
+{
+	struct i2c_adapter *adap = &dev->adapter;
+	int r;
+
+	init_completion(&dev->cmd_complete);
+
+	r = i2c_dw_init_slave(dev);
+	if (r)
+		return r;
+
+	r = i2c_dw_acquire_lock(dev);
+	if (r)
+		return r;
+
+	i2c_dw_release_lock(dev);
+	snprintf(adap->name, sizeof(adap->name),
+		 "Synopsys DesignWare I2C Slave adapter");
+	adap->retries = 3;
+	adap->algo = &i2c_dw_algo;
+	adap->dev.parent = dev->dev;
+	i2c_set_adapdata(adap, dev);
+
+	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
+			     IRQF_SHARED | IRQF_COND_SUSPEND,
+			     dev_name(dev->dev), dev);
+	if (r) {
+		dev_err(dev->dev, "failure requesting irq %i: %d\n",
+			dev->irq, r);
+		return r;
+	}
+	/*
+	 * Increment PM usage count during adapter registration in order to
+	 * avoid possible spurious runtime suspend when adapter device is
+	 * registered to the device core and immediate resume in case bus has
+	 * registered I2C slaves that do I2C transfers in their probe.
+	 */
+	pm_runtime_get_noresume(dev->dev);
+	r = i2c_add_numbered_adapter(adap);
+	if (r)
+		dev_err(dev->dev, "failure adding adapter: %d\n", r);
+	pm_runtime_put_noidle(dev->dev);
+
+	return r;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_probe_slave);
+
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus slave adapter");
+MODULE_LICENSE("GPL");
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation
  2016-12-07 17:55 [PATCH v4 0/5] i2c: designware: Add slave support Luis Oliveira
                   ` (3 preceding siblings ...)
  2016-12-07 17:55 ` [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver Luis Oliveira
@ 2016-12-07 17:55 ` Luis Oliveira
  2016-12-07 19:52   ` Andy Shevchenko
  2016-12-13 15:08   ` Jarkko Nikula
  4 siblings, 2 replies; 22+ messages in thread
From: Luis Oliveira @ 2016-12-07 17:55 UTC (permalink / raw)
  To: wsa, robh+dt, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

- Missspelling, comment formatation and fix a string of
  the existing code

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
Changes V3->V4: (Andy Shevchenko)
- created a commit message

 drivers/i2c/busses/i2c-designware-common.c |  2 +-
 drivers/i2c/busses/i2c-designware-slave.c  | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 41b38d8b8732..838ef662d2c8 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -42,7 +42,7 @@ static char *abort_sources[] = {
 	[ABRT_TXDATA_NOACK] =
 		"data not acknowledged",
 	[ABRT_GCALL_NOACK] =
-		"no acknowledgement for a general call",
+		"no acknowledgment for a general call",
 	[ABRT_GCALL_READ] =
 		"read after general call",
 	[ABRT_SBYTE_ACKDET] =
diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index 1c7f82bb2513..442bc5ce6d47 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -70,8 +70,8 @@ int i2c_dw_init_slave(struct dw_i2c_dev *dev)
 		/* Configure register access mode 16bit */
 		dev->accessor_flags |= ACCESS_16BIT;
 	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
-		dev_err(dev->dev, "Unknown Synopsys component type: "
-			"0x%08x\n", reg);
+		dev_err(dev->dev,
+		 "Unknown Synopsys component type: 0x%08x\n", reg);
 		i2c_dw_release_lock(dev);
 		return -ENODEV;
 	}
@@ -181,8 +181,10 @@ int i2c_dw_reg_slave(struct i2c_client *slave)
 		return -EBUSY;
 	if (slave->flags & I2C_CLIENT_TEN)
 		return -EAFNOSUPPORT;
-		/* set slave address in the IC_SAR register,
-		* the address to which the DW_apb_i2c responds */
+		/*
+		 * set slave address in the IC_SAR register,
+		 * the address to which the DW_apb_i2c responds
+		 */
 
 	__i2c_dw_enable(dev, false);
 	dw_writel(dev, slave->addr, DW_IC_SAR);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 1/5] i2c: designware: Refactoring of the i2c-designware
  2016-12-07 17:55 ` [PATCH v4 1/5] i2c: designware: Refactoring of the i2c-designware Luis Oliveira
@ 2016-12-07 18:58   ` Andy Shevchenko
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2016-12-07 18:58 UTC (permalink / raw)
  To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On Wed, 2016-12-07 at 17:55 +0000, Luis Oliveira wrote:
> - Factor out all _master() part of code from i2c-designware-core
>   and i2c-designware-platdrv to separate functions.
> - Standardize all code related with MASTER modes.
> 

Couple of comments, after addressing them
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> +	if ((dev->master_cfg & DW_IC_CON_MASTER) &&
> +		 (dev->master_cfg & DW_IC_CON_SLAVE_DISABLE))
> +		i2c_dw_configure_fifo_master(dev);

So, logically it's a part of slave patch.
For now it would be just 
 i2c_dw_configure_fifo_master(dev);

> +static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> +{
> +	struct dw_i2c_dev *dev = dev_id;
> +	u32 stat, enabled, mode;

mode is unused here, this is a part of slave patch either.

> +static void i2c_dw_configure_master(struct platform_device *pdev)
> +{
> +	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);

> +	dev_info(&pdev->dev, "I am registed as a I2C Master!\n");

I don't want bikeshedding here, but the question just comes:
"Do we need to have this available via sysfs as a part of ABI?" So. user
space can check for / set a mode.

In any case this one is a separate story and another patch, here just to
make the message less annoying, it looks like dev_dbg() to me.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 2/5] i2c: designware: Master mode as separated driver
  2016-12-07 17:55 ` [PATCH v4 2/5] i2c: designware: Master mode as separated driver Luis Oliveira
@ 2016-12-07 19:03   ` Andy Shevchenko
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2016-12-07 19:03 UTC (permalink / raw)
  To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On Wed, 2016-12-07 at 17:55 +0000, Luis Oliveira wrote:
> - The functions related to I2C master mode of operation were moved
>   to a single file: i2c-designware-master.c
> - Common functions were moved into i2c-designware-common.c
> - Common definitions were moved into i2c-designware-core.h (were in
> core.c)
> 

Yeah, there are some places that might be cleaned up but it came from
the original and could be done in the future.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
> Changes V3->V4: (Andy Shevchenko)
> - The name of the i2c-designware-src.c was changed to i2c-designware-
> common.c
>   as suggested by Andy.
> 
>  drivers/i2c/busses/Makefile                        |   1 +
>  drivers/i2c/busses/i2c-designware-common.c         | 252
> +++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h           | 131 ++++++++
>  ...c-designware-core.c => i2c-designware-master.c} | 347 +-----------
> ---------
>  4 files changed, 390 insertions(+), 341 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-designware-common.c
>  rename drivers/i2c/busses/{i2c-designware-core.c => i2c-designware-
> master.c} (66%)
> 
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 1c1bac87a9db..4f8f6a2b9346 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_I2C_CBUS_GPIO)	+= i2c-cbus-
> gpio.o
>  obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
>  obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
>  obj-$(CONFIG_I2C_DESIGNWARE_CORE)	+= i2c-designware-core.o
> +i2c-designware-core-objs := i2c-designware-common.o i2c-designware-
> master.o
>  obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-
> platform.o
>  i2c-designware-platform-objs := i2c-designware-platdrv.o
>  i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c-
> designware-baytrail.o
> diff --git a/drivers/i2c/busses/i2c-designware-common.c
> b/drivers/i2c/busses/i2c-designware-common.c
> new file mode 100644
> index 000000000000..6afd2ff5d73f
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -0,0 +1,252 @@
> +/*
> + * Synopsys DesignWare I2C adapter driver (master only).
> + *
> + * Based on the TI DAVINCI I2C adapter driver.
> + *
> + * Copyright (C) 2006 Texas Instruments.
> + * Copyright (C) 2007 MontaVista Software Inc.
> + * Copyright (C) 2009 Provigent Ltd.
> + *
> + * ------------------------------------------------------------------
> ----------
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + * ------------------------------------------------------------------
> ----------
> + *
> + */
> +#include <linux/export.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include "i2c-designware-core.h"
> +
> +static char *abort_sources[] = {
> +	[ABRT_7B_ADDR_NOACK] =
> +		"slave address not acknowledged (7bit mode)",
> +	[ABRT_10ADDR1_NOACK] =
> +		"first address byte not acknowledged (10bit mode)",
> +	[ABRT_10ADDR2_NOACK] =
> +		"second address byte not acknowledged (10bit mode)",
> +	[ABRT_TXDATA_NOACK] =
> +		"data not acknowledged",
> +	[ABRT_GCALL_NOACK] =
> +		"no acknowledgement for a general call",
> +	[ABRT_GCALL_READ] =
> +		"read after general call",
> +	[ABRT_SBYTE_ACKDET] =
> +		"start byte acknowledged",
> +	[ABRT_SBYTE_NORSTRT] =
> +		"trying to send start byte when restart is disabled",
> +	[ABRT_10B_RD_NORSTRT] =
> +		"trying to read when restart is disabled (10bit
> mode)",
> +	[ABRT_MASTER_DIS] =
> +		"trying to use disabled adapter",
> +	[ARB_LOST] =
> +		"lost arbitration",
> +};
> +
> +u32 dw_readl(struct dw_i2c_dev *dev, int offset)
> +{
> +	u32 value;
> +
> +	if (dev->accessor_flags & ACCESS_16BIT)
> +		value = readw_relaxed(dev->base + offset) |
> +			(readw_relaxed(dev->base + offset + 2) <<
> 16);
> +	else
> +		value = readl_relaxed(dev->base + offset);
> +
> +	if (dev->accessor_flags & ACCESS_SWAP)
> +		return swab32(value);
> +	else
> +		return value;
> +}
> +
> +void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
> +{
> +	if (dev->accessor_flags & ACCESS_SWAP)
> +		b = swab32(b);
> +
> +	if (dev->accessor_flags & ACCESS_16BIT) {
> +		writew_relaxed((u16)b, dev->base + offset);
> +		writew_relaxed((u16)(b >> 16), dev->base + offset +
> 2);
> +	} else {
> +		writel_relaxed(b, dev->base + offset);
> +	}
> +}
> +
> +u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int
> offset)
> +{
> +	/*
> +	 * DesignWare I2C core doesn't seem to have solid strategy to
> meet
> +	 * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH
> spec
> +	 * will result in violation of the tHD;STA spec.
> +	 */
> +	if (cond)
> +		/*
> +		 * Conditional expression:
> +		 *
> +		 *   IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH
> +		 *
> +		 * This is based on the DW manuals, and represents an
> ideal
> +		 * configuration.  The resulting I2C bus speed will
> be
> +		 * faster than any of the others.
> +		 *
> +		 * If your hardware is free from tHD;STA issue, try
> this one.
> +		 */
> +		return (ic_clk * tSYMBOL + 500000) / 1000000 - 8 +
> offset;
> +	else
> +		/*
> +		 * Conditional expression:
> +		 *
> +		 *   IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)
> +		 *
> +		 * This is just experimental rule; the tHD;STA period
> turned
> +		 * out to be proportinal to (_HCNT + 3).  With this
> setting,
> +		 * we could meet both tHIGH and tHD;STA timing specs.
> +		 *
> +		 * If unsure, you'd better to take this alternative.
> +		 *
> +		 * The reason why we need to take into account "tf"
> here,
> +		 * is the same as described in i2c_dw_scl_lcnt().
> +		 */
> +		return (ic_clk * (tSYMBOL + tf) + 500000) / 1000000
> +			- 3 + offset;
> +}
> +
> +u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
> +{
> +	/*
> +	 * Conditional expression:
> +	 *
> +	 *   IC_[FS]S_SCL_LCNT + 1 >= IC_CLK * (tLOW + tf)
> +	 *
> +	 * DW I2C core starts counting the SCL CNTs for the LOW
> period
> +	 * of the SCL clock (tLOW) as soon as it pulls the SCL line.
> +	 * In order to meet the tLOW timing spec, we need to take
> into
> +	 * account the fall time of SCL signal (tf).  Default tf
> value
> +	 * should be 0.3 us, for safety.
> +	 */
> +	return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 +
> offset;
> +}
> +
> +void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> +{
> +	dw_writel(dev, enable, DW_IC_ENABLE);
> +}
> +
> +void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable)
> +{
> +	int timeout = 100;
> +
> +	do {
> +		__i2c_dw_enable(dev, enable);
> +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) ==
> enable)
> +			return;
> +
> +		/*
> +		 * Wait 10 times the signaling period of the highest
> I2C
> +		 * transfer supported by the driver (for 400KHz this
> is
> +		 * 25us) as described in the DesignWare I2C databook.
> +		 */
> +		usleep_range(25, 250);
> +	} while (timeout--);
> +
> +	dev_warn(dev->dev, "timeout in %sabling adapter\n",
> +		 enable ? "en" : "dis");
> +}
> +
> +unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
> +{
> +	/*
> +	 * Clock is not necessary if we got LCNT/HCNT values directly
> from
> +	 * the platform code.
> +	 */
> +	if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
> +		return 0;
> +	return dev->get_clk_rate_khz(dev);
> +}
> +
> +int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
> +{
> +	int ret;
> +
> +	if (!dev->acquire_lock)
> +		return 0;
> +
> +	ret = dev->acquire_lock(dev);
> +	if (!ret)
> +		return 0;
> +
> +	dev_err(dev->dev, "couldn't acquire bus ownership\n");
> +
> +	return ret;
> +}
> +
> +void i2c_dw_release_lock(struct dw_i2c_dev *dev)
> +{
> +	if (dev->release_lock)
> +		dev->release_lock(dev);
> +}
> +
> +/*
> + * Waiting for bus not busy
> + */
> +int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
> +{
> +	int timeout = TIMEOUT;
> +
> +	while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
> +		if (timeout <= 0) {
> +			dev_warn(dev->dev, "timeout waiting for bus
> ready\n");
> +			return -ETIMEDOUT;
> +		}
> +		timeout--;
> +		usleep_range(1000, 1100);
> +	}
> +
> +	return 0;
> +}
> +
> +int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
> +{
> +	unsigned long abort_source = dev->abort_source;
> +	int i;
> +
> +	if (abort_source & DW_IC_TX_ABRT_NOACK) {
> +		for_each_set_bit(i, &abort_source,
> ARRAY_SIZE(abort_sources))
> +			dev_dbg(dev->dev,
> +				"%s: %s\n", __func__,
> abort_sources[i]);
> +		return -EREMOTEIO;
> +	}
> +
> +	for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
> +		dev_err(dev->dev, "%s: %s\n", __func__,
> abort_sources[i]);
> +
> +	if (abort_source & DW_IC_TX_ARB_LOST)
> +		return -EAGAIN;
> +	else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
> +		return -EINVAL; /* wrong msgs[] data */
> +	else
> +		return -EIO;
> +}
> +
> +u32 i2c_dw_func(struct i2c_adapter *adap)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	return dev->functionality;
> +}
> +
> +MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index 26250b425e2f..8bba7a37c3ce 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -40,6 +40,124 @@
>  #define DW_IC_CON_RESTART_EN		0x20
>  #define DW_IC_CON_SLAVE_DISABLE		0x40
>  
> +/*
> + * Registers offset
> + */
> +#define DW_IC_CON		0x0
> +#define DW_IC_TAR		0x4
> +#define DW_IC_DATA_CMD		0x10
> +#define DW_IC_SS_SCL_HCNT	0x14
> +#define DW_IC_SS_SCL_LCNT	0x18
> +#define DW_IC_FS_SCL_HCNT	0x1c
> +#define DW_IC_FS_SCL_LCNT	0x20
> +#define DW_IC_HS_SCL_HCNT	0x24
> +#define DW_IC_HS_SCL_LCNT	0x28
> +#define DW_IC_INTR_STAT		0x2c
> +#define DW_IC_INTR_MASK		0x30
> +#define DW_IC_RAW_INTR_STAT	0x34
> +#define DW_IC_RX_TL		0x38
> +#define DW_IC_TX_TL		0x3c
> +#define DW_IC_CLR_INTR		0x40
> +#define DW_IC_CLR_RX_UNDER	0x44
> +#define DW_IC_CLR_RX_OVER	0x48
> +#define DW_IC_CLR_TX_OVER	0x4c
> +#define DW_IC_CLR_RD_REQ	0x50
> +#define DW_IC_CLR_TX_ABRT	0x54
> +#define DW_IC_CLR_RX_DONE	0x58
> +#define DW_IC_CLR_ACTIVITY	0x5c
> +#define DW_IC_CLR_STOP_DET	0x60
> +#define DW_IC_CLR_START_DET	0x64
> +#define DW_IC_CLR_GEN_CALL	0x68
> +#define DW_IC_ENABLE		0x6c
> +#define DW_IC_STATUS		0x70
> +#define DW_IC_TXFLR		0x74
> +#define DW_IC_RXFLR		0x78
> +#define DW_IC_SDA_HOLD		0x7c
> +#define DW_IC_TX_ABRT_SOURCE	0x80
> +#define DW_IC_ENABLE_STATUS	0x9c
> +#define DW_IC_COMP_PARAM_1	0xf4
> +#define DW_IC_COMP_VERSION	0xf8
> +#define DW_IC_SDA_HOLD_MIN_VERS	0x3131312A
> +#define DW_IC_COMP_TYPE		0xfc
> +#define DW_IC_COMP_TYPE_VALUE	0x44570140
> +
> +#define DW_IC_INTR_RX_UNDER	0x001
> +#define DW_IC_INTR_RX_OVER	0x002
> +#define DW_IC_INTR_RX_FULL	0x004
> +#define DW_IC_INTR_TX_OVER	0x008
> +#define DW_IC_INTR_TX_EMPTY	0x010
> +#define DW_IC_INTR_RD_REQ	0x020
> +#define DW_IC_INTR_TX_ABRT	0x040
> +#define DW_IC_INTR_RX_DONE	0x080
> +#define DW_IC_INTR_ACTIVITY	0x100
> +#define DW_IC_INTR_STOP_DET	0x200
> +#define DW_IC_INTR_START_DET	0x400
> +#define DW_IC_INTR_GEN_CALL	0x800
> +
> +#define DW_IC_INTR_DEFAULT_MASK		(DW_IC_INTR_RX_FULL |
> \
> +					 DW_IC_INTR_TX_ABRT | \
> +					 DW_IC_INTR_STOP_DET)
> +#define DW_IC_INTR_MASTER_MASK		(DW_IC_INTR_DEFAULT_MAS
> K | \
> +					 DW_IC_INTR_TX_EMPTY)
> +#define DW_IC_STATUS_ACTIVITY		0x1
> +#define DW_IC_STATUS_TFE		BIT(2)
> +#define DW_IC_STATUS_MASTER_ACTIVITY	BIT(5)
> +
> +#define DW_IC_SDA_HOLD_RX_SHIFT		16
> +#define DW_IC_SDA_HOLD_RX_MASK		GENMASK(23,
> DW_IC_SDA_HOLD_RX_SHIFT)
> +
> +#define DW_IC_ERR_TX_ABRT	0x1
> +
> +#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
> +
> +#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH	(BIT(2) | BIT(3))
> +#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK	GENMASK(3, 2)
> +
> +/*
> + * status codes
> + */
> +#define STATUS_IDLE			0x0
> +#define STATUS_WRITE_IN_PROGRESS	0x1
> +#define STATUS_READ_IN_PROGRESS		0x2
> +
> +#define TIMEOUT			20 /* ms */
> +
> +/*
> + * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
> + *
> + * only expected abort codes are listed here
> + * refer to the datasheet for the full list
> + */
> +#define ABRT_7B_ADDR_NOACK	0
> +#define ABRT_10ADDR1_NOACK	1
> +#define ABRT_10ADDR2_NOACK	2
> +#define ABRT_TXDATA_NOACK	3
> +#define ABRT_GCALL_NOACK	4
> +#define ABRT_GCALL_READ		5
> +#define ABRT_SBYTE_ACKDET	7
> +#define ABRT_SBYTE_NORSTRT	9
> +#define ABRT_10B_RD_NORSTRT	10
> +#define ABRT_MASTER_DIS		11
> +#define ARB_LOST		12
> +
> +#define DW_IC_TX_ABRT_7B_ADDR_NOACK	(1UL <<
> ABRT_7B_ADDR_NOACK)
> +#define DW_IC_TX_ABRT_10ADDR1_NOACK	(1UL <<
> ABRT_10ADDR1_NOACK)
> +#define DW_IC_TX_ABRT_10ADDR2_NOACK	(1UL <<
> ABRT_10ADDR2_NOACK)
> +#define DW_IC_TX_ABRT_TXDATA_NOACK	(1UL << ABRT_TXDATA_NOACK)
> +#define DW_IC_TX_ABRT_GCALL_NOACK	(1UL << ABRT_GCALL_NOACK)
> +#define DW_IC_TX_ABRT_GCALL_READ	(1UL << ABRT_GCALL_READ)
> +#define DW_IC_TX_ABRT_SBYTE_ACKDET	(1UL << ABRT_SBYTE_ACKDET)
> +#define DW_IC_TX_ABRT_SBYTE_NORSTRT	(1UL <<
> ABRT_SBYTE_NORSTRT)
> +#define DW_IC_TX_ABRT_10B_RD_NORSTRT	(1UL <<
> ABRT_10B_RD_NORSTRT)
> +#define DW_IC_TX_ABRT_MASTER_DIS	(1UL << ABRT_MASTER_DIS)
> +#define DW_IC_TX_ARB_LOST		(1UL << ARB_LOST)
> +
> +#define DW_IC_TX_ABRT_NOACK		(DW_IC_TX_ABRT_7B_ADDR_NOA
> CK | \
> +					 DW_IC_TX_ABRT_10ADDR1_NOACK
> | \
> +					 DW_IC_TX_ABRT_10ADDR2_NOACK
> | \
> +					 DW_IC_TX_ABRT_TXDATA_NOACK |
> \
> +					 DW_IC_TX_ABRT_GCALL_NOACK)
> +
>  
>  /**
>   * struct dw_i2c_dev - private i2c-designware data
> @@ -132,6 +250,19 @@ struct dw_i2c_dev {
>  #define ACCESS_16BIT		0x00000002
>  #define ACCESS_INTR_MASK	0x00000004
>  
> +u32 dw_readl(struct dw_i2c_dev *dev, int offset);
> +void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
> +u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int
> offset);
> +u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset);
> +void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable);
> +void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable);
> +unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev);
> +int i2c_dw_acquire_lock(struct dw_i2c_dev *dev);
> +void i2c_dw_release_lock(struct dw_i2c_dev *dev);
> +int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev);
> +int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev);
> +u32 i2c_dw_func(struct i2c_adapter *adap);
> +
>  extern int i2c_dw_init(struct dw_i2c_dev *dev);
>  extern void i2c_dw_disable(struct dw_i2c_dev *dev);
>  extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
> diff --git a/drivers/i2c/busses/i2c-designware-core.c
> b/drivers/i2c/busses/i2c-designware-master.c
> similarity index 66%
> rename from drivers/i2c/busses/i2c-designware-core.c
> rename to drivers/i2c/busses/i2c-designware-master.c
> index a51addfde565..9943addac3d7 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -32,305 +32,18 @@
>  #include <linux/module.h>
>  #include "i2c-designware-core.h"
>  
> -/*
> - * Registers offset
> - */
> -#define DW_IC_CON		0x0
> -#define DW_IC_TAR		0x4
> -#define DW_IC_DATA_CMD		0x10
> -#define DW_IC_SS_SCL_HCNT	0x14
> -#define DW_IC_SS_SCL_LCNT	0x18
> -#define DW_IC_FS_SCL_HCNT	0x1c
> -#define DW_IC_FS_SCL_LCNT	0x20
> -#define DW_IC_HS_SCL_HCNT	0x24
> -#define DW_IC_HS_SCL_LCNT	0x28
> -#define DW_IC_INTR_STAT		0x2c
> -#define DW_IC_INTR_MASK		0x30
> -#define DW_IC_RAW_INTR_STAT	0x34
> -#define DW_IC_RX_TL		0x38
> -#define DW_IC_TX_TL		0x3c
> -#define DW_IC_CLR_INTR		0x40
> -#define DW_IC_CLR_RX_UNDER	0x44
> -#define DW_IC_CLR_RX_OVER	0x48
> -#define DW_IC_CLR_TX_OVER	0x4c
> -#define DW_IC_CLR_RD_REQ	0x50
> -#define DW_IC_CLR_TX_ABRT	0x54
> -#define DW_IC_CLR_RX_DONE	0x58
> -#define DW_IC_CLR_ACTIVITY	0x5c
> -#define DW_IC_CLR_STOP_DET	0x60
> -#define DW_IC_CLR_START_DET	0x64
> -#define DW_IC_CLR_GEN_CALL	0x68
> -#define DW_IC_ENABLE		0x6c
> -#define DW_IC_STATUS		0x70
> -#define DW_IC_TXFLR		0x74
> -#define DW_IC_RXFLR		0x78
> -#define DW_IC_SDA_HOLD		0x7c
> -#define DW_IC_TX_ABRT_SOURCE	0x80
> -#define DW_IC_ENABLE_STATUS	0x9c
> -#define DW_IC_COMP_PARAM_1	0xf4
> -#define DW_IC_COMP_VERSION	0xf8
> -#define DW_IC_SDA_HOLD_MIN_VERS	0x3131312A
> -#define DW_IC_COMP_TYPE		0xfc
> -#define DW_IC_COMP_TYPE_VALUE	0x44570140
> -
> -#define DW_IC_INTR_RX_UNDER	0x001
> -#define DW_IC_INTR_RX_OVER	0x002
> -#define DW_IC_INTR_RX_FULL	0x004
> -#define DW_IC_INTR_TX_OVER	0x008
> -#define DW_IC_INTR_TX_EMPTY	0x010
> -#define DW_IC_INTR_RD_REQ	0x020
> -#define DW_IC_INTR_TX_ABRT	0x040
> -#define DW_IC_INTR_RX_DONE	0x080
> -#define DW_IC_INTR_ACTIVITY	0x100
> -#define DW_IC_INTR_STOP_DET	0x200
> -#define DW_IC_INTR_START_DET	0x400
> -#define DW_IC_INTR_GEN_CALL	0x800
> -
> -#define DW_IC_INTR_DEFAULT_MASK		(DW_IC_INTR_RX_FULL |
> \
> -					 DW_IC_INTR_TX_ABRT | \
> -					 DW_IC_INTR_STOP_DET)
> -
> -#define DW_IC_INTR_MASTER_MASK		(DW_IC_INTR_DEFAULT_MAS
> K | \
> -					 DW_IC_INTR_TX_EMPTY)
> -
> -#define DW_IC_STATUS_ACTIVITY	0x1
> -
> -#define DW_IC_SDA_HOLD_RX_SHIFT		16
> -#define DW_IC_SDA_HOLD_RX_MASK		GENMASK(23,
> DW_IC_SDA_HOLD_RX_SHIFT)
> -
> -#define DW_IC_ERR_TX_ABRT	0x1
> -
> -#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
> -
> -#define DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH	(BIT(2) | BIT(3))
> -#define DW_IC_COMP_PARAM_1_SPEED_MODE_MASK	GENMASK(3, 2)
> -
> -/*
> - * status codes
> - */
> -#define STATUS_IDLE			0x0
> -#define STATUS_WRITE_IN_PROGRESS	0x1
> -#define STATUS_READ_IN_PROGRESS		0x2
> -
> -#define TIMEOUT			20 /* ms */
> -
> -/*
> - * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
> - *
> - * only expected abort codes are listed here
> - * refer to the datasheet for the full list
> - */
> -#define ABRT_7B_ADDR_NOACK	0
> -#define ABRT_10ADDR1_NOACK	1
> -#define ABRT_10ADDR2_NOACK	2
> -#define ABRT_TXDATA_NOACK	3
> -#define ABRT_GCALL_NOACK	4
> -#define ABRT_GCALL_READ		5
> -#define ABRT_SBYTE_ACKDET	7
> -#define ABRT_SBYTE_NORSTRT	9
> -#define ABRT_10B_RD_NORSTRT	10
> -#define ABRT_MASTER_DIS		11
> -#define ARB_LOST		12
> -
> -#define DW_IC_TX_ABRT_7B_ADDR_NOACK	(1UL <<
> ABRT_7B_ADDR_NOACK)
> -#define DW_IC_TX_ABRT_10ADDR1_NOACK	(1UL <<
> ABRT_10ADDR1_NOACK)
> -#define DW_IC_TX_ABRT_10ADDR2_NOACK	(1UL <<
> ABRT_10ADDR2_NOACK)
> -#define DW_IC_TX_ABRT_TXDATA_NOACK	(1UL << ABRT_TXDATA_NOACK)
> -#define DW_IC_TX_ABRT_GCALL_NOACK	(1UL << ABRT_GCALL_NOACK)
> -#define DW_IC_TX_ABRT_GCALL_READ	(1UL << ABRT_GCALL_READ)
> -#define DW_IC_TX_ABRT_SBYTE_ACKDET	(1UL << ABRT_SBYTE_ACKDET)
> -#define DW_IC_TX_ABRT_SBYTE_NORSTRT	(1UL <<
> ABRT_SBYTE_NORSTRT)
> -#define DW_IC_TX_ABRT_10B_RD_NORSTRT	(1UL <<
> ABRT_10B_RD_NORSTRT)
> -#define DW_IC_TX_ABRT_MASTER_DIS	(1UL << ABRT_MASTER_DIS)
> -#define DW_IC_TX_ARB_LOST		(1UL << ARB_LOST)
> -
> -#define DW_IC_TX_ABRT_NOACK		(DW_IC_TX_ABRT_7B_ADDR_NOA
> CK | \
> -					 DW_IC_TX_ABRT_10ADDR1_NOACK
> | \
> -					 DW_IC_TX_ABRT_10ADDR2_NOACK
> | \
> -					 DW_IC_TX_ABRT_TXDATA_NOACK |
> \
> -					 DW_IC_TX_ABRT_GCALL_NOACK)
> -
> -static char *abort_sources[] = {
> -	[ABRT_7B_ADDR_NOACK] =
> -		"slave address not acknowledged (7bit mode)",
> -	[ABRT_10ADDR1_NOACK] =
> -		"first address byte not acknowledged (10bit mode)",
> -	[ABRT_10ADDR2_NOACK] =
> -		"second address byte not acknowledged (10bit mode)",
> -	[ABRT_TXDATA_NOACK] =
> -		"data not acknowledged",
> -	[ABRT_GCALL_NOACK] =
> -		"no acknowledgement for a general call",
> -	[ABRT_GCALL_READ] =
> -		"read after general call",
> -	[ABRT_SBYTE_ACKDET] =
> -		"start byte acknowledged",
> -	[ABRT_SBYTE_NORSTRT] =
> -		"trying to send start byte when restart is disabled",
> -	[ABRT_10B_RD_NORSTRT] =
> -		"trying to read when restart is disabled (10bit
> mode)",
> -	[ABRT_MASTER_DIS] =
> -		"trying to use disabled adapter",
> -	[ARB_LOST] =
> -		"lost arbitration",
> -};
> -
> -static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
> -{
> -	u32 value;
> -
> -	if (dev->accessor_flags & ACCESS_16BIT)
> -		value = readw_relaxed(dev->base + offset) |
> -			(readw_relaxed(dev->base + offset + 2) <<
> 16);
> -	else
> -		value = readl_relaxed(dev->base + offset);
> -
> -	if (dev->accessor_flags & ACCESS_SWAP)
> -		return swab32(value);
> -	else
> -		return value;
> -}
> -
> -static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
> -{
> -	if (dev->accessor_flags & ACCESS_SWAP)
> -		b = swab32(b);
> -
> -	if (dev->accessor_flags & ACCESS_16BIT) {
> -		writew_relaxed((u16)b, dev->base + offset);
> -		writew_relaxed((u16)(b >> 16), dev->base + offset +
> 2);
> -	} else {
> -		writel_relaxed(b, dev->base + offset);
> -	}
> -}
> -
>  static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
>  {
>  	/* Configure Tx/Rx FIFO threshold levels */
>  	dw_writel(dev, dev->tx_fifo_depth / 2, DW_IC_TX_TL);
>  	dw_writel(dev, 0, DW_IC_RX_TL);
>  
> -	/* configure the i2c master */
> +	/* configure the I2C master */
>  	dw_writel(dev, dev->master_cfg, DW_IC_CON);
>  }
>  
> -static u32
> -i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int
> offset)
> -{
> -	/*
> -	 * DesignWare I2C core doesn't seem to have solid strategy to
> meet
> -	 * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH
> spec
> -	 * will result in violation of the tHD;STA spec.
> -	 */
> -	if (cond)
> -		/*
> -		 * Conditional expression:
> -		 *
> -		 *   IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH
> -		 *
> -		 * This is based on the DW manuals, and represents an
> ideal
> -		 * configuration.  The resulting I2C bus speed will
> be
> -		 * faster than any of the others.
> -		 *
> -		 * If your hardware is free from tHD;STA issue, try
> this one.
> -		 */
> -		return (ic_clk * tSYMBOL + 500000) / 1000000 - 8 +
> offset;
> -	else
> -		/*
> -		 * Conditional expression:
> -		 *
> -		 *   IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)
> -		 *
> -		 * This is just experimental rule; the tHD;STA period
> turned
> -		 * out to be proportinal to (_HCNT + 3).  With this
> setting,
> -		 * we could meet both tHIGH and tHD;STA timing specs.
> -		 *
> -		 * If unsure, you'd better to take this alternative.
> -		 *
> -		 * The reason why we need to take into account "tf"
> here,
> -		 * is the same as described in i2c_dw_scl_lcnt().
> -		 */
> -		return (ic_clk * (tSYMBOL + tf) + 500000) / 1000000
> -			- 3 + offset;
> -}
> -
> -static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
> -{
> -	/*
> -	 * Conditional expression:
> -	 *
> -	 *   IC_[FS]S_SCL_LCNT + 1 >= IC_CLK * (tLOW + tf)
> -	 *
> -	 * DW I2C core starts counting the SCL CNTs for the LOW
> period
> -	 * of the SCL clock (tLOW) as soon as it pulls the SCL line.
> -	 * In order to meet the tLOW timing spec, we need to take
> into
> -	 * account the fall time of SCL signal (tf).  Default tf
> value
> -	 * should be 0.3 us, for safety.
> -	 */
> -	return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 +
> offset;
> -}
> -
> -static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> -{
> -	dw_writel(dev, enable, DW_IC_ENABLE);
> -}
> -
> -static void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool
> enable)
> -{
> -	int timeout = 100;
> -
> -	do {
> -		__i2c_dw_enable(dev, enable);
> -		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) ==
> enable)
> -			return;
> -
> -		/*
> -		 * Wait 10 times the signaling period of the highest
> I2C
> -		 * transfer supported by the driver (for 400KHz this
> is
> -		 * 25us) as described in the DesignWare I2C databook.
> -		 */
> -		usleep_range(25, 250);
> -	} while (timeout--);
> -
> -	dev_warn(dev->dev, "timeout in %sabling adapter\n",
> -		 enable ? "en" : "dis");
> -}
> -
> -static unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
> -{
> -	/*
> -	 * Clock is not necessary if we got LCNT/HCNT values directly
> from
> -	 * the platform code.
> -	 */
> -	if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
> -		return 0;
> -	return dev->get_clk_rate_khz(dev);
> -}
> -
> -static int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
> -{
> -	int ret;
> -
> -	if (!dev->acquire_lock)
> -		return 0;
> -
> -	ret = dev->acquire_lock(dev);
> -	if (!ret)
> -		return 0;
> -
> -	dev_err(dev->dev, "couldn't acquire bus ownership\n");
> -
> -	return ret;
> -}
> -
> -static void i2c_dw_release_lock(struct dw_i2c_dev *dev)
> -{
> -	if (dev->release_lock)
> -		dev->release_lock(dev);
> -}
> -
>  /**
> - * i2c_dw_init() - initialize the designware i2c hardware
> + * i2c_dw_init() - initialize the designware i2c master hardware
>   * @dev: device private data
>   *
>   * This functions configures and enables the I2C.
> @@ -462,25 +175,6 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(i2c_dw_init);
>  
> -/*
> - * Waiting for bus not busy
> - */
> -static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
> -{
> -	int timeout = TIMEOUT;
> -
> -	while (dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
> -		if (timeout <= 0) {
> -			dev_warn(dev->dev, "timeout waiting for bus
> ready\n");
> -			return -ETIMEDOUT;
> -		}
> -		timeout--;
> -		usleep_range(1000, 1100);
> -	}
> -
> -	return 0;
> -}
> -
>  static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
>  {
>  	struct i2c_msg *msgs = dev->msgs;
> @@ -715,29 +409,6 @@ i2c_dw_read(struct dw_i2c_dev *dev)
>  	}
>  }
>  
> -static int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
> -{
> -	unsigned long abort_source = dev->abort_source;
> -	int i;
> -
> -	if (abort_source & DW_IC_TX_ABRT_NOACK) {
> -		for_each_set_bit(i, &abort_source,
> ARRAY_SIZE(abort_sources))
> -			dev_dbg(dev->dev,
> -				"%s: %s\n", __func__,
> abort_sources[i]);
> -		return -EREMOTEIO;
> -	}
> -
> -	for_each_set_bit(i, &abort_source, ARRAY_SIZE(abort_sources))
> -		dev_err(dev->dev, "%s: %s\n", __func__,
> abort_sources[i]);
> -
> -	if (abort_source & DW_IC_TX_ARB_LOST)
> -		return -EAGAIN;
> -	else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
> -		return -EINVAL; /* wrong msgs[] data */
> -	else
> -		return -EIO;
> -}
> -
>  /*
>   * Prepare controller for a transaction and call i2c_dw_xfer_msg
>   */
> @@ -825,12 +496,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct
> i2c_msg msgs[], int num)
>  	return ret;
>  }
>  
> -static u32 i2c_dw_func(struct i2c_adapter *adap)
> -{
> -	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> -	return dev->functionality;
> -}
> -
>  static struct i2c_algorithm i2c_dw_algo = {
>  	.master_xfer	= i2c_dw_xfer,
>  	.functionality	= i2c_dw_func,
> @@ -892,10 +557,10 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
>  }
>  
>  /*
> - * Interrupt service routine. This gets called whenever an I2C
> interrupt
> + * Interrupt service routine. This gets called whenever an I2C master
> interrupt
>   * occurs.
>   */
> -int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
> +static int i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
>  {
>  	u32 stat;
>  
> @@ -940,7 +605,7 @@ int i2c_dw_irq_handler_master(struct dw_i2c_dev
> *dev)
>  static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
>  {
>  	struct dw_i2c_dev *dev = dev_id;
> -	u32 stat, enabled, mode;
> +	u32 stat, enabled;
>  
>  	enabled = dw_readl(dev, DW_IC_ENABLE);
>  	stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> @@ -1041,5 +706,5 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(i2c_dw_probe);
>  
> -MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
> +MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter master");
>  MODULE_LICENSE("GPL");

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver
  2016-12-07 17:55 ` [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver Luis Oliveira
@ 2016-12-07 19:07   ` Mark Rutland
  2016-12-07 19:49   ` Andy Shevchenko
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 22+ messages in thread
From: Mark Rutland @ 2016-12-07 19:07 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: wsa, robh+dt, jarkko.nikula, andriy.shevchenko, mika.westerberg,
	linux-i2c, devicetree, linux-kernel, Ramiro.Oliveira, Joao.Pinto,
	CARLOS.PALMINHA

On Wed, Dec 07, 2016 at 05:55:51PM +0000, Luis Oliveira wrote:
> +#ifndef CONFIG_ACPI
> +	if (!device_property_match_string(&pdev->dev, "mode", "slave"))
> +		i2c_dw_configure_slave(pdev);
> +	else
> +#endif

This kind of ifdeffery doesn't make sense. A single kernel binary may
support both ACPI and DT (but only one is used at runtime). Note that
this is the case for arm64 (our Kconfig has select OF, and our defconfig
has CONFIG_ACPI=y), so this logic is broken for DT arm64 platforms.

If you're trying to ensure that this *only* works in the DT case,
explicitly check for an of_node, or use the of_* accessors.

That applies for all instances of this pattern in this driver.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-07 17:55 ` [PATCH v4 3/5] i2c: designware: Add slave definitions Luis Oliveira
@ 2016-12-07 19:11   ` Andy Shevchenko
  2016-12-12 17:02   ` Rob Herring
  1 sibling, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2016-12-07 19:11 UTC (permalink / raw)
  To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On Wed, 2016-12-07 at 17:55 +0000, Luis Oliveira wrote:
> - Add slave definitions to i2c-designware-core
> - Changes in Kconfig to auto-enable I2C_SLAVE when compiling the
> modules
> - Add mode property to designware-core.txt that enable the "slave"
> selection:
>   - "mode" is an optional property that could be "slave" or "master"
>   - if "mode" is not set the block is considered master by default
> 
> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>

I'm okay with the DT portion (still needs Ack from DT people), but the
problem with the patch that you break bisectability, i.e. you introduce
pieces of code that are not present ATM. So, this should go *after*
actual slave patch.

> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -20,6 +20,9 @@ Optional properties :
>   - i2c-sda-falling-time-ns : should contain the SDA falling time in
> nanoseconds.
>     This value which is by default 300ns is used to compute the tHIGH
> period.
>  
> + - mode : should be either:
> +           - "master" to setup the hardware block as a I2C master
> +           - "slave" to setup the hardware block as a I2C slave
>  Example :
>  
>  	i2c@f0000 {
> @@ -42,4 +45,5 @@ Example :
>  		i2c-sda-hold-time-ns = <300>;
>  		i2c-sda-falling-time-ns = <300>;
>  		i2c-scl-falling-time-ns = <300>;
> +		mode = "slave";

I would suggest to use "master" here, since most common use is master.

>  	};

So, the above can go to the patch
"... Add new property to describe mode"

> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -469,6 +469,7 @@ config I2C_DESIGNWARE_CORE
>  
>  config I2C_DESIGNWARE_PLATFORM
>  	tristate "Synopsys DesignWare Platform"
> +	select I2C_SLAVE
> 

> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -55,6 +55,12 @@ static char *abort_sources[] = {
>  		"trying to use disabled adapter",
>  	[ARB_LOST] =
>  		"lost arbitration",
> +	[ABRT_SLAVE_FLUSH_TXFIFO] =
> +		"read command so flush old data in the TX FIFO",
> +	[ABRT_SLAVE_ARBLOST] =
> +		"slave lost the bus while transmitting data to a
> remote master",
> +	[ABRT_SLAVE_RD_INTX] =
> +		"slave request for data to be transmitted and",

These are part of slave patch.

> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -36,15 +36,20 @@
>  #define DW_IC_CON_SPEED_FAST		0x4
>  #define DW_IC_CON_SPEED_HIGH		0x6
>  #define DW_IC_CON_SPEED_MASK		0x6
> +#define DW_IC_CON_10BITADDR_SLAVE		0x8

All definitions would be split to a patch like
"... Introduce definitions for i2c slave mode"

> @@ -206,6 +225,7 @@ struct dw_i2c_dev {
>  	void __iomem		*base;
>  	struct completion	cmd_complete;
>  	struct clk		*clk;
> +	struct i2c_client		*slave;
> 

> @@ -225,6 +245,7 @@ struct dw_i2c_dev {
>  	struct i2c_adapter	adapter;
>  	u32			functionality;
>  	u32			master_cfg;
> +	u32			slave_cfg;
> 

> +extern int i2c_dw_init_slave(struct dw_i2c_dev *dev);
> +extern void i2c_dw_disable_slave(struct dw_i2c_dev *dev);
> +extern void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev);
> +extern u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev);
> +extern int i2c_dw_probe_slave(struct dw_i2c_dev *dev);

The above is a part of slave patch.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver
  2016-12-07 17:55 ` [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver Luis Oliveira
  2016-12-07 19:07   ` Mark Rutland
@ 2016-12-07 19:49   ` Andy Shevchenko
  2016-12-08  4:38   ` kbuild test robot
  2016-12-10 23:44   ` kbuild test robot
  3 siblings, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2016-12-07 19:49 UTC (permalink / raw)
  To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On Wed, 2016-12-07 at 17:55 +0000, Luis Oliveira wrote:
> - Slave mode selected by compatibility string in platform module
> - Changes in Makefile to Kbuild successfully compile i2c-designware-
> core
>   with slave functions

This needs more work.

First of all I would split logically this to two patches:

1. slave patch: introducing header definitions and -slave.c module
2. slave patch: actually use it

Always try to split you changes to smaller logically finished parts.
There is still freedom of choice, though I bet many maintainers will
agree with me.

Last but not least is to keep an eye on bisectability. It means your
each patch should: a) be compiled without problems, b) bring the
features, definitions, whatsoever it uses, c) go with priority — bug
fixes first followed by clean ups followed by feature enhancements.


> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
> Changes V3->V4: (Andy Shevchenko)
> - nothing changed

Hmm... I think there were comments about this (perhaps not mine).
Anyway, see below.

 
> +static void i2c_dw_configure_slave(struct platform_device *pdev)
> +{
> +	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> +
> +	dev->functionality = I2C_FUNC_SLAVE |
> DW_IC_DEFAULT_FUNCTIONALITY;
> +
> +	dev->slave_cfg = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL |
> +			 DW_IC_CON_RESTART_EN |
> DW_IC_CON_STOP_DET_IFADDRESSED |
> +			 DW_IC_CON_SPEED_FAST;
> +

> +	dev_info(&pdev->dev, "I am registed as a I2C Slave!\n");

Same side note as for master case.

> +
> +	switch (dev->clk_freq) {
> +	case 100000:
> +		dev->slave_cfg |= DW_IC_CON_SPEED_STD;
> +		break;
> +	case 3400000:
> +		dev->slave_cfg |= DW_IC_CON_SPEED_HIGH;
> +		break;
> +	default:
> +		dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
> +	}
> +}
> +
>  static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool
> prepare)
>  {
>  	if (IS_ERR(i_dev->clk))
> @@ -240,9 +266,12 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  	if (r)
>  		return r;
>  
> -	dev->functionality = I2C_FUNC_10BIT_ADDR |
> DW_IC_DEFAULT_FUNCTIONALITY;
> -
> -	i2c_dw_configure_master(pdev);
> +#ifndef CONFIG_ACPI

This is no-no. And basically you got already comment on it.
Please, fix all occurrences.

> +	if (!device_property_match_string(&pdev->dev, "mode",
> "slave"))
> +		i2c_dw_configure_slave(pdev);
> +	else
> +#endif
> +		i2c_dw_configure_master(pdev);
>  
>  	dev->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (!i2c_dw_plat_prepare_clk(dev, true)) {
> @@ -255,7 +284,14 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  	}
>  
>  	if (!dev->tx_fifo_depth) {
> -		u32 param1 = i2c_dw_read_comp_param(dev);
> +		u32 param1;
> +#ifndef CONFIG_ACPI
> +		if (!device_property_match_string(&pdev->dev,
> +			 "mode", "slave"))
> +			param1 = i2c_dw_read_comp_param_slave(dev);
> +		else
> +#endif
> +			param1 = i2c_dw_read_comp_param(dev);
>  
>  		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
>  		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;

Here is the patch for FIFO flying around and I believe it will make
upstream before yours. Which means try rebase your next version against
latest i2c-next and / or linux-next.

> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -0,0 +1,433 @@
> +/*

> + * Synopsys DesignWare I2C adapter driver (master only).
> + *
> + * Based on the TI DAVINCI I2C adapter driver.
> + *
> + * Copyright (C) 2006 Texas Instruments.
> + * Copyright (C) 2007 MontaVista Software Inc.
> + * Copyright (C) 2009 Provigent Ltd.

Something wrong here. Perhaps slave for the first?
Why copyrights are kept? Needs explanation.

> + *
> + * ------------------------------------------------------------------
> ----------
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + * ------------------------------------------------------------------
> ----------
> + *
> + */
> 

> +#include <linux/export.h>

I don't think you need this one explicit. You have already module.h.

> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>

Keep above sorted.

+ empty line here.

+#include "i2c-designware-core.h"
> +
> +static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev)
> +{
> +	/* Configure Tx/Rx FIFO threshold levels */
> +	dw_writel(dev, 0, DW_IC_TX_TL);
> +	dw_writel(dev, 0, DW_IC_RX_TL);
> +
> +	/* configure the I2C slave */
> +	dw_writel(dev, dev->slave_cfg, DW_IC_CON);
> +	dw_writel(dev, DW_IC_INTR_SLAVE_MASK, DW_IC_INTR_MASK);

I understand the copy'n'paste development method, but, please, keep
consistent style in new code, e.g. comments should start from Capital
letter. Multi-line comments have separate open and close lines.

> +}
> +
> +/**
> + * i2c_dw_init_slave() - initialize the designware i2c slave hardware
> + * @dev: device private data
> + *
> + * This functions configures and enables the I2C.
> + * This function is called during I2C init function, and in case of
> timeout at
> + * run time.
> + */
> +int i2c_dw_init_slave(struct dw_i2c_dev *dev)
> +{

> +	u32 hcnt, lcnt;
> +	u32 reg, comp_param1;
> +	u32 sda_falling_time, scl_falling_time;
> +	int ret;

Better to use reversed tree.

aaaaaaaaaaa
bbbbbbb
cccc


> +
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		return ret;
> +
> +	reg = dw_readl(dev, DW_IC_COMP_TYPE);
> +	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
> +		/* Configure register endianness access */
> +		dev->accessor_flags |= ACCESS_SWAP;
> +	} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
> +		/* Configure register access mode 16bit */
> +		dev->accessor_flags |= ACCESS_16BIT;
> +	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
> +		dev_err(dev->dev, "Unknown Synopsys component type: "
> +			"0x%08x\n", reg);
> +		i2c_dw_release_lock(dev);
> +		return -ENODEV;
> +	}
> +
> +	comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1);
> +
> +	/* Disable the adapter */
> +	__i2c_dw_enable_and_wait(dev, false);
> +
> +	/* set standard and fast speed deviders for high/low periods
> */

Same about comments and style.

> +	sda_falling_time = dev->sda_falling_time ?: 300; /* ns */
> +	scl_falling_time = dev->scl_falling_time ?: 300; /* ns */
> +
> +	/* Set SCL timing parameters for standard-mode */
> +	if (dev->ss_hcnt && dev->ss_lcnt) {
> +		hcnt = dev->ss_hcnt;
> +		lcnt = dev->ss_lcnt;
> +	} else {
> +		hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
> +					4000,	/* tHD;STA =
> tHIGH = 4.0 us */
> +					sda_falling_time,
> +					0,	/* 0: DW default,
> 1: Ideal */
> +					0);	/* No offset */
> +		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
> +					4700,	/* tLOW = 4.7 us
> */
> +					scl_falling_time,
> +					0);	/* No offset */
> +	}
> +	dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
> +	dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
> +	dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt,
> lcnt);
> +
> +	/* Set SCL timing parameters for fast-mode or fast-mode plus
> */
> +	if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev-
> >fp_lcnt) {
> +		hcnt = dev->fp_hcnt;
> +		lcnt = dev->fp_lcnt;
> +	} else if (dev->fs_hcnt && dev->fs_lcnt) {
> +		hcnt = dev->fs_hcnt;
> +		lcnt = dev->fs_lcnt;
> +	} else {
> +		hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
> +					600,	/* tHD;STA =
> tHIGH = 0.6 us */
> +					sda_falling_time,
> +					0,	/* 0: DW default,
> 1: Ideal */
> +					0);	/* No offset */
> +		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
> +					1300,	/* tLOW = 1.3 us
> */
> +					scl_falling_time,
> +					0);	/* No offset */
> +	}

I didn't read carefully, but on the first glance how does it differ from
master code?

If it doesn't, split the original function of the master (better in
separate patch) to few to prepare for slave.

> +	dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
> +	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
> +	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt,
> lcnt);
> +
> +	if ((dev->slave_cfg & DW_IC_CON_SPEED_MASK) ==
> +		DW_IC_CON_SPEED_HIGH) {
> +		if ((comp_param1 &
> DW_IC_COMP_PARAM_1_SPEED_MODE_MASK)
> +			!= DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) {
> +			dev_err(dev->dev, "High Speed not
> supported!\n");
> +			dev->slave_cfg &= ~DW_IC_CON_SPEED_MASK;
> +			dev->slave_cfg |= DW_IC_CON_SPEED_FAST;
> +		} else if (dev->hs_hcnt && dev->hs_lcnt) {
> +			hcnt = dev->hs_hcnt;
> +			lcnt = dev->hs_lcnt;
> +			dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT);
> +			dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT);
> +			dev_dbg(dev->dev, "HighSpeed-mode HCNT:LCNT =
> %d:%d\n",
> +				hcnt, lcnt);
> +		}
> +	}
> +
> +	/* Configure SDA Hold Time if required */
> +	reg = dw_readl(dev, DW_IC_COMP_VERSION);
> +	reg = dw_readl(dev, DW_IC_COMP_VERSION);
> +	if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
> +		if (!dev->sda_hold_time) {
> +			/* Keep previous hold time setting if no one
> set it */
> +			dev->sda_hold_time = dw_readl(dev,
> DW_IC_SDA_HOLD);
> +		}
> +		/*
> +		 * Workaround for avoiding TX arbitration lost in
> case I2C
> +		 * slave pulls SDA down "too quickly" after falling
> egde of
> +		 * SCL by enabling non-zero SDA RX hold.
> Specification says it
> +		 * extends incoming SDA low to high transition while
> SCL is
> +		 * high but it apprears to help also above issue.
> +		 */
> +		if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
> +			dev->sda_hold_time |= 1 <<
> DW_IC_SDA_HOLD_RX_SHIFT;
> +		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> +	} else {
> +		dev_warn(dev->dev,
> +			"Hardware too old to adjust SDA hold
> time.\n");
> +	}
> +
> +	i2c_dw_configure_fifo_slave(dev);
> +	i2c_dw_release_lock(dev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_dw_init_slave);
> +
> +int i2c_dw_reg_slave(struct i2c_client *slave)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);
> +
> +	if (dev->slave)
> +		return -EBUSY;
> +	if (slave->flags & I2C_CLIENT_TEN)
> +		return -EAFNOSUPPORT;
> +		/* set slave address in the IC_SAR register,
> +		* the address to which the DW_apb_i2c responds */
> +
> +	__i2c_dw_enable(dev, false);
> +	dw_writel(dev, slave->addr, DW_IC_SAR);
> +	dev->slave = slave;
> +
> +	__i2c_dw_enable(dev, true);
> +
> +	dev->cmd_err = 0;
> +	dev->msg_write_idx = 0;
> +	dev->msg_read_idx = 0;
> +	dev->msg_err = 0;
> +	dev->status = STATUS_IDLE;
> +	dev->abort_source = 0;
> +	dev->rx_outstanding = 0;
> +
> +	return 0;
> +}
> +
> +static int i2c_dw_unreg_slave(struct i2c_client *slave)
> +{
> +	struct dw_i2c_dev *dev =  i2c_get_adapdata(slave->adapter);
> +
> +	i2c_dw_disable_int_slave(dev);
> +	i2c_dw_disable_slave(dev);
> +	dev->slave =  NULL;
> +
> +	return 0;
> +}
> +
> +static u32 i2c_dw_read_clear_intrbits_slave(struct dw_i2c_dev *dev)
> +{
> +	u32 stat;
> +
> +	/*
> +	 * The IC_INTR_STAT register just indicates "enabled"
> interrupts.
> +	 * Ths unmasked raw version of interrupt status bits are
> available
> +	 * in the IC_RAW_INTR_STAT register.
> +	 *
> +	 * That is,
> +	 *   stat = dw_readl(IC_INTR_STAT);
> +	 * equals to,
> +	 *   stat = dw_readl(IC_RAW_INTR_STAT) &
> dw_readl(IC_INTR_MASK);
> +	 *
> +	 * The raw version might be useful for debugging purposes.
> +	 */
> +	stat = dw_readl(dev, DW_IC_INTR_STAT);
> +
> +	/*
> +	 * Do not use the IC_CLR_INTR register to clear interrupts,
> or
> +	 * you'll miss some interrupts, triggered during the period
> from
> +	 * dw_readl(IC_INTR_STAT) to dw_readl(IC_CLR_INTR).
> +	 *
> +	 * Instead, use the separately-prepared IC_CLR_* registers.
> +	 */
> +	if (stat & DW_IC_INTR_TX_ABRT)
> +		dw_readl(dev, DW_IC_CLR_TX_ABRT);
> +	if (stat & DW_IC_INTR_RX_UNDER)
> +		dw_readl(dev, DW_IC_CLR_RX_UNDER);
> +	if (stat & DW_IC_INTR_RX_OVER)
> +		dw_readl(dev, DW_IC_CLR_RX_OVER);
> +	if (stat & DW_IC_INTR_TX_OVER)
> +		dw_readl(dev, DW_IC_CLR_TX_OVER);
> +	if (stat & DW_IC_INTR_RX_DONE)
> +		dw_readl(dev, DW_IC_CLR_RX_DONE);
> +	if (stat & DW_IC_INTR_ACTIVITY)
> +		dw_readl(dev, DW_IC_CLR_ACTIVITY);
> +	if (stat & DW_IC_INTR_STOP_DET)
> +		dw_readl(dev, DW_IC_CLR_STOP_DET);
> +	if (stat & DW_IC_INTR_START_DET)
> +		dw_readl(dev, DW_IC_CLR_START_DET);
> +	if (stat & DW_IC_INTR_GEN_CALL)
> +		dw_readl(dev, DW_IC_CLR_GEN_CALL);
> +
> +	return stat;
> +}
> +
> +/*
> + * Interrupt service routine. This gets called whenever an I2C slave
> interrupt
> + * occurs.
> + */
> +
> +static bool i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)

static int
Choose 0 for IRQ_NONE, negative for error (if needed, perhaps not your
case), positive for IRQ_HANDLED case.

> +{
> +	u32 raw_stat, stat, enabled;
> +	u8 val, slave_activity;
> +
> +	stat = dw_readl(dev, DW_IC_INTR_STAT);
> +	enabled = dw_readl(dev, DW_IC_ENABLE);
> +	raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> +	slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
> +		 DW_IC_STATUS_SLAVE_ACTIVITY)>>6);

x >> y

style.

> +
> +	if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY))
> +		return false;
> +
> +	dev_dbg(dev->dev,
> +	 "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :
> INTR_STAT=%#x\n",
> +	 __func__, enabled, slave_activity, raw_stat, stat);

__func__ is redundant in *_dbg() calls. Dynamic debug could do it for
you.

> +
> +	if (stat & DW_IC_INTR_RESTART_DET)
> +		dw_readl(dev, DW_IC_CLR_RESTART_DET);
> +	if (stat & DW_IC_INTR_START_DET)
> +		dw_readl(dev, DW_IC_CLR_START_DET);
> +	if (stat & DW_IC_INTR_ACTIVITY)
> +		dw_readl(dev, DW_IC_CLR_ACTIVITY);
> +	if (stat & DW_IC_INTR_RX_OVER)
> +		dw_readl(dev, DW_IC_CLR_RX_OVER);
> +	if ((stat & DW_IC_INTR_RX_FULL) && (stat &
> DW_IC_INTR_STOP_DET))
> +		i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_REQUESTED, &val);
> +
> +	if (slave_activity) {
> +		if (stat & DW_IC_INTR_RD_REQ) {
> +			if (stat & DW_IC_INTR_RX_FULL) {
> +				val = dw_readl(dev, DW_IC_DATA_CMD);
> +				if (!i2c_slave_event(dev->slave,
> +				 I2C_SLAVE_WRITE_RECEIVED, &val)) {
> +					dev_dbg(dev->dev, "Byte %X
> acked!",
> +					 val);
> +				}
> +				dw_readl(dev, DW_IC_CLR_RD_REQ);
> +				stat =
> i2c_dw_read_clear_intrbits_slave(dev);
> +			} else {
> +				dw_readl(dev, DW_IC_CLR_RD_REQ);
> +				dw_readl(dev, DW_IC_CLR_RX_UNDER);
> +				stat =
> i2c_dw_read_clear_intrbits_slave(dev);
> +			}
> +			if (!i2c_slave_event(dev->slave,
> +					 I2C_SLAVE_READ_REQUESTED,
> &val))
> +				dw_writel(dev, val, DW_IC_DATA_CMD);
> +		}
> +	}
> +
> +	if (stat & DW_IC_INTR_RX_DONE) {
> +		if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_READ_PROCESSED,
> +		 &val))
> +			dw_readl(dev, DW_IC_CLR_RX_DONE);
> +
> +		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
> +		stat = i2c_dw_read_clear_intrbits_slave(dev);
> +		return true;
> +	}
> +
> +	if (stat & DW_IC_INTR_RX_FULL) {
> +		val = dw_readl(dev, DW_IC_DATA_CMD);
> +		if (!i2c_slave_event(dev->slave,
> I2C_SLAVE_WRITE_RECEIVED,
> +		 &val))
> +			dev_dbg(dev->dev, "Byte %X acked!", val);
> +	} else {
> +		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
> +		stat = i2c_dw_read_clear_intrbits_slave(dev);
> +	}
> +
> +	if (stat & DW_IC_INTR_TX_OVER)
> +		dw_readl(dev, DW_IC_CLR_TX_OVER);
> +
> +	return true;
> +}
> +
> +static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
> +{
> +	struct dw_i2c_dev *dev = dev_id;
> +
> +	i2c_dw_read_clear_intrbits_slave(dev);
> +	if (!i2c_dw_irq_handler_slave(dev))
> +		return IRQ_NONE;
> +
> +	complete(&dev->cmd_complete);
> +	return IRQ_HANDLED;

...and here (if you want to, this might be kept as is for now, but still
would be nice to get int return code anyway)

int ret;

ret = i2c_dw_irq_handler_slave(dev);
if (ret > 0) {
 complete(...);
}

return IRQ_RETVAL(ret);

> +}
> +
> +static struct i2c_algorithm i2c_dw_algo = {
> +	.functionality	= i2c_dw_func,
> +	.reg_slave	= i2c_dw_reg_slave,
> +	.unreg_slave	= i2c_dw_unreg_slave,
> +};
> +
> +void i2c_dw_disable_slave(struct dw_i2c_dev *dev)
> +{
> +	/* Disable controller */
> +	__i2c_dw_enable_and_wait(dev, false);
> +
> +	/* Disable all interupts */
> +	dw_writel(dev, 0, DW_IC_INTR_MASK);
> +	dw_readl(dev, DW_IC_CLR_INTR);
> +}
> +EXPORT_SYMBOL_GPL(i2c_dw_disable_slave);

No, please use
struct i2c_dw_ops {
 (*func1)();
 (*func2)();
...
};

for both (master/slave).

> +
> +void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev)
> +{
> +	dw_writel(dev, 0, DW_IC_INTR_MASK);
> +}
> +EXPORT_SYMBOL_GPL(i2c_dw_disable_int_slave);
> +
> +u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev)
> +{
> +	return dw_readl(dev, DW_IC_COMP_PARAM_1);
> +}
> +EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param_slave);
> +
> +int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
> +{
> +	struct i2c_adapter *adap = &dev->adapter;

> +	int r;

New code — consistency.

int ret; as above.

> +
> +	init_completion(&dev->cmd_complete);
> +
> +	r = i2c_dw_init_slave(dev);
> +	if (r)
> +		return r;
> +
> +	r = i2c_dw_acquire_lock(dev);
> +	if (r)
> +		return r;
> +
> +	i2c_dw_release_lock(dev);
> +	snprintf(adap->name, sizeof(adap->name),
> +		 "Synopsys DesignWare I2C Slave adapter");
> +	adap->retries = 3;
> +	adap->algo = &i2c_dw_algo;
> +	adap->dev.parent = dev->dev;
> +	i2c_set_adapdata(adap, dev);
> +
> +	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
> 

> +			     IRQF_SHARED | IRQF_COND_SUSPEND,

I don't think IRQF_COND_SUSPEND makes sense in slave mode (I know
exactly one user of it and I ensure you that it will quite unlikely use
slave mode), though it will not block others anyhow.

> +			     dev_name(dev->dev), dev);
> +	if (r) {
> +		dev_err(dev->dev, "failure requesting irq %i: %d\n",
> +			dev->irq, r);
> +		return r;
> +	}
> +	/*
> +	 * Increment PM usage count during adapter registration in
> order to
> +	 * avoid possible spurious runtime suspend when adapter
> device is
> +	 * registered to the device core and immediate resume in case
> bus has
> +	 * registered I2C slaves that do I2C transfers in their
> probe.
> +	 */
> +	pm_runtime_get_noresume(dev->dev);
> +	r = i2c_add_numbered_adapter(adap);
> +	if (r)
> +		dev_err(dev->dev, "failure adding adapter: %d\n", r);
> +	pm_runtime_put_noidle(dev->dev);
> +
> +	return r;
> +}
> +EXPORT_SYMBOL_GPL(i2c_dw_probe_slave);
> +
> +MODULE_DESCRIPTION("Synopsys DesignWare I2C bus slave adapter");
> +MODULE_LICENSE("GPL");

"GPL v2"?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation
  2016-12-07 17:55 ` [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation Luis Oliveira
@ 2016-12-07 19:52   ` Andy Shevchenko
  2016-12-13 15:08   ` Jarkko Nikula
  1 sibling, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2016-12-07 19:52 UTC (permalink / raw)
  To: Luis Oliveira, wsa, robh+dt, mark.rutland, jarkko.nikula,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On Wed, 2016-12-07 at 17:55 +0000, Luis Oliveira wrote:
> - Missspelling, comment formatation and fix a string of
>   the existing code
> 

Good, but after addressing below comments:
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> --- a/drivers/i2c/busses/i2c-designware-slave.c
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -70,8 +70,8 @@ int i2c_dw_init_slave(struct dw_i2c_dev *dev)
>  		/* Configure register access mode 16bit */
>  		dev->accessor_flags |= ACCESS_16BIT;
>  	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
> -		dev_err(dev->dev, "Unknown Synopsys component type: "
> -			"0x%08x\n", reg);
> +		dev_err(dev->dev,
> +		 "Unknown Synopsys component type: 0x%08x\n", reg);

Keep indentation like 

some_call(param, param2, ...
	  paramX, paramY, ...);

It applies to all your new code you bring by this series.


> @@ -181,8 +181,10 @@ int i2c_dw_reg_slave(struct i2c_client *slave)
>  		return -EBUSY;
>  	if (slave->flags & I2C_CLIENT_TEN)
>  		return -EAFNOSUPPORT;
> -		/* set slave address in the IC_SAR register,
> -		* the address to which the DW_apb_i2c responds */
> +		/*
> +		 * set slave address in the IC_SAR register,
> +		 * the address to which the DW_apb_i2c responds

Again, Capital letter here, dot at the end of sentence. You may fix (if
you want to) the rest of comment lines in entire driver.

> +		 */
>  
>  	__i2c_dw_enable(dev, false);
>  	dw_writel(dev, slave->addr, DW_IC_SAR);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver
  2016-12-07 17:55 ` [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver Luis Oliveira
  2016-12-07 19:07   ` Mark Rutland
  2016-12-07 19:49   ` Andy Shevchenko
@ 2016-12-08  4:38   ` kbuild test robot
  2016-12-10 23:44   ` kbuild test robot
  3 siblings, 0 replies; 22+ messages in thread
From: kbuild test robot @ 2016-12-08  4:38 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: kbuild-all, wsa, robh+dt, mark.rutland, jarkko.nikula,
	andriy.shevchenko, mika.westerberg, linux-i2c, devicetree,
	linux-kernel, Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto,
	CARLOS.PALMINHA

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

Hi Luis,

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on next-20161207]
[cannot apply to v4.9-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Luis-Oliveira/i2c-designware-Refactoring-of-the-i2c-designware/20161208-044045
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-h0-12081126 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/i2c/busses/i2c-designware-slave.c: In function 'i2c_dw_irq_handler_slave':
   drivers/i2c/busses/i2c-designware-slave.c:294:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
      i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
      ^~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-designware-slave.c:294:31: error: 'I2C_SLAVE_WRITE_REQUESTED' undeclared (first use in this function)
      i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-designware-slave.c:294:31: note: each undeclared identifier is reported only once for each function it appears in
   In file included from include/linux/err.h:4:0,
                    from drivers/i2c/busses/i2c-designware-slave.c:26:
   drivers/i2c/busses/i2c-designware-slave.c:301:6: error: 'I2C_SLAVE_WRITE_RECEIVED' undeclared (first use in this function)
         I2C_SLAVE_WRITE_RECEIVED, &val)) {
         ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/i2c/busses/i2c-designware-slave.c:300:5: note: in expansion of macro 'if'
        if (!i2c_slave_event(dev->slave,
        ^~
   drivers/i2c/busses/i2c-designware-slave.c:313:7: error: 'I2C_SLAVE_READ_REQUESTED' undeclared (first use in this function)
          I2C_SLAVE_READ_REQUESTED, &val))
          ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
   drivers/i2c/busses/i2c-designware-slave.c:312:4: note: in expansion of macro 'if'
       if (!i2c_slave_event(dev->slave,
       ^~
   drivers/i2c/busses/i2c-designware-slave.c:319:36: error: 'I2C_SLAVE_READ_PROCESSED' undeclared (first use in this function)
      if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
                                       ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
   drivers/i2c/busses/i2c-designware-slave.c:319:3: note: in expansion of macro 'if'
      if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
      ^~
   drivers/i2c/busses/i2c-designware-slave.c:323:31: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
      i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
                                  ^~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-designware-slave.c: At top level:
   drivers/i2c/busses/i2c-designware-slave.c:358:2: error: unknown field 'reg_slave' specified in initializer
     .reg_slave = i2c_dw_reg_slave,
     ^
   drivers/i2c/busses/i2c-designware-slave.c:358:15: warning: excess elements in struct initializer
     .reg_slave = i2c_dw_reg_slave,
                  ^~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-designware-slave.c:358:15: note: (near initialization for 'i2c_dw_algo')
   drivers/i2c/busses/i2c-designware-slave.c:359:2: error: unknown field 'unreg_slave' specified in initializer
     .unreg_slave = i2c_dw_unreg_slave,
     ^
   drivers/i2c/busses/i2c-designware-slave.c:359:17: warning: excess elements in struct initializer
     .unreg_slave = i2c_dw_unreg_slave,
                    ^~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-designware-slave.c:359:17: note: (near initialization for 'i2c_dw_algo')
   cc1: some warnings being treated as errors

vim +/if +300 drivers/i2c/busses/i2c-designware-slave.c

   288			dw_readl(dev, DW_IC_CLR_START_DET);
   289		if (stat & DW_IC_INTR_ACTIVITY)
   290			dw_readl(dev, DW_IC_CLR_ACTIVITY);
   291		if (stat & DW_IC_INTR_RX_OVER)
   292			dw_readl(dev, DW_IC_CLR_RX_OVER);
   293		if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
 > 294			i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
   295	
   296		if (slave_activity) {
   297			if (stat & DW_IC_INTR_RD_REQ) {
   298				if (stat & DW_IC_INTR_RX_FULL) {
   299					val = dw_readl(dev, DW_IC_DATA_CMD);
 > 300					if (!i2c_slave_event(dev->slave,
   301					 I2C_SLAVE_WRITE_RECEIVED, &val)) {
   302						dev_dbg(dev->dev, "Byte %X acked!",
   303						 val);

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver
  2016-12-07 17:55 ` [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver Luis Oliveira
                     ` (2 preceding siblings ...)
  2016-12-08  4:38   ` kbuild test robot
@ 2016-12-10 23:44   ` kbuild test robot
  3 siblings, 0 replies; 22+ messages in thread
From: kbuild test robot @ 2016-12-10 23:44 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: kbuild-all, wsa, robh+dt, mark.rutland, jarkko.nikula,
	andriy.shevchenko, mika.westerberg, linux-i2c, devicetree,
	linux-kernel, Luis.Oliveira, Ramiro.Oliveira, Joao.Pinto,
	CARLOS.PALMINHA

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

Hi Luis,

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on next-20161209]
[cannot apply to v4.9-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Luis-Oliveira/i2c-designware-Refactoring-of-the-i2c-designware/20161208-044045
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-c0-12110449 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-designware-slave.c: In function 'i2c_dw_irq_handler_slave':
>> drivers/i2c/busses/i2c-designware-slave.c:294:3: error: implicit declaration of function 'i2c_slave_event' [-Werror=implicit-function-declaration]
      i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
      ^
>> drivers/i2c/busses/i2c-designware-slave.c:294:31: error: 'I2C_SLAVE_WRITE_REQUESTED' undeclared (first use in this function)
      i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
                                  ^
   drivers/i2c/busses/i2c-designware-slave.c:294:31: note: each undeclared identifier is reported only once for each function it appears in
   In file included from include/linux/err.h:4:0,
                    from drivers/i2c/busses/i2c-designware-slave.c:26:
>> drivers/i2c/busses/i2c-designware-slave.c:301:6: error: 'I2C_SLAVE_WRITE_RECEIVED' undeclared (first use in this function)
         I2C_SLAVE_WRITE_RECEIVED, &val)) {
         ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^
   drivers/i2c/busses/i2c-designware-slave.c:300:5: note: in expansion of macro 'if'
        if (!i2c_slave_event(dev->slave,
        ^
>> drivers/i2c/busses/i2c-designware-slave.c:313:7: error: 'I2C_SLAVE_READ_REQUESTED' undeclared (first use in this function)
          I2C_SLAVE_READ_REQUESTED, &val))
          ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^
   drivers/i2c/busses/i2c-designware-slave.c:312:4: note: in expansion of macro 'if'
       if (!i2c_slave_event(dev->slave,
       ^
>> drivers/i2c/busses/i2c-designware-slave.c:319:36: error: 'I2C_SLAVE_READ_PROCESSED' undeclared (first use in this function)
      if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
                                       ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^
   drivers/i2c/busses/i2c-designware-slave.c:319:3: note: in expansion of macro 'if'
      if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
      ^
>> drivers/i2c/busses/i2c-designware-slave.c:323:31: error: 'I2C_SLAVE_STOP' undeclared (first use in this function)
      i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
                                  ^
   drivers/i2c/busses/i2c-designware-slave.c: At top level:
>> drivers/i2c/busses/i2c-designware-slave.c:358:2: error: unknown field 'reg_slave' specified in initializer
     .reg_slave = i2c_dw_reg_slave,
     ^
   drivers/i2c/busses/i2c-designware-slave.c:358:2: warning: excess elements in struct initializer
   drivers/i2c/busses/i2c-designware-slave.c:358:2: warning: (near initialization for 'i2c_dw_algo')
>> drivers/i2c/busses/i2c-designware-slave.c:359:2: error: unknown field 'unreg_slave' specified in initializer
     .unreg_slave = i2c_dw_unreg_slave,
     ^
   drivers/i2c/busses/i2c-designware-slave.c:359:2: warning: excess elements in struct initializer
   drivers/i2c/busses/i2c-designware-slave.c:359:2: warning: (near initialization for 'i2c_dw_algo')
   cc1: some warnings being treated as errors

vim +/i2c_slave_event +294 drivers/i2c/busses/i2c-designware-slave.c

   288			dw_readl(dev, DW_IC_CLR_START_DET);
   289		if (stat & DW_IC_INTR_ACTIVITY)
   290			dw_readl(dev, DW_IC_CLR_ACTIVITY);
   291		if (stat & DW_IC_INTR_RX_OVER)
   292			dw_readl(dev, DW_IC_CLR_RX_OVER);
   293		if ((stat & DW_IC_INTR_RX_FULL) && (stat & DW_IC_INTR_STOP_DET))
 > 294			i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED, &val);
   295	
   296		if (slave_activity) {
   297			if (stat & DW_IC_INTR_RD_REQ) {
   298				if (stat & DW_IC_INTR_RX_FULL) {
   299					val = dw_readl(dev, DW_IC_DATA_CMD);
   300					if (!i2c_slave_event(dev->slave,
 > 301					 I2C_SLAVE_WRITE_RECEIVED, &val)) {
   302						dev_dbg(dev->dev, "Byte %X acked!",
   303						 val);
   304					}
   305					dw_readl(dev, DW_IC_CLR_RD_REQ);
   306					stat = i2c_dw_read_clear_intrbits_slave(dev);
   307				} else {
   308					dw_readl(dev, DW_IC_CLR_RD_REQ);
   309					dw_readl(dev, DW_IC_CLR_RX_UNDER);
   310					stat = i2c_dw_read_clear_intrbits_slave(dev);
   311				}
   312				if (!i2c_slave_event(dev->slave,
 > 313						 I2C_SLAVE_READ_REQUESTED, &val))
   314					dw_writel(dev, val, DW_IC_DATA_CMD);
   315			}
   316		}
   317	
   318		if (stat & DW_IC_INTR_RX_DONE) {
 > 319			if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
   320			 &val))
   321				dw_readl(dev, DW_IC_CLR_RX_DONE);
   322	
 > 323			i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
   324			stat = i2c_dw_read_clear_intrbits_slave(dev);
   325			return true;
   326		}
   327	
   328		if (stat & DW_IC_INTR_RX_FULL) {
   329			val = dw_readl(dev, DW_IC_DATA_CMD);
   330			if (!i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_RECEIVED,
   331			 &val))
   332				dev_dbg(dev->dev, "Byte %X acked!", val);
   333		} else {
   334			i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
   335			stat = i2c_dw_read_clear_intrbits_slave(dev);
   336		}
   337	
   338		if (stat & DW_IC_INTR_TX_OVER)
   339			dw_readl(dev, DW_IC_CLR_TX_OVER);
   340	
   341		return true;
   342	}
   343	
   344	static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
   345	{
   346		struct dw_i2c_dev *dev = dev_id;
   347	
   348		i2c_dw_read_clear_intrbits_slave(dev);
   349		if (!i2c_dw_irq_handler_slave(dev))
   350			return IRQ_NONE;
   351	
   352		complete(&dev->cmd_complete);
   353		return IRQ_HANDLED;
   354	}
   355	
   356	static struct i2c_algorithm i2c_dw_algo = {
   357		.functionality	= i2c_dw_func,
 > 358		.reg_slave	= i2c_dw_reg_slave,
 > 359		.unreg_slave	= i2c_dw_unreg_slave,
   360	};
   361	
   362	void i2c_dw_disable_slave(struct dw_i2c_dev *dev)

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

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

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-07 17:55 ` [PATCH v4 3/5] i2c: designware: Add slave definitions Luis Oliveira
  2016-12-07 19:11   ` Andy Shevchenko
@ 2016-12-12 17:02   ` Rob Herring
  2016-12-12 18:35     ` Luis de Oliveira
  1 sibling, 1 reply; 22+ messages in thread
From: Rob Herring @ 2016-12-12 17:02 UTC (permalink / raw)
  To: Luis Oliveira
  Cc: wsa, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel,
	Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On Wed, Dec 07, 2016 at 05:55:50PM +0000, Luis Oliveira wrote:
> - Add slave definitions to i2c-designware-core
> - Changes in Kconfig to auto-enable I2C_SLAVE when compiling the modules
> - Add mode property to designware-core.txt that enable the "slave" selection:
>   - "mode" is an optional property that could be "slave" or "master"
>   - if "mode" is not set the block is considered master by default
> 
> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
> Changes V3->V4: (Andy Shevchenko)
> - created a common property for modes 
> - placed the generic dependency first
> 
>  .../devicetree/bindings/i2c/i2c-designware.txt     |  4 ++++
>  drivers/i2c/busses/Kconfig                         |  1 +
>  drivers/i2c/busses/i2c-designware-common.c         |  6 +++++
>  drivers/i2c/busses/i2c-designware-core.h           | 26 ++++++++++++++++++++++
>  4 files changed, 37 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index fee26dc3e858..8ed2b532cd54 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -20,6 +20,9 @@ Optional properties :
>   - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
>     This value which is by default 300ns is used to compute the tHIGH period.
>  
> + - mode : should be either:
> +           - "master" to setup the hardware block as a I2C master
> +           - "slave" to setup the hardware block as a I2C slave

This should be documented in a common location. Can't it be a boolean to 
enable slave mode? Or don't you need to set the slave address? That 
could be enough to enable slave mode and there's already one example 
doing that.

Rob

^ permalink raw reply	[flat|nested] 22+ messages in thread
* RE: [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-12 17:02   ` Rob Herring
@ 2016-12-12 18:35     ` Luis de Oliveira
  2016-12-12 23:15       ` Rob Herring
  0 siblings, 1 reply; 22+ messages in thread
From: Luis de Oliveira @ 2016-12-12 18:35 UTC (permalink / raw)
  To: Rob Herring, Luis Oliveira
  Cc: wsa, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel,
	Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

Hi all,

The slave address could be set by the I2C slave backend so I can't use it to setup the controller. 
A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way. 
Can you please tell me where should it be documented? 

Luis

-----Original Message-----
From: Rob Herring [mailto:robh@kernel.org] 
Sent: Monday, December 12, 2016 17:02
To: Luis Oliveira <Luis.Oliveira@synopsys.com>
Cc: wsa@the-dreams.de; mark.rutland@arm.com; jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Ramiro.Oliveira@synopsys.com; Joao.Pinto@synopsys.com; CARLOS.PALMINHA@synopsys.com
Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions

On Wed, Dec 07, 2016 at 05:55:50PM +0000, Luis Oliveira wrote:
> - Add slave definitions to i2c-designware-core
> - Changes in Kconfig to auto-enable I2C_SLAVE when compiling the 
> modules
> - Add mode property to designware-core.txt that enable the "slave" selection:
>   - "mode" is an optional property that could be "slave" or "master"
>   - if "mode" is not set the block is considered master by default
> 
> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
> Changes V3->V4: (Andy Shevchenko)
> - created a common property for modes
> - placed the generic dependency first
> 
>  .../devicetree/bindings/i2c/i2c-designware.txt     |  4 ++++
>  drivers/i2c/busses/Kconfig                         |  1 +
>  drivers/i2c/busses/i2c-designware-common.c         |  6 +++++
>  drivers/i2c/busses/i2c-designware-core.h           | 26 ++++++++++++++++++++++
>  4 files changed, 37 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt 
> b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index fee26dc3e858..8ed2b532cd54 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -20,6 +20,9 @@ Optional properties :
>   - i2c-sda-falling-time-ns : should contain the SDA falling time in nanoseconds.
>     This value which is by default 300ns is used to compute the tHIGH period.
>  
> + - mode : should be either:
> +           - "master" to setup the hardware block as a I2C master
> +           - "slave" to setup the hardware block as a I2C slave

This should be documented in a common location. Can't it be a boolean to enable slave mode? Or don't you need to set the slave address? That could be enough to enable slave mode and there's already one example doing that.

Rob

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-12 18:35     ` Luis de Oliveira
@ 2016-12-12 23:15       ` Rob Herring
  2016-12-13 10:50         ` Luis de Oliveira
  0 siblings, 1 reply; 22+ messages in thread
From: Rob Herring @ 2016-12-12 23:15 UTC (permalink / raw)
  To: Luis de Oliveira
  Cc: wsa, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel,
	Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira
<Luis.Oliveira@synopsys.com> wrote:
> Hi all,

Please don't top post.

>
> The slave address could be set by the I2C slave backend so I can't use it to setup the controller.
> A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way.
> Can you please tell me where should it be documented?

bindings/i2c/i2c.txt.

Actually, looking at this some more, we already have a way to describe
the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS
flag in the reg property. We should just need a helper to read reg
property of each child and check for the bit set.

Rob

^ permalink raw reply	[flat|nested] 22+ messages in thread
* RE: [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-12 23:15       ` Rob Herring
@ 2016-12-13 10:50         ` Luis de Oliveira
  2016-12-13 14:11           ` Rob Herring
  0 siblings, 1 reply; 22+ messages in thread
From: Luis de Oliveira @ 2016-12-13 10:50 UTC (permalink / raw)
  To: Rob Herring, Luis de Oliveira
  Cc: wsa, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel,
	Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

The controller for i2c-designware cannot be slave/master at the same time and it has to be enabled knowing beforehand if we want it to be slave or master by something outside of the controller itself.

I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the "linux,slave-24c02" slave interface  but I am not seeing how it will help me identify a selected i2c-designware block as a "slave" device before instantiation. I'm sorry if I'm not understanding properly.
I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave with an address so I can do write/read operations, it is how I tested it. 

Luis

-----Original Message-----
From: Rob Herring [mailto:robh@kernel.org] 
Sent: Monday, December 12, 2016 23:16
To: Luis de Oliveira <Luis.Oliveira@synopsys.com>
Cc: wsa@the-dreams.de; mark.rutland@arm.com; jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Ramiro.Oliveira@synopsys.com; Joao.Pinto@synopsys.com; CARLOS.PALMINHA@synopsys.com
Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions

On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira <Luis.Oliveira@synopsys.com> wrote:
> Hi all,

Please don't top post.

>
> The slave address could be set by the I2C slave backend so I can't use it to setup the controller.
> A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way.
> Can you please tell me where should it be documented?

bindings/i2c/i2c.txt.

Actually, looking at this some more, we already have a way to describe the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the reg property. We should just need a helper to read reg property of each child and check for the bit set.

Rob

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-13 10:50         ` Luis de Oliveira
@ 2016-12-13 14:11           ` Rob Herring
  2016-12-22 14:59             ` Luis Oliveira
  0 siblings, 1 reply; 22+ messages in thread
From: Rob Herring @ 2016-12-13 14:11 UTC (permalink / raw)
  To: Luis de Oliveira
  Cc: wsa, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel,
	Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

Again, please don't top post. And your line wrapping is messed up.
IOW, you can't use Outlook.

On Tue, Dec 13, 2016 at 4:50 AM, Luis de Oliveira
<Luis.Oliveira@synopsys.com> wrote:
> The controller for i2c-designware cannot be slave/master at the same time and it has to be enabled knowing beforehand if we want it to be slave or master by something outside of the controller itself.
>
> I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the "linux,slave-24c02" slave interface  but I am not seeing how it will help me identify a selected i2c-designware block as a "slave" device before instantiation. I'm sorry if I'm not understanding properly.
> I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave with an address so I can do write/read operations, it is how I tested it.

Something like this:

of_for_each_child_node(child) {
  of_property_read_u32(child, "reg", &reg);
  if (reg & I2C_OWN_SLAVE_ADDRESS))
    im_a_slave = true;
}

...rather than testing "mode" is equal to "slave".

Rob

>
> Luis
>
> -----Original Message-----
> From: Rob Herring [mailto:robh@kernel.org]
> Sent: Monday, December 12, 2016 23:16
> To: Luis de Oliveira <Luis.Oliveira@synopsys.com>
> Cc: wsa@the-dreams.de; mark.rutland@arm.com; jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Ramiro.Oliveira@synopsys.com; Joao.Pinto@synopsys.com; CARLOS.PALMINHA@synopsys.com
> Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
>
> On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira <Luis.Oliveira@synopsys.com> wrote:
>> Hi all,
>
> Please don't top post.
>
>>
>> The slave address could be set by the I2C slave backend so I can't use it to setup the controller.
>> A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way.
>> Can you please tell me where should it be documented?
>
> bindings/i2c/i2c.txt.
>
> Actually, looking at this some more, we already have a way to describe the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the reg property. We should just need a helper to read reg property of each child and check for the bit set.
>
> Rob

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation
  2016-12-07 17:55 ` [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation Luis Oliveira
  2016-12-07 19:52   ` Andy Shevchenko
@ 2016-12-13 15:08   ` Jarkko Nikula
  1 sibling, 0 replies; 22+ messages in thread
From: Jarkko Nikula @ 2016-12-13 15:08 UTC (permalink / raw)
  To: Luis Oliveira, wsa, robh+dt, mark.rutland, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel
  Cc: Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On 12/07/2016 07:55 PM, Luis Oliveira wrote:
> - Missspelling, comment formatation and fix a string of
>   the existing code
>
> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
> Changes V3->V4: (Andy Shevchenko)
> - created a commit message
>
>  drivers/i2c/busses/i2c-designware-common.c |  2 +-
>  drivers/i2c/busses/i2c-designware-slave.c  | 10 ++++++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index 41b38d8b8732..838ef662d2c8 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -42,7 +42,7 @@ static char *abort_sources[] = {
>  	[ABRT_TXDATA_NOACK] =
>  		"data not acknowledged",
>  	[ABRT_GCALL_NOACK] =
> -		"no acknowledgement for a general call",
> +		"no acknowledgment for a general call",

I'm not a native speaker but are both acknowledgement and acknowledgment 
ok? I.e. is there need to change?

> diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
> index 1c7f82bb2513..442bc5ce6d47 100644
> --- a/drivers/i2c/busses/i2c-designware-slave.c
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -70,8 +70,8 @@ int i2c_dw_init_slave(struct dw_i2c_dev *dev)
>  		/* Configure register access mode 16bit */
>  		dev->accessor_flags |= ACCESS_16BIT;
>  	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
> -		dev_err(dev->dev, "Unknown Synopsys component type: "
> -			"0x%08x\n", reg);
> +		dev_err(dev->dev,
> +		 "Unknown Synopsys component type: 0x%08x\n", reg);
>  		i2c_dw_release_lock(dev);
>  		return -ENODEV;
>  	}
> @@ -181,8 +181,10 @@ int i2c_dw_reg_slave(struct i2c_client *slave)
>  		return -EBUSY;
>  	if (slave->flags & I2C_CLIENT_TEN)
>  		return -EAFNOSUPPORT;
> -		/* set slave address in the IC_SAR register,
> -		* the address to which the DW_apb_i2c responds */
> +		/*
> +		 * set slave address in the IC_SAR register,
> +		 * the address to which the DW_apb_i2c responds
> +		 */

These two should be done already in the patch 4/5 since it introduced 
the lines that you are changing here.

-- 
Jarkko

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-13 14:11           ` Rob Herring
@ 2016-12-22 14:59             ` Luis Oliveira
  2016-12-22 15:29               ` Andy Shevchenko
  0 siblings, 1 reply; 22+ messages in thread
From: Luis Oliveira @ 2016-12-22 14:59 UTC (permalink / raw)
  To: Rob Herring, Luis de Oliveira
  Cc: wsa, mark.rutland, jarkko.nikula, andriy.shevchenko,
	mika.westerberg, linux-i2c, devicetree, linux-kernel,
	Ramiro.Oliveira, Joao.Pinto, CARLOS.PALMINHA

On 13-Dec-16 14:11, Rob Herring wrote:
> Again, please don't top post. And your line wrapping is messed up.
> IOW, you can't use Outlook.
> 
> On Tue, Dec 13, 2016 at 4:50 AM, Luis de Oliveira
> <Luis.Oliveira@synopsys.com> wrote:
>> The controller for i2c-designware cannot be slave/master at the same time and it has to be enabled knowing beforehand if we want it to be slave or master by something outside of the controller itself.
>>
>> I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the "linux,slave-24c02" slave interface  but I am not seeing how it will help me identify a selected i2c-designware block as a "slave" device before instantiation. I'm sorry if I'm not understanding properly.
>> I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave with an address so I can do write/read operations, it is how I tested it.
> 
> Something like this:
> 
> of_for_each_child_node(child) {
>   of_property_read_u32(child, "reg", &reg);
>   if (reg & I2C_OWN_SLAVE_ADDRESS))
>     im_a_slave = true;
> }
> 
> ...rather than testing "mode" is equal to "slave".
> 
> Rob
> 

Hi Rob, Andy,

I'm struggling to implement your suggestion @Rob. I checked the
tegra124-jetson-tk1.dts that uses that approach but I have some doubts.

My DT is as follows

	i2c@0x2000 {
                        compatible = "snps,designware-i2c";
                        reg = <0x2000 0x100>;
                        clock-frequency = <400000>;
                        clocks = <&i2cclk>;
                        interrupts = <0>;

I could add something like this:

			eeprom@64 {
				compatible = "linux,slave-24c02";
				reg = <(I2C_OWN_SLAVE_ADDRESS | 0x64)>;
			}

But I think this is different form what I was doing before. I have two questions:

- This way the I2C controller is identified as a slave controller or just the
subnode eeprom?
- This way looks like my slave address will be fixed

In the previous Patch v3 submission @Andy suggested a property that selects mode
that I did and it's working. And you @Rob suggested to do it a common property.
It is implemented in the DT like:

			mode = "slave";

So before I do this changes can you please agree both if you still think this is
the best approach?


Thank you both for your time,
Luis

>>
>> Luis
>>
>> -----Original Message-----
>> From: Rob Herring [mailto:robh@kernel.org]
>> Sent: Monday, December 12, 2016 23:16
>> To: Luis de Oliveira <Luis.Oliveira@synopsys.com>
>> Cc: wsa@the-dreams.de; mark.rutland@arm.com; jarkko.nikula@linux.intel.com; andriy.shevchenko@linux.intel.com; mika.westerberg@linux.intel.com; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Ramiro.Oliveira@synopsys.com; Joao.Pinto@synopsys.com; CARLOS.PALMINHA@synopsys.com
>> Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
>>
>> On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira <Luis.Oliveira@synopsys.com> wrote:
>>> Hi all,
>>
>> Please don't top post.
>>
>>>
>>> The slave address could be set by the I2C slave backend so I can't use it to setup the controller.
>>> A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way.
>>> Can you please tell me where should it be documented?
>>
>> bindings/i2c/i2c.txt.
>>
>> Actually, looking at this some more, we already have a way to describe the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the reg property. We should just need a helper to read reg property of each child and check for the bit set.
>>
>> Rob

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
  2016-12-22 14:59             ` Luis Oliveira
@ 2016-12-22 15:29               ` Andy Shevchenko
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2016-12-22 15:29 UTC (permalink / raw)
  To: Luis Oliveira, Rob Herring
  Cc: wsa, mark.rutland, jarkko.nikula, mika.westerberg, linux-i2c,
	devicetree, linux-kernel, Ramiro.Oliveira, Joao.Pinto,
	CARLOS.PALMINHA

On Thu, 2016-12-22 at 14:59 +0000, Luis Oliveira wrote:
> On 13-Dec-16 14:11, Rob Herring wrote:

> > Something like this:
> > 
> > of_for_each_child_node(child) {
> >   of_property_read_u32(child, "reg", &reg);
> >   if (reg & I2C_OWN_SLAVE_ADDRESS))
> >     im_a_slave = true;
> > }
> > 
> > ...rather than testing "mode" is equal to "slave".
> > 
> > Rob
> > 
> 
> Hi Rob, Andy,
> 
> I'm struggling to implement your suggestion @Rob. I checked the
> tegra124-jetson-tk1.dts that uses that approach but I have some
> doubts.
> 
> My DT is as follows
> 
> 	i2c@0x2000 {
>                         compatible = "snps,designware-i2c";
>                         reg = <0x2000 0x100>;
>                         clock-frequency = <400000>;
>                         clocks = <&i2cclk>;
>                         interrupts = <0>;
> 
> I could add something like this:
> 
> 			eeprom@64 {
> 				compatible = "linux,slave-24c02";
> 				reg = <(I2C_OWN_SLAVE_ADDRESS | 0x64)>;
> 			}
> 
> But I think this is different form what I was doing before. I have two
> questions:
> 
> - This way the I2C controller is identified as a slave controller or
> just the
> subnode eeprom?
> - This way looks like my slave address will be fixed
> 
> In the previous Patch v3 submission @Andy suggested a property that
> selects mode
> that I did and it's working. And you @Rob suggested to do it a common
> property.
> It is implemented in the DT like:
> 
> 			mode = "slave";
> 
> So before I do this changes can you please agree both if you still
> think this is
> the best approach?

I'm a bit lost in the discussion (and TBH busy by something else), so I
would agree on whatever you and Rob make an agreement on.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 22+ messages in thread
end of thread, other threads:[~2016-12-22 15:30 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-07 17:55 [PATCH v4 0/5] i2c: designware: Add slave support Luis Oliveira
2016-12-07 17:55 ` [PATCH v4 1/5] i2c: designware: Refactoring of the i2c-designware Luis Oliveira
2016-12-07 18:58   ` Andy Shevchenko
2016-12-07 17:55 ` [PATCH v4 2/5] i2c: designware: Master mode as separated driver Luis Oliveira
2016-12-07 19:03   ` Andy Shevchenko
2016-12-07 17:55 ` [PATCH v4 3/5] i2c: designware: Add slave definitions Luis Oliveira
2016-12-07 19:11   ` Andy Shevchenko
2016-12-12 17:02   ` Rob Herring
2016-12-12 18:35     ` Luis de Oliveira
2016-12-12 23:15       ` Rob Herring
2016-12-13 10:50         ` Luis de Oliveira
2016-12-13 14:11           ` Rob Herring
2016-12-22 14:59             ` Luis Oliveira
2016-12-22 15:29               ` Andy Shevchenko
2016-12-07 17:55 ` [PATCH v4 4/5] i2c: designware: Add slave mode as separated driver Luis Oliveira
2016-12-07 19:07   ` Mark Rutland
2016-12-07 19:49   ` Andy Shevchenko
2016-12-08  4:38   ` kbuild test robot
2016-12-10 23:44   ` kbuild test robot
2016-12-07 17:55 ` [PATCH v4 5/5] i2c: designware: Cleaning comments and formatation Luis Oliveira
2016-12-07 19:52   ` Andy Shevchenko
2016-12-13 15:08   ` Jarkko Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).