i2c: designware: add I2C SLAVE support

- Factor out all _master() part of code from i2c-designware-core and i2c-designware-platdrv to separate functions.
- Standardize all code related with MASTER mode.
- I have to take off DW_IC_INTR_TX_EMPTY from DW_IC_INTR_DEFAULT_MASK because it is master specific.

The purpose of this is to prepare the controller to have is I2C MASTER flow in a separate driver. To do this first all the functions/definitions related to the MASTER flow were identified.

Signed-off-by Luis Oliveira <[email protected]>

---
Changes V4->V5: (ACK by Andy)

+       dev_dbg(dev->dev,
+               "%s: enabled=%#x stat=%#x\n", __func__, enabled,

(quoted diff) +2 -0

+
+	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE

|

drivers/i2c/busses/i2c-designware-platdrv.c +16 -0

+			  DW_IC_CON_RESTART_EN;
+
+	dev_dbg(&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;
+	}
+}
+

Quoted 9 lines · Luis Oliveira

- Factor out all _master() part of code from i2c-designware-core and i2c-designware-platdrv to separate functions.
- Standardize all code related with MASTER mode.
- I have to take off DW_IC_INTR_TX_EMPTY from DW_IC_INTR_DEFAULT_MASK because it is master specific.

The purpose of this is to prepare the controller to have is I2C MASTER flow in a separate driver. To do this first all the functions/definitions related to the MASTER flow were identified.

Thanks for an update.
Some style related comments below (For the code related is up to you, my tag still stands).

Quoted 1 line

Quoted 3 lines

Signed-off-by Luis Oliveira <[email protected]>

---
Changes V4->V5: (ACK by Andy)

When you get an Ack, or other tag (Reviewed-by, Tested-by, etc), and you send new version, include this tag to your commit message (it applies to all affected patches in your series).

It would be also good to have some high level changelog in the cover letter, from this series I don't see, for example, which base you did use (i2c-next? linux-next? v4.9? v4.10-rc1?).

Review

Quoted 2 lines

(quoted diff) +2 -0

+       dev_dbg(dev->dev,
+               "%s: enabled=%#x stat=%#x\n", __func__, enabled,

stat);

I hope you can fit format string on the first line. __func__ is redundant when you are using debug printing (Dynamic Debug would include it if asked for).

drivers/i2c/busses/i2c-designware-platdrv.c static inline int dw_i2c_acpi_configure(struct platform_device *pdev) · line 144

Quoted 4 lines

drivers/i2c/busses/i2c-designware-platdrv.c +3 -0

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

By the way, does it make sense to pass struct dw_i2c_dev * as a parameter of the function?

Quoted 20 lines

(quoted diff) +2 -0

+
+	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE

|

drivers/i2c/busses/i2c-designware-platdrv.c +16 -0

+			  DW_IC_CON_RESTART_EN;
+
+	dev_dbg(&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;
+	}
+}
+

- Definitions were added

SLAVE related definitions were added to the core of the controller.

Signed-off-by Luis Oliveira <[email protected]>

---
Changes V4->V5: (Andy Shevchenko)
- This patch just introduces SLAVE definitions (as suggested in V4)

 drivers/i2c/busses/i2c-designware-core.h | 27
+++++++++++++++++++++++++++

 1 file changed, 27 insertions(+)

drivers/i2c/busses/i2c-designware-core.h +4 -0

index 8bba7a37c3ce..5080f1d2d2ec 100644
@@ -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

drivers/i2c/busses/i2c-designware-core.h +2 -0

@@ -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_MAS
K | \
   DW_IC_INTR_TX_EMPTY)
+#define DW_IC_INTR_SLAVE_MASK (DW_IC_INTR_DEFAULT_MASK
| \

drivers/i2c/busses/i2c-designware-core.h +4 -0

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

drivers/i2c/busses/i2c-designware-core.h +0 -0

@@ -123,6 +136,12 @@
  #define TIMEOUT			20 /* ms */

 /*
+ * operation modes
+ */

drivers/i2c/busses/i2c-designware-core.h +4 -0

