[PATCH v2 0/4] Add slave mode to Synopsys I2C driver

[PATCH v2 0/4] Add slave mode to Synopsys I2C driver

[Luis.Oliveira]

From: Luis Oliveira <lolivei@synopsys.com>

Add support in existing I2C Designware Core driver for I2C slave mode. 
Refactored *_master() functions out of existing ones in the first patch
Added *_slave() functions to enable Slave mode. 
Updated the description of the i2c-designware.txt and changed Kconfig to 
auto enable I2C Slave support.

V2: Splitted the patch in *_master() and *_slave() and updated Kconfig. 

Luis Oliveira (4):
  Factor out _master() parts of code and identify as much as possible
    all related with MASTER mode
  Added I2C_SLAVE as a dependency to I2C_DESIGNWARE_CORE Enable _slave()
    mode Review of the pm_runtime...() methods and cleaning
  Device bindings documentation updated ACPI-enabled platforms not
    currently supported
  Cleaning

 .../devicetree/bindings/i2c/i2c-designware.txt     |   5 +-
 drivers/i2c/busses/Kconfig                         |   3 +-
 drivers/i2c/busses/i2c-designware-core.c           | 238 ++++++++++++++++++---
 drivers/i2c/busses/i2c-designware-core.h           |   6 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |  68 ++++--
 5 files changed, 280 insertions(+), 40 deletions(-)

-- 
2.10.1

[PATCH v2 1/4] Factor out _master() parts of code and identify as much as possible all related with MASTER mode

[Luis.Oliveira]

From: Luis Oliveira <lolivei@synopsys.com>

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
 drivers/i2c/busses/i2c-designware-core.c    | 71 +++++++++++++++++++----------
 drivers/i2c/busses/i2c-designware-platdrv.c | 37 +++++++++------
 2 files changed, 71 insertions(+), 37 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 1fe93c4..cc38329 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -87,13 +87,15 @@
 #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_STATUS_TFE		BIT(2)
-#define DW_IC_STATUS_MST_ACTIVITY	BIT(5)
+#define DW_IC_STATUS_MASTER_ACTIVITY	BIT(5)
 
 #define DW_IC_ERR_TX_ABRT	0x1
 
@@ -201,6 +203,17 @@ 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);
+		dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
+}
+
 static u32
 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 {
@@ -317,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.
  */
@@ -431,12 +444,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);
 
@@ -480,7 +490,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 		 */
 		ic_status = dw_readl(dev, DW_IC_STATUS);
 		if (!dev->dynamic_tar_update_enabled ||
-		    (ic_status & DW_IC_STATUS_MST_ACTIVITY) ||
+		    (ic_status & DW_IC_STATUS_MASTER_ACTIVITY) ||
 		    !(ic_status & DW_IC_STATUS_TFE)) {
 			__i2c_dw_enable_and_wait(dev, false);
 			enabled = false;
@@ -520,7 +530,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);
 }
 
 /*
@@ -540,7 +550,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++) {
 		/*
@@ -839,19 +849,13 @@ 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)
-{
-	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;
 
+static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
+{
+	u32 stat;
+	
 	stat = i2c_dw_read_clear_intrbits(dev);
-
+	
 	if (stat & DW_IC_INTR_TX_ABRT) {
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
 		dev->status = STATUS_IDLE;
@@ -895,7 +899,26 @@ tx_aborted:
 		i2c_dw_disable_int(dev);
 		dw_writel(dev, stat, DW_IC_INTR_MASK);
 	}
+	return true;
+}
+
+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);
+	mode = dw_readl(dev, DW_IC_CON);
+	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;
 
+	if(i2c_dw_irq_handler_master(dev))
+		return IRQ_HANDLED;
+			
+	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 0b42a12..d5986c2 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -138,6 +138,29 @@ 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->functionality |= I2C_FUNC_10BIT_ADDR;
+	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))
@@ -222,19 +245,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		I2C_FUNC_SMBUS_WORD_DATA |
 		I2C_FUNC_SMBUS_I2C_BLOCK;
 
-	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.10.1

[PATCH v2 2/4] Added I2C_SLAVE as a dependency to I2C_DESIGNWARE_CORE Enable _slave() mode Review of the pm_runtime...() methods and cleaning

[Luis.Oliveira]

From: Luis Oliveira <lolivei@synopsys.com>

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
 drivers/i2c/busses/Kconfig                  |   3 +-
 drivers/i2c/busses/i2c-designware-core.c    | 180 ++++++++++++++++++++++++++--
 drivers/i2c/busses/i2c-designware-core.h    |   6 +
 drivers/i2c/busses/i2c-designware-platdrv.c |  33 ++++-
 4 files changed, 212 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 6d94e2e..f93ad70 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -470,10 +470,11 @@ config I2C_DESIGNWARE_CORE
 config I2C_DESIGNWARE_PLATFORM
 	tristate "Synopsys DesignWare Platform"
 	select I2C_DESIGNWARE_CORE
+	select I2C_SLAVE
 	depends on (ACPI && COMMON_CLK) || !ACPI
 	help
 	  If you say yes to this option, support will be included for the
-	  Synopsys DesignWare I2C adapter. Only master mode is supported.
+	  Synopsys DesignWare I2C adapter.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-designware-platform.
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index cc38329..71a377e 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -37,6 +37,7 @@
  */
 #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
@@ -92,10 +93,16 @@
  
 #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_ERR_TX_ABRT	0x1
 
