Re: [PATCH v4] i2c: designware: Implement support for SMBus block read and write

[Andy Shevchenko]

On Thu, 2016-11-10 at 09:56 +0700, tnhuynh@xxxxxxx wrote:
> From: Tin Huynh <tnhuynh@xxxxxxx>
> 
> Free and Open IPMI use SMBUS BLOCK Read/Write to support SSIF
> protocol.
> However, I2C Designware Core Driver doesn't handle the case at the
> moment.
> The below patch supports this feature.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> 
> Signed-off-by: Tin Huynh <tnhuynh@xxxxxxx>
> ---
> Change from V3:
> - Correct coding conventions
> - Make clean
> Change from V2:
> - Change subject of email
> - Add a helper function to handle
> Â length byte receiving
> Change from V1:
> - Remove empty lines
> - Add flags variable to make clean code
> - Change DW_DEFAULT_FUNCTIONALITY
> Â in i2c-designware-pcidrv.c
> ---
> Âdrivers/i2c/busses/i2c-designware-core.cÂÂÂÂ|ÂÂÂ46
> +++++++++++++++++++++++++--
> Âdrivers/i2c/busses/i2c-designware-pcidrv.cÂÂ|ÂÂÂÂ1 +
> Âdrivers/i2c/busses/i2c-designware-platdrv.c |ÂÂÂÂ1 +
> Â3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c
> b/drivers/i2c/busses/i2c-designware-core.c
> index 1fe93c4..c91d1b4 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -543,6 +543,8 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> Â	intr_mask = DW_IC_INTR_DEFAULT_MASK;
> Â
> Â	for (; dev->msg_write_idx < dev->msgs_num; dev-
> >msg_write_idx++) {
> +		u32 flags = msgs[dev->msg_write_idx].flags;
> +
> Â		/*
> Â		Â* if target address has changed, we need to
> Â		Â* reprogram the target address in the i2c
> @@ -588,8 +590,15 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> Â			Â* detected from the registers so we set it
> always
> Â			Â* when writing/reading the last byte.
> Â			Â*/
> +
> +			/*
> +			Â* i2c-core.c always sets the buffer length
> of
> +			Â* I2C_FUNC_SMBUS_BLOCK_DATA to 1. The length
> will
> +			Â* be adjusted when receiving the first byte.
> +			Â* Thus we can't stop the transaction here.
> +			Â*/
> Â			if (dev->msg_write_idx == dev->msgs_num - 1
> &&
> -			ÂÂÂÂbuf_len == 1)
> +			ÂÂÂÂbuf_len == 1 && !(flags &
> I2C_M_RECV_LEN))
> Â				cmd |= BIT(9);
> Â
> Â			if (need_restart) {
> @@ -614,7 +623,12 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> Â		dev->tx_buf = buf;
> Â		dev->tx_buf_len = buf_len;
> Â
> -		if (buf_len > 0) {
> +		/*
> +		Â* Because we don't know the buffer length in the
> +		Â* I2C_FUNC_SMBUS_BLOCK_DATA case, we can't stop
> +		Â* the transaction here.
> +		Â*/
> +		if (buf_len > 0 || flags & I2C_M_RECV_LEN) {
> Â			/* more bytes to be written */
> Â			dev->status |= STATUS_WRITE_IN_PROGRESS;
> Â			break;
> @@ -635,6 +649,24 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> Â	dw_writel(dev, intr_mask,ÂÂDW_IC_INTR_MASK);
> Â}
> Â
> +static u8
> +i2c_dw_recv_len(struct dw_i2c_dev *dev, u8 len)
> +{
> +	struct i2c_msg *msgs = dev->msgs;
> +	u32 flags = msgs[dev->msg_read_idx].flags;
> +
> +	/*
> +	Â* Adjust the buffer length and mask the flag
> +	Â* after receiving the first byte.
> +	Â*/
> +	len += (flags & I2C_CLIENT_PEC) ? 2 : 1;
> +	dev->tx_buf_len = len - min_t(u8, len, dev->rx_outstanding);
> +	msgs[dev->msg_read_idx].len = len;
> +	msgs[dev->msg_read_idx].flags &= ~I2C_M_RECV_LEN;
> +
> +	return len;
> +}
> +
> Âstatic void
> Âi2c_dw_read(struct dw_i2c_dev *dev)
> Â{
> @@ -659,7 +691,15 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev
> *dev)
> Â		rx_valid = dw_readl(dev, DW_IC_RXFLR);
> Â
> Â		for (; len > 0 && rx_valid > 0; len--, rx_valid--) {
> -			*buf++ = dw_readl(dev, DW_IC_DATA_CMD);
> +			u32 flags = msgs[dev->msg_read_idx].flags;
> +
> +			*buf = dw_readl(dev, DW_IC_DATA_CMD);
> +			/* Ensure length byte is a valid value */
> +			if (flags & I2C_M_RECV_LEN &&
> +				*buf <= I2C_SMBUS_BLOCK_MAX && *buf >
> 0) {
> +				len = i2c_dw_recv_len(dev, *buf);
> +			}
> +			buf++;
> Â			dev->rx_outstanding--;
> Â		}
> Â
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 96f8230..8ffe2da 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -75,6 +75,7 @@ struct dw_pci_controller {
> Â					I2C_FUNC_SMBUS_BYTE |		
> \
> Â					I2C_FUNC_SMBUS_BYTE_DATA |	
> \
> Â					I2C_FUNC_SMBUS_WORD_DATA |	
> \
> +					I2C_FUNC_SMBUS_BLOCK_DATA |	
> \
> Â					I2C_FUNC_SMBUS_I2C_BLOCK)
> Â
> Â/* Merrifield HCNT/LCNT/SDA hold time */
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0b42a12..886fb62 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -220,6 +220,7 @@ static int dw_i2c_plat_probe(struct
> platform_device *pdev)
> Â		I2C_FUNC_SMBUS_BYTE |
> Â		I2C_FUNC_SMBUS_BYTE_DATA |
> Â		I2C_FUNC_SMBUS_WORD_DATA |
> +		I2C_FUNC_SMBUS_BLOCK_DATA |
> Â		I2C_FUNC_SMBUS_I2C_BLOCK;
> Â
> Â	dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE
> |

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy