i2c: recovery: make pin init look like STOP

Message ID 20180712174919.14447-1-wsa+renesas@sang-engineering.com
State Superseded
Headers show
Series
  • i2c: recovery: make pin init look like STOP
Related show

Commit Message

Wolfram Sang July 12, 2018, 5:49 p.m.
When we we initialize the pins, make sure it looks like STOP by dividing
the delay into halves. It shouldn't matter because SDA is expected to be
held low by a device, but for super-safety, let's do it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Ulrich Hecht July 16, 2018, 9:29 a.m. | #1
On Thu, Jul 12, 2018 at 7:49 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> When we we initialize the pins, make sure it looks like STOP by dividing
> the delay into halves. It shouldn't matter because SDA is expected to be
> held low by a device,

Yeah, what could possibly go wrong? :)

>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 51cbb0c158f2..e57231ccb32a 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -191,9 +191,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>                 bri->prepare_recovery(adap);
>
>         bri->set_scl(adap, scl);
> +       ndelay(RECOVERY_NDELAY / 2);
>         if (bri->set_sda)
> -               bri->set_sda(adap, 1);
> -       ndelay(RECOVERY_NDELAY);
> +               bri->set_sda(adap, scl);
> +       ndelay(RECOVERY_NDELAY / 2);
>
>         /*
>          * By this time SCL is high, as we need to give 9 falling-rising edges
> --
> 2.11.0
>

Reviewed-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>

CU
Uli
Peter Rosin July 16, 2018, 9:40 a.m. | #2
On 2018-07-12 19:49, Wolfram Sang wrote:
> When we we initialize the pins, make sure it looks like STOP by dividing
> the delay into halves. It shouldn't matter because SDA is expected to be
> held low by a device, but for super-safety, let's do it.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 51cbb0c158f2..e57231ccb32a 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -191,9 +191,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  		bri->prepare_recovery(adap);
>  
>  	bri->set_scl(adap, scl);

For me, it would be more natural to have

	bri->set_scl(adap, 1);

> +	ndelay(RECOVERY_NDELAY / 2);
>  	if (bri->set_sda)
> -		bri->set_sda(adap, 1);
> -	ndelay(RECOVERY_NDELAY);
> +		bri->set_sda(adap, scl);

instead of changing this "1" to "scl"? Same-same, but it looks odd
to use scl as argument to sda (at least without that comment about
sda following scl that is present inside the loop below).

At the same time, your version make the code inside the loop the
same as this initializing code. Oh well, your call...

Either way

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

Cheers,
Peter

> +	ndelay(RECOVERY_NDELAY / 2);
>  
>  	/*
>  	 * By this time SCL is high, as we need to give 9 falling-rising edges
>
Geert Uytterhoeven July 16, 2018, 10:37 a.m. | #3
On Thu, Jul 12, 2018 at 7:49 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> When we we initialize the pins, make sure it looks like STOP by dividing
> the delay into halves. It shouldn't matter because SDA is expected to be
> held low by a device, but for super-safety, let's do it.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 51cbb0c158f2..e57231ccb32a 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -191,9 +191,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>                 bri->prepare_recovery(adap);
>
>         bri->set_scl(adap, scl);
> +       ndelay(RECOVERY_NDELAY / 2);

Any change someone changes RECOVERY_NDELAY to 1, leading to a
zero delay here? Is that an issue?

>         if (bri->set_sda)
> -               bri->set_sda(adap, 1);
> -       ndelay(RECOVERY_NDELAY);
> +               bri->set_sda(adap, scl);
> +       ndelay(RECOVERY_NDELAY / 2);
>
>         /*
>          * By this time SCL is high, as we need to give 9 falling-rising edges

Gr{oetje,eeting}s,

                        Geert
Peter Rosin July 16, 2018, 11:16 a.m. | #4
On 2018-07-16 12:37, Geert Uytterhoeven wrote:
> On Thu, Jul 12, 2018 at 7:49 PM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>> When we we initialize the pins, make sure it looks like STOP by dividing
>> the delay into halves. It shouldn't matter because SDA is expected to be
>> held low by a device, but for super-safety, let's do it.
>>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>  drivers/i2c/i2c-core-base.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 51cbb0c158f2..e57231ccb32a 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -191,9 +191,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>>                 bri->prepare_recovery(adap);
>>
>>         bri->set_scl(adap, scl);
>> +       ndelay(RECOVERY_NDELAY / 2);
> 
> Any change someone changes RECOVERY_NDELAY to 1, leading to a
> zero delay here? Is that an issue?

No!

Above this, there is this line:

#define RECOVERY_NDELAY		5000

Cheers,
Peter

>>         if (bri->set_sda)
>> -               bri->set_sda(adap, 1);
>> -       ndelay(RECOVERY_NDELAY);
>> +               bri->set_sda(adap, scl);
>> +       ndelay(RECOVERY_NDELAY / 2);
>>
>>         /*
>>          * By this time SCL is high, as we need to give 9 falling-rising edges
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
Geert Uytterhoeven July 16, 2018, 11:20 a.m. | #5
Hi Peter,

On Mon, Jul 16, 2018 at 1:16 PM Peter Rosin <peda@axentia.se> wrote:
> On 2018-07-16 12:37, Geert Uytterhoeven wrote:
> > On Thu, Jul 12, 2018 at 7:49 PM Wolfram Sang
> > <wsa+renesas@sang-engineering.com> wrote:
> >> When we we initialize the pins, make sure it looks like STOP by dividing
> >> the delay into halves. It shouldn't matter because SDA is expected to be
> >> held low by a device, but for super-safety, let's do it.
> >>
> >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >> ---
> >>  drivers/i2c/i2c-core-base.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> >> index 51cbb0c158f2..e57231ccb32a 100644
> >> --- a/drivers/i2c/i2c-core-base.c
> >> +++ b/drivers/i2c/i2c-core-base.c
> >> @@ -191,9 +191,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
> >>                 bri->prepare_recovery(adap);
> >>
> >>         bri->set_scl(adap, scl);
> >> +       ndelay(RECOVERY_NDELAY / 2);
> >
> > Any change someone changes RECOVERY_NDELAY to 1, leading to a
> > zero delay here? Is that an issue?
>
> No!
>
> Above this, there is this line:
>
> #define RECOVERY_NDELAY         5000

I did say "change".... (and the first once should have been "chance" ;-)

Gr{oetje,eeting}s,

                        Geert
Wolfram Sang July 17, 2018, 7:30 a.m. | #6
> > > Any change someone changes RECOVERY_NDELAY to 1, leading to a
> > > zero delay here? Is that an issue?
> >
> > No!
> >
> > Above this, there is this line:
> >
> > #define RECOVERY_NDELAY         5000
> 
> I did say "change".... (and the first once should have been "chance" ;-)

Still no ;) The pulses would be so small that devices won't recognize
them. 1 or 0 as a delay won't make a difference then.
Wolfram Sang July 17, 2018, 8:58 a.m. | #7
> instead of changing this "1" to "scl"? Same-same, but it looks odd
> to use scl as argument to sda (at least without that comment about
> sda following scl that is present inside the loop below).
> 
> At the same time, your version make the code inside the loop the
> same as this initializing code. Oh well, your call...

Yes, I wanted them to look alike. Yet, I agree to your comment about
moving the comment. Will update.

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 51cbb0c158f2..e57231ccb32a 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -191,9 +191,10 @@  int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 		bri->prepare_recovery(adap);
 
 	bri->set_scl(adap, scl);
+	ndelay(RECOVERY_NDELAY / 2);
 	if (bri->set_sda)
-		bri->set_sda(adap, 1);
-	ndelay(RECOVERY_NDELAY);
+		bri->set_sda(adap, scl);
+	ndelay(RECOVERY_NDELAY / 2);
 
 	/*
 	 * By this time SCL is high, as we need to give 9 falling-rising edges