[RFC] i2c: add I2C_M_FORCE_STOP

Message ID 1525875877-10164-1-git-send-email-akinobu.mita@gmail.com
State Superseded
Headers show
Series
  • [RFC] i2c: add I2C_M_FORCE_STOP
Related show

Commit Message

Akinobu Mita May 9, 2018, 2:24 p.m.
This adds a new I2C_M_FORCE_STOP flag that forces a stop condition after
the message in a combined transaction.

This flag is intended to be used by the devices that don't support
repeated starts like SCCB (Serial Camera Control Bus) devices.

Here is an example usage for ov772x driver that needs to issue two
separated I2C messages as the ov772x device doesn't support repeated
starts.

static int ov772x_read(struct i2c_client *client, u8 addr)
{
        u8 val;
        struct i2c_msg msg[] = {
                {
                        .addr = client->addr,
                        .flags = I2C_M_FORCE_STOP,
                        .len = 1,
                        .buf = &addr,
                },
                {
                        .addr = client->addr,
                        .flags = I2C_M_RD,
                        .len = 1,
                        .buf = &val,
                },
        };
        int ret;

        ret = i2c_transfer(client->adapter, msg, 2);
        if (ret != 2)
                return (ret < 0) ? ret : -EIO;

        return val;
}

This is another approach based on Mauro's advise for the initial attempt
(http://patchwork.ozlabs.org/patch/905192/).

Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Wolfram Sang <wsa@the-dreams.de>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/i2c/i2c-core-base.c | 46 ++++++++++++++++++++++++++++++++++-----------
 include/uapi/linux/i2c.h    |  1 +
 2 files changed, 36 insertions(+), 11 deletions(-)

Comments

Mauro Carvalho Chehab May 29, 2018, 12:42 p.m. | #1
Em Wed,  9 May 2018 23:24:37 +0900
Akinobu Mita <akinobu.mita@gmail.com> escreveu:

> This adds a new I2C_M_FORCE_STOP flag that forces a stop condition after
> the message in a combined transaction.
> 
> This flag is intended to be used by the devices that don't support
> repeated starts like SCCB (Serial Camera Control Bus) devices.
> 
> Here is an example usage for ov772x driver that needs to issue two
> separated I2C messages as the ov772x device doesn't support repeated
> starts.
> 
> static int ov772x_read(struct i2c_client *client, u8 addr)
> {
>         u8 val;
>         struct i2c_msg msg[] = {
>                 {
>                         .addr = client->addr,
>                         .flags = I2C_M_FORCE_STOP,
>                         .len = 1,
>                         .buf = &addr,
>                 },
>                 {
>                         .addr = client->addr,
>                         .flags = I2C_M_RD,
>                         .len = 1,
>                         .buf = &val,
>                 },
>         };
>         int ret;
> 
>         ret = i2c_transfer(client->adapter, msg, 2);
>         if (ret != 2)
>                 return (ret < 0) ? ret : -EIO;
> 
>         return val;
> }
> 
> This is another approach based on Mauro's advise for the initial attempt
> (http://patchwork.ozlabs.org/patch/905192/).
> 
> Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Patch looks good to me.

Reviewed-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>


> ---
>  drivers/i2c/i2c-core-base.c | 46 ++++++++++++++++++++++++++++++++++-----------
>  include/uapi/linux/i2c.h    |  1 +
>  2 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 1ba40bb..6b73484 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1828,6 +1828,25 @@ static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  	return 0;
>  }
>  
> +static int i2c_transfer_nolock(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			       int num)
> +{
> +	unsigned long orig_jiffies;
> +	int ret, try;
> +
> +	/* Retry automatically on arbitration loss */
> +	orig_jiffies = jiffies;
> +	for (ret = 0, try = 0; try <= adap->retries; try++) {
> +		ret = adap->algo->master_xfer(adap, msgs, num);
> +		if (ret != -EAGAIN)
> +			break;
> +		if (time_after(jiffies, orig_jiffies + adap->timeout))
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * __i2c_transfer - unlocked flavor of i2c_transfer
>   * @adap: Handle to I2C bus
> @@ -1842,8 +1861,8 @@ static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs,
>   */
>  int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  {
> -	unsigned long orig_jiffies;
> -	int ret, try;
> +	int ret;
> +	int i, n;
>  
>  	if (WARN_ON(!msgs || num < 1))
>  		return -EINVAL;
> @@ -1857,7 +1876,6 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  	 * being executed when not needed.
>  	 */
>  	if (static_branch_unlikely(&i2c_trace_msg_key)) {
> -		int i;
>  		for (i = 0; i < num; i++)
>  			if (msgs[i].flags & I2C_M_RD)
>  				trace_i2c_read(adap, &msgs[i], i);
> @@ -1865,18 +1883,24 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  				trace_i2c_write(adap, &msgs[i], i);
>  	}
>  
> -	/* Retry automatically on arbitration loss */
> -	orig_jiffies = jiffies;
> -	for (ret = 0, try = 0; try <= adap->retries; try++) {
> -		ret = adap->algo->master_xfer(adap, msgs, num);
> -		if (ret != -EAGAIN)
> -			break;
> -		if (time_after(jiffies, orig_jiffies + adap->timeout))
> +	for (i = 0; i < num; i += n) {
> +		for (n = 0; i + n < num; n++) {
> +			if (msgs[i + n].flags & I2C_M_FORCE_STOP) {
> +				n++;
> +				break;
> +			}
> +		}
> +
> +		ret = i2c_transfer_nolock(adap, &msgs[i], n);
> +		if (ret != n) {
> +			if (i > 0)
> +				ret = (ret < 0) ? i : i + ret;
>  			break;
> +		}
> +		ret = i + n;
>  	}
>  
>  	if (static_branch_unlikely(&i2c_trace_msg_key)) {
> -		int i;
>  		for (i = 0; i < ret; i++)
>  			if (msgs[i].flags & I2C_M_RD)
>  				trace_i2c_reply(adap, &msgs[i], i);
> diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> index f71a175..36e8c7c 100644
> --- a/include/uapi/linux/i2c.h
> +++ b/include/uapi/linux/i2c.h
> @@ -72,6 +72,7 @@ struct i2c_msg {
>  #define I2C_M_RD		0x0001	/* read data, from slave to master */
>  					/* I2C_M_RD is guaranteed to be 0x0001! */
>  #define I2C_M_TEN		0x0010	/* this is a ten bit chip address */
> +#define I2C_M_FORCE_STOP	0x0100	/* force a stop condition after the message */
>  #define I2C_M_DMA_SAFE		0x0200	/* the buffer of this message is DMA safe */
>  					/* makes only sense in kernelspace */
>  					/* userspace buffers are copied anyway */



Thanks,
Mauro

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 1ba40bb..6b73484 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1828,6 +1828,25 @@  static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	return 0;
 }
 
+static int i2c_transfer_nolock(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			       int num)
+{
+	unsigned long orig_jiffies;
+	int ret, try;
+
+	/* Retry automatically on arbitration loss */
+	orig_jiffies = jiffies;
+	for (ret = 0, try = 0; try <= adap->retries; try++) {
+		ret = adap->algo->master_xfer(adap, msgs, num);
+		if (ret != -EAGAIN)
+			break;
+		if (time_after(jiffies, orig_jiffies + adap->timeout))
+			break;
+	}
+
+	return ret;
+}
+
 /**
  * __i2c_transfer - unlocked flavor of i2c_transfer
  * @adap: Handle to I2C bus
@@ -1842,8 +1861,8 @@  static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs,
  */
 int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
-	unsigned long orig_jiffies;
-	int ret, try;
+	int ret;
+	int i, n;
 
 	if (WARN_ON(!msgs || num < 1))
 		return -EINVAL;
@@ -1857,7 +1876,6 @@  int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	 * being executed when not needed.
 	 */
 	if (static_branch_unlikely(&i2c_trace_msg_key)) {
-		int i;
 		for (i = 0; i < num; i++)
 			if (msgs[i].flags & I2C_M_RD)
 				trace_i2c_read(adap, &msgs[i], i);
@@ -1865,18 +1883,24 @@  int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 				trace_i2c_write(adap, &msgs[i], i);
 	}
 
-	/* Retry automatically on arbitration loss */
-	orig_jiffies = jiffies;
-	for (ret = 0, try = 0; try <= adap->retries; try++) {
-		ret = adap->algo->master_xfer(adap, msgs, num);
-		if (ret != -EAGAIN)
-			break;
-		if (time_after(jiffies, orig_jiffies + adap->timeout))
+	for (i = 0; i < num; i += n) {
+		for (n = 0; i + n < num; n++) {
+			if (msgs[i + n].flags & I2C_M_FORCE_STOP) {
+				n++;
+				break;
+			}
+		}
+
+		ret = i2c_transfer_nolock(adap, &msgs[i], n);
+		if (ret != n) {
+			if (i > 0)
+				ret = (ret < 0) ? i : i + ret;
 			break;
+		}
+		ret = i + n;
 	}
 
 	if (static_branch_unlikely(&i2c_trace_msg_key)) {
-		int i;
 		for (i = 0; i < ret; i++)
 			if (msgs[i].flags & I2C_M_RD)
 				trace_i2c_reply(adap, &msgs[i], i);
diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index f71a175..36e8c7c 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -72,6 +72,7 @@  struct i2c_msg {
 #define I2C_M_RD		0x0001	/* read data, from slave to master */
 					/* I2C_M_RD is guaranteed to be 0x0001! */
 #define I2C_M_TEN		0x0010	/* this is a ten bit chip address */
+#define I2C_M_FORCE_STOP	0x0100	/* force a stop condition after the message */
 #define I2C_M_DMA_SAFE		0x0200	/* the buffer of this message is DMA safe */
 					/* makes only sense in kernelspace */
 					/* userspace buffers are copied anyway */