@@ -130,6 +137,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)
@@ -142,6 +152,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 | \
@@ -172,6 +185,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",
 };
 
 static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
@@ -214,6 +233,17 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
 		dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
 }
 
+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);
+}
+
 static u32
 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 {
@@ -447,6 +477,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	if ((dev->master_cfg & DW_IC_CON_MASTER) &&
 		(dev->master_cfg & DW_IC_CON_SLAVE_DISABLE)) 
 		i2c_dw_configure_fifo_master(dev);
+	else
+		i2c_dw_configure_fifo_slave(dev);
 
 	i2c_dw_release_lock(dev);
 
@@ -785,9 +817,59 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
 	return dev->functionality;
 }
 
+static 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);
+
+	pm_runtime_get_sync(dev->dev);
+	
+	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);
+
+	WARN_ON(!dev->slave);
+
+	i2c_dw_disable_int(dev);
+	i2c_dw_disable(dev);
+
+	dev->slave =  NULL;
+
+	pm_runtime_put(dev->dev);
+
+	return 0;
+}
+
 static struct i2c_algorithm i2c_dw_algo = {
 	.master_xfer	= i2c_dw_xfer,
 	.functionality	= i2c_dw_func,
+	.reg_slave	= i2c_dw_reg_slave,
+	.unreg_slave	= i2c_dw_unreg_slave,
 };
 
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
@@ -821,8 +903,6 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
 		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_RD_REQ)
-		dw_readl(dev, DW_IC_CLR_RD_REQ);
 	if (stat & DW_IC_INTR_TX_ABRT) {
 		/*
 		 * The IC_TX_ABRT_SOURCE register is cleared whenever
@@ -849,6 +929,84 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
  * Interrupt service routine. This gets called whenever an I2C 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);
+		
+	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_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(dev);
+			}
+			else {
+				dw_readl(dev, DW_IC_CLR_RD_REQ);
+				dw_readl(dev, DW_IC_CLR_RX_UNDER);
+				stat = i2c_dw_read_clear_intrbits(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(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(dev);
+	}
+
+	if (stat & DW_IC_INTR_TX_OVER) {		
+		dw_readl(dev, DW_IC_CLR_TX_OVER);
+		return true;
+	}
+
+	return true;
+}
 
 static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
 {
@@ -910,14 +1068,20 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	enabled = dw_readl(dev, DW_IC_ENABLE);
 	mode = dw_readl(dev, DW_IC_CON);
 	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;
-
+	
+	if (!(mode & DW_IC_CON_MASTER) && !(mode & DW_IC_CON_SLAVE_DISABLE)) {
+		stat = i2c_dw_read_clear_intrbits(dev);
+		if (!i2c_dw_irq_handler_slave(dev))
+			return IRQ_NONE;
+	} else {
 	if(i2c_dw_irq_handler_master(dev))
 		return IRQ_HANDLED;
-			
+	}
+
 	complete(&dev->cmd_complete);
 	return IRQ_HANDLED;
 }
@@ -984,7 +1148,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 	adap->dev.parent = dev->dev;
 	i2c_set_adapdata(adap, dev);
 
-	i2c_dw_disable_int(dev);
+	if (!i2c_check_functionality(adap,I2C_FUNC_SLAVE)) 
+		i2c_dw_disable_int(dev);
+
 	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
 			     IRQF_SHARED | IRQF_COND_SUSPEND,
 			     dev_name(dev->dev), dev);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 0d44d2a..de5e4a0 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -28,9 +28,13 @@
 #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
 
 
 /**
@@ -80,6 +84,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;
@@ -99,6 +104,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;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d5986c2..b9076da 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -161,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->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->functionality |= I2C_FUNC_SLAVE;
+	dev->functionality &= ~I2C_FUNC_10BIT_ADDR;
+	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;
+		
+	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))
@@ -181,6 +205,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	struct resource *mem;
 	int irq, r;
 	u32 acpi_speed, ht = 0;
+	bool is_slave = false;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -213,6 +238,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 					 &dev->scl_falling_time);
 		device_property_read_u32(&pdev->dev, "clock-frequency",
 					 &dev->clk_freq);
+		is_slave = device_property_read_bool(&pdev->dev, "isslave");
 	}
 
 	acpi_speed = i2c_acpi_find_bus_speed(&pdev->dev);
@@ -244,8 +270,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		I2C_FUNC_SMBUS_BYTE_DATA |
 		I2C_FUNC_SMBUS_WORD_DATA |
 		I2C_FUNC_SMBUS_I2C_BLOCK;
-
-	i2c_dw_configure_master(pdev);
+	
+	if (is_slave)
+		i2c_dw_configure_slave(pdev);
+	else 
+		i2c_dw_configure_master(pdev);
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (!i2c_dw_plat_prepare_clk(dev, true)) {
-- 
2.10.1

[PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Luis.Oliveira]

From: Luis Oliveira <lolivei@synopsys.com>

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
 Documentation/devicetree/bindings/i2c/i2c-designware.txt | 5 ++++-
 drivers/i2c/busses/i2c-designware-platdrv.c              | 4 +++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index fee26dc..d3d163c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -19,6 +19,8 @@ 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.
+   
+ - is-slave : indicates if the block is a I2C slave device.
 
 Example :
 
@@ -30,7 +32,7 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
-
+	
 	i2c@1120000 {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -42,4 +44,5 @@ Example :
 		i2c-sda-hold-time-ns = <300>;
 		i2c-sda-falling-time-ns = <300>;
 		i2c-scl-falling-time-ns = <300>;
+		is-slave;
 	};
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index b9076da..f29e657 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -238,7 +238,9 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 					 &dev->scl_falling_time);
 		device_property_read_u32(&pdev->dev, "clock-frequency",
 					 &dev->clk_freq);
-		is_slave = device_property_read_bool(&pdev->dev, "isslave");
+#ifndef CONFIG_ACPI
+		is_slave = device_property_read_bool(&pdev->dev, "is-slave");
+#endif
 	}
 
 	acpi_speed = i2c_acpi_find_bus_speed(&pdev->dev);
-- 
2.10.1

[PATCH v2 4/4] Cleaned the code, no functional changes.

[Luis.Oliveira]

From: Luis Oliveira <lolivei@synopsys.com>

Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
---
 drivers/i2c/busses/i2c-designware-core.c    | 113 ++++++++++++++--------------
 drivers/i2c/busses/i2c-designware-platdrv.c |  24 +++---
 2 files changed, 68 insertions(+), 69 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 71a377e..4196491 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -90,15 +90,15 @@
 #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)
-					 
+				    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)
@@ -229,7 +229,7 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
 		dw_writel(dev, 0, DW_IC_RX_TL);
 
 		/* configure the i2c master */
-		dw_writel(dev, dev->master_cfg , DW_IC_CON);
+		dw_writel(dev, dev->master_cfg, DW_IC_CON);
 		dw_writel(dev, DW_IC_INTR_MASTER_MASK, DW_IC_INTR_MASK);
 }
 
