[PATCH] hwmon: adt7411: add rev5 workaround

[PATCH] hwmon: adt7411: add rev5 workaround

[Michael Walle]

Bit 3 in the CFG1 register has to be 1 to make AIN3 work properly since
silicon revision 5. Although otherwise stated in the datasheet, the default
value is not 1. Set it to 1 to make AIN3 work.

Signed-off-by: Michael Walle <michael@walle.cc>
Cc: stable@vger.kernel.org
---

There was already submission back in 2010.
  https://www.spinics.net/lists/lm-sensors/msg29973.html

I don't want to take the ownership, but there wasn't a reaction of the
original author. Anyway, I want to answer the raised questions.

> And what is the problem with this? And what's your hardware?
ADT7411 silicon rev5 either forgot to default this bit to 1 or any earlier
revision didn't care about it. But with rev5, this bit has to be set
otherwise the AIN3 input returns garbage.

> [two uses of adt7411_modify_bit()]
>
> This is inefficient: you read and write the same register twice in a
> row. It would be much better to read it once, modify all bits and write
> it once.
This patch does it the same way. IMHO the small size of the change
justifies the slight inefficiency in the one time probe. The other way you
have to take the lock, add extra code for unlock in error cases etc. But of
course its up to you to decide. I'm happy to provide another patch with
read, modify both bits and write back the value.

 drivers/hwmon/adt7411.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
index 827c037..5ed9b11 100644
--- a/drivers/hwmon/adt7411.c
+++ b/drivers/hwmon/adt7411.c
@@ -30,6 +30,7 @@
 
 #define ADT7411_REG_CFG1			0x18
 #define ADT7411_CFG1_START_MONITOR		(1 << 0)
+#define ADT7411_CFG1_RESERVED_BIT3		(1 << 3)
 
 #define ADT7411_REG_CFG2			0x19
 #define ADT7411_CFG2_DISABLE_AVG		(1 << 5)
@@ -296,6 +297,16 @@ static int adt7411_probe(struct i2c_client *client,
 	mutex_init(&data->device_lock);
 	mutex_init(&data->update_lock);
 
+	/*
+	 * Silicon rev5 workaround, bit 3 has to be set, but unlike mentioned
+	 * in the datasheet, this is not set by default. If this bit is not
+	 * set, AIN3 won't work at all.
+	 */
+	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
+				 ADT7411_CFG1_RESERVED_BIT3, 1);
+	if (ret < 0)
+		return ret;
+
 	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
 				 ADT7411_CFG1_START_MONITOR, 1);
 	if (ret < 0)
-- 
2.1.4

Re: [PATCH] hwmon: adt7411: add rev5 workaround

[Guenter Roeck]

On Thu, Jul 14, 2016 at 01:52:07PM +0200, Michael Walle wrote:
> Bit 3 in the CFG1 register has to be 1 to make AIN3 work properly since
> silicon revision 5. Although otherwise stated in the datasheet, the default
> value is not 1. Set it to 1 to make AIN3 work.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Cc: stable@vger.kernel.org
> ---
> 
> There was already submission back in 2010.
>   https://www.spinics.net/lists/lm-sensors/msg29973.html
> 
> I don't want to take the ownership, but there wasn't a reaction of the
> original author. Anyway, I want to answer the raised questions.
> 
> > And what is the problem with this? And what's your hardware?
> ADT7411 silicon rev5 either forgot to default this bit to 1 or any earlier
> revision didn't care about it. But with rev5, this bit has to be set
> otherwise the AIN3 input returns garbage.
> 
> > [two uses of adt7411_modify_bit()]
> >
> > This is inefficient: you read and write the same register twice in a
> > row. It would be much better to read it once, modify all bits and write
> > it once.
> This patch does it the same way. IMHO the small size of the change
> justifies the slight inefficiency in the one time probe. The other way you
> have to take the lock, add extra code for unlock in error cases etc. But of
> course its up to you to decide. I'm happy to provide another patch with
> read, modify both bits and write back the value.
> 
>  drivers/hwmon/adt7411.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
> index 827c037..5ed9b11 100644
> --- a/drivers/hwmon/adt7411.c
> +++ b/drivers/hwmon/adt7411.c
> @@ -30,6 +30,7 @@
>  
>  #define ADT7411_REG_CFG1			0x18
>  #define ADT7411_CFG1_START_MONITOR		(1 << 0)
> +#define ADT7411_CFG1_RESERVED_BIT3		(1 << 3)
>  
>  #define ADT7411_REG_CFG2			0x19
>  #define ADT7411_CFG2_DISABLE_AVG		(1 << 5)
> @@ -296,6 +297,16 @@ static int adt7411_probe(struct i2c_client *client,
>  	mutex_init(&data->device_lock);
>  	mutex_init(&data->update_lock);
>  
> +	/*
> +	 * Silicon rev5 workaround, bit 3 has to be set, but unlike mentioned
> +	 * in the datasheet, this is not set by default. If this bit is not
> +	 * set, AIN3 won't work at all.

The datasheet actually says "Write only 1 to this bit", period. While it
_does_ say that its default is 1, it doesn't say that it is read-only
(meaning any entity, such as the rommon, the BIOS, or a user having fun,
may have set the bit to 0 on any chip revision).

> +	 */
> +	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
> +				 ADT7411_CFG1_RESERVED_BIT3, 1);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
>  				 ADT7411_CFG1_START_MONITOR, 1);

	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
		ADT7411_CFG1_RESERVED_BIT3 | ADT7411_CFG1_START_MONITOR, 1);

