On 08-May-17 17:53, Andy Shevchenko wrote:

On Mon, 2017-05-08 at 11:37 +0100, Luis Oliveira wrote:

quoted

- Changes in Kconfig to enable I2C_DESIGNWARE_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 the I2C_DESIGNWARE_SLAVE module
  when supported by the architecture.

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

My comments below.
Overall entire series looks much much better than first version.

quoted

@@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_DESIGNWARE_CORE)	+= i2c-designware-core.o
 i2c-designware-core-objs := i2c-designware-common.o i2c-designware-

master.o

quoted

+i2c-designware-core-$(CONFIG_I2C_DESIGNWARE_SLAVE) += i2c-designware-
slave.o

What is your intention here?

AFAIUC it should be like

ifneq ($(CONFIG_I2C_DESIGNWARE_SLAVE),n)
i2c-designware-core-objs += i2c-designware-slave.o
endif

Ok, I will change it in the next patchset.

quoted

+/**
+ * i2c_dw_init_slave() - Initialize the designware i2c slave hardware
+ * @dev: device private data
+ *
+ * This functions configures and enables the I2C.

functions -> function
I2C -> I2C in slave mode

Yes.

quoted

+ * This function is called during I2C init function, and in case of
timeout at
+ * run time.
+ */
+int i2c_dw_init_slave(struct dw_i2c_dev *dev)
+{
+	u32 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->flags |= ACCESS_SWAP;

quoted

+	} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {

GENMASK(15, 0)

I think I will leave it as it is. As Jarkko said it has more readability this way.

quoted

+		/* Configure register access mode 16bit. */
+		dev->flags |= ACCESS_16BIT;
+	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
+		dev_err(dev->dev,
+			"Unknown Synopsys component type: 0x%08x\n",
reg);
+		i2c_dw_release_lock(dev);
+		return -ENODEV;
+	}

quoted

+/*
+ * Interrupt service routine. This gets called whenever an I2C slave
interrupt
+ * occurs.
+ */
+
+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,
+		"%#x SLAVE_ACTV=%#x : RAW_INTR_STAT=%#x :
INTR_STAT=%#x\n",

SLAVE_ACTV -> STATUS_SLAVE_ACTIVITY

Ok.

quoted

+		enabled, slave_activity, raw_stat, stat);
+

quoted

+	if (slave_activity) {
+		if (stat & DW_IC_INTR_RD_REQ) {

Looking into next condition (stat & DW_IC_INTR_RX_DONE) I would 
exchange these lines

By order? I don't think I understood your point. Please elaborate a little more.

quoted

+			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_vdbg(dev->dev, "Byte %X
acked!",
+					val);
+					}
+				dw_readl(dev, DW_IC_CLR_RD_REQ);
+				stat =
i2c_dw_read_clear_intrbits_slave(dev);
+			} else {
+				dw_readl(dev, DW_IC_CLR_RD_REQ);
+				dw_readl(dev, DW_IC_CLR_RX_UNDER);
+				stat =
i2c_dw_read_clear_intrbits_slave(dev);
+			}
+			if (!i2c_slave_event(dev->slave,
+					I2C_SLAVE_READ_REQUESTED,
&val))
+				dw_writel(dev, val, DW_IC_DATA_CMD);
+		}
+	}
+
+	if (stat & DW_IC_INTR_RX_DONE) {
+		if (!i2c_slave_event(dev->slave,
I2C_SLAVE_READ_PROCESSED,
+			&val))
+			dw_readl(dev, DW_IC_CLR_RX_DONE);
+
+		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+		stat = i2c_dw_read_clear_intrbits_slave(dev);
+		return 1;
+	}
+
+	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_vdbg(dev->dev, "Byte %X acked!", val);
+	} else {
+		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
+		stat = i2c_dw_read_clear_intrbits_slave(dev);
+	}
+
+	if (stat & DW_IC_INTR_TX_OVER)
+		dw_readl(dev, DW_IC_CLR_TX_OVER);
+
+	return 1;
+}

quoted

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

quoted

+

I would remove this line.

Ok. Thank you.

quoted

+	if (ret > 0)
+		complete(&dev->cmd_complete);
+
+	return IRQ_RETVAL(ret);
+}