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