@@ -240,7 +240,7 @@ static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev)
 		dw_writel(dev, 0, DW_IC_RX_TL);
 
 		/* configure the i2c slave */
-		dw_writel(dev, dev->slave_cfg , DW_IC_CON);
+		dw_writel(dev, dev->slave_cfg, DW_IC_CON);
 		dw_writel(dev, DW_IC_INTR_SLAVE_MASK, DW_IC_INTR_MASK);
 }
 
@@ -386,8 +386,8 @@ int i2c_dw_init(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;
 	}
@@ -475,7 +475,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	}
 
 	if ((dev->master_cfg & DW_IC_CON_MASTER) &&
-		(dev->master_cfg & DW_IC_CON_SLAVE_DISABLE)) 
+		(dev->master_cfg & DW_IC_CON_SLAVE_DISABLE))
 		i2c_dw_configure_fifo_master(dev);
 	else
 		i2c_dw_configure_fifo_slave(dev);
@@ -814,6 +814,7 @@ done_nolock:
 static u32 i2c_dw_func(struct i2c_adapter *adap)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
 	return dev->functionality;
 }
 
@@ -821,19 +822,19 @@ static int i2c_dw_reg_slave(struct i2c_client *slave)
 {
 	struct dw_i2c_dev *dev =  i2c_get_adapdata(slave->adapter);
 
-	if(dev->slave)
+	if (dev->slave)
 		return -EBUSY;
-	if(slave->flags & I2C_CLIENT_TEN)
+	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);
 
 	pm_runtime_get_sync(dev->dev);
-	
+
 	dev->slave = slave;
 
 	__i2c_dw_enable(dev, true);
@@ -929,78 +930,76 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
  * Interrupt service routine. This gets called whenever an I2C interrupt
  * occurs.
  */
- 
-static bool i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev )
+
+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);
-		
-	dev_dbg(dev->dev, 
-	"%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x : INTR_STAT=%#x\n", 
-	 __func__, enabled, slave_activity, raw_stat, stat); 
+	slave_activity = ((dw_readl(dev, DW_IC_STATUS)
+		& DW_IC_STATUS_SLAVE_ACTIVITY)>>6);
+
+	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_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)) 
+
+	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 (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, 
+				if (!i2c_slave_event(dev->slave,
 					I2C_SLAVE_WRITE_RECEIVED, &val)) {
-					dev_dbg(dev->dev, "Byte %X acked! ",val);
+					dev_dbg(dev->dev, "Byte %X acked! ", val);
 				}
 				dw_readl(dev, DW_IC_CLR_RD_REQ);
 				stat = i2c_dw_read_clear_intrbits(dev);
-			}
-			else {
+			} else {
 				dw_readl(dev, DW_IC_CLR_RD_REQ);
 				dw_readl(dev, DW_IC_CLR_RX_UNDER);
 				stat = i2c_dw_read_clear_intrbits(dev);
-			}     
-			if (!i2c_slave_event(dev->slave, 
+			}
+			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)) 
+
+		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);
+
+		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
 		stat = i2c_dw_read_clear_intrbits(dev);
 
 		return true;
 	}
-        
-	if (stat & DW_IC_INTR_RX_FULL) { 
+
+	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);
+			dev_dbg(dev->dev, "Byte %X acked! ", val);
+	} else {
+		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
 		stat = i2c_dw_read_clear_intrbits(dev);
 	}
 
-	if (stat & DW_IC_INTR_TX_OVER) {		
+	if (stat & DW_IC_INTR_TX_OVER) {
 		dw_readl(dev, DW_IC_CLR_TX_OVER);
 		return true;
 	}
@@ -1008,12 +1007,12 @@ static bool i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev )
 	return true;
 }
 
