[PATCH] I2C: i2c_bit_add_bus should initialize SDA and SCL lines

[PATCH] I2C: i2c_bit_add_bus should initialize SDA and SCL lines

[Krzysztof Halasa]

Hi,

Another thing: I noticed the i2c_bit_add_bus doesn't set SDA and SCL
lines to a known levels. If the hw driver set them to 1 all is fine
and the first START condition is detected correctly. But if they're
set differently (for example, if both are zero), the START will not
work.

I'm not sure if the following patch isn't an overkill, though, and
if the lack of initialization is a real problem which shows in
practice and not only on my analyzer.

In case you think it's needed:

This patch makes i2c_bit_add_bus() initialize SDA and SCL lines
as required by subsequent START condition.

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -544,6 +544,13 @@ int i2c_bit_add_bus(struct i2c_adapter *
 	adap->timeout = 100;	/* default values, should	*/
 	adap->retries = 3;	/* be replaced by defines	*/
 
+	setsda(bit_adap, 0);	/* may mean START if SCL = 1 */
+	udelay(bit_adap->udelay);
+	setscl(bit_adap, 1);	/* may clock a zero bit in */
+	udelay(bit_adap->udelay);
+	setsda(bit_adap, 1);	/* STOP */
+	udelay(bit_adap->udelay);
+
 	i2c_add_adapter(adap);
 	return 0;
 }

Re: [lm-sensors] [PATCH] I2C: i2c_bit_add_bus should initialize SDA and SCL lines

[Mark M. Hoffman]

Hi Krzysztof:

* Krzysztof Halasa <khc@pm.waw.pl> [2006-06-06 18:58:46 +0200]:
> Another thing: I noticed the i2c_bit_add_bus doesn't set SDA and SCL
> lines to a known levels. If the hw driver set them to 1 all is fine
> and the first START condition is detected correctly. But if they're
> set differently (for example, if both are zero), the START will not
> work.
> 
> I'm not sure if the following patch isn't an overkill, though, and
> if the lack of initialization is a real problem which shows in
> practice and not only on my analyzer.
> 
> In case you think it's needed:
> 
> This patch makes i2c_bit_add_bus() initialize SDA and SCL lines
> as required by subsequent START condition.
> 
> Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>
> 
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -544,6 +544,13 @@ int i2c_bit_add_bus(struct i2c_adapter *
>  	adap->timeout = 100;	/* default values, should	*/
>  	adap->retries = 3;	/* be replaced by defines	*/
>  
> +	setsda(bit_adap, 0);	/* may mean START if SCL = 1 */
> +	udelay(bit_adap->udelay);
> +	setscl(bit_adap, 1);	/* may clock a zero bit in */
> +	udelay(bit_adap->udelay);
> +	setsda(bit_adap, 1);	/* STOP */
> +	udelay(bit_adap->udelay);
> +
>  	i2c_add_adapter(adap);
>  	return 0;
>  }

NACK.  The I2C bus spec says[1]:

	A START condition immediately followed by a STOP condition
	(void message) is an illegal format.

SCL and SDA must be pulled high by hardware.  If a driver inits to
setting them low, that's a bug in the driver.

[1] (page 14, note 5)
http://www.semiconductors.philips.com/acrobat_download/literature/9398/39340011.pdf

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [lm-sensors] [PATCH] I2C: i2c_bit_add_bus should initialize SDA and SCL lines

[Krzysztof Halasa]

Hi,

"Mark M. Hoffman" <mhoffman@lightlink.com> writes:

> SCL and SDA must be pulled high by hardware.  If a driver inits to
> setting them low, that's a bug in the driver.

Thanks for your response.

The question is rather who inits the lines: a) the hw driver,
b) the I2C algorithm driver.

With a) every hw driver has to know how to init them (duplicated
code but there might be positive side).

With b) I2C algorithm driver inits the lines and hw driver
doesn't worry about but it might have some limitations such
as unknown SCL state.

I understand the current case is a) - right?


The other question is _how_ to init the lines. There are 4 possible
hardware initial conditions:

  SCL SDA
a)  0   0 (outputs zeroed by default)
b)  0   1 (uncommon but may be left in this state by previous operations)
c)  1   0 (ditto)
d)  1   1 (I/O lines configured as input by default)

The internal state of devices connected to the bus is potentially
unknown. Some implementations just start with STOP to eliminate
this problem, I don't know what Linux driver is supposed to do.

(Other implementation I know are rather specialized and thus they
know their hardware init state, Linux I2C algorithm handles many
devices with potentially different initial state of hardware lines).


To summarize questions:
- is it the hw driver who has to init the bus
- how to init the bus (depending on init state)
-- 
Krzysztof Halasa

Re: [lm-sensors] [PATCH] I2C: i2c_bit_add_bus should initialize SDA and SCL lines

[Mark M. Hoffman]

Hi Krzysztof:

> "Mark M. Hoffman" <mhoffman@lightlink.com> writes:
> > SCL and SDA must be pulled high by hardware.  If a driver inits to
> > setting them low, that's a bug in the driver.

* Krzysztof Halasa <khc@pm.waw.pl> [2006-06-11 00:27:14 +0200]:
> Thanks for your response.
> 
> The question is rather who inits the lines: a) the hw driver,
> b) the I2C algorithm driver.
> 
> With a) every hw driver has to know how to init them (duplicated
> code but there might be positive side).
> 
> With b) I2C algorithm driver inits the lines and hw driver
> doesn't worry about but it might have some limitations such
> as unknown SCL state.
> 
> I understand the current case is a) - right?

I think it should be, yes.

> The other question is _how_ to init the lines. There are 4 possible
> hardware initial conditions:
> 
>   SCL SDA
> a)  0   0 (outputs zeroed by default)
> b)  0   1 (uncommon but may be left in this state by previous operations)
> c)  1   0 (ditto)
> d)  1   1 (I/O lines configured as input by default)
> 
> The internal state of devices connected to the bus is potentially
> unknown. Some implementations just start with STOP to eliminate
> this problem, I don't know what Linux driver is supposed to do.

If you can read the line state, then yes... I would detect which of
the above four states you're in, and generate a STOP condition for
a, b, and c:

a) setscl(1), setsda(1)
b) setsda(0), setscl(1), setsda(1)
c) setsda(1)

If you can't read the line state... well, that's not actually a proper
I2C bus anyway.  At that point I suggest 'whatever works'.

But even if you need to generate a 'null transfer' (prohibited by spec)
to get reliable operation... that doesn't belong in the algorithm driver.

> (Other implementation I know are rather specialized and thus they
> know their hardware init state, Linux I2C algorithm handles many
> devices with potentially different initial state of hardware lines).
> 
> 
> To summarize questions:
> - is it the hw driver who has to init the bus
> - how to init the bus (depending on init state)

Regards,

-- 
Mark M. Hoffman
mhoffman@lightlink.com

Re: [lm-sensors] [PATCH] I2C: i2c_bit_add_bus should initialize SDA and SCL lines

[Krzysztof Halasa]

Hi,

"Mark M. Hoffman" <mhoffman@lightlink.com> writes:

> If you can't read the line state... well, that's not actually a proper
> I2C bus anyway.

Actually you only have to be able to read SDA state, it might not be
true WRT SCL. I think most implementations use a single master and
a fixed access timing and thus only read SDA (SCL can as well be
a simple 2-state normal output from master instead of open
drain/collector or 3-state).

I'm not sure initializing the bus (in terms of ~ handshaking)
belongs to hw driver and not to the algorithm but such is live.

Thanks.
-- 
Krzysztof Halasa