I don't really see why that would require a lock, much less an extra lock.

Guenter

Re: [PATCH] hwmon: adt7411: add rev5 workaround

[Michael Walle]

Am 2016-07-14 22:56, schrieb Guenter Roeck:
> On Thu, Jul 14, 2016 at 01:52:07PM +0200, Michael Walle wrote:
>> Bit 3 in the CFG1 register has to be 1 to make AIN3 work properly 
>> since
>> silicon revision 5. Although otherwise stated in the datasheet, the 
>> default
>> value is not 1. Set it to 1 to make AIN3 work.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> Cc: stable@vger.kernel.org
>> ---
>> 
>> There was already submission back in 2010.
>>   https://www.spinics.net/lists/lm-sensors/msg29973.html
>> 
>> I don't want to take the ownership, but there wasn't a reaction of the
>> original author. Anyway, I want to answer the raised questions.
>> 
>> > And what is the problem with this? And what's your hardware?
>> ADT7411 silicon rev5 either forgot to default this bit to 1 or any 
>> earlier
>> revision didn't care about it. But with rev5, this bit has to be set
>> otherwise the AIN3 input returns garbage.
>> 
>> > [two uses of adt7411_modify_bit()]
>> >
>> > This is inefficient: you read and write the same register twice in a
>> > row. It would be much better to read it once, modify all bits and write
>> > it once.
>> This patch does it the same way. IMHO the small size of the change
>> justifies the slight inefficiency in the one time probe. The other way 
>> you
>> have to take the lock, add extra code for unlock in error cases etc. 
>> But of
>> course its up to you to decide. I'm happy to provide another patch 
>> with
>> read, modify both bits and write back the value.
>> 
>>  drivers/hwmon/adt7411.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
>> index 827c037..5ed9b11 100644
>> --- a/drivers/hwmon/adt7411.c
>> +++ b/drivers/hwmon/adt7411.c
>> @@ -30,6 +30,7 @@
>> 
>>  #define ADT7411_REG_CFG1			0x18
>>  #define ADT7411_CFG1_START_MONITOR		(1 << 0)
>> +#define ADT7411_CFG1_RESERVED_BIT3		(1 << 3)
>> 
>>  #define ADT7411_REG_CFG2			0x19
>>  #define ADT7411_CFG2_DISABLE_AVG		(1 << 5)
>> @@ -296,6 +297,16 @@ static int adt7411_probe(struct i2c_client 
>> *client,
>>  	mutex_init(&data->device_lock);
>>  	mutex_init(&data->update_lock);
>> 
>> +	/*
>> +	 * Silicon rev5 workaround, bit 3 has to be set, but unlike 
>> mentioned
>> +	 * in the datasheet, this is not set by default. If this bit is not
>> +	 * set, AIN3 won't work at all.
> 
> The datasheet actually says "Write only 1 to this bit", period. While 
> it
> _does_ say that its default is 1, it doesn't say that it is read-only
> (meaning any entity, such as the rommon, the BIOS, or a user having 
> fun,
> may have set the bit to 0 on any chip revision).

Yeah this is possible. But IIRC, older chip revisions (which we 
assembled on our boards before) worked out of the box with this driver. 
And I'm pretty sure, no other entity sets this bit to zero before linux 
starts, meaning at least rev5 doesn't work without this. But I can 
double check this tomorrow.

Nevertheless, I don't know what you wanna say ;) Should I remove the 
comment entirely or replace it with "Datasheet says write only 1"?

> 
>> +	 */
>> +	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
>> +				 ADT7411_CFG1_RESERVED_BIT3, 1);
>> +	if (ret < 0)
>> +		return ret;
>> +
>>  	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
>>  				 ADT7411_CFG1_START_MONITOR, 1);
> 
> 	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
> 		ADT7411_CFG1_RESERVED_BIT3 | ADT7411_CFG1_START_MONITOR, 1);

Mh, ok. Never thought of using this function with a bitmask.

-michael