-static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
+static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
 {
 	u32 stat;
-	
+
 	stat = i2c_dw_read_clear_intrbits(dev);
-	
+
 	if (stat & DW_IC_INTR_TX_ABRT) {
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
 		dev->status = STATUS_IDLE;
@@ -1068,17 +1067,17 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	enabled = dw_readl(dev, DW_IC_ENABLE);
 	mode = dw_readl(dev, DW_IC_CON);
 	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;
-	
+
 	if (!(mode & DW_IC_CON_MASTER) && !(mode & DW_IC_CON_SLAVE_DISABLE)) {
 		stat = i2c_dw_read_clear_intrbits(dev);
 		if (!i2c_dw_irq_handler_slave(dev))
 			return IRQ_NONE;
 	} else {
-	if(i2c_dw_irq_handler_master(dev))
+	if (i2c_dw_irq_handler_master(dev))
 		return IRQ_HANDLED;
 	}
 
@@ -1148,7 +1147,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 	adap->dev.parent = dev->dev;
 	i2c_set_adapdata(adap, dev);
 
-	if (!i2c_check_functionality(adap,I2C_FUNC_SLAVE)) 
+	if (!i2c_check_functionality(adap, I2C_FUNC_SLAVE))
 		i2c_dw_disable_int(dev);
 
 	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index f29e657..035b93f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -141,13 +141,13 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev)
 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->functionality |= I2C_FUNC_10BIT_ADDR;
 	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;
@@ -158,25 +158,25 @@ static void i2c_dw_configure_master(struct platform_device *pdev)
 	default:
 		dev->master_cfg |= DW_IC_CON_SPEED_FAST;
 	}
-	
+
 }
 
 static void i2c_dw_configure_slave(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
-	
-	dev->slave_cfg = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL | 
-			  DW_IC_CON_RESTART_EN | DW_IC_CON_STOP_DET_IFADDRESSED | 
+
+	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->functionality |= I2C_FUNC_SLAVE;
 	dev->functionality &= ~I2C_FUNC_10BIT_ADDR;
 	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;
-		
+
 	case 3400000:
 		dev->slave_cfg |= DW_IC_CON_SPEED_HIGH;
 		break;
@@ -272,10 +272,10 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		I2C_FUNC_SMBUS_BYTE_DATA |
 		I2C_FUNC_SMBUS_WORD_DATA |
 		I2C_FUNC_SMBUS_I2C_BLOCK;
-	
+
 	if (is_slave)
 		i2c_dw_configure_slave(pdev);
-	else 
+	else
 		i2c_dw_configure_master(pdev);
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
-- 
2.10.1

Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Mark Rutland]

On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote:
> -		is_slave = device_property_read_bool(&pdev->dev, "isslave");

Which tree is this based on? I cant see the existing isslave property in
mainline HEAD (commit 29fbff8698fc0ac1).

> +#ifndef CONFIG_ACPI
> +		is_slave = device_property_read_bool(&pdev->dev, "is-slave");
> +#endif

This ifdef is broken. At least for arm64, a single kernel image can be
booted with either ACPI or DT. We need separate accessors for DT and
ACPI to handle these differently, or you need to explicitly check
whether or not you have ACPI or DT at runtime.

Thanks,
Mark.

Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Wolfram Sang]

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

On Fri, Oct 14, 2016 at 06:30:15PM +0100, Mark Rutland wrote:
> On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote:
> > -		is_slave = device_property_read_bool(&pdev->dev, "isslave");
> 
> Which tree is this based on? I cant see the existing isslave property in
> mainline HEAD (commit 29fbff8698fc0ac1).

Same surprise here. Because I likely would have NAKed the binding.

Why is it needed? Doesn't the driver allow to be master/slave on the
same bus?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Rob Herring]

On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote:
> From: Luis Oliveira <lolivei@synopsys.com>
> 
> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-designware.txt | 5 ++++-
>  drivers/i2c/busses/i2c-designware-platdrv.c              | 4 +++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index fee26dc..d3d163c 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -19,6 +19,8 @@ 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.
> +   
> + - is-slave : indicates if the block is a I2C slave device.

This should be documented as a generic property.

>  
>  Example :
>  
> @@ -30,7 +32,7 @@ Example :
>  		interrupts = <11>;
>  		clock-frequency = <400000>;
>  	};
> -
> +	

Adding trailing whitespace...

>  	i2c@1120000 {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> @@ -42,4 +44,5 @@ Example :
>  		i2c-sda-hold-time-ns = <300>;
>  		i2c-sda-falling-time-ns = <300>;
>  		i2c-scl-falling-time-ns = <300>;
> +		is-slave;
>  	};

Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Ramiro Oliveira]

On 10/14/2016 6:30 PM, Mark Rutland wrote:
> On Fri, Oct 14, 2016 at 05:52:50PM +0100, Luis.Oliveira@synopsys.com wrote:
>> -		is_slave = device_property_read_bool(&pdev->dev, "isslave");
> Which tree is this based on? I cant see the existing isslave property in
> mainline HEAD (commit 29fbff8698fc0ac1).

I'm replying for Luís, since at the moment he's not able to reply.

This is a custom property since this I2C core doesn't support both master and
slave at the same time.

