[v2,2/3] i2c: recovery: require either get_sda or set_sda

Message ID 20180710214217.1336-3-wsa+renesas@sang-engineering.com
State Accepted
Headers show
Series
  • i2c: recovery: make sure pulses are not misinterpreted
Related show

Commit Message

Wolfram Sang July 10, 2018, 9:42 p.m.
For bus recovery, we either need to bail out early if we can read SDA or
we need to send STOP after every pulse. Otherwise recovery might be
misinterpreted as an unwanted write. So, require one of those SDA
handling functions to avoid this problem.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c |  7 ++++++-
 include/linux/i2c.h         | 12 ++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

Comments

Peter Rosin July 11, 2018, 5:51 a.m. | #1
On 2018-07-10 23:42, Wolfram Sang wrote:
> For bus recovery, we either need to bail out early if we can read SDA or
> we need to send STOP after every pulse. Otherwise recovery might be
> misinterpreted as an unwanted write. So, require one of those SDA
> handling functions to avoid this problem.

Assuming that all users fulfill the stricter requirement...

Acked-by: Peter Rosin <peda@axentia.se>

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c |  7 ++++++-
>  include/linux/i2c.h         | 12 ++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 301285c54603..871a9731894f 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -202,7 +202,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  		/*
>  		 * If we can set SDA, we will always create STOP here to ensure
>  		 * the additional pulses will do no harm. This is achieved by
> -		 * letting SDA follow SCL half a cycle later.
> +		 * letting SDA follow SCL half a cycle later. Check the
> +		 * 'incomplete_write_byte' fault injector for details.
>  		 */
>  		ndelay(RECOVERY_NDELAY / 2);
>  		if (bri->set_sda)
> @@ -274,6 +275,10 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>  			err_str = "no {get|set}_scl() found";
>  			goto err;
>  		}
> +		if (!bri->set_sda && !bri->get_sda) {
> +			err_str = "either get_sda() or set_sda() needed";
> +			goto err;
> +		}
>  	}
>  
>  	return;
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 465afb092fa7..9d1818c56775 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -581,12 +581,12 @@ struct i2c_timings {
>   *      recovery. Populated internally for generic GPIO recovery.
>   * @set_scl: This sets/clears the SCL line. Mandatory for generic SCL recovery.
>   *      Populated internally for generic GPIO recovery.
> - * @get_sda: This gets current value of SDA line. Optional for generic SCL
> - *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
> - *      GPIO recovery.
> - * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
> - *	Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
> - *	recovery.
> + * @get_sda: This gets current value of SDA line. This or set_sda() is mandatory
> + *	for generic SCL recovery. Populated internally, if sda_gpio is a valid
> + *	GPIO, for generic GPIO recovery.
> + * @set_sda: This sets/clears the SDA line. This or get_sda() is mandatory for
> + *	generic SCL recovery. Populated internally, if sda_gpio is a valid GPIO,
> + *	for generic GPIO recovery.
>   * @prepare_recovery: This will be called before starting recovery. Platform may
>   *	configure padmux here for SDA/SCL line or something else they want.
>   * @unprepare_recovery: This will be called after completing recovery. Platform
>
Wolfram Sang July 11, 2018, 4:42 p.m. | #2
On Wed, Jul 11, 2018 at 07:51:02AM +0200, Peter Rosin wrote:
> On 2018-07-10 23:42, Wolfram Sang wrote:
> > For bus recovery, we either need to bail out early if we can read SDA or
> > we need to send STOP after every pulse. Otherwise recovery might be
> > misinterpreted as an unwanted write. So, require one of those SDA
> > handling functions to avoid this problem.
> 
> Assuming that all users fulfill the stricter requirement...

Yes, I checked all in-tree users for that.
Wolfram Sang July 17, 2018, 8:43 a.m. | #3
On Tue, Jul 10, 2018 at 11:42:16PM +0200, Wolfram Sang wrote:
> For bus recovery, we either need to bail out early if we can read SDA or
> we need to send STOP after every pulse. Otherwise recovery might be
> misinterpreted as an unwanted write. So, require one of those SDA
> handling functions to avoid this problem.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 301285c54603..871a9731894f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -202,7 +202,8 @@  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 		/*
 		 * If we can set SDA, we will always create STOP here to ensure
 		 * the additional pulses will do no harm. This is achieved by
-		 * letting SDA follow SCL half a cycle later.
+		 * letting SDA follow SCL half a cycle later. Check the
+		 * 'incomplete_write_byte' fault injector for details.
 		 */
 		ndelay(RECOVERY_NDELAY / 2);
 		if (bri->set_sda)
@@ -274,6 +275,10 @@  static void i2c_init_recovery(struct i2c_adapter *adap)
 			err_str = "no {get|set}_scl() found";
 			goto err;
 		}
+		if (!bri->set_sda && !bri->get_sda) {
+			err_str = "either get_sda() or set_sda() needed";
+			goto err;
+		}
 	}
 
 	return;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 465afb092fa7..9d1818c56775 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -581,12 +581,12 @@  struct i2c_timings {
  *      recovery. Populated internally for generic GPIO recovery.
  * @set_scl: This sets/clears the SCL line. Mandatory for generic SCL recovery.
  *      Populated internally for generic GPIO recovery.
- * @get_sda: This gets current value of SDA line. Optional for generic SCL
- *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
- *      GPIO recovery.
- * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
- *	Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
- *	recovery.
+ * @get_sda: This gets current value of SDA line. This or set_sda() is mandatory
+ *	for generic SCL recovery. Populated internally, if sda_gpio is a valid
+ *	GPIO, for generic GPIO recovery.
+ * @set_sda: This sets/clears the SDA line. This or get_sda() is mandatory for
+ *	generic SCL recovery. Populated internally, if sda_gpio is a valid GPIO,
+ *	for generic GPIO recovery.
  * @prepare_recovery: This will be called before starting recovery. Platform may
  *	configure padmux here for SDA/SCL line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform