On 08-May-17 17:53, Andy Shevchenko wrote: > On Mon, 2017-05-08 at 11:37 +0100, Luis Oliveira wrote: >> - 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. > >> @@ -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 > >> +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. >> +/** >> + * 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. >> + * 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; > >> + } 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. >> + /* 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; >> + } > >> +/* >> + * 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. > >> + enabled, slave_activity, raw_stat, stat); >> + > >> + 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. >> + 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; >> +} > >> +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); > >> + > > I would remove this line. Ok. Thank you. > >> + if (ret > 0) >> + complete(&dev->cmd_complete); >> + >> + return IRQ_RETVAL(ret); >> +} > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html