>
>> +#ifndef CONFIG_ACPI
>> +		is_slave = device_property_read_bool(&pdev->dev, "is-slave");
>> +#endif
> This ifdef is broken. At least for arm64, a single kernel image can be
> booted with either ACPI or DT. We need separate accessors for DT and
> ACPI to handle these differently, or you need to explicitly check
> whether or not you have ACPI or DT at runtime.

Thanks for the feedback, I'll take a look at this

>
> Thanks,
> Mark.

Thanks,
Ramiro

Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Wolfram Sang]

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


> This is needed because the configuration is different and the i2c-designware
> cannot be master/slave without a reset. To resolve that I added this property
> to bind it as a slave when needed.

Aww, pity that the HW can't do that. Do you have details why?

If that is really a HW limitation, then I'd suggest having a seperate
driver for slave-only mode so we can differentiate by compatible
strings.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Luis Oliveira]

Since practically 90% of the code is shared between master and slave, I was
thinking if it will be acceptable to use the same driver for both but
differentiate the master/slave mode by the compatible strings.

Thanks,
Luis

On 10/18/2016 16:17, Wolfram Sang wrote:
>> This is needed because the configuration is different and the i2c-designware
>> cannot be master/slave without a reset. To resolve that I added this property
>> to bind it as a slave when needed.
> Aww, pity that the HW can't do that. Do you have details why?
>
> If that is really a HW limitation, then I'd suggest having a seperate
> driver for slave-only mode so we can differentiate by compatible
> strings.
>

Re: [PATCH v2 1/4] Factor out _master() parts of code and identify as much as possible all related with MASTER mode

[Andy Shevchenko]

On Fri, 2016-10-14 at 17:52 +0100, Luis.Oliveira@synopsys.com wrote:
> From: Luis Oliveira <lolivei@synopsys.com>

This wouldn't be here.

> 

Something wrong with your commit message. Perhaps you were into SVN,
here is a bit different format of the commit messages, i.e.

1. Summary / Subject line — short description
2. Empty line
3. Full description in free form
4. Empty line
5. Tags, such as SoB (Signed-off-by), TB (Tested-by), Fixes, etc.


> @@ -201,6 +203,17 @@ 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 */

I understand you just moved existing lines, though here is a good chance
to do small style fixes to the comments, i.e. use capital letter.

> +		dw_writel(dev, dev->master_cfg , DW_IC_CON);
> +		dw_writel(dev, DW_IC_INTR_MASTER_MASK,
> DW_IC_INTR_MASK);

Too much indentation. One tab is enough here.

> +}

> @@ -431,12 +444,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)) 

Ditto. Align both conditions to be like tabular view.

> +		i2c_dw_configure_fifo_master(dev);
>  
>  	i2c_dw_release_lock(dev);
>  
> @@ -480,7 +490,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
>  		 */
>  		ic_status = dw_readl(dev, DW_IC_STATUS);
>  		if (!dev->dynamic_tar_update_enabled ||
> -		    (ic_status & DW_IC_STATUS_MST_ACTIVITY) ||
> +		    (ic_status & DW_IC_STATUS_MASTER_ACTIVITY) ||
>  		    !(ic_status & DW_IC_STATUS_TFE)) {
>  			__i2c_dw_enable_and_wait(dev, false);
>  			enabled = false;
> @@ -520,7 +530,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);
>  }
>  
>  /*
> @@ -540,7 +550,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++) {
>  		/*
> @@ -839,19 +849,13 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
>   * Interrupt service routine. This gets called whenever an I2C
> interrupt

Perhaps '...I2C master interrupt occurs.' Or move this comment towards
common handler function.

>   * occurs.
>   */
> -static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> -{
> -	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;
>  
> +static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
> +{
> +	u32 stat;
> +	
>  	stat = i2c_dw_read_clear_intrbits(dev);

> -
> +	

This change shouldn't be here.

>  	if (stat & DW_IC_INTR_TX_ABRT) {
>  		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
>  		dev->status = STATUS_IDLE;
> @@ -895,7 +899,26 @@ tx_aborted:
>  		i2c_dw_disable_int(dev);
>  		dw_writel(dev, stat, DW_IC_INTR_MASK);
>  	}
> +	return true;
> +}
> +
> +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);

> +	mode = dw_readl(dev, DW_IC_CON);

Hmm... It's defined but not used?

> +	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;
>  
> +	if(i2c_dw_irq_handler_master(dev))

Keep style — 'if ('.

> +		return IRQ_HANDLED;

> +			

Check for trailing whitespaces. Please, don't add new ones. Good start
is to run checkpatch.pl before send them.


> +	complete(&dev->cmd_complete);

Didn't find removal of the line. Is it new code? Shouldn't be here if
so.

>  	return IRQ_HANDLED;
>  }
>  
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..d5986c2 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -138,6 +138,29 @@ 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->functionality |= I2C_FUNC_10BIT_ADDR;
> +	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;
> +	}

> +	

Again, trailing white space and moreover redundant entire line since
it's empty and doesn't bring any readability.