Re: [PATCH] hwmon: adt7411: add rev5 workaround

[Guenter Roeck]

On Fri, Jul 15, 2016 at 12:24:54AM +0200, Michael Walle wrote:
> Am 2016-07-14 22:56, schrieb Guenter Roeck:
> >On Thu, Jul 14, 2016 at 01:52:07PM +0200, Michael Walle wrote:
> >>Bit 3 in the CFG1 register has to be 1 to make AIN3 work properly since
> >>silicon revision 5. Although otherwise stated in the datasheet, the
> >>default
> >>value is not 1. Set it to 1 to make AIN3 work.
> >>
> >>Signed-off-by: Michael Walle <michael@walle.cc>
> >>Cc: stable@vger.kernel.org
> >>---
> >>
> >>There was already submission back in 2010.
> >>  https://www.spinics.net/lists/lm-sensors/msg29973.html
> >>
> >>I don't want to take the ownership, but there wasn't a reaction of the
> >>original author. Anyway, I want to answer the raised questions.
> >>
> >>> And what is the problem with this? And what's your hardware?
> >>ADT7411 silicon rev5 either forgot to default this bit to 1 or any
> >>earlier
> >>revision didn't care about it. But with rev5, this bit has to be set
> >>otherwise the AIN3 input returns garbage.
> >>
> >>> [two uses of adt7411_modify_bit()]
> >>>
> >>> This is inefficient: you read and write the same register twice in a
> >>> row. It would be much better to read it once, modify all bits and write
> >>> it once.
> >>This patch does it the same way. IMHO the small size of the change
> >>justifies the slight inefficiency in the one time probe. The other way
> >>you
> >>have to take the lock, add extra code for unlock in error cases etc. But
> >>of
> >>course its up to you to decide. I'm happy to provide another patch with
> >>read, modify both bits and write back the value.
> >>
> >> drivers/hwmon/adt7411.c | 11 +++++++++++
> >> 1 file changed, 11 insertions(+)
> >>
> >>diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
> >>index 827c037..5ed9b11 100644
> >>--- a/drivers/hwmon/adt7411.c
> >>+++ b/drivers/hwmon/adt7411.c
> >>@@ -30,6 +30,7 @@
> >>
> >> #define ADT7411_REG_CFG1			0x18
> >> #define ADT7411_CFG1_START_MONITOR		(1 << 0)
> >>+#define ADT7411_CFG1_RESERVED_BIT3		(1 << 3)
> >>
> >> #define ADT7411_REG_CFG2			0x19
> >> #define ADT7411_CFG2_DISABLE_AVG		(1 << 5)
> >>@@ -296,6 +297,16 @@ static int adt7411_probe(struct i2c_client *client,
> >> 	mutex_init(&data->device_lock);
> >> 	mutex_init(&data->update_lock);
> >>
> >>+	/*
> >>+	 * Silicon rev5 workaround, bit 3 has to be set, but unlike mentioned
> >>+	 * in the datasheet, this is not set by default. If this bit is not
> >>+	 * set, AIN3 won't work at all.
> >
> >The datasheet actually says "Write only 1 to this bit", period. While it
> >_does_ say that its default is 1, it doesn't say that it is read-only
> >(meaning any entity, such as the rommon, the BIOS, or a user having fun,
> >may have set the bit to 0 on any chip revision).
> 
> Yeah this is possible. But IIRC, older chip revisions (which we assembled on
> our boards before) worked out of the box with this driver. And I'm pretty
> sure, no other entity sets this bit to zero before linux starts, meaning at
> least rev5 doesn't work without this. But I can double check this tomorrow.
> 
> Nevertheless, I don't know what you wanna say ;) Should I remove the comment
> entirely or replace it with "Datasheet says write only 1"?
> 
The latter. Strictly speaking, each write to CR1 should explicitly set bit 3
and clear bit 4, but that would be a bit too invasive. And the same really
applies to the reserved bits in CR3.

Guenter

> >
> >>+	 */
> >>+	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
> >>+				 ADT7411_CFG1_RESERVED_BIT3, 1);
> >>+	if (ret < 0)
> >>+		return ret;
> >>+
> >> 	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
> >> 				 ADT7411_CFG1_START_MONITOR, 1);
> >
> >	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
> >		ADT7411_CFG1_RESERVED_BIT3 | ADT7411_CFG1_START_MONITOR, 1);
> 
> Mh, ok. Never thought of using this function with a bitmask.
> 
> -michael

Re: [PATCH] hwmon: adt7411: add rev5 workaround

[Michael Walle]

Am 2016-07-15 00:37, schrieb Guenter Roeck:
> The latter. Strictly speaking, each write to CR1 should explicitly set 
> bit 3
> and clear bit 4, but that would be a bit too invasive. And the same 
> really
> applies to the reserved bits in CR3.

