diff mbox

[1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

Message ID 20171013092705.7038-2-peda@axentia.se (mailing list archive)
State Superseded
Headers show

Commit Message

Peter Rosin Oct. 13, 2017, 9:27 a.m. UTC
With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
is not always capable of avoiding the 25-35 ms timeout as specified by
the SMBUS protocol. This may cause silent corruption of the last bit of
any transfer, e.g. a one is read instead of a zero if the sensor chip
times out. This also affects the eeprom half of the nxp-se97 chip, where
this silent corruption was originally noticed. Other I2C adapters probably
suffer similar issues, e.g. bit-banging comes to mind as risky...

The SMBUS register in the nxp chip is not a standard Jedec register, but
it is not special to the nxp chips either, at least the atmel chips
have the same mechanism. Therefore, do not special case this on the
manufacturer, it is opt-in via the device property anyway.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
 drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

Comments

Guenter Roeck Oct. 13, 2017, 12:51 p.m. UTC | #1
On 10/13/2017 02:27 AM, Peter Rosin wrote:
> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> is not always capable of avoiding the 25-35 ms timeout as specified by
> the SMBUS protocol. This may cause silent corruption of the last bit of
> any transfer, e.g. a one is read instead of a zero if the sensor chip
> times out. This also affects the eeprom half of the nxp-se97 chip, where
> this silent corruption was originally noticed. Other I2C adapters probably
> suffer similar issues, e.g. bit-banging comes to mind as risky...
> 
> The SMBUS register in the nxp chip is not a standard Jedec register, but
> it is not special to the nxp chips either, at least the atmel chips
> have the same mechanism. Therefore, do not special case this on the
> manufacturer, it is opt-in via the device property anyway.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> index 07a250498fbb..f569db58f64a 100644
> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> @@ -34,6 +34,10 @@ Required properties:
>   
>   - reg: I2C address
>   
> +Optional properties:
> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> +			 This is not supported on all chips.
> +
>   Example:
>   
>   temp-sensor@1a {
> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> index 1bf22eff0b08..fd816902fa30 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
>   #define JC42_REG_TEMP		0x05
>   #define JC42_REG_MANID		0x06
>   #define JC42_REG_DEVICEID	0x07
> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
>   
>   /* Status bits in temperature register */
>   #define JC42_ALARM_CRIT_BIT	15
> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
>   #define STM_MANID		0x104a  /* ST Microelectronics */
>   
> +/* SMBUS register */
> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */

Requires bitops.h.

Otherwise looks good, though we'll have to wait for Rob's ack.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck Oct. 13, 2017, 1:50 p.m. UTC | #2
On 10/13/2017 02:27 AM, Peter Rosin wrote:
> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> is not always capable of avoiding the 25-35 ms timeout as specified by
> the SMBUS protocol. This may cause silent corruption of the last bit of
> any transfer, e.g. a one is read instead of a zero if the sensor chip
> times out. This also affects the eeprom half of the nxp-se97 chip, where
> this silent corruption was originally noticed. Other I2C adapters probably
> suffer similar issues, e.g. bit-banging comes to mind as risky...
> 
> The SMBUS register in the nxp chip is not a standard Jedec register, but
> it is not special to the nxp chips either, at least the atmel chips
> have the same mechanism. Therefore, do not special case this on the
> manufacturer, it is opt-in via the device property anyway.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> index 07a250498fbb..f569db58f64a 100644
> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> @@ -34,6 +34,10 @@ Required properties:
>   
>   - reg: I2C address
>   
> +Optional properties:
> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> +			 This is not supported on all chips.
> +
>   Example:
>   
>   temp-sensor@1a {
> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> index 1bf22eff0b08..fd816902fa30 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
>   #define JC42_REG_TEMP		0x05
>   #define JC42_REG_MANID		0x06
>   #define JC42_REG_DEVICEID	0x07
> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
>   
>   /* Status bits in temperature register */
>   #define JC42_ALARM_CRIT_BIT	15
> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
>   #define STM_MANID		0x104a  /* ST Microelectronics */
>   
> +/* SMBUS register */
> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
> +
>   /* Supported chips */
>   
>   /* Analog Devices */
> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   
>   	data->extended = !!(cap & JC42_CAP_RANGE);
>   
> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
> +		int smbus;
> +
> +		/*
> +		 * Not all chips support this register, but from a
> +		 * quick read of various datasheets no chip appears
> +		 * incompatible with the below attempt to disable
> +		 * the timeout. And the whole thing is opt-in...
> +		 */
> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
> +		if (smbus < 0)
> +			return smbus;
> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
> +					     smbus | SMBUS_STMOUT);

Looking into the SE97 datasheet, the bit is only writable if the alarm bits
are not locked. Should we take this into account and unlock the alarm bits
if necessary ?

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Rosin Oct. 13, 2017, 2:26 p.m. UTC | #3
On 2017-10-13 15:50, Guenter Roeck wrote:
> On 10/13/2017 02:27 AM, Peter Rosin wrote:
>> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
>> is not always capable of avoiding the 25-35 ms timeout as specified by
>> the SMBUS protocol. This may cause silent corruption of the last bit of
>> any transfer, e.g. a one is read instead of a zero if the sensor chip
>> times out. This also affects the eeprom half of the nxp-se97 chip, where
>> this silent corruption was originally noticed. Other I2C adapters probably
>> suffer similar issues, e.g. bit-banging comes to mind as risky...
>>
>> The SMBUS register in the nxp chip is not a standard Jedec register, but
>> it is not special to the nxp chips either, at least the atmel chips
>> have the same mechanism. Therefore, do not special case this on the
>> manufacturer, it is opt-in via the device property anyway.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
>> index 07a250498fbb..f569db58f64a 100644
>> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
>> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
>> @@ -34,6 +34,10 @@ Required properties:
>>   
>>   - reg: I2C address
>>   
>> +Optional properties:
>> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
>> +			 This is not supported on all chips.
>> +
>>   Example:
>>   
>>   temp-sensor@1a {
>> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
>> index 1bf22eff0b08..fd816902fa30 100644
>> --- a/drivers/hwmon/jc42.c
>> +++ b/drivers/hwmon/jc42.c
>> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
>>   #define JC42_REG_TEMP		0x05
>>   #define JC42_REG_MANID		0x06
>>   #define JC42_REG_DEVICEID	0x07
>> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
>>   
>>   /* Status bits in temperature register */
>>   #define JC42_ALARM_CRIT_BIT	15
>> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
>>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
>>   #define STM_MANID		0x104a  /* ST Microelectronics */
>>   
>> +/* SMBUS register */
>> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
>> +
>>   /* Supported chips */
>>   
>>   /* Analog Devices */
>> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>   
>>   	data->extended = !!(cap & JC42_CAP_RANGE);
>>   
>> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
>> +		int smbus;
>> +
>> +		/*
>> +		 * Not all chips support this register, but from a
>> +		 * quick read of various datasheets no chip appears
>> +		 * incompatible with the below attempt to disable
>> +		 * the timeout. And the whole thing is opt-in...
>> +		 */
>> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
>> +		if (smbus < 0)
>> +			return smbus;
>> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
>> +					     smbus | SMBUS_STMOUT);
> 
> Looking into the SE97 datasheet, the bit is only writable if the alarm bits
> are not locked. Should we take this into account and unlock the alarm bits
> if necessary ?

Right. And I thought about the case when the timeout was disabled before
probing but with the property not present (perhaps by someone trying things
out, like I have). Should the timeout be re-enabled in that case?
But, someone might have disabled the timeout by some previous arrangement
(e.g. in a boot-loader) but without having this newfangled property in the
device tree. Re-enabling the timeout in that case would break things. Slim
chance for that to be an issue, but perhaps not?

Unlocking the alarm bits is somewhat similar, since it should only be an
issue for warm starts. But the risk of breakage is perhaps not there at
all?

Your call, I can fix thing however you like...

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck Oct. 13, 2017, 8:35 p.m. UTC | #4
On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
> On 2017-10-13 15:50, Guenter Roeck wrote:
> > On 10/13/2017 02:27 AM, Peter Rosin wrote:
> >> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> >> is not always capable of avoiding the 25-35 ms timeout as specified by
> >> the SMBUS protocol. This may cause silent corruption of the last bit of
> >> any transfer, e.g. a one is read instead of a zero if the sensor chip
> >> times out. This also affects the eeprom half of the nxp-se97 chip, where
> >> this silent corruption was originally noticed. Other I2C adapters probably
> >> suffer similar issues, e.g. bit-banging comes to mind as risky...
> >>
> >> The SMBUS register in the nxp chip is not a standard Jedec register, but
> >> it is not special to the nxp chips either, at least the atmel chips
> >> have the same mechanism. Therefore, do not special case this on the
> >> manufacturer, it is opt-in via the device property anyway.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
> >>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
> >>   2 files changed, 24 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> index 07a250498fbb..f569db58f64a 100644
> >> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> @@ -34,6 +34,10 @@ Required properties:
> >>   
> >>   - reg: I2C address
> >>   
> >> +Optional properties:
> >> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> >> +			 This is not supported on all chips.
> >> +
> >>   Example:
> >>   
> >>   temp-sensor@1a {
> >> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> >> index 1bf22eff0b08..fd816902fa30 100644
> >> --- a/drivers/hwmon/jc42.c
> >> +++ b/drivers/hwmon/jc42.c
> >> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
> >>   #define JC42_REG_TEMP		0x05
> >>   #define JC42_REG_MANID		0x06
> >>   #define JC42_REG_DEVICEID	0x07
> >> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
> >>   
> >>   /* Status bits in temperature register */
> >>   #define JC42_ALARM_CRIT_BIT	15
> >> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
> >>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
> >>   #define STM_MANID		0x104a  /* ST Microelectronics */
> >>   
> >> +/* SMBUS register */
> >> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
> >> +
> >>   /* Supported chips */
> >>   
> >>   /* Analog Devices */
> >> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >>   
> >>   	data->extended = !!(cap & JC42_CAP_RANGE);
> >>   
> >> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
> >> +		int smbus;
> >> +
> >> +		/*
> >> +		 * Not all chips support this register, but from a
> >> +		 * quick read of various datasheets no chip appears
> >> +		 * incompatible with the below attempt to disable
> >> +		 * the timeout. And the whole thing is opt-in...
> >> +		 */
> >> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
> >> +		if (smbus < 0)
> >> +			return smbus;
> >> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
> >> +					     smbus | SMBUS_STMOUT);
> > 
> > Looking into the SE97 datasheet, the bit is only writable if the alarm bits
> > are not locked. Should we take this into account and unlock the alarm bits
> > if necessary ?
> 
> Right. And I thought about the case when the timeout was disabled before
> probing but with the property not present (perhaps by someone trying things
> out, like I have). Should the timeout be re-enabled in that case?

No, because the property only states that the timeout should be disabled.
It does not say that it should be _enabled_ if the property is not there.
That would require a different property. A -> B does not imply B -> A.

> But, someone might have disabled the timeout by some previous arrangement
> (e.g. in a boot-loader) but without having this newfangled property in the
> device tree. Re-enabling the timeout in that case would break things. Slim
> chance for that to be an issue, but perhaps not?
> 
> Unlocking the alarm bits is somewhat similar, since it should only be an
> issue for warm starts. But the risk of breakage is perhaps not there at
> all?
> 
We would have to lock the alarm bits again, leaving them in a consistent
state.

> Your call, I can fix thing however you like...
> 

Let's just leave it as-is. If we encounter a problem later we can always
add code to unlock/lock the alarm bits.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck Oct. 13, 2017, 11:44 p.m. UTC | #5
[ resending - looks like my primary email provider ended up on a spam list
  and almost all of my e-mail gets dropped ]

On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
> On 2017-10-13 15:50, Guenter Roeck wrote:
> > On 10/13/2017 02:27 AM, Peter Rosin wrote:
> >> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> >> is not always capable of avoiding the 25-35 ms timeout as specified by
> >> the SMBUS protocol. This may cause silent corruption of the last bit of
> >> any transfer, e.g. a one is read instead of a zero if the sensor chip
> >> times out. This also affects the eeprom half of the nxp-se97 chip, where
> >> this silent corruption was originally noticed. Other I2C adapters probably
> >> suffer similar issues, e.g. bit-banging comes to mind as risky...
> >>
> >> The SMBUS register in the nxp chip is not a standard Jedec register, but
> >> it is not special to the nxp chips either, at least the atmel chips
> >> have the same mechanism. Therefore, do not special case this on the
> >> manufacturer, it is opt-in via the device property anyway.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
> >>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
> >>   2 files changed, 24 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> index 07a250498fbb..f569db58f64a 100644
> >> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> @@ -34,6 +34,10 @@ Required properties:
> >>   
> >>   - reg: I2C address
> >>   
> >> +Optional properties:
> >> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> >> +			 This is not supported on all chips.
> >> +
> >>   Example:
> >>   
> >>   temp-sensor@1a {
> >> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> >> index 1bf22eff0b08..fd816902fa30 100644
> >> --- a/drivers/hwmon/jc42.c
> >> +++ b/drivers/hwmon/jc42.c
> >> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
> >>   #define JC42_REG_TEMP		0x05
> >>   #define JC42_REG_MANID		0x06
> >>   #define JC42_REG_DEVICEID	0x07
> >> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
> >>   
> >>   /* Status bits in temperature register */
> >>   #define JC42_ALARM_CRIT_BIT	15
> >> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
> >>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
> >>   #define STM_MANID		0x104a  /* ST Microelectronics */
> >>   
> >> +/* SMBUS register */
> >> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
> >> +
> >>   /* Supported chips */
> >>   
> >>   /* Analog Devices */
> >> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >>   
> >>   	data->extended = !!(cap & JC42_CAP_RANGE);
> >>   
> >> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
> >> +		int smbus;
> >> +
> >> +		/*
> >> +		 * Not all chips support this register, but from a
> >> +		 * quick read of various datasheets no chip appears
> >> +		 * incompatible with the below attempt to disable
> >> +		 * the timeout. And the whole thing is opt-in...
> >> +		 */
> >> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
> >> +		if (smbus < 0)
> >> +			return smbus;
> >> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
> >> +					     smbus | SMBUS_STMOUT);
> > 
> > Looking into the SE97 datasheet, the bit is only writable if the alarm bits
> > are not locked. Should we take this into account and unlock the alarm bits
> > if necessary ?
> 
> Right. And I thought about the case when the timeout was disabled before
> probing but with the property not present (perhaps by someone trying things
> out, like I have). Should the timeout be re-enabled in that case?

No, because the property only states that the timeout should be disabled.
It does not say that it should be _enabled_ if the property is not there.
That would require a different property. A -> B does not imply B -> A.

> But, someone might have disabled the timeout by some previous arrangement
> (e.g. in a boot-loader) but without having this newfangled property in the
> device tree. Re-enabling the timeout in that case would break things. Slim
> chance for that to be an issue, but perhaps not?
> 
> Unlocking the alarm bits is somewhat similar, since it should only be an
> issue for warm starts. But the risk of breakage is perhaps not there at
> all?
> 
We would have to lock the alarm bits again, leaving them in a consistent
state.

> Your call, I can fix thing however you like...
> 

Let's just leave it as-is. If we encounter a problem later we can always
add code to unlock/lock the alarm bits.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) Oct. 17, 2017, 10:16 p.m. UTC | #6
On Fri, Oct 13, 2017 at 01:35:27PM -0700, Guenter Roeck wrote:
> On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
> > On 2017-10-13 15:50, Guenter Roeck wrote:
> > > On 10/13/2017 02:27 AM, Peter Rosin wrote:
> > >> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> > >> is not always capable of avoiding the 25-35 ms timeout as specified by
> > >> the SMBUS protocol. This may cause silent corruption of the last bit of
> > >> any transfer, e.g. a one is read instead of a zero if the sensor chip
> > >> times out. This also affects the eeprom half of the nxp-se97 chip, where
> > >> this silent corruption was originally noticed. Other I2C adapters probably
> > >> suffer similar issues, e.g. bit-banging comes to mind as risky...
> > >>
> > >> The SMBUS register in the nxp chip is not a standard Jedec register, but
> > >> it is not special to the nxp chips either, at least the atmel chips
> > >> have the same mechanism. Therefore, do not special case this on the
> > >> manufacturer, it is opt-in via the device property anyway.
> > >>
> > >> Signed-off-by: Peter Rosin <peda@axentia.se>
> > >> ---
> > >>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
> > >>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
> > >>   2 files changed, 24 insertions(+)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> > >> index 07a250498fbb..f569db58f64a 100644
> > >> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> > >> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> > >> @@ -34,6 +34,10 @@ Required properties:
> > >>   
> > >>   - reg: I2C address
> > >>   
> > >> +Optional properties:
> > >> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> > >> +			 This is not supported on all chips.

Is this only for jc24 devices or could be any smbus device?

> > >> +
> > >>   Example:
> > >>   
> > >>   temp-sensor@1a {
> > >> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> > >> index 1bf22eff0b08..fd816902fa30 100644
> > >> --- a/drivers/hwmon/jc42.c
> > >> +++ b/drivers/hwmon/jc42.c
> > >> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
> > >>   #define JC42_REG_TEMP		0x05
> > >>   #define JC42_REG_MANID		0x06
> > >>   #define JC42_REG_DEVICEID	0x07
> > >> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
> > >>   
> > >>   /* Status bits in temperature register */
> > >>   #define JC42_ALARM_CRIT_BIT	15
> > >> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
> > >>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
> > >>   #define STM_MANID		0x104a  /* ST Microelectronics */
> > >>   
> > >> +/* SMBUS register */
> > >> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
> > >> +
> > >>   /* Supported chips */
> > >>   
> > >>   /* Analog Devices */
> > >> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > >>   
> > >>   	data->extended = !!(cap & JC42_CAP_RANGE);
> > >>   
> > >> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
> > >> +		int smbus;
> > >> +
> > >> +		/*
> > >> +		 * Not all chips support this register, but from a
> > >> +		 * quick read of various datasheets no chip appears
> > >> +		 * incompatible with the below attempt to disable
> > >> +		 * the timeout. And the whole thing is opt-in...
> > >> +		 */
> > >> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
> > >> +		if (smbus < 0)
> > >> +			return smbus;
> > >> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
> > >> +					     smbus | SMBUS_STMOUT);
> > > 
> > > Looking into the SE97 datasheet, the bit is only writable if the alarm bits
> > > are not locked. Should we take this into account and unlock the alarm bits
> > > if necessary ?
> > 
> > Right. And I thought about the case when the timeout was disabled before
> > probing but with the property not present (perhaps by someone trying things
> > out, like I have). Should the timeout be re-enabled in that case?
> 
> No, because the property only states that the timeout should be disabled.
> It does not say that it should be _enabled_ if the property is not there.
> That would require a different property. A -> B does not imply B -> A.

A not-present/0/1 property is typically used for such cases. Perhaps you 
want that?

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck Oct. 18, 2017, 2:38 a.m. UTC | #7
On 10/17/2017 03:16 PM, Rob Herring wrote:
> On Fri, Oct 13, 2017 at 01:35:27PM -0700, Guenter Roeck wrote:
>> On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
>>> On 2017-10-13 15:50, Guenter Roeck wrote:
>>>> On 10/13/2017 02:27 AM, Peter Rosin wrote:
>>>>> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
>>>>> is not always capable of avoiding the 25-35 ms timeout as specified by
>>>>> the SMBUS protocol. This may cause silent corruption of the last bit of
>>>>> any transfer, e.g. a one is read instead of a zero if the sensor chip
>>>>> times out. This also affects the eeprom half of the nxp-se97 chip, where
>>>>> this silent corruption was originally noticed. Other I2C adapters probably
>>>>> suffer similar issues, e.g. bit-banging comes to mind as risky...
>>>>>
>>>>> The SMBUS register in the nxp chip is not a standard Jedec register, but
>>>>> it is not special to the nxp chips either, at least the atmel chips
>>>>> have the same mechanism. Therefore, do not special case this on the
>>>>> manufacturer, it is opt-in via the device property anyway.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>>>>>    drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>>>>>    2 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>> index 07a250498fbb..f569db58f64a 100644
>>>>> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>> @@ -34,6 +34,10 @@ Required properties:
>>>>>    
>>>>>    - reg: I2C address
>>>>>    
>>>>> +Optional properties:
>>>>> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
>>>>> +			 This is not supported on all chips.
> 
> Is this only for jc24 devices or could be any smbus device?
> 

SMBus timeout is a standard SMBus functionality, so I would say any. It is by
default enabled on an SMBus device (actually it is not just enabled, it is
mandatory). The ability to disable it comes handy if a SMBus chip is connected
to an I2C controller which does not (or not necessarily) follow SMBus rules.

I had seen that problem myself with MAX6697, and STTS751 (and its driver) also
supports it.

>>>>> +
>>>>>    Example:
>>>>>    
>>>>>    temp-sensor@1a {
>>>>> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
>>>>> index 1bf22eff0b08..fd816902fa30 100644
>>>>> --- a/drivers/hwmon/jc42.c
>>>>> +++ b/drivers/hwmon/jc42.c
>>>>> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
>>>>>    #define JC42_REG_TEMP		0x05
>>>>>    #define JC42_REG_MANID		0x06
>>>>>    #define JC42_REG_DEVICEID	0x07
>>>>> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
>>>>>    
>>>>>    /* Status bits in temperature register */
>>>>>    #define JC42_ALARM_CRIT_BIT	15
>>>>> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
>>>>>    #define ONS_MANID		0x1b09  /* ON Semiconductor */
>>>>>    #define STM_MANID		0x104a  /* ST Microelectronics */
>>>>>    
>>>>> +/* SMBUS register */
>>>>> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
>>>>> +
>>>>>    /* Supported chips */
>>>>>    
>>>>>    /* Analog Devices */
>>>>> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>>>>    
>>>>>    	data->extended = !!(cap & JC42_CAP_RANGE);
>>>>>    
>>>>> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
>>>>> +		int smbus;
>>>>> +
>>>>> +		/*
>>>>> +		 * Not all chips support this register, but from a
>>>>> +		 * quick read of various datasheets no chip appears
>>>>> +		 * incompatible with the below attempt to disable
>>>>> +		 * the timeout. And the whole thing is opt-in...
>>>>> +		 */
>>>>> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
>>>>> +		if (smbus < 0)
>>>>> +			return smbus;
>>>>> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
>>>>> +					     smbus | SMBUS_STMOUT);
>>>>
>>>> Looking into the SE97 datasheet, the bit is only writable if the alarm bits
>>>> are not locked. Should we take this into account and unlock the alarm bits
>>>> if necessary ?
>>>
>>> Right. And I thought about the case when the timeout was disabled before
>>> probing but with the property not present (perhaps by someone trying things
>>> out, like I have). Should the timeout be re-enabled in that case?
>>
>> No, because the property only states that the timeout should be disabled.
>> It does not say that it should be _enabled_ if the property is not there.
>> That would require a different property. A -> B does not imply B -> A.
> 
> A not-present/0/1 property is typically used for such cases. Perhaps you
> want that?
> 

I don't want to change behavior if the property is not present. After all,
the timeout may have been disabled by the BIOS/ROMMON (especially in systems
w/o DT support). So far having the boolean flag was never a problem; as
mentioned above, the timeout is by default (and per spec) enabled on SMBus
devices. I would argue that anyone who disabled it must have done so on
purpose (including "trying out things"), and that it should not be DT
responsibility to have a flag along the line of "restore default
configuration".

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Rosin Oct. 26, 2017, 6:44 a.m. UTC | #8
On 2017-10-18 04:38, Guenter Roeck wrote:
> On 10/17/2017 03:16 PM, Rob Herring wrote:
>> On Fri, Oct 13, 2017 at 01:35:27PM -0700, Guenter Roeck wrote:
>>> On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
>>>> On 2017-10-13 15:50, Guenter Roeck wrote:
>>>>> On 10/13/2017 02:27 AM, Peter Rosin wrote:
>>>>>> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
>>>>>> is not always capable of avoiding the 25-35 ms timeout as specified by
>>>>>> the SMBUS protocol. This may cause silent corruption of the last bit of
>>>>>> any transfer, e.g. a one is read instead of a zero if the sensor chip
>>>>>> times out. This also affects the eeprom half of the nxp-se97 chip, where
>>>>>> this silent corruption was originally noticed. Other I2C adapters probably
>>>>>> suffer similar issues, e.g. bit-banging comes to mind as risky...
>>>>>>
>>>>>> The SMBUS register in the nxp chip is not a standard Jedec register, but
>>>>>> it is not special to the nxp chips either, at least the atmel chips
>>>>>> have the same mechanism. Therefore, do not special case this on the
>>>>>> manufacturer, it is opt-in via the device property anyway.
>>>>>>
>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>>> ---
>>>>>>    Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>>>>>>    drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>>>>>>    2 files changed, 24 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>> index 07a250498fbb..f569db58f64a 100644
>>>>>> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>> @@ -34,6 +34,10 @@ Required properties:
>>>>>>    
>>>>>>    - reg: I2C address
>>>>>>    
>>>>>> +Optional properties:
>>>>>> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
>>>>>> +			 This is not supported on all chips.
>>
>> Is this only for jc24 devices or could be any smbus device?
>>
> 
> SMBus timeout is a standard SMBus functionality, so I would say any. It is by
> default enabled on an SMBus device (actually it is not just enabled, it is
> mandatory). The ability to disable it comes handy if a SMBus chip is connected
> to an I2C controller which does not (or not necessarily) follow SMBus rules.
> 
> I had seen that problem myself with MAX6697, and STTS751 (and its driver) also
> supports it.

So, is the approach with an optional smbus-timeout-disable property documented
in .../bindings/hwmon/jc42.txt good-to-go or should it be documented in some
common SMBus client-device file? I don't fine any such beast, so I'm unsure
how to proceed in that case.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck Oct. 26, 2017, 1:45 p.m. UTC | #9
On 10/25/2017 11:44 PM, Peter Rosin wrote:
> On 2017-10-18 04:38, Guenter Roeck wrote:
>> On 10/17/2017 03:16 PM, Rob Herring wrote:
>>> On Fri, Oct 13, 2017 at 01:35:27PM -0700, Guenter Roeck wrote:
>>>> On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
>>>>> On 2017-10-13 15:50, Guenter Roeck wrote:
>>>>>> On 10/13/2017 02:27 AM, Peter Rosin wrote:
>>>>>>> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
>>>>>>> is not always capable of avoiding the 25-35 ms timeout as specified by
>>>>>>> the SMBUS protocol. This may cause silent corruption of the last bit of
>>>>>>> any transfer, e.g. a one is read instead of a zero if the sensor chip
>>>>>>> times out. This also affects the eeprom half of the nxp-se97 chip, where
>>>>>>> this silent corruption was originally noticed. Other I2C adapters probably
>>>>>>> suffer similar issues, e.g. bit-banging comes to mind as risky...
>>>>>>>
>>>>>>> The SMBUS register in the nxp chip is not a standard Jedec register, but
>>>>>>> it is not special to the nxp chips either, at least the atmel chips
>>>>>>> have the same mechanism. Therefore, do not special case this on the
>>>>>>> manufacturer, it is opt-in via the device property anyway.
>>>>>>>
>>>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>>>> ---
>>>>>>>     Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>>>>>>>     drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>>>>>>>     2 files changed, 24 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>>> index 07a250498fbb..f569db58f64a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>>> @@ -34,6 +34,10 @@ Required properties:
>>>>>>>     
>>>>>>>     - reg: I2C address
>>>>>>>     
>>>>>>> +Optional properties:
>>>>>>> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
>>>>>>> +			 This is not supported on all chips.
>>>
>>> Is this only for jc24 devices or could be any smbus device?
>>>
>>
>> SMBus timeout is a standard SMBus functionality, so I would say any. It is by
>> default enabled on an SMBus device (actually it is not just enabled, it is
>> mandatory). The ability to disable it comes handy if a SMBus chip is connected
>> to an I2C controller which does not (or not necessarily) follow SMBus rules.
>>
>> I had seen that problem myself with MAX6697, and STTS751 (and its driver) also
>> supports it.
> 
> So, is the approach with an optional smbus-timeout-disable property documented
> in .../bindings/hwmon/jc42.txt good-to-go or should it be documented in some
> common SMBus client-device file? I don't fine any such beast, so I'm unsure
> how to proceed in that case.
> 

I would suggest .../bindings/hwmon/jc42.txt. Even though the functionality is
supported by various SMBus chips, it is not supported by _every_ SMBus chip.

I also found this:

https://github.com/opennetworklinux/linux/blob/master/3.2.65-1%2Bdeb7u2/patches/driver-adt7470-knob-to-disable-smbus-timeout.patch

which suggests that we should find a common solution (even though that patch
never found its way upstream).

Rob, are you ok with "smbus-timeout-disable" as suggested above ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
index 07a250498fbb..f569db58f64a 100644
--- a/Documentation/devicetree/bindings/hwmon/jc42.txt
+++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
@@ -34,6 +34,10 @@  Required properties:
 
 - reg: I2C address
 
+Optional properties:
+- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
+			 This is not supported on all chips.
+
 Example:
 
 temp-sensor@1a {
diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 1bf22eff0b08..fd816902fa30 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -45,6 +45,7 @@  static const unsigned short normal_i2c[] = {
 #define JC42_REG_TEMP		0x05
 #define JC42_REG_MANID		0x06
 #define JC42_REG_DEVICEID	0x07
+#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
 
 /* Status bits in temperature register */
 #define JC42_ALARM_CRIT_BIT	15
@@ -73,6 +74,9 @@  static const unsigned short normal_i2c[] = {
 #define ONS_MANID		0x1b09  /* ON Semiconductor */
 #define STM_MANID		0x104a  /* ST Microelectronics */
 
+/* SMBUS register */
+#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
+
 /* Supported chips */
 
 /* Analog Devices */
@@ -476,6 +480,22 @@  static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	data->extended = !!(cap & JC42_CAP_RANGE);
 
+	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
+		int smbus;
+
+		/*
+		 * Not all chips support this register, but from a
+		 * quick read of various datasheets no chip appears
+		 * incompatible with the below attempt to disable
+		 * the timeout. And the whole thing is opt-in...
+		 */
+		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
+		if (smbus < 0)
+			return smbus;
+		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
+					     smbus | SMBUS_STMOUT);
+	}
+
 	config = i2c_smbus_read_word_swapped(client, JC42_REG_CONFIG);
 	if (config < 0)
 		return config;