[PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Rayagonda Kokatanur]

From: Lori Hikichi <lori.hikichi@broadcom.com>

Enable handling of i2c repeated start. The current code
handles a multi msg i2c transfer as separate i2c bus
transactions. This change will now handle this case
using the i2c repeated start protocol. The number of msgs
in a transfer is limited to two, and must be a write
followed by a read.

Signed-off-by: Lori Hikichi <lori.hikichi@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Signed-off-by: Icarus Chau <icarus.chau@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Signed-off-by: Shivaraj Shetty <sshetty1@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 70 +++++++++++++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index d7fd76b..15fedcf 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -81,6 +81,7 @@
 #define M_CMD_PROTOCOL_MASK          0xf
 #define M_CMD_PROTOCOL_BLK_WR        0x7
 #define M_CMD_PROTOCOL_BLK_RD        0x8
+#define M_CMD_PROTOCOL_PROCESS       0xa
 #define M_CMD_PEC_SHIFT              8
 #define M_CMD_RD_CNT_SHIFT           0
 #define M_CMD_RD_CNT_MASK            0xff
@@ -675,13 +676,20 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
 	return 0;
 }
 
-static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
-					 struct i2c_msg *msg)
+/*
+ * If 'process_call' is true, then this is a multi-msg transfer that requires
+ * a repeated start between the messages.
+ * More specifically, it must be a write (reg) followed by a read (data).
+ * The i2c quirks are set to enforce this rule.
+ */
+static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,
+					struct i2c_msg *msgs, bool process_call)
 {
 	int i;
 	u8 addr;
 	u32 val, tmp, val_intr_en;
 	unsigned int tx_bytes;
+	struct i2c_msg *msg = &msgs[0];
 
 	/* check if bus is busy */
 	if (!!(iproc_i2c_rd_reg(iproc_i2c,
@@ -707,14 +715,29 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 			val = msg->buf[i];
 
 			/* mark the last byte */
-			if (i == msg->len - 1)
-				val |= BIT(M_TX_WR_STATUS_SHIFT);
+			if (!process_call && (i == msg->len - 1))
+				val |= 1 << M_TX_WR_STATUS_SHIFT;
 
 			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}
 		iproc_i2c->tx_bytes = tx_bytes;
 	}
 
+	/* Process the read message if this is process call */
+	if (process_call) {
+		msg++;
+		iproc_i2c->msg = msg;  /* point to second msg */
+
+		/*
+		 * The last byte to be sent out should be a slave
+		 * address with read operation
+		 */
+		addr = msg->addr << 1 | 1;
+		/* mark it the last byte out */
+		val = addr | (1 << M_TX_WR_STATUS_SHIFT);
+		iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
+	}
+
 	/* mark as incomplete before starting the transaction */
 	if (iproc_i2c->irq)
 		reinit_completion(&iproc_i2c->done);
@@ -733,7 +756,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	 * underrun interrupt, which will be triggerred when the TX FIFO is
 	 * empty. When that happens we can then pump more data into the FIFO
 	 */
-	if (!(msg->flags & I2C_M_RD) &&
+	if (!process_call && !(msg->flags & I2C_M_RD) &&
 	    msg->len > iproc_i2c->tx_bytes)
 		val_intr_en |= BIT(IE_M_TX_UNDERRUN_SHIFT);
 
@@ -743,6 +766,8 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	 */
 	val = BIT(M_CMD_START_BUSY_SHIFT);
 	if (msg->flags & I2C_M_RD) {
+		u32 protocol;
+
 		iproc_i2c->rx_bytes = 0;
 		if (msg->len > M_RX_FIFO_MAX_THLD_VALUE)
 			iproc_i2c->thld_bytes = M_RX_FIFO_THLD_VALUE;
@@ -758,7 +783,10 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 		/* enable the RX threshold interrupt */
 		val_intr_en |= BIT(IE_M_RX_THLD_SHIFT);
 
-		val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
+		protocol = process_call ?
+				M_CMD_PROTOCOL_PROCESS : M_CMD_PROTOCOL_BLK_RD;
+
+		val |= (protocol << M_CMD_PROTOCOL_SHIFT) |
 		       (msg->len << M_CMD_RD_CNT_SHIFT);
 	} else {
 		val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
@@ -774,17 +802,31 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
 			      struct i2c_msg msgs[], int num)
 {
 	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adapter);
-	int ret, i;
+	bool process_call = false;
+	int ret;
 
-	/* go through all messages */
-	for (i = 0; i < num; i++) {
-		ret = bcm_iproc_i2c_xfer_single_msg(iproc_i2c, &msgs[i]);
-		if (ret) {
-			dev_dbg(iproc_i2c->device, "xfer failed\n");
-			return ret;
+	if (num > 2) {
+		dev_err(iproc_i2c->device,
+			"Only support up to 2 messages. Current msg count %d\n",
+			num);
+		return -EOPNOTSUPP;
+	}
+
+	if (num == 2) {
+		/* Repeated start, use process call */
+		process_call = true;
+		if (msgs[1].flags & I2C_M_NOSTART) {
+			dev_err(iproc_i2c->device, "Invalid repeated start\n");
+			return -EOPNOTSUPP;
 		}
 	}
 
+	ret = bcm_iproc_i2c_xfer_internal(iproc_i2c, msgs, process_call);
+	if (ret) {
+		dev_dbg(iproc_i2c->device, "xfer failed\n");
+		return ret;
+	}
+
 	return num;
 }
 
@@ -806,6 +848,8 @@ static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
 };
 
 static struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
+	.flags = I2C_AQ_COMB_WRITE_THEN_READ,
+	.max_comb_1st_msg_len = M_TX_RX_FIFO_SIZE,
 	.max_read_len = M_RX_MAX_READ_LEN,
 };
 
-- 
1.9.1

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Ray Jui]

Hi Wolfram,

On 8/6/19 9:09 PM, Rayagonda Kokatanur wrote:
> From: Lori Hikichi <lori.hikichi@broadcom.com>
> 
> Enable handling of i2c repeated start. The current code
> handles a multi msg i2c transfer as separate i2c bus
> transactions. This change will now handle this case
> using the i2c repeated start protocol. The number of msgs
> in a transfer is limited to two, and must be a write
> followed by a read.
> 
> Signed-off-by: Lori Hikichi <lori.hikichi@broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> Signed-off-by: Icarus Chau <icarus.chau@broadcom.com>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Signed-off-by: Shivaraj Shetty <sshetty1@broadcom.com>
> ---

Note this patch has gone through internal review and testing on various 
I2C slave devices. It is introduced to work around limitation of our I2C 
controller and allows it to work on certain I2C slave devices that are 
sensitive and requires repeated start between transactions instead of a 
stop.

Given that my name is also on the Signed-off-by since I helped to 
rewrite part of the patch, I'm not going to add my Reviewed-by tag here.

Please help to review.

Thanks,

Ray

>   drivers/i2c/busses/i2c-bcm-iproc.c | 70 +++++++++++++++++++++++++++++++-------
>   1 file changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index d7fd76b..15fedcf 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -81,6 +81,7 @@
>   #define M_CMD_PROTOCOL_MASK          0xf
>   #define M_CMD_PROTOCOL_BLK_WR        0x7
>   #define M_CMD_PROTOCOL_BLK_RD        0x8
> +#define M_CMD_PROTOCOL_PROCESS       0xa
>   #define M_CMD_PEC_SHIFT              8
>   #define M_CMD_RD_CNT_SHIFT           0
>   #define M_CMD_RD_CNT_MASK            0xff
> @@ -675,13 +676,20 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
>   	return 0;
>   }
>   
> -static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
> -					 struct i2c_msg *msg)
> +/*
> + * If 'process_call' is true, then this is a multi-msg transfer that requires
> + * a repeated start between the messages.
> + * More specifically, it must be a write (reg) followed by a read (data).
> + * The i2c quirks are set to enforce this rule.
> + */
> +static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,
> +					struct i2c_msg *msgs, bool process_call)
>   {
>   	int i;
>   	u8 addr;
>   	u32 val, tmp, val_intr_en;
>   	unsigned int tx_bytes;
> +	struct i2c_msg *msg = &msgs[0];
>   
>   	/* check if bus is busy */
>   	if (!!(iproc_i2c_rd_reg(iproc_i2c,
> @@ -707,14 +715,29 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
>   			val = msg->buf[i];
>   
>   			/* mark the last byte */
> -			if (i == msg->len - 1)
> -				val |= BIT(M_TX_WR_STATUS_SHIFT);
> +			if (!process_call && (i == msg->len - 1))
> +				val |= 1 << M_TX_WR_STATUS_SHIFT;
>   
>   			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
>   		}
>   		iproc_i2c->tx_bytes = tx_bytes;
>   	}
>   
> +	/* Process the read message if this is process call */
> +	if (process_call) {
> +		msg++;
> +		iproc_i2c->msg = msg;  /* point to second msg */
> +
> +		/*
> +		 * The last byte to be sent out should be a slave
> +		 * address with read operation
> +		 */
> +		addr = msg->addr << 1 | 1;
> +		/* mark it the last byte out */
> +		val = addr | (1 << M_TX_WR_STATUS_SHIFT);
> +		iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
> +	}
> +
>   	/* mark as incomplete before starting the transaction */
>   	if (iproc_i2c->irq)
>   		reinit_completion(&iproc_i2c->done);
> @@ -733,7 +756,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
>   	 * underrun interrupt, which will be triggerred when the TX FIFO is
>   	 * empty. When that happens we can then pump more data into the FIFO
>   	 */
> -	if (!(msg->flags & I2C_M_RD) &&
> +	if (!process_call && !(msg->flags & I2C_M_RD) &&
>   	    msg->len > iproc_i2c->tx_bytes)
>   		val_intr_en |= BIT(IE_M_TX_UNDERRUN_SHIFT);
>   
> @@ -743,6 +766,8 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
>   	 */
>   	val = BIT(M_CMD_START_BUSY_SHIFT);
>   	if (msg->flags & I2C_M_RD) {
> +		u32 protocol;
> +
>   		iproc_i2c->rx_bytes = 0;
>   		if (msg->len > M_RX_FIFO_MAX_THLD_VALUE)
>   			iproc_i2c->thld_bytes = M_RX_FIFO_THLD_VALUE;
> @@ -758,7 +783,10 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
>   		/* enable the RX threshold interrupt */
>   		val_intr_en |= BIT(IE_M_RX_THLD_SHIFT);
>   
> -		val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
> +		protocol = process_call ?
> +				M_CMD_PROTOCOL_PROCESS : M_CMD_PROTOCOL_BLK_RD;
> +
> +		val |= (protocol << M_CMD_PROTOCOL_SHIFT) |
>   		       (msg->len << M_CMD_RD_CNT_SHIFT);
>   	} else {
>   		val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
> @@ -774,17 +802,31 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
>   			      struct i2c_msg msgs[], int num)
>   {
>   	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adapter);
> -	int ret, i;
> +	bool process_call = false;
> +	int ret;
>   
> -	/* go through all messages */
> -	for (i = 0; i < num; i++) {
> -		ret = bcm_iproc_i2c_xfer_single_msg(iproc_i2c, &msgs[i]);
> -		if (ret) {
> -			dev_dbg(iproc_i2c->device, "xfer failed\n");
> -			return ret;
> +	if (num > 2) {
> +		dev_err(iproc_i2c->device,
> +			"Only support up to 2 messages. Current msg count %d\n",
> +			num);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (num == 2) {
> +		/* Repeated start, use process call */
> +		process_call = true;
> +		if (msgs[1].flags & I2C_M_NOSTART) {
> +			dev_err(iproc_i2c->device, "Invalid repeated start\n");
> +			return -EOPNOTSUPP;
>   		}
>   	}
>   
> +	ret = bcm_iproc_i2c_xfer_internal(iproc_i2c, msgs, process_call);
> +	if (ret) {
> +		dev_dbg(iproc_i2c->device, "xfer failed\n");
> +		return ret;
> +	}
> +
>   	return num;
>   }
>   
> @@ -806,6 +848,8 @@ static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
>   };
>   
>   static struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
> +	.flags = I2C_AQ_COMB_WRITE_THEN_READ,
> +	.max_comb_1st_msg_len = M_TX_RX_FIFO_SIZE,
>   	.max_read_len = M_RX_MAX_READ_LEN,
>   };
>   
> 

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Wolfram Sang]

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


> Given that my name is also on the Signed-off-by since I helped to rewrite
> part of the patch, I'm not going to add my Reviewed-by tag here.
> 
> Please help to review.

Outstanding iproc patches are next in my queue.


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

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Wolfram Sang]

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

Hi everyone,

> +/*
> + * If 'process_call' is true, then this is a multi-msg transfer that requires
> + * a repeated start between the messages.
> + * More specifically, it must be a write (reg) followed by a read (data).
> + * The i2c quirks are set to enforce this rule.
> + */

With all the limitations in place, I wonder if it might be easier to
implement an smbus_xfer callback instead? What is left that makes this
controller more than SMBus and real I2C?

> +	/* Process the read message if this is process call */

Also, the term "process call" here seriously sounds like SMBus.

> +		addr = msg->addr << 1 | 1;

addr = i2c_8bit_addr_from_msg(msg);

> +		u32 protocol;

Hmm, another SMBus terminology.


> +	if (num > 2) {
> +		dev_err(iproc_i2c->device,
> +			"Only support up to 2 messages. Current msg count %d\n",
> +			num);
> +		return -EOPNOTSUPP;
> +	}

With your quirks flags set, the core checks it for you.

Kind regards,

   Wolfram


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

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Ray Jui]

On 8/30/19 5:56 AM, Wolfram Sang wrote:
> Hi everyone,
> 
>> +/*
>> + * If 'process_call' is true, then this is a multi-msg transfer that requires
>> + * a repeated start between the messages.
>> + * More specifically, it must be a write (reg) followed by a read (data).
>> + * The i2c quirks are set to enforce this rule.
>> + */
> 
> With all the limitations in place, I wonder if it might be easier to
> implement an smbus_xfer callback instead? What is left that makes this
> controller more than SMBus and real I2C?
> 

Right. But what is the implication of using smbus_xfer instead of 
master_xfer in our driver?

Does it mean it will break existing functions of the i2c app that our 
customers developed based on i2cdev (e.g., I2C_RDWR)?

1) Does
>> +	/* Process the read message if this is process call */
> 
> Also, the term "process call" here seriously sounds like SMBus.
> 
>> +		addr = msg->addr << 1 | 1;
> 
> addr = i2c_8bit_addr_from_msg(msg);
> 
>> +		u32 protocol;
> 
> Hmm, another SMBus terminology.
> 
> 
>> +	if (num > 2) {
>> +		dev_err(iproc_i2c->device,
>> +			"Only support up to 2 messages. Current msg count %d\n",
>> +			num);
>> +		return -EOPNOTSUPP;
>> +	}
> 
> With your quirks flags set, the core checks it for you.
> 
> Kind regards,
> 
>     Wolfram
> 

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Wolfram Sang]

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