+#define DW_IC_MASTER		0
+#define DW_IC_SLAVE		1
+
+/*

  * hardware abort codes from the DW_IC_TX_ABRT_SOURCE register *
  * only expected abort codes are listed here

drivers/i2c/busses/i2c-designware-core.h +3 -0

@@ -139,6 +158,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)

drivers/i2c/busses/i2c-designware-core.h +3 -0

@@ -151,6 +173,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_NOA
CK | \
   DW_IC_TX_ABRT_10ADDR1_NOACK | \

drivers/i2c/busses/i2c-designware-core.h +1 -0

@@ -206,6 +231,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;

drivers/i2c/busses/i2c-designware-core.h +1 -0

@@ -225,6 +251,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;

- Changes in Kconfig to enable I2C_SLAVE support
- Slave functions added to core library file
- Slave abort sources added to common source file
- New driver: i2c-designware-slave added
- Changes in the Makefile to compile it all

All the SLAVE flow is added but it is not enabled via platform driver.

drivers/i2c/busses/i2c-designware-common.c +1 -0

@@ -30,6 +30,7 @@
  #include <linux/pm_runtime.h>
  #include <linux/delay.h>
  #include <linux/module.h>
+
  #include "i2c-designware-core.h"

 static char *abort_sources[] = {

drivers/i2c/busses/i2c-designware-common.c +0 -0

@@ -42,7 +43,7 @@ static char *abort_sources[] = {

  [ABRT_TXDATA_NOACK] =
  "data not acknowledged",
  [ABRT_GCALL_NOACK] =

drivers/i2c/busses/i2c-designware-common.c +21 -0

+int i2c_dw_init_slave(struct dw_i2c_dev *dev)
+{
+	u32 sda_falling_time, scl_falling_time;
+	u32 reg, comp_param1;
+	u32 hcnt, lcnt;
+	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);

drivers/i2c/busses/i2c-designware-common.c +10 -0

+		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.

*/

drivers/i2c/busses/i2c-designware-common.c +10 -0

+	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 */

drivers/i2c/busses/i2c-designware-common.c +2 -0

+					sda_falling_time,
+					0,	/* 0: DW default,

1: Ideal */

drivers/i2c/busses/i2c-designware-common.c +3 -0

+					0);	/* No offset */
+		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+					4700,	/* tLOW = 4.7 us

*/

drivers/i2c/busses/i2c-designware-common.c +6 -0

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

drivers/i2c/busses/i2c-designware-common.c +2 -0

+
+	/* Set SCL timing parameters for fast-mode or fast-mode plus.

*/

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev-

Quoted 1 line

fp_lcnt) {

drivers/i2c/busses/i2c-designware-common.c +8 -0

+		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 */

drivers/i2c/busses/i2c-designware-common.c +2 -0

+					sda_falling_time,
+					0,	/* 0: DW default,

1: Ideal */

drivers/i2c/busses/i2c-designware-common.c +3 -0

+					0);	/* No offset */
+		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+					1300,	/* tLOW = 1.3 us

*/

drivers/i2c/busses/i2c-designware-common.c +6 -0

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

drivers/i2c/busses/i2c-designware-common.c +4 -0

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

drivers/i2c/busses/i2c-designware-common.c +2 -0

+			!= DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) {
+			dev_err(dev->dev, "High Speed not

supported!\n");

drivers/i2c/busses/i2c-designware-common.c +8 -0

+			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",

drivers/i2c/busses/i2c-designware-common.c +9 -0

+				hcnt, lcnt);
+		}
+	}
+
+	/* Configure SDA Hold Time if required. */
+	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. */

drivers/i2c/busses/i2c-designware-common.c +1 -0

+			dev->sda_hold_time = dw_readl(dev,

DW_IC_SDA_HOLD);

drivers/i2c/busses/i2c-designware-common.c +3 -0

