[lm-sensors] PATCH- i2c-iop3xx platform driver avoid addressing self

[lm-sensors] PATCH- i2c-iop3xx platform driver avoid addressing self

[Peter Milne]

Avoid addressing self when sending a Master address. Follows instruction 
in Intel 80331/80321 manuals.
Ignoring this worked previously on 80321, but causes a hang on i2cdetect 
on 80331

Signed-off-by: Peter Milne <peter.milne at d-tacq.com>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-iop3xx.patch
Type: text/x-patch
Size: 1181 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060616/453289ad/attachment.bin 

[lm-sensors] PATCH- i2c-iop3xx platform driver avoid addressing

[Jean Delvare]

Hi Peter,

> Avoid addressing self when sending a Master address. Follows instruction 
> in Intel 80331/80321 manuals.
> Ignoring this worked previously on 80321, but causes a hang on i2cdetect 
> on 80331

Can't you simply disable the slave address? You don't seem to use it
anyway.

BTW, you must be using an old version of i2cdetect, as the slave address
appears to be forced to 0x02 in the i2c-iop3xx driver (not a smart
choice...) and i2cdetect does no more scan this address by default.

> diff -uprN -X dontdiff linux-2.6.16.20/drivers/i2c/busses/i2c-iop3xx.h linux-2.6.16.20-iq80331/drivers/i2c/busses/i2c-iop3xx.h
> --- linux-2.6.16.20/drivers/i2c/busses/i2c-iop3xx.h	2006-06-05 18:18:23.000000000 +0100
> +++ linux-2.6.16.20-iq80331/drivers/i2c/busses/i2c-iop3xx.h	2006-06-16 12:20:07.000000000 +0100
> @@ -85,6 +85,7 @@
>  #define I2C_ERR			321
>  #define I2C_ERR_BERR		(I2C_ERR+0)
>  #define I2C_ERR_ALD		(I2C_ERR+1)
> +#define I2C_ERR_WRITEMYSAR      (I2C_ERR+10)

Please use tabs, not spaces, for alignment.

-- 
Jean Delvare

[lm-sensors] PATCH- i2c-iop3xx platform driver avoid addressing

[Peter Milne]

Hi Jean

Thanks very much for reviewing this patch, you have given me some useful 
insights.
Please consider the revised patch which addresses the issues you raise.

Jean Delvare wrote:

>Hi Peter,
>  
>
>>Avoid addressing self when sending a Master address. Follows instruction 
>>in Intel 80331/80321 manuals.
>>Ignoring this worked previously on 80321, but causes a hang on i2cdetect 
>>on 80331
>>    
>>
>
>Can't you simply disable the slave address? You don't seem to use it
>anyway.
>  
>
Unfortunately there doesn't seem to be any way to disable the slave address.

>BTW, you must be using an old version of i2cdetect, as the slave address
>appears to be forced to 0x02 in the i2c-iop3xx driver (not a smart
>choice...) and i2cdetect does no more scan this address by default.
>  
>
OK, new patch leaves the slave address at default 0.
I note that i2cdetect now scans from 0x3, but I'd still prefer to 
prevent writes to the device's own address since this wedges the unit.


>>+#define I2C_ERR_WRITEMYSAR      (I2C_ERR+10)
>>    
>>
>
>Please use tabs, not spaces, for alignment.
>  
>
After forcing emacs to cooperate, tabs it is.

Thanks


Peter.

-- 
Peter Milne			Peter.Milne at d-tacq.com
D-TACQ Solutions Ltd		www.d-tacq.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-iop3xx.2.patch
Type: text/x-patch
Size: 2430 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060618/4d1092ad/attachment.bin 

[lm-sensors] PATCH- i2c-iop3xx platform driver avoid addressing

[Jean Delvare]

Hi Peter,

> Thanks very much for reviewing this patch, you have given me some useful 
> insights.
> Please consider the revised patch which addresses the issues you raise.

My pleasure, here we go:

> > BTW, you must be using an old version of i2cdetect, as the slave address
> > appears to be forced to 0x02 in the i2c-iop3xx driver (not a smart
> > choice...) and i2cdetect does no more scan this address by default.
>
> OK, new patch leaves the slave address at default 0.

Huh, this might be worst :/ 0 is the "general call address", think of
it as a broadcast address. 2 is reserved for "CBUS compatibility"
(whatever it is.)

I would suggest 8, which is the "SMBus host" address in the SMBus spec
- unless you have to deal with multi-master busses where this address
might be already taken by another master.

Note that I have no strong opinion on this either, as long as nothing
breaks and you are happy...

> I note that i2cdetect now scans from 0x3, but I'd still prefer to 
> prevent writes to the device's own address since this wedges the unit.

Sure, I have no problem with this.

