| Message ID | 20180712174919.14447-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: recovery: make pin init look like STOP | expand |
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
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 >
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
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 >
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
> > > 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.
> 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.
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
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(-)