+		}
+		/*
+		 * Workaround for avoiding TX arbitration lost in

case I2C

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		 * slave pulls SDA down "too quickly" after falling

egde of

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		 * SCL by enabling non-zero SDA RX hold.

Specification says it

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		 * extends incoming SDA low to high transition while

SCL is

drivers/i2c/busses/i2c-designware-common.c +4 -0

+		 * 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;

drivers/i2c/busses/i2c-designware-common.c +4 -0

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

drivers/i2c/busses/i2c-designware-common.c +4 -0

+
+int i2c_dw_reg_slave(struct i2c_client *slave)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(slave->adapter);

drivers/i2c/busses/i2c-designware-common.c +16 -0

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

drivers/i2c/busses/i2c-designware-common.c +19 -0

+
+	return 0;
+}
 
 
+static int 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 0;
+
+	dev_dbg(dev->dev,
+	 "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :

INTR_STAT=%#x\n",

drivers/i2c/busses/i2c-designware-common.c +10 -0

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

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		i2c_slave_event(dev->slave,

I2C_SLAVE_WRITE_REQUESTED, &val);

drivers/i2c/busses/i2c-designware-common.c +8 -0

+
+	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!",

drivers/i2c/busses/i2c-designware-common.c +3 -0

+				}
+				dw_readl(dev, DW_IC_CLR_RD_REQ);
+				stat =

i2c_dw_read_clear_intrbits_slave(dev);

drivers/i2c/busses/i2c-designware-common.c +4 -0

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

drivers/i2c/busses/i2c-designware-common.c +3 -0

+			}
+			if (!i2c_slave_event(dev->slave,
+					 I2C_SLAVE_READ_REQUESTED,

&val))

drivers/i2c/busses/i2c-designware-common.c +6 -0

+				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,

drivers/i2c/busses/i2c-designware-common.c +11 -0

+	} 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 1;
+}
+

drivers/i2c/busses/i2c-designware-common.c +12 -0

+	if (ret > 0)
+		complete(&dev->cmd_complete);
+
+	return IRQ_RETVAL(ret);
+}
 
 
+
+int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
+{
+	struct i2c_adapter *adap = &dev->adapter;
+	int ret;
+
+	init_completion(&dev->cmd_complete);

drivers/i2c/busses/i2c-designware-common.c +18 -0

+
+	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);
+
+	ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
+			     IRQF_SHARED, dev_name(dev->dev), dev);
+	if (ret) {
+		dev_err(dev->dev, "failure requesting irq %i: %d\n",
+			dev->irq, ret);
+		return ret;
+	}
 
+	/*
+	 * Increment PM usage count during adapter registration in

order to

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	 * avoid possible spurious runtime suspend when adapter

device is

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	 * registered to the device core and immediate resume in case

bus has

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	 * registered I2C slaves that do I2C transfers in their

probe.

drivers/i2c/busses/i2c-designware-common.c +3 -0

+	ret = i2c_add_numbered_adapter(adap);
+	if (ret)
+		dev_err(dev->dev, "failure adding adapter: %d\n",

ret);

drivers/i2c/busses/i2c-designware-common.c +5 -0

+	pm_runtime_put_noidle(dev->dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_probe_slave);

Quoted 18 lines · Luis Oliveira

- Changes in Kconfig to enable I2C_SLAVE support
- Slave functions added to core library file
- Slave abort sources added to common source file
- New driver: i2c-designware-slave added
- Changes in the Makefile to compile it all

All the SLAVE flow is added but it is not enabled via platform driver.

drivers/i2c/busses/i2c-designware-common.c +1 -0

@@ -30,6 +30,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+
 #include "i2c-designware-core.h"
 

drivers/i2c/busses/i2c-designware-common.c static char *abort_sources[] = { · line 46

Quoted 7 lines

drivers/i2c/busses/i2c-designware-common.c +1 -1

 static char *abort_sources[] = {

@@ -42,7 +43,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",

So, what's the point after your confirmation that both variants are okay?

Quoted 7 lines

drivers/i2c/busses/i2c-designware-common.c +6 -0

+#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>

drivers/i2c/busses/i2c-designware-common.c

Quoted 2 lines

drivers/i2c/busses/i2c-designware-common.c +2 -0

+#include <linux/delay.h>
+#include <linux/module.h>

Alphabetical order?

Quoted 19 lines

drivers/i2c/busses/i2c-designware-common.c +19 -0

+int i2c_dw_init_slave(struct dw_i2c_dev *dev)
+{
+	u32 sda_falling_time, scl_falling_time;
+	u32 reg, comp_param1;
+	u32 hcnt, lcnt;
+	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) {

Quoted 3 lines

drivers/i2c/busses/i2c-designware-common.c +2 -0

+		dev_err(dev->dev,
+			 "Unknown Synopsys component type: 0x%08x\n",

reg);

Is it correct indentation?

Quoted 116 lines

drivers/i2c/busses/i2c-designware-common.c +10 -0

+		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.

*/

drivers/i2c/busses/i2c-designware-common.c +10 -0

+	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 */

drivers/i2c/busses/i2c-designware-common.c +2 -0

+					sda_falling_time,
+					0,	/* 0: DW default,

1: Ideal */

drivers/i2c/busses/i2c-designware-common.c +3 -0

+					0);	/* No offset */
+		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+					4700,	/* tLOW = 4.7 us

*/

drivers/i2c/busses/i2c-designware-common.c +6 -0

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

drivers/i2c/busses/i2c-designware-common.c +2 -0

+
+	/* Set SCL timing parameters for fast-mode or fast-mode plus.

*/

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev-

Quoted 1 line

fp_lcnt) {

drivers/i2c/busses/i2c-designware-common.c +8 -0

+		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 */

drivers/i2c/busses/i2c-designware-common.c +2 -0

+					sda_falling_time,
+					0,	/* 0: DW default,

1: Ideal */

drivers/i2c/busses/i2c-designware-common.c +3 -0

+					0);	/* No offset */
+		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+					1300,	/* tLOW = 1.3 us

*/

drivers/i2c/busses/i2c-designware-common.c +6 -0

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

drivers/i2c/busses/i2c-designware-common.c +4 -0

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

drivers/i2c/busses/i2c-designware-common.c +2 -0

+			!= DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) {
+			dev_err(dev->dev, "High Speed not

supported!\n");

drivers/i2c/busses/i2c-designware-common.c +8 -0

+			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",

drivers/i2c/busses/i2c-designware-common.c +9 -0

+				hcnt, lcnt);
+		}
+	}
+
+	/* Configure SDA Hold Time if required. */
+	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. */

drivers/i2c/busses/i2c-designware-common.c +1 -0

+			dev->sda_hold_time = dw_readl(dev,

DW_IC_SDA_HOLD);

drivers/i2c/busses/i2c-designware-common.c +3 -0