> @@ -243,12 +240,19 @@ iop3xx_i2c_wait_idle(struct i2c_algo_iop
>  
>  static int 
>  iop3xx_i2c_send_target_addr(struct i2c_algo_iop3xx_data *iop3xx_adap, 
> -				struct i2c_msg* msg)
> +			    struct i2c_msg* msg)
>  {

Please don't mix cleanups with real changes!

>  	unsigned long cr = __raw_readl(iop3xx_adap->ioaddr + CR_OFFSET);
>  	int status;
>  	int rc;
>  
> +	/* avoid writing to my slave address (hangs on 80331),
> +	 * forbidden in Intel developer manual
> +	 */
> +	if (msg->addr = MYSAR) {
> +		return -I2C_ERR_WRITEMYSAR;
> +	}
> +	

How will the address be reported by i2cdetect? As if there was no chip
there, I guess (XX)? I'm a bit worried about this, as someone might
then want to use the address for something else without realizing that
the address is already in use. Would it be possible to make it so that
the address would be reported as busy (UU)? I'm not too sure myself if
the i2c-core currently allows this, but I'd like you to investigate in
that direction as it might avoid trouble in the future. I guess the
only way right now would be to instanciante a fake client at that
address.

Thanks,
-- 
Jean Delvare

[lm-sensors] PATCH- i2c-iop3xx platform driver avoid addressing

[Peter Milne]

Hi Jean

Jean Delvare wrote:
<snip>

>>OK, new patch leaves the slave address at default 0.
>>    
>>
>
>Huh, this might be worst :/ 0 is the "general call address", think of
>it as a broadcast address. 2 is reserved for "CBUS compatibility"
>(whatever it is.)
>  
>
Sorry, I should have explained before, there is a bit set in the control 
register: IOP3XX_ICR_GCD - this according to the manual "disables i2c 
unit response to general call messages as a slave".

So it really shouldn't respond to an external master making a General 
Call, the issue I'm trying to avoid is the problem of addressing itself.

>I would suggest 8, which is the "SMBus host" address in the SMBus spec
>- unless you have to deal with multi-master busses where this address
>might be already taken by another master.
>
>Note that I have no strong opinion on this either, as long as nothing
>breaks and you are happy...
>  
>
Prefer to stick with zero, thanks, it works. I don't think anyone uses 
this device as a slave, but if they did, it would be easy to configure a 
valid address.

>  
>
>>I note that i2cdetect now scans from 0x3, but I'd still prefer to 
>>prevent writes to the device's own address since this wedges the unit.
>>    
>>
>
>Sure, I have no problem with this.
>
>  
>
>>@@ -243,12 +240,19 @@ iop3xx_i2c_wait_idle(struct i2c_algo_iop
>> 
>> static int 
>> iop3xx_i2c_send_target_addr(struct i2c_algo_iop3xx_data *iop3xx_adap, 
>>-				struct i2c_msg* msg)
>>+			    struct i2c_msg* msg)
>> {
>>    
>>
>
>Please don't mix cleanups with real changes!
>  
>
fixed in patch -

>  
>
>> 	unsigned long cr = __raw_readl(iop3xx_adap->ioaddr + CR_OFFSET);
>> 	int status;
>> 	int rc;
>> 
>>+	/* avoid writing to my slave address (hangs on 80331),
>>+	 * forbidden in Intel developer manual
>>+	 */
>>+	if (msg->addr = MYSAR) {
>>+		return -I2C_ERR_WRITEMYSAR;
>>+	}
>>+	
>>    
>>
>
>How will the address be reported by i2cdetect? As if there was no chip
>there, I guess (XX)? 
>
At zero, comes out as "XX" on the "old" i2cdetect, skipped in the new one.

>I'm a bit worried about this, as someone might
>then want to use the address for something else without realizing that
>the address is already in use. Would it be possible to make it so that
>the address would be reported as busy (UU)? I'm not too sure myself if
>the i2c-core currently allows this, but I'd like you to investigate in
>that direction as it might avoid trouble in the future. I guess the
>only way right now would be to instanciante a fake client at that
>address.
>  
>
The routine now returns -EBUSY, but i2cdetect still reports "XX". When I 
address it with another low level utility, it reports:
"Device or resource busy" - which is fair enough, the device is busy 
being a master so it cannot simultaneously be a slave ...

Maybe Intel knew what they were doing when they set 0 as the default 
slave address.
Because nobody is going to choose that as a real slave address, are they 
:-).

So, are we there with this patch now?

Thanks


Peter.

-- 
Peter Milne			Peter.Milne at d-tacq.com
D-TACQ Solutions Ltd		www.d-tacq.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: i2c-iop3xx.3.patch
Type: text/x-patch
Size: 2071 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060618/078c5765/attachment.bin 

[lm-sensors] PATCH- i2c-iop3xx platform driver avoid addressing

[Jean Delvare]

Hi Peter,

> > How will the address be reported by i2cdetect? As if there was no chip
> > there, I guess (XX)? 
>
> At zero, comes out as "XX" on the "old" i2cdetect, skipped in the new one.

Just FYI, you can restore the old behavior of scanning all addresses
with the -a flag.

> > I'm a bit worried about this, as someone might
> > then want to use the address for something else without realizing that
> > the address is already in use. Would it be possible to make it so that
> > the address would be reported as busy (UU)? I'm not too sure myself if
> > the i2c-core currently allows this, but I'd like you to investigate in
> > that direction as it might avoid trouble in the future. I guess the
> > only way right now would be to instanciante a fake client at that
> > address.
>
> The routine now returns -EBUSY, but i2cdetect still reports "XX". When I 
> address it with another low level utility, it reports:
> "Device or resource busy" - which is fair enough, the device is busy 
> being a master so it cannot simultaneously be a slave ...

Indeed, the business of an I2C address for the i2c core is solely based
on the fact that a client claimed that address. The exact error value
returned by the transfer function is merely ignored as far as I can see.

> Maybe Intel knew what they were doing when they set 0 as the default 
> slave address.
> Because nobody is going to choose that as a real slave address, are they 
> :-).

Seems so.

> So, are we there with this patch now?

I'm happy with it, except for the lack of heading comment and
signed-off-by line. But I assumed that the ones from the original patch
still applied to this new one, and carried them over :)

-- 
Jean Delvare

[lm-sensors] PATCH- i2c-iop3xx platform driver avoid addressing

[Peter Milne]

Hi Jean

Thanks very much for helping with the patch, I really appreciate your 
review, in particular for bringing up issues that weren't immediately 
obvious.

Cheers

Peter.


-- 
Peter Milne			Peter.Milne at d-tacq.com
D-TACQ Solutions Ltd		www.d-tacq.com