[PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Jean Delvare]

The I2C specification explicitly describes both SDA and SCL as
bidirectional lines. An I2C master with a read-only SCL is thus not
compliant. If a slow slave stretches the clock, errors will happen,
so the bus can't be considered as reliable.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-bit.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

--- linux-2.6.37-rc5.orig/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 11:02:38.000000000 +0100
+++ linux-2.6.37-rc5/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 11:03:23.000000000 +0100
@@ -604,9 +604,10 @@ static int __i2c_bit_add_bus(struct i2c_
 			     int (*add_adapter)(struct i2c_adapter *))
 {
 	struct i2c_algo_bit_data *bit_adap = adap->algo_data;
+	int ret;
 
 	if (bit_test) {
-		int ret = test_bus(bit_adap, adap->name);
+		ret = test_bus(bit_adap, adap->name);
 		if (ret < 0)
 			return -ENODEV;
 	}
@@ -615,7 +616,16 @@ static int __i2c_bit_add_bus(struct i2c_
 	adap->algo = &i2c_bit_algo;
 	adap->retries = 3;
 
-	return add_adapter(adap);
+	ret = add_adapter(adap);
+	if (ret < 0)
+		return ret;
+
+	/* Complain if SCL can't be read */
+	if (bit_adap->getscl == NULL) {
+		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
+		dev_warn(&adap->dev, "Bus may be unreliable\n");
+	}
+	return 0;
 }
 
 int i2c_bit_add_bus(struct i2c_adapter *adap)


-- 
Jean Delvare

Re: [PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Ben Dooks]

On Tue, Dec 07, 2010 at 11:07:05AM +0100, Jean Delvare wrote:
> The I2C specification explicitly describes both SDA and SCL as
> bidirectional lines. An I2C master with a read-only SCL is thus not
> compliant. If a slow slave stretches the clock, errors will happen,
> so the bus can't be considered as reliable.
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> --- linux-2.6.37-rc5.orig/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 11:02:38.000000000 +0100
> +++ linux-2.6.37-rc5/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 11:03:23.000000000 +0100
> @@ -604,9 +604,10 @@ static int __i2c_bit_add_bus(struct i2c_
>  			     int (*add_adapter)(struct i2c_adapter *))
>  {
>  	struct i2c_algo_bit_data *bit_adap = adap->algo_data;
> +	int ret;
>  
>  	if (bit_test) {
> -		int ret = test_bus(bit_adap, adap->name);
> +		ret = test_bus(bit_adap, adap->name);
>  		if (ret < 0)
>  			return -ENODEV;
>  	}
> @@ -615,7 +616,16 @@ static int __i2c_bit_add_bus(struct i2c_
>  	adap->algo = &i2c_bit_algo;
>  	adap->retries = 3;
>  
> -	return add_adapter(adap);
> +	ret = add_adapter(adap);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Complain if SCL can't be read */
> +	if (bit_adap->getscl == NULL) {
> +		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");

I'll have a think about wording. I might be able to improve it.

As a note, I always prefer cannot to can't. 

> +		dev_warn(&adap->dev, "Bus may be unreliable\n");

if there are any other warnings, should we print just one unreliable warning
at the end of the scan?

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Mark Brown]

On Tue, Dec 07, 2010 at 11:07:05AM +0100, Jean Delvare wrote:
> The I2C specification explicitly describes both SDA and SCL as
> bidirectional lines. An I2C master with a read-only SCL is thus not
> compliant. If a slow slave stretches the clock, errors will happen,
> so the bus can't be considered as reliable.
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

It'd be good to also flag this via sysfs so that it's more readily
visible to things like diagnostic tools.

Re: [PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Ben Dooks]

On Tue, Dec 07, 2010 at 12:10:10PM +0000, Mark Brown wrote:
> On Tue, Dec 07, 2010 at 11:07:05AM +0100, Jean Delvare wrote:
> > The I2C specification explicitly describes both SDA and SCL as
> > bidirectional lines. An I2C master with a read-only SCL is thus not
> > compliant. If a slow slave stretches the clock, errors will happen,
> > so the bus can't be considered as reliable.
> > 
> > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> 
> It'd be good to also flag this via sysfs so that it's more readily
> visible to things like diagnostic tools.

Could be, I wonder how many other hardware based adapters just don't
do this. Quite a few devices i've seen don't use it as they're generally
fast enough, or implement some other status polling system.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Jean Delvare]

Hi Ben,

On Tue, 7 Dec 2010 11:53:25 +0000, Ben Dooks wrote:
> On Tue, Dec 07, 2010 at 11:07:05AM +0100, Jean Delvare wrote:
> > The I2C specification explicitly describes both SDA and SCL as
> > bidirectional lines. An I2C master with a read-only SCL is thus not
> > compliant. If a slow slave stretches the clock, errors will happen,
> > so the bus can't be considered as reliable.
> > 
> > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > ---
> >  drivers/i2c/algos/i2c-algo-bit.c |   14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > --- linux-2.6.37-rc5.orig/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 11:02:38.000000000 +0100
> > +++ linux-2.6.37-rc5/drivers/i2c/algos/i2c-algo-bit.c	2010-12-07 11:03:23.000000000 +0100
> > @@ -604,9 +604,10 @@ static int __i2c_bit_add_bus(struct i2c_
> >  			     int (*add_adapter)(struct i2c_adapter *))
> >  {
> >  	struct i2c_algo_bit_data *bit_adap = adap->algo_data;
> > +	int ret;
> >  
> >  	if (bit_test) {
> > -		int ret = test_bus(bit_adap, adap->name);
> > +		ret = test_bus(bit_adap, adap->name);
> >  		if (ret < 0)
> >  			return -ENODEV;
> >  	}
> > @@ -615,7 +616,16 @@ static int __i2c_bit_add_bus(struct i2c_
> >  	adap->algo = &i2c_bit_algo;
> >  	adap->retries = 3;
> >  
> > -	return add_adapter(adap);
> > +	ret = add_adapter(adap);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Complain if SCL can't be read */
> > +	if (bit_adap->getscl == NULL) {
> > +		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
> 
> I'll have a think about wording. I might be able to improve it.
> 
> As a note, I always prefer cannot to can't. 

And I thought you were a busy developer ;)

There is no trend one way or another, neither in the kernel tree nor
even in drivers/i2c specifically.

> > +		dev_warn(&adap->dev, "Bus may be unreliable\n");
> 
> if there are any other warnings, should we print just one unreliable warning
> at the end of the scan?

I'm not sure what "scan" you refer to, and in all honesty I'm not sure
I get your point at all.

-- 
Jean Delvare

Re: [PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Jean Delvare]

Hi Marc, Ben,

On Tue, 7 Dec 2010 12:30:11 +0000, Ben Dooks wrote:
> On Tue, Dec 07, 2010 at 12:10:10PM +0000, Mark Brown wrote:
> > On Tue, Dec 07, 2010 at 11:07:05AM +0100, Jean Delvare wrote:
> > > The I2C specification explicitly describes both SDA and SCL as
> > > bidirectional lines. An I2C master with a read-only SCL is thus not
> > > compliant. If a slow slave stretches the clock, errors will happen,
> > > so the bus can't be considered as reliable.
> > > 
> > > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > 
> > It'd be good to also flag this via sysfs so that it's more readily
> > visible to things like diagnostic tools.

In what form do you expect this, and what do you expect diagnostic
tools to do about it?

I can't think of anything straightforward and useful. We could abuse
one functionality flag for unreliable masters, but then this is a
boolean flag, with no way to report the actual problem to the customer.
And write-only SCL is only one thing they can do wrongly.

> Could be, I wonder how many other hardware based adapters just don't
> do this. Quite a few devices i've seen don't use it as they're generally
> fast enough, or implement some other status polling system.

I don't know either, and I didn't mean to provide a general solution to
this problem as I have no clue so far that a general problem exists.
This patch is in response to the thread started by Matthias Zacharias
where we were troubleshooting his I2C bus problems. I just wanted to
make sure that the same problem doesn't happen again, i.e. developers
don't set getscl to NULL in the hope that it will help in any way.

I don't know of that many pieces of hardware driven by i2c-algo-bit and
not having a bidirectional SCL line. In fact I only know of the
i2c-parport, i2c-parport-light and i2c-gpio drivers doing this, and
then again, only for some of the supported devices. So I'm not sure
there is much point in spending a lot of time on this.

Or, to make it clearer, you can spend as much time as _you_ want on
this, but I won't. I'm busy enough as is.

-- 
Jean Delvare

Re: [PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Mark Brown]

On Tue, Dec 07, 2010 at 04:29:33PM +0100, Jean Delvare wrote:
> Hi Marc, Ben,

Ahem.

> > On Tue, Dec 07, 2010 at 12:10:10PM +0000, Mark Brown wrote:

> > > It'd be good to also flag this via sysfs so that it's more readily
> > > visible to things like diagnostic tools.

> In what form do you expect this, and what do you expect diagnostic

A sysfs file that got created if the issue is present, or a warnings file
that read back a set of values as a bitmask.

> tools to do about it?

Report it - it'd increase the chances of us finding out about the error.
A message in dmesg will often just be lost in the noise, and may be long
gone before the users tarts doing diagnostics.

Re: [PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Ben Dooks]

On Tue, Dec 07, 2010 at 04:07:39PM +0100, Jean Delvare wrote:
> Hi Ben,
> 
> On Tue, 7 Dec 2010 11:53:25 +0000, Ben Dooks wrote:
> > On Tue, Dec 07, 2010 at 11:07:05AM +0100, Jean Delvare wrote:
> > > The I2C specification explicitly describes both SDA and SCL as
> > > bidirectional lines. An I2C master with a read-only SCL is thus not
> > > compliant. If a slow slave stretches the clock, errors will happen,
> > > so the bus can't be considered as reliable.

> > > +	if (bit_adap->getscl == NULL) {
> > > +		dev_warn(&adap->dev, "Not I2C compliant: can't read SCL\n");
> > 
> > I'll have a think about wording. I might be able to improve it.
> > 
> > As a note, I always prefer cannot to can't. 
> 
> And I thought you were a busy developer ;)

I've my reasons for this, blame my parents.
 
> There is no trend one way or another, neither in the kernel tree nor
> even in drivers/i2c specifically.

no, just my preference.
 
> > > +		dev_warn(&adap->dev, "Bus may be unreliable\n");
> > 
> > if there are any other warnings, should we print just one unreliable warning
> > at the end of the scan?
> 
> I'm not sure what "scan" you refer to, and in all honesty I'm not sure
> I get your point at all.

ok, looking at the code it isn't important.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH 2/2] i2c-algo-bit: Complain about masters which can't read SCL

[Ben Dooks]

On Tue, Dec 07, 2010 at 04:29:33PM +0100, Jean Delvare wrote:
> Hi Marc, Ben,
> 
> On Tue, 7 Dec 2010 12:30:11 +0000, Ben Dooks wrote:
> > On Tue, Dec 07, 2010 at 12:10:10PM +0000, Mark Brown wrote:
> > > On Tue, Dec 07, 2010 at 11:07:05AM +0100, Jean Delvare wrote:
> > > > The I2C specification explicitly describes both SDA and SCL as
> > > > bidirectional lines. An I2C master with a read-only SCL is thus not
> > > > compliant. If a slow slave stretches the clock, errors will happen,
> > > > so the bus can't be considered as reliable.
> > > > 
> > > > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > > 
> > > It'd be good to also flag this via sysfs so that it's more readily
> > > visible to things like diagnostic tools.
> 
> In what form do you expect this, and what do you expect diagnostic
> tools to do about it?
> 
> I can't think of anything straightforward and useful. We could abuse
> one functionality flag for unreliable masters, but then this is a
> boolean flag, with no way to report the actual problem to the customer.
> And write-only SCL is only one thing they can do wrongly.

Hmm, something to think about.
 
> > Could be, I wonder how many other hardware based adapters just don't
> > do this. Quite a few devices i've seen don't use it as they're generally
> > fast enough, or implement some other status polling system.
> 
> I don't know either, and I didn't mean to provide a general solution to
> this problem as I have no clue so far that a general problem exists.
> This patch is in response to the thread started by Matthias Zacharias
> where we were troubleshooting his I2C bus problems. I just wanted to
> make sure that the same problem doesn't happen again, i.e. developers
> don't set getscl to NULL in the hope that it will help in any way.

yes, sounds good.
 
> I don't know of that many pieces of hardware driven by i2c-algo-bit and
> not having a bidirectional SCL line. In fact I only know of the
> i2c-parport, i2c-parport-light and i2c-gpio drivers doing this, and
> then again, only for some of the supported devices. So I'm not sure
> there is much point in spending a lot of time on this.

I was talking all hardware, not just ones integrated with this specific
driver. Not to worry, this is just rambling on.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.