+		}
+		/*
+		 * Workaround for avoiding TX arbitration lost in

case I2C

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		 * slave pulls SDA down "too quickly" after falling

egde of

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		 * SCL by enabling non-zero SDA RX hold.

Specification says it

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		 * extends incoming SDA low to high transition while

SCL is

drivers/i2c/busses/i2c-designware-common.c +4 -0

+		 * 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;

drivers/i2c/busses/i2c-designware-common.c +4 -0

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

drivers/i2c/busses/i2c-designware-common.c +2 -0

+	}
+

drivers/i2c/busses/i2c-designware-common.c

Quoted 6 lines

drivers/i2c/busses/i2c-designware-common.c +6 -0

+	i2c_dw_configure_fifo_slave(dev);
+	i2c_dw_release_lock(dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_init_slave);

Can we introduce ops structure for those? (private callbacks)

You increase noise in namespace by several i2c_dw_*() functions. Introduction of ops will keep everything private.

Quoted 9 lines

drivers/i2c/busses/i2c-designware-common.c +9 -0

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

drivers/i2c/busses/i2c-designware-common.c

Quoted 4 lines

drivers/i2c/busses/i2c-designware-common.c +4 -0

+		/*
+		 * Set slave address in the IC_SAR register,
+		 * the address to which the DW_apb_i2c responds.
+		 */

Wrong indentation?

Quoted 16 lines

drivers/i2c/busses/i2c-designware-common.c +16 -0

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

drivers/i2c/busses/i2c-designware-common.c

Quoted 10 lines

drivers/i2c/busses/i2c-designware-common.c +9 -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;

Extra spaces, remove.

Quoted 22 lines

drivers/i2c/busses/i2c-designware-common.c +19 -0

+
+	return 0;
+}
 
 
+static int 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 0;
+
+	dev_dbg(dev->dev,
+	 "%s: %#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :

INTR_STAT=%#x\n",

drivers/i2c/busses/i2c-designware-common.c

Quoted 1 line

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	 __func__, enabled, slave_activity, raw_stat, stat);

__func__ is redundant.

Quoted 22 lines

drivers/i2c/busses/i2c-designware-common.c +10 -0

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

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		i2c_slave_event(dev->slave,

I2C_SLAVE_WRITE_REQUESTED, &val);

drivers/i2c/busses/i2c-designware-common.c +8 -0

+
+	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!",

drivers/i2c/busses/i2c-designware-common.c

Quoted 1 line

drivers/i2c/busses/i2c-designware-common.c +1 -0

+					 val);

Perhaps dev_vdbg() ?

Quoted 25 lines

drivers/i2c/busses/i2c-designware-common.c +3 -0

+				}
+				dw_readl(dev, DW_IC_CLR_RD_REQ);
+				stat =

i2c_dw_read_clear_intrbits_slave(dev);

drivers/i2c/busses/i2c-designware-common.c +4 -0

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

drivers/i2c/busses/i2c-designware-common.c +3 -0

+			}
+			if (!i2c_slave_event(dev->slave,
+					 I2C_SLAVE_READ_REQUESTED,

&val))

drivers/i2c/busses/i2c-designware-common.c +6 -0

+				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,

drivers/i2c/busses/i2c-designware-common.c +5 -0

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

drivers/i2c/busses/i2c-designware-common.c

Quoted 1 line

drivers/i2c/busses/i2c-designware-common.c +1 -0

+		return true;

Mistype of value. Should be 1?

drivers/i2c/busses/i2c-designware-common.c

Quoted 9 lines

drivers/i2c/busses/i2c-designware-common.c +5 -0

+	}
+
+	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,

drivers/i2c/busses/i2c-designware-common.c +2 -0

+		 &val))
+			dev_dbg(dev->dev, "Byte %X acked!", val);

dev_vdbg() ?

Quoted 11 lines

drivers/i2c/busses/i2c-designware-common.c +11 -0

+	} 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 1;
+}
+

drivers/i2c/busses/i2c-designware-common.c

Quoted 10 lines

drivers/i2c/busses/i2c-designware-common.c +8 -0

+static irqreturn_t i2c_dw_isr_slave(int this_irq, void *dev_id)
+{
+	struct dw_i2c_dev *dev = dev_id;
+	int ret;
+
+	i2c_dw_read_clear_intrbits_slave(dev);
 
 
+	ret = i2c_dw_irq_handler_slave(dev);
+

Swap these lines.

Quoted 20 lines

drivers/i2c/busses/i2c-designware-common.c +17 -0

+	if (ret > 0)
+		complete(&dev->cmd_complete);
+
+	return IRQ_RETVAL(ret);
+}
 
 
+
+int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
+{
+	struct i2c_adapter *adap = &dev->adapter;
+	int ret;
+
+	init_completion(&dev->cmd_complete);
+
+	ret = i2c_dw_init_slave(dev);
+	if (ret)
+		return ret;
+

drivers/i2c/busses/i2c-designware-common.c

Quoted 3 lines

drivers/i2c/busses/i2c-designware-common.c +3 -0

+	ret = i2c_dw_acquire_lock(dev);
+	if (ret)
+		return ret;

I'm not sure you need this in slave code.

Quoted 17 lines

drivers/i2c/busses/i2c-designware-common.c +16 -0

+
+	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);
+
+	ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
+			     IRQF_SHARED, dev_name(dev->dev), dev);
+	if (ret) {
+		dev_err(dev->dev, "failure requesting irq %i: %d\n",
+			dev->irq, ret);
+		return ret;
+	}

