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@xxxxxxxxxxxx wrote:
> From: Luis Oliveira <lolivei@xxxxxxxxxxxx>
> 

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@xxxxxxxxxxxxxxx>
Intel Finland Oy