Hi Ray,

> > With all the limitations in place, I wonder if it might be easier to
> > implement an smbus_xfer callback instead? What is left that makes this
> > controller more than SMBus and real I2C?
> > 
> 
> Right. But what is the implication of using smbus_xfer instead of
> master_xfer in our driver?
> 
> Does it mean it will break existing functions of the i2c app that our
> customers developed based on i2cdev (e.g., I2C_RDWR)?

If the customers uses I2C_RDWR (and it cannot be mapped to i2c_smbus_*
calls) then this is an indication that there is some I2C functionality
left which the HW can provide. I'd be interested which one, though.

> 
> 1) Does

Maybe you wanted to describe it here and it got accidently cut off?

Regards,

   Wolfram


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

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Ray Jui]

On 8/31/19 2:49 AM, Wolfram Sang wrote:
> Hi Ray,
> 
>>> With all the limitations in place, I wonder if it might be easier to
>>> implement an smbus_xfer callback instead? What is left that makes this
>>> controller more than SMBus and real I2C?
>>>
>>
>> Right. But what is the implication of using smbus_xfer instead of
>> master_xfer in our driver?
>>
>> Does it mean it will break existing functions of the i2c app that our
>> customers developed based on i2cdev (e.g., I2C_RDWR)?
> 
> If the customers uses I2C_RDWR (and it cannot be mapped to i2c_smbus_*
> calls) then this is an indication that there is some I2C functionality
> left which the HW can provide. I'd be interested which one, though.
> 
>>
>> 1) Does
> 
> Maybe you wanted to describe it here and it got accidently cut off? >