drivers/i2c/busses/i2c-designware-common.c

Quoted 11 lines

drivers/i2c/busses/i2c-designware-common.c +2 -0

+	/*
+	 * Increment PM usage count during adapter registration in

order to

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	 * avoid possible spurious runtime suspend when adapter

device is

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	 * registered to the device core and immediate resume in case

bus has

drivers/i2c/busses/i2c-designware-common.c +1 -0

+	 * registered I2C slaves that do I2C transfers in their

probe.

drivers/i2c/busses/i2c-designware-common.c +2 -0

+	 */
+	pm_runtime_get_noresume(dev->dev);

Looks like you blindly copied this from master code. This is about slave enumeration. How does it related to slave mode?

drivers/i2c/busses/i2c-designware-common.c

Quoted 10 lines

drivers/i2c/busses/i2c-designware-common.c +3 -0

+	ret = i2c_add_numbered_adapter(adap);
+	if (ret)
+		dev_err(dev->dev, "failure adding adapter: %d\n",

ret);

drivers/i2c/busses/i2c-designware-common.c +5 -0

+	pm_runtime_put_noidle(dev->dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_dw_probe_slave);

- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
  - Changed the ifndef style to the use of ACPI_HANDLE that returns NULL
    if the device was not enumerated from ACPI namespace.

drivers/i2c/busses/i2c-designware-platdrv.c +7 -0

+			if (reg & I2C_OWN_SLAVE_ADDRESS)
+				i2c_dw_configure_slave(pdev);
+			else
+				i2c_dw_configure_master(pdev);
+		}
+	} else
+		i2c_dw_configure_master(pdev);

Quoted 5 lines · Luis Oliveira

- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
- Changed the ifndef style to the use of ACPI_HANDLE that returns
NULL
if the device was not enumerated from ACPI namespace.

I'm not sure what is wrong with ACPI?

Quoted 6 lines

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

@@ -264,9 +297,16 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
 	if (r)
 		return r;
 

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev)

Quoted 6 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -3

-	dev->functionality = I2C_FUNC_10BIT_ADDR |
 DW_IC_DEFAULT_FUNCTIONALITY;
-
-	i2c_dw_configure_master(pdev);
 
+	if (ACPI_HANDLE(&pdev->dev) == NULL) {

I don't think you need this at all.

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 301

Quoted 2 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -0

+		device_for_each_child_node(&pdev->dev, child) {

This is resource agnostic.

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 302

Quoted 1 line

drivers/i2c/busses/i2c-designware-platdrv.c +1 -0

+			fwnode_property_read_u32(child, "reg", &reg);

This is as well.

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 309

Quoted 8 lines

drivers/i2c/busses/i2c-designware-platdrv.c +7 -0

+			if (reg & I2C_OWN_SLAVE_ADDRESS)
+				i2c_dw_configure_slave(pdev);
+			else
+				i2c_dw_configure_master(pdev);
+		}
+	} else
+		i2c_dw_configure_master(pdev);

Quoted 8 lines · Andy Shevchenko

Quoted 6 lines · Luis Oliveira

- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
  - Changed the ifndef style to the use of ACPI_HANDLE that returns
NULL
    if the device was not enumerated from ACPI namespace.

Quoted 1 line

I'm not sure what is wrong with ACPI?

I dont have a way to test it. Just that.

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 300

Quoted 5 lines

(quoted diff) +0 -1

-	dev->functionality = I2C_FUNC_10BIT_ADDR |

DW_IC_DEFAULT_FUNCTIONALITY;

drivers/i2c/busses/i2c-designware-platdrv.c +1 -2