> +}
> +
>  static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool
> prepare)
>  {
>  	if (IS_ERR(i_dev->clk))
> @@ -222,19 +245,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  		I2C_FUNC_SMBUS_WORD_DATA |
>  		I2C_FUNC_SMBUS_I2C_BLOCK;
>  
> -	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)) {

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

Re: [PATCH v2 2/4] Added I2C_SLAVE as a dependency to I2C_DESIGNWARE_CORE Enable _slave() mode Review of the pm_runtime...() methods and cleaning

[Andy Shevchenko]

On Fri, 2016-10-14 at 17:52 +0100, Luis.Oliveira@synopsys.com wrote:
> From: Luis Oliveira <lolivei@synopsys.com>
> 

Same style issues here and in the code itself. Check all your patches
before submitting.

More comments below.

> @@ -785,9 +817,59 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
>  	return dev->functionality;
>  }
>  
> +static int i2c_dw_reg_slave(struct i2c_client *slave)
> +{
> +	struct dw_i2c_dev *dev =  i2c_get_adapdata(slave->adapter);

Just '...dev = i2c...'.

> +

> +	if(dev->slave)
> +		return -EBUSY;

Hmm... Is it possible that function be called twice?

> +	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);
> +
> +	pm_runtime_get_sync(dev->dev);
> +	

> +	dev->slave = slave;

So, you are using this variable to serialize the calls. Assume it's
possible that upper layer calls several time the function how you
protect the potential race here?

No need runtime PM for this line as well.

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

> +	WARN_ON(!dev->slave);

I'm not sure it's needed. Consider together with comment regarding
_reg_slave().

> +
> +	i2c_dw_disable_int(dev);
> +	i2c_dw_disable(dev);
> +

> +	dev->slave =  NULL;

Same comments as for _reg_slave().