I think you are right that the controller does not seem to support 
additional I2C features in addition to SMBUS.

However, my concern of switching to the smbus_xfer API is:

1) Some customers might have used I2C_RDWR based API from i2cdev. 
Changing from master_xfer to smbus_xfer may break the existing 
applications that are already developed.

2) The sound subsystem I2C regmap based implementation seems to be using 
i2c_ based API instead of smbus_ based API. Does this mean this will 
also break most of the audio codec drivers with I2C regmap API based usage?

Thanks,

Ray

> Regards,
> 
>     Wolfram
> 

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Wolfram Sang]

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


> I think you are right that the controller does not seem to support
> additional I2C features in addition to SMBUS.
> 
> However, my concern of switching to the smbus_xfer API is:
> 
> 1) Some customers might have used I2C_RDWR based API from i2cdev. Changing
> from master_xfer to smbus_xfer may break the existing applications that are
> already developed.

Well, given that you add new quirks in the original patch here, you are
kind of breaking it already. Most transfers which are not SMBus-alike
transfers would now be rejected. For SMBus-alike transfers which are
sent via I2C_RDWR (which is ugly), I have to think about it.

> 2) The sound subsystem I2C regmap based implementation seems to be using
> i2c_ based API instead of smbus_ based API. Does this mean this will also
> break most of the audio codec drivers with I2C regmap API based usage?