-
-	i2c_dw_configure_master(pdev);
+	if (ACPI_HANDLE(&pdev->dev) == NULL) {

I don't think you need this at all.

This is to avoid the use of the "ifdef" style I used before.

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 301

Quoted 2 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -0

+		device_for_each_child_node(&pdev->dev, child) {

This is resource agnostic.

Quoted 2 lines

(quoted diff) +1 -0

+			fwnode_property_read_u32(child, "reg",

&reg);

This is as well.

Are you suggesting I use of_ functions?

Quoted 9 lines · Luis Oliveira

Quoted 8 lines · Andy Shevchenko

Quoted 6 lines · Luis Oliveira

- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
- Changed the ifndef style to the use of ACPI_HANDLE that
returns
NULL
if the device was not enumerated from ACPI namespace.

Quoted 3 lines

Quoted 1 line

I'm not sure what is wrong with ACPI?

I dont have a way to test it. Just that.

Okay, can you provide an excerpt to see how it will look like in DTS?

Quoted 11 lines

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 300

Quoted 7 lines

(quoted diff) +0 -1

-	dev->functionality = I2C_FUNC_10BIT_ADDR |

DW_IC_DEFAULT_FUNCTIONALITY;

drivers/i2c/busses/i2c-designware-platdrv.c +1 -2

-
-	i2c_dw_configure_master(pdev);
+	if (ACPI_HANDLE(&pdev->dev) == NULL) {

I don't think you need this at all.

This is to avoid the use of the "ifdef" style I used before.

My point is to drop it completely.

Quoted 11 lines

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 301

Quoted 2 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -0

+		device_for_each_child_node(&pdev->dev, child) {

This is resource agnostic.

Quoted 2 lines

(quoted diff) +1 -0

+			fwnode_property_read_u32(child, "reg",

&reg);

This is as well.

Are you suggesting I use of_ functions?

Nope. See above.

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Luis Oliveira

Quoted 8 lines · Andy Shevchenko

Quoted 6 lines · Luis Oliveira

- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
- Changed the ifndef style to the use of ACPI_HANDLE that
returns
NULL
if the device was not enumerated from ACPI namespace.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

I'm not sure what is wrong with ACPI?

I dont have a way to test it. Just that.

Okay, can you provide an excerpt to see how it will look like in DTS?

Yes, it looks like this now:

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

			eeprom@64 {
				compatible = "linux,slave-24c02";
				reg = <0x40000064>;
			};
	};

Quoted 28 lines

Quoted 10 lines

Quoted 8 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 300

Quoted 6 lines

(quoted diff) +0 -1

-	dev->functionality = I2C_FUNC_10BIT_ADDR |

DW_IC_DEFAULT_FUNCTIONALITY;

drivers/i2c/busses/i2c-designware-platdrv.c +1 -2

-
-	i2c_dw_configure_master(pdev);
+	if (ACPI_HANDLE(&pdev->dev) == NULL) {

I don't think you need this at all.

This is to avoid the use of the "ifdef" style I used before.

My point is to drop it completely.

Quoted 11 lines

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 301

Quoted 2 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -0

+		device_for_each_child_node(&pdev->dev, child) {

This is resource agnostic.

Quoted 2 lines

(quoted diff) +1 -0

+			fwnode_property_read_u32(child, "reg",

&reg);

This is as well.

Are you suggesting I use of_ functions?

Nope. See above.

Quoted 11 lines · Andy Shevchenko

Quoted 10 lines · Luis Oliveira

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Luis Oliveira

- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
  - Changed the ifndef style to the use of ACPI_HANDLE that returns
NULL
    if the device was not enumerated from ACPI namespace.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

I'm not sure what is wrong with ACPI?

I dont have a way to test it. Just that.

Okay, can you provide an excerpt to see how it will look like in DTS?

Yes, it looks like this now:

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

			eeprom@64 {
				compatible = "linux,slave-24c02";
				reg = <0x40000064>;
			};
	};

Quoted 25 lines

Quoted 9 lines

Quoted 7 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 300

Quoted 5 lines

(quoted diff) +0 -1

-	dev->functionality = I2C_FUNC_10BIT_ADDR |

DW_IC_DEFAULT_FUNCTIONALITY;

drivers/i2c/busses/i2c-designware-platdrv.c +1 -2

-
-	i2c_dw_configure_master(pdev);
+	if (ACPI_HANDLE(&pdev->dev) == NULL) {

I don't think you need this at all.

This is to avoid the use of the "ifdef" style I used before.

My point is to drop it completely.

Quoted 12 lines

Quoted 11 lines

Quoted 3 lines

(quoted diff) +1 -0

+		device_for_each_child_node(&pdev->dev, child)

{

This is resource agnostic.

Quoted 3 lines

(quoted diff) +1 -0

+			fwnode_property_read_u32(child,

"reg",
&reg);

This is as well.

Quoted 3 lines

Quoted 1 line

Are you suggesting I use of_ functions?

Nope. See above.

Quoted 30 lines · Luis Oliveira

Quoted 11 lines · Andy Shevchenko

Quoted 10 lines · Luis Oliveira

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Luis Oliveira

- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
- Changed the ifndef style to the use of ACPI_HANDLE that
returns
NULL
if the device was not enumerated from ACPI namespace.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

I'm not sure what is wrong with ACPI?

I dont have a way to test it. Just that.

Okay, can you provide an excerpt to see how it will look like in DTS?

Yes, it looks like this now:

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

Quoted 5 lines

			eeprom@64 {
				compatible = "linux,slave-24c02";
				reg = <0x40000064>;
			};
	};

+1 to Carlos' comment.

Quoted 26 lines

Quoted 26 lines

Quoted 11 lines

Quoted 9 lines

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_probe(struct platform_device *pdev) · line 300

Quoted 7 lines

(quoted diff) +0 -1

-	dev->functionality = I2C_FUNC_10BIT_ADDR |

DW_IC_DEFAULT_FUNCTIONALITY;

drivers/i2c/busses/i2c-designware-platdrv.c +1 -2

-
-	i2c_dw_configure_master(pdev);
+	if (ACPI_HANDLE(&pdev->dev) == NULL) {

I don't think you need this at all.

This is to avoid the use of the "ifdef" style I used before.

My point is to drop it completely.

Quoted 12 lines

Quoted 11 lines

Quoted 3 lines

(quoted diff) +1 -0

+		device_for_each_child_node(&pdev->dev, child)

{

This is resource agnostic.

Quoted 3 lines

(quoted diff) +1 -0

+			fwnode_property_read_u32(child,

"reg",
&reg);

This is as well.

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Are you suggesting I use of_ functions?

Nope. See above.

So, ACPI has a property to support slave mode for I2CSerialBus() macro.

I would propose to create a helper function in i2c-core.c which will be responsible for mode detection

... i2c_slave_mode_detect()
{
...
if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
... (use of_*() here) ...
} else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev))
dev_dbg(..., "ACPI slave is not supported yet\n"); ... to master ...
} else {
... default to master ...
}
}
EXPORT_...();

Make it as a separate patch.

Quoted 31 lines · Andy Shevchenko

Quoted 30 lines · Luis Oliveira

Quoted 11 lines · Andy Shevchenko

Quoted 10 lines · Luis Oliveira

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Luis Oliveira

- Slave mode selected in platform module (devicetree support only)
- Check for ACPI - not supported in SLAVE mode:
- Changed the ifndef style to the use of ACPI_HANDLE that
returns
NULL
if the device was not enumerated from ACPI namespace.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

I'm not sure what is wrong with ACPI?

I dont have a way to test it. Just that.

Okay, can you provide an excerpt to see how it will look like in DTS?

Yes, it looks like this now:

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

Quoted 7 lines

Quoted 5 lines

			eeprom@64 {
				compatible = "linux,slave-24c02";
				reg = <0x40000064>;
			};
	};

+1 to Carlos' comment.

Agree, I'm on it.

Quoted 49 lines

Quoted 26 lines

Quoted 26 lines

Quoted 11 lines

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +0 -1

-	dev->functionality = I2C_FUNC_10BIT_ADDR |

DW_IC_DEFAULT_FUNCTIONALITY;

(quoted diff) +1 -2

-
-	i2c_dw_configure_master(pdev);
+	if (ACPI_HANDLE(&pdev->dev) == NULL) {

I don't think you need this at all.

This is to avoid the use of the "ifdef" style I used before.

My point is to drop it completely.

Quoted 12 lines

Quoted 11 lines

Quoted 3 lines

(quoted diff) +1 -0

+		device_for_each_child_node(&pdev->dev, child)

{

This is resource agnostic.

Quoted 3 lines

(quoted diff) +1 -0

+			fwnode_property_read_u32(child,

"reg",
&reg);

This is as well.

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Are you suggesting I use of_ functions?

Nope. See above.

So, ACPI has a property to support slave mode for I2CSerialBus() macro.

I would propose to create a helper function in i2c-core.c which will be responsible for mode detection

... i2c_slave_mode_detect()
{
...
if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
... (use of_*() here) ...
} else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev))
dev_dbg(..., "ACPI slave is not supported yet\n"); ... to master ...
} else {
... default to master ...
}
}
EXPORT_...();

Quoted 2 lines

Make it as a separate patch.

Oh I see, yes it looks good. I will check it. Thanks

Quoted 26 lines · Luis Oliveira

Quoted 25 lines · Andy Shevchenko

Quoted 1 line · Luis Oliveira

On 28-Dec-16 16:31, Andy Shevchenko wrote:

So, ACPI has a property to support slave mode for I2CSerialBus() macro.

I would propose to create a helper function in i2c-core.c which will be
responsible for mode detection

... i2c_slave_mode_detect()
{
...
 if (IS_BUILTIN(CONFIG_OF) && dev->of_node) { ... (use of_*() here) ...
 } else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev)) dev_dbg(..., "ACPI slave is not supported yet\n"); ... to master ...
 } else {
  ... default to master ...
 }
}
EXPORT_...();

Quoted 5 lines

Quoted 2 lines

Make it as a separate patch.

Oh I see, yes it looks good. I will check it. Thanks

Hi Andy,

I implemented a helper function as you proposed and it looks like this:

int i2c_slave_mode_detect(struct device *dev)

{
	struct device_node *child;
	u32 reg;
	
	if (IS_BUILTIN(CONFIG_OF) && dev->of_node) {
		for_each_child_of_node(dev->of_node, child) {
			of_property_read_u32(child, "reg", &reg);
			if (reg & I2C_OWN_SLAVE_ADDRESS)
				return 1;
		}
	} else if (IS_BUILTIN(CONFIG_ACPI) && ACPI_HANDLE(dev))
		dev_dbg(dev, "ACPI slave is not supported yet\n");

	else
		return 0;

	return 0;
}

