Re: [PATCH] i2c: mv64xxx: Implement I2C_M_RECV_LEN and I2C_FUNC_SMBUS_READ_BLOCK_DATA

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 4667 bytes --]

On Sat, Jan 18, 2020 at 12:58:20PM +0100, Federico Fuga wrote:
> The i2c_mv64xxx driver doesn't implement the I2C_M_REC_LEN function
> essential to allow blocks with variable length to be read from an i2c
>  slave.
> This is needed to implement the SMBus Read Block Data function.
> 
> This patch implements the function by changing the bytes_left and
> msg len on the fly if the flag is specified.
> 
> It has been successfully tested on Allwinner A33 with a special
> i2c chip that returns variable length blocks on reading.
> 
> Signed-off-by: Federico Fuga <fuga@studiofuga.com>
> ---

Gregory, any comment? I can't say much about the implementation. In
general, this is a nice feature to have...

>  drivers/i2c/busses/i2c-mv64xxx.c | 67 +++++++++++++++++++++++++-------
>  1 file changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index a5a95ea5b81a..cff9cb20bcc9 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -128,6 +128,7 @@ struct mv64xxx_i2c_data {
>  	u32			addr1;
>  	u32			addr2;
>  	u32			bytes_left;
> +	u32         effective_length;
>  	u32			byte_posn;
>  	u32			send_stop;
>  	u32			block;
> @@ -333,7 +334,18 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
>  {
>  	drv_data->msg = drv_data->msgs;
>  	drv_data->byte_posn = 0;
> -	drv_data->bytes_left = drv_data->msg->len;
> +
> +    /* If we should retrieve the length from the buffer, make sure */
> +	/* to read enough bytes to avoid sending the */
> +	/* STOP bit after the read if the first byte */
> +	if (drv_data->msg->flags & I2C_M_RECV_LEN) {
> +		drv_data->effective_length = -1;
> +		drv_data->bytes_left = 3;
> +	} else {
> +		drv_data->effective_length = drv_data->msg->len;
> +		drv_data->bytes_left = drv_data->msg->len;
> +	}
> +
>  	drv_data->aborting = 0;
>  	drv_data->rc = 0;
>  
> @@ -342,6 +354,42 @@ static void mv64xxx_i2c_send_start(struct mv64xxx_i2c_data *drv_data)
>  	       drv_data->reg_base + drv_data->reg_offsets.control);
>  }
>  
> +static void
> +mv64xxx_i2c_do_send_stop(struct mv64xxx_i2c_data *drv_data)
> +{
> +	drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
> +	writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
> +		drv_data->reg_base + drv_data->reg_offsets.control);
> +	drv_data->block = 0;
> +	if (drv_data->errata_delay)
> +		udelay(5);
> +
> +	wake_up(&drv_data->waitq);
> +}
> +
> +static void
> +mv64xxx_i2c_do_read_data(struct mv64xxx_i2c_data *drv_data)
> +{
> +	u8 data;
> +
> +	data = readl(drv_data->reg_base + drv_data->reg_offsets.data);
> +	drv_data->msg->buf[drv_data->byte_posn++] = data;
> +
> +	if (drv_data->effective_length == -1) {
> +		/* length=0 should not be allowed, but is indeed possible.
> +		 * To avoid locking the chip, we keep reading at least 2 bytes
> +		 */
> +		if (data < 1)
> +			data = 1;
> +		drv_data->effective_length = data+1;
> +		drv_data->bytes_left = data+1;
> +		drv_data->msg->len = data+1;
> +	}
> +
> +	writel(drv_data->cntl_bits,
> +		drv_data->reg_base + drv_data->reg_offsets.control);
> +}
> +
>  static void
>  mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  {
> @@ -392,23 +440,13 @@ mv64xxx_i2c_do_action(struct mv64xxx_i2c_data *drv_data)
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_RCV_DATA:
> -		drv_data->msg->buf[drv_data->byte_posn++] =
> -			readl(drv_data->reg_base + drv_data->reg_offsets.data);
> -		writel(drv_data->cntl_bits,
> -			drv_data->reg_base + drv_data->reg_offsets.control);
> +	    mv64xxx_i2c_do_read_data(drv_data);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_RCV_DATA_STOP:
>  		drv_data->msg->buf[drv_data->byte_posn++] =
>  			readl(drv_data->reg_base + drv_data->reg_offsets.data);
> -		drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_INTEN;
> -		writel(drv_data->cntl_bits | MV64XXX_I2C_REG_CONTROL_STOP,
> -			drv_data->reg_base + drv_data->reg_offsets.control);
> -		drv_data->block = 0;
> -		if (drv_data->errata_delay)
> -			udelay(5);
> -
> -		wake_up(&drv_data->waitq);
> +	    mv64xxx_i2c_do_send_stop(drv_data);
>  		break;
>  
>  	case MV64XXX_I2C_ACTION_INVALID:
> @@ -706,7 +744,8 @@ mv64xxx_i2c_can_offload(struct mv64xxx_i2c_data *drv_data)
>  static u32
>  mv64xxx_i2c_functionality(struct i2c_adapter *adap)
>  {
> -	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
> +	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR |
> +		I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_EMUL;
>  }
>  
>  static int
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]