Should I address this issue, too? If so, I would like to provide a 
second patch, which isn't flagged for stable.

-michael

Re: [PATCH] hwmon: adt7411: add rev5 workaround

[Guenter Roeck]

On 07/15/2016 02:16 AM, Michael Walle wrote:
> Am 2016-07-15 00:37, schrieb Guenter Roeck:
>> The latter. Strictly speaking, each write to CR1 should explicitly set bit 3
>> and clear bit 4, but that would be a bit too invasive. And the same really
>> applies to the reserved bits in CR3.
>
> Should I address this issue, too? If so, I would like to provide a second patch, which isn't flagged for stable.
>

If you like. On a side note, you should not tag patches for -stable yourself.
Better would be to add a Fixes: tag and reference the patch introducing the problem.
This reduces the noise on the stable mailing list, and gets it applied automatically
to all affected stable releases.

Thanks,
Guenter

[PATCH] hwmon: adt7411: set bit 3 in CFG1 register

[Michael Walle]

According to the datasheet you should only write 1 to this bit. If it is
not set, at least AIN3 will return bad values on newer silicon revisions.

Fixes: d84ca5b345c2 ("hwmon: Add driver for ADT7411 voltage and temperature sensor")
Signed-off-by: Michael Walle <michael@walle.cc>
---

This is v2 of the previous patch "hwmon: adt7411: add rev5 workaround".

Changes since v1:
 - use adt7411_modify_bit once with a bitmask
 - change wording of the commit message and the comment. Just mention
   that the datasheet requires to set this bit on writes.

 drivers/hwmon/adt7411.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
index 827c037..a7f8869 100644
--- a/drivers/hwmon/adt7411.c
+++ b/drivers/hwmon/adt7411.c
@@ -30,6 +30,7 @@
 
 #define ADT7411_REG_CFG1			0x18
 #define ADT7411_CFG1_START_MONITOR		(1 << 0)
+#define ADT7411_CFG1_RESERVED_BIT3		(1 << 3)
 
 #define ADT7411_REG_CFG2			0x19
 #define ADT7411_CFG2_DISABLE_AVG		(1 << 5)
@@ -296,8 +297,10 @@ static int adt7411_probe(struct i2c_client *client,
 	mutex_init(&data->device_lock);
 	mutex_init(&data->update_lock);
 
+	/* According to the datasheet, we must only write 1 to bit 3 */
 	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
-				 ADT7411_CFG1_START_MONITOR, 1);
+				 ADT7411_CFG1_RESERVED_BIT3
+				 | ADT7411_CFG1_START_MONITOR, 1);
 	if (ret < 0)
 		return ret;
 
-- 
2.1.4

Re: [PATCH] hwmon: adt7411: set bit 3 in CFG1 register

[Guenter Roeck]

On 07/19/2016 07:43 AM, Michael Walle wrote:
> According to the datasheet you should only write 1 to this bit. If it is
> not set, at least AIN3 will return bad values on newer silicon revisions.
>
> Fixes: d84ca5b345c2 ("hwmon: Add driver for ADT7411 voltage and temperature sensor")
> Signed-off-by: Michael Walle <michael@walle.cc>

Applied.

Thanks,
Guenter

> ---
>
> This is v2 of the previous patch "hwmon: adt7411: add rev5 workaround".
>
> Changes since v1:
>   - use adt7411_modify_bit once with a bitmask
>   - change wording of the commit message and the comment. Just mention
>     that the datasheet requires to set this bit on writes.
>
>   drivers/hwmon/adt7411.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/adt7411.c b/drivers/hwmon/adt7411.c
> index 827c037..a7f8869 100644
> --- a/drivers/hwmon/adt7411.c
> +++ b/drivers/hwmon/adt7411.c
> @@ -30,6 +30,7 @@
>
>   #define ADT7411_REG_CFG1			0x18
>   #define ADT7411_CFG1_START_MONITOR		(1 << 0)
> +#define ADT7411_CFG1_RESERVED_BIT3		(1 << 3)
>
>   #define ADT7411_REG_CFG2			0x19
>   #define ADT7411_CFG2_DISABLE_AVG		(1 << 5)
> @@ -296,8 +297,10 @@ static int adt7411_probe(struct i2c_client *client,
>   	mutex_init(&data->device_lock);
>   	mutex_init(&data->update_lock);
>
> +	/* According to the datasheet, we must only write 1 to bit 3 */
>   	ret = adt7411_modify_bit(client, ADT7411_REG_CFG1,
> -				 ADT7411_CFG1_START_MONITOR, 1);
> +				 ADT7411_CFG1_RESERVED_BIT3
> +				 | ADT7411_CFG1_START_MONITOR, 1);
>   	if (ret < 0)
>   		return ret;
>
>