Before I submit the patch to the i2c-core.c I wonder if I could have some
comment on the implementation.

Replaced all the return variables 'r' in the existing code by 'ret' to make the code easier to read (and more standard).

Signed-off-by Luis Oliveira <lolivei-HKixBCOQz3hWk0Htik3J/[email protected]>

---
Changes V4->V5: (Andy Shevchenko)
- Replaced all the old code using "r" as return to "ret". For consistency
purposes.

 drivers/i2c/busses/i2c-designware-master.c  | 30 ++++++++++++++----
-----------
 drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++--------

 2 files changed, 23 insertions(+), 23 deletions(-)

drivers/i2c/busses/i2c-designware-master.c +1 -1

index b55a7f4c5149..0d5aca6edb48 100644
@@ -644,18 +644,18 @@ EXPORT_SYMBOL_GPL(i2c_dw_read_comp_param);
  int i2c_dw_probe(struct dw_i2c_dev *dev)
  {
  	struct i2c_adapter *adap = &dev->adapter;
-	int r;
+	int ret;
  	u32 reg;

  init_completion(&dev->cmd_complete);

drivers/i2c/busses/i2c-designware-master.c +3 -3

-	r = i2c_dw_init(dev);
-	if (r)
-		return r;
+	ret = i2c_dw_init(dev);
+	if (ret)
+		return ret;

drivers/i2c/busses/i2c-designware-master.c +3 -3

-	r = i2c_dw_acquire_lock(dev);
-	if (r)
-		return r;
+	ret = i2c_dw_acquire_lock(dev);
+	if (ret)
+		return ret;

  /*
   * Test if dynamic TAR update is enabled in this controller by writing

drivers/i2c/busses/i2c-designware-master.c +0 -0

@@ -681,13 +681,13 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
  	i2c_set_adapdata(adap, dev);

  i2c_dw_disable_int(dev);

drivers/i2c/busses/i2c-designware-master.c +1 -1

-	r = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
+	ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,

       IRQF_SHARED | IRQF_COND_SUSPEND, dev_name(dev->dev), dev);

drivers/i2c/busses/i2c-designware-master.c +1 -1

-	if (r) {
+	if (ret) {

  dev_err(dev->dev, "failure requesting irq %i: %d\n",

drivers/i2c/busses/i2c-designware-master.c +2 -2

-			dev->irq, r);
-		return r;
+			dev->irq, ret);
+		return ret;

  }

  /*

drivers/i2c/busses/i2c-designware-master.c +0 -0

@@ -697,12 +697,12 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
  	 * registered I2C slaves that do I2C transfers in their

probe.
   */
  pm_runtime_get_noresume(dev->dev);

drivers/i2c/busses/i2c-designware-master.c +3 -3

-	r = i2c_add_numbered_adapter(adap);
-	if (r)
-		dev_err(dev->dev, "failure adding adapter: %d\n", r);
+	ret = i2c_add_numbered_adapter(adap);
+	if (ret)
+		dev_err(dev->dev, "failure adding adapter: %d\n",

ret);
  pm_runtime_put_noidle(dev->dev);

drivers/i2c/busses/i2c-designware-master.c +1 -1

-	return r;
+	return ret;

 }
 EXPORT_SYMBOL_GPL(i2c_dw_probe);

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

index ef75031f8a62..785f4380c9a9 100644
@@ -234,7 +234,7 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
  	struct fwnode_handle *child;
  	u32 acpi_speed, ht = 0;
  	struct resource *mem;
-	int irq, r;
+	int irq, ret;
  	u32 reg;

  irq = platform_get_irq(pdev, 0);

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

@@ -293,9 +293,9 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
  		return -EINVAL;
  	}

drivers/i2c/busses/i2c-designware-platdrv.c +3 -3

-	r = i2c_dw_eval_lock_support(dev);
-	if (r)
-		return r;
+	ret = i2c_dw_eval_lock_support(dev);
+	if (ret)
+		return ret;

  if (ACPI_HANDLE(&pdev->dev) == NULL) { device_for_each_child_node(&pdev->dev, child) {

drivers/i2c/busses/i2c-designware-platdrv.c +0 -0

@@ -336,14 +336,14 @@ static int dw_i2c_plat_probe(struct
 platform_device *pdev)
  	}

  if (dev->mode == DW_IC_SLAVE)

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

-		r = i2c_dw_probe_slave(dev);
+		ret = i2c_dw_probe_slave(dev);

  else

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

-		r = i2c_dw_probe(dev);
+		ret = i2c_dw_probe(dev);

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

-	if (r && !dev->pm_runtime_disabled)
+	if (ret && !dev->pm_runtime_disabled)

  pm_runtime_disable(&pdev->dev);

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

-	return r;
+	return ret;

 }

 static int dw_i2c_plat_remove(struct platform_device *pdev)