I don't think so. If you check regmap_get_i2c_bus() then it checks the
adapter functionality and chooses the best transfer option then. I may
be missing something but I would wonder if the sound system does
something special and different.


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

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Ray Jui]

Hi Wolfram,

On 9/4/19 2:37 PM, Wolfram Sang wrote:
> 
>> I think you are right that the controller does not seem to support
>> additional I2C features in addition to SMBUS.
>>
>> However, my concern of switching to the smbus_xfer API is:
>>
>> 1) Some customers might have used I2C_RDWR based API from i2cdev. Changing
>> from master_xfer to smbus_xfer may break the existing applications that are
>> already developed.
> 
> Well, given that you add new quirks in the original patch here, you are
> kind of breaking it already. Most transfers which are not SMBus-alike
> transfers would now be rejected. For SMBus-alike transfers which are
> sent via I2C_RDWR (which is ugly), I have to think about it.
> 
>> 2) The sound subsystem I2C regmap based implementation seems to be using
>> i2c_ based API instead of smbus_ based API. Does this mean this will also
>> break most of the audio codec drivers with I2C regmap API based usage?
> 
> I don't think so. If you check regmap_get_i2c_bus() then it checks the
> adapter functionality and chooses the best transfer option then. I may
> be missing something but I would wonder if the sound system does
> something special and different.
> 

We did more investigation on this.

First of all, like you said, there's no concern on regmap based API, the 
smbus_xfer only based approach should just work.

Secondly, for most i2ctools like i2cget, i2cset, i2cdump, there's no 
concern either, given that they already use I2C_SMBUS based IOCTL.

However, for i2ctransfer or any customer applications that use I2C_RDWR 
IOCTL, i2c_transfer (master_xfer) is the only supported function. And we 
can confirm we do have at least one customer using i2ctransfer for 
EEPROM access on their system, e.g.,  i2ctransfer 1 w2@0x50 0x00 0x00 r64.

In my opinion, it's probably better to continue to support master_xfer 
in our driver (with obvious limitations), in order to allow i2ctransfer 
(or any apps that use I2C RDWR) to continue to work.

What do you think?

Regards,

Ray

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Wolfram Sang]

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


> In my opinion, it's probably better to continue to support master_xfer in
> our driver (with obvious limitations), in order to allow i2ctransfer (or any
> apps that use I2C RDWR) to continue to work.
> 
> What do you think?

Yes, don't break it for users. We should have paid more attention to it
in the beginning. But, while not ideal, it is not such a big deal to
keep it like this.

Thanks for your investigations!


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

Re: [PATCH v1 1/1] i2c: iproc: Add i2c repeated start capability

[Ray Jui]

On 9/24/19 11:57 AM, Wolfram Sang wrote:
> 
>> In my opinion, it's probably better to continue to support master_xfer in
>> our driver (with obvious limitations), in order to allow i2ctransfer (or any
>> apps that use I2C RDWR) to continue to work.
>>
>> What do you think?
> 
> Yes, don't break it for users. We should have paid more attention to it
> in the beginning. But, while not ideal, it is not such a big deal to
> keep it like this.
> 
> Thanks for your investigations!
> 

Thanks, Wolfram.

Let's please continue with the review process on the current patch then?

Note the patch was already internally reviewed by me.

Please help to review it and let us know if there's any change that 
needs to be made?

Regards,

Ray