> +
> +	pm_runtime_put(dev->dev);
> +
> +	return 0;
> +}
> +
>  static struct i2c_algorithm i2c_dw_algo = {
>  	.master_xfer	= i2c_dw_xfer,
>  	.functionality	= i2c_dw_func,
> +	.reg_slave	= i2c_dw_reg_slave,
> +	.unreg_slave	= i2c_dw_unreg_slave,
>  };
>  
>  static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
> @@ -821,8 +903,6 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
>  		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_RD_REQ)
> -		dw_readl(dev, DW_IC_CLR_RD_REQ);
>  	if (stat & DW_IC_INTR_TX_ABRT) {
>  		/*
>  		 * The IC_TX_ABRT_SOURCE register is cleared whenever
> @@ -849,6 +929,84 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
>   * Interrupt service routine. This gets called whenever an I2C
> 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);
> +		
> +	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_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(dev);

> +			}
> +			else {

Style is '} else {'.

> +				dw_readl(dev, DW_IC_CLR_RD_REQ);
> +				dw_readl(dev, DW_IC_CLR_RX_UNDER);
> +				stat =
> i2c_dw_read_clear_intrbits(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(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(dev);
> +	}
> +
> +	if (stat & DW_IC_INTR_TX_OVER) {		
> +		dw_readl(dev, DW_IC_CLR_TX_OVER);
> +		return true;
> +	}
> +
> +	return true;
> +}
>  
>  static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
>  {
> @@ -910,14 +1068,20 @@ static irqreturn_t i2c_dw_isr(int this_irq,
> void *dev_id)
>  	enabled = dw_readl(dev, DW_IC_ENABLE);
>  	mode = dw_readl(dev, DW_IC_CON);
>  	stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
> -
> +	

Shouldn't be here.

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

Ditto.

> +	if (!(mode & DW_IC_CON_MASTER) && !(mode &
> DW_IC_CON_SLAVE_DISABLE)) {
> +		stat = i2c_dw_read_clear_intrbits(dev);
> +		if (!i2c_dw_irq_handler_slave(dev))
> +			return IRQ_NONE;
> +	} else {
>  	if(i2c_dw_irq_handler_master(dev))
>  		return IRQ_HANDLED;

> -			

> +	}

> +

Ditto.

>  	complete(&dev->cmd_complete);
>  	return IRQ_HANDLED;
>  }
> @@ -984,7 +1148,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
>  	adap->dev.parent = dev->dev;
>  	i2c_set_adapdata(adap, dev);
>  
> -	i2c_dw_disable_int(dev);
> +	if (!i2c_check_functionality(adap,I2C_FUNC_SLAVE)) 
> +		i2c_dw_disable_int(dev);
> +
>  	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
>  			     IRQF_SHARED | IRQF_COND_SUSPEND,
>  			     dev_name(dev->dev), dev);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h
> b/drivers/i2c/busses/i2c-designware-core.h
> index 0d44d2a..de5e4a0 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -28,9 +28,13 @@
>  #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
>  

Is it possible to split your approach to something like:
1. Add necessary definitions (not actual code).
2. Enable slave.


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

Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Andy Shevchenko]

On Fri, 2016-10-21 at 10:56 +0100, Luis Oliveira wrote:
> Since practically 90% of the code is shared between master and slave,
> I was
> thinking if it will be acceptable to use the same driver for both but
> differentiate the master/slave mode by the compatible strings.

It might be possible to split like other drivers do:

1. Core part (i2c-designware-core.c)
2. Master part (i2c-designware-master.c)
3. Slave part (i2c-designware-slave.c)
4. Glue drivers (like: i2c-designware-platdrv.c)

> 
> Thanks,
> Luis
> 
> On 10/18/2016 16:17, Wolfram Sang wrote:
> > > This is needed because the configuration is different and the i2c-
> > > designware
> > > cannot be master/slave without a reset. To resolve that I added
> > > this property
> > > to bind it as a slave when needed.
> > 
> > Aww, pity that the HW can't do that. Do you have details why?
> > 
> > If that is really a HW limitation, then I'd suggest having a
> > seperate
> > driver for slave-only mode so we can differentiate by compatible
> > strings.
> > 
> 
> 

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

Re: [PATCH v2 4/4] Cleaned the code, no functional changes.

[Andy Shevchenko]

On Fri, 2016-10-14 at 17:52 +0100, Luis.Oliveira@synopsys.com wrote:
> From: Luis Oliveira <lolivei@synopsys.com>
> 

Style issues.

The parts of this patch shouldn't be brought by the others patches in
the series. Consider carefully check your patches before submitting.

> Signed-off-by: Luis Oliveira <lolivei@synopsys.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c    | 113 ++++++++++++++-----
> ---------
>  drivers/i2c/busses/i2c-designware-platdrv.c |  24 +++---
>  2 files changed, 68 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c
> b/drivers/i2c/busses/i2c-designware-core.c
> index 71a377e..4196491 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -90,15 +90,15 @@
>  #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_INTR_SLAVE_MASK		(DW_IC_INTR_DEFAULT_MASK
> | \
>  					 DW_IC_INTR_RX_DONE | \
> -                                    DW_IC_INTR_RX_UNDER | \
> -                                    DW_IC_INTR_RD_REQ)
> -					 
> +				    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)
> @@ -229,7 +229,7 @@ static void i2c_dw_configure_fifo_master(struct
> dw_i2c_dev *dev)
>  		dw_writel(dev, 0, DW_IC_RX_TL);
>  
>  		/* configure the i2c master */
> -		dw_writel(dev, dev->master_cfg , DW_IC_CON);
> +		dw_writel(dev, dev->master_cfg, DW_IC_CON);
>  		dw_writel(dev, DW_IC_INTR_MASTER_MASK,
> DW_IC_INTR_MASK);
>  }
>  
> @@ -240,7 +240,7 @@ static void i2c_dw_configure_fifo_slave(struct
> dw_i2c_dev *dev)
>  		dw_writel(dev, 0, DW_IC_RX_TL);
>  
>  		/* configure the i2c slave */
> -		dw_writel(dev, dev->slave_cfg , DW_IC_CON);
> +		dw_writel(dev, dev->slave_cfg, DW_IC_CON);
>  		dw_writel(dev, DW_IC_INTR_SLAVE_MASK,
> DW_IC_INTR_MASK);
>  }
>  
> @@ -386,8 +386,8 @@ int i2c_dw_init(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;
>  	}
> @@ -475,7 +475,7 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	}
>  
>  	if ((dev->master_cfg & DW_IC_CON_MASTER) &&
> -		(dev->master_cfg & DW_IC_CON_SLAVE_DISABLE)) 
> +		(dev->master_cfg & DW_IC_CON_SLAVE_DISABLE))
>  		i2c_dw_configure_fifo_master(dev);
>  	else
>  		i2c_dw_configure_fifo_slave(dev);
> @@ -814,6 +814,7 @@ done_nolock:
>  static u32 i2c_dw_func(struct i2c_adapter *adap)
>  {
>  	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +
>  	return dev->functionality;
>  }
>  
> @@ -821,19 +822,19 @@ static int i2c_dw_reg_slave(struct i2c_client
> *slave)
>  {
>  	struct dw_i2c_dev *dev =  i2c_get_adapdata(slave->adapter);
>  
> -	if(dev->slave)
> +	if (dev->slave)
>  		return -EBUSY;
> -	if(slave->flags & I2C_CLIENT_TEN)
> +	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);
>  
>  	pm_runtime_get_sync(dev->dev);
> -	
> +
>  	dev->slave = slave;
>  
>  	__i2c_dw_enable(dev, true);
> @@ -929,78 +930,76 @@ static u32 i2c_dw_read_clear_intrbits(struct
> dw_i2c_dev *dev)
>   * Interrupt service routine. This gets called whenever an I2C
> interrupt
>   * occurs.
>   */
> - 
> -static bool i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev )
> +
> +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);
> -		
> -	dev_dbg(dev->dev, 
> -	"%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :
> INTR_STAT=%#x\n", 
> -	 __func__, enabled, slave_activity, raw_stat, stat); 
> +	slave_activity = ((dw_readl(dev, DW_IC_STATUS)
> +		& DW_IC_STATUS_SLAVE_ACTIVITY)>>6);
> +
> +	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_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)) 
> +
> +	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 (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, 
> +				if (!i2c_slave_event(dev->slave,
>  					I2C_SLAVE_WRITE_RECEIVED,
> &val)) {
> -					dev_dbg(dev->dev, "Byte %X
> acked! ",val);
> +					dev_dbg(dev->dev, "Byte %X
> acked! ", val);
>  				}
>  				dw_readl(dev, DW_IC_CLR_RD_REQ);
>  				stat =
> i2c_dw_read_clear_intrbits(dev);
> -			}
> -			else {
> +			} else {
>  				dw_readl(dev, DW_IC_CLR_RD_REQ);
>  				dw_readl(dev, DW_IC_CLR_RX_UNDER);
>  				stat =
> i2c_dw_read_clear_intrbits(dev);
> -			}     
> -			if (!i2c_slave_event(dev->slave, 
> +			}
> +			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)) 
> +
> +		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);
> +
> +		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
>  		stat = i2c_dw_read_clear_intrbits(dev);
>  
>  		return true;
>  	}
> -        
> -	if (stat & DW_IC_INTR_RX_FULL) { 
> +
> +	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);
> +			dev_dbg(dev->dev, "Byte %X acked! ", val);
> +	} else {
> +		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
>  		stat = i2c_dw_read_clear_intrbits(dev);
>  	}
>  
> -	if (stat & DW_IC_INTR_TX_OVER) {		
> +	if (stat & DW_IC_INTR_TX_OVER) {
>  		dw_readl(dev, DW_IC_CLR_TX_OVER);
>  		return true;
>  	}
> @@ -1008,12 +1007,12 @@ static bool i2c_dw_irq_handler_slave(struct
> dw_i2c_dev *dev )
>  	return true;
>  }
>  
> -static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev )
> +static bool i2c_dw_irq_handler_master(struct dw_i2c_dev *dev)
>  {
>  	u32 stat;
> -	
> +
>  	stat = i2c_dw_read_clear_intrbits(dev);
> -	
> +
>  	if (stat & DW_IC_INTR_TX_ABRT) {
>  		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
>  		dev->status = STATUS_IDLE;
> @@ -1068,17 +1067,17 @@ static irqreturn_t i2c_dw_isr(int this_irq,
> void *dev_id)
>  	enabled = dw_readl(dev, DW_IC_ENABLE);
>  	mode = dw_readl(dev, DW_IC_CON);
>  	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;
> -	
> +
>  	if (!(mode & DW_IC_CON_MASTER) && !(mode &
> DW_IC_CON_SLAVE_DISABLE)) {
>  		stat = i2c_dw_read_clear_intrbits(dev);
>  		if (!i2c_dw_irq_handler_slave(dev))
>  			return IRQ_NONE;
>  	} else {
> -	if(i2c_dw_irq_handler_master(dev))
> +	if (i2c_dw_irq_handler_master(dev))
>  		return IRQ_HANDLED;
>  	}
>  
> @@ -1148,7 +1147,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
>  	adap->dev.parent = dev->dev;
>  	i2c_set_adapdata(adap, dev);
>  
> -	if (!i2c_check_functionality(adap,I2C_FUNC_SLAVE)) 
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SLAVE))
>  		i2c_dw_disable_int(dev);
>  
>  	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index f29e657..035b93f 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -141,13 +141,13 @@ static inline int dw_i2c_acpi_configure(struct
> platform_device *pdev)
>  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->functionality |= I2C_FUNC_10BIT_ADDR;
>  	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;
> @@ -158,25 +158,25 @@ static void i2c_dw_configure_master(struct
> platform_device *pdev)
>  	default:
>  		dev->master_cfg |= DW_IC_CON_SPEED_FAST;
>  	}
> -	
> +
>  }
>  
>  static void i2c_dw_configure_slave(struct platform_device *pdev)
>  {
>  	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> -	
> -	dev->slave_cfg = DW_IC_CON_RX_FIFO_FULL_HLD_CTRL | 
> -			  DW_IC_CON_RESTART_EN |
> DW_IC_CON_STOP_DET_IFADDRESSED | 
> +
> +	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->functionality |= I2C_FUNC_SLAVE;
>  	dev->functionality &= ~I2C_FUNC_10BIT_ADDR;
>  	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;
> -		
> +
>  	case 3400000:
>  		dev->slave_cfg |= DW_IC_CON_SPEED_HIGH;
>  		break;
> @@ -272,10 +272,10 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
>  		I2C_FUNC_SMBUS_BYTE_DATA |
>  		I2C_FUNC_SMBUS_WORD_DATA |
>  		I2C_FUNC_SMBUS_I2C_BLOCK;
> -	
> +
>  	if (is_slave)
>  		i2c_dw_configure_slave(pdev);
> -	else 
> +	else
>  		i2c_dw_configure_master(pdev);
>  
>  	dev->clk = devm_clk_get(&pdev->dev, NULL);

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

Re: [PATCH v2 3/4] Device bindings documentation updated ACPI-enabled platforms not currently supported

[Luis Oliveira]

Hi,


As you suggested I will split the drivers. I am thinking of doing 5 patches:

- factor out master() parts

- separate Master part to i2c-designware-master.c (changes in i2c-designware-core.c)

- enable Slave part to i2c-designware-slave (changes in i2c-designware-core.c)

- glue drivers and device bindings

- cleaning


Regards,

Luis

On 21-Oct-16 11:54, Andy Shevchenko wrote:
> On Fri, 2016-10-21 at 10:56 +0100, Luis Oliveira wrote:
>> Since practically 90% of the code is shared between master and slave,
>> I was
>> thinking if it will be acceptable to use the same driver for both but
>> differentiate the master/slave mode by the compatible strings.
> It might be possible to split like other drivers do:
>
> 1. Core part (i2c-designware-core.c)
> 2. Master part (i2c-designware-master.c)
> 3. Slave part (i2c-designware-slave.c)
> 4. Glue drivers (like: i2c-designware-platdrv.c)
>
>> Thanks,
>> Luis
>>
>> On 10/18/2016 16:17, Wolfram Sang wrote:
>>>> This is needed because the configuration is different and the i2c-
>>>> designware
>>>> cannot be master/slave without a reset. To resolve that I added
>>>> this property
>>>> to bind it as a slave when needed.
>>> Aww, pity that the HW can't do that. Do you have details why?
>>>
>>> If that is really a HW limitation, then I'd suggest having a
>>> seperate
>>> driver for slave-only mode so we can differentiate by compatible
>>> strings.
>>>
>>