Message-ID: <SA1PR09MB74400C313FFCCEAE32A4C565E7869@SA1PR09MB7440.namprd09.prod.outlook.com>
Date:   Thu, 28 Oct 2021 01:54:01 +0000
From:   "Wilson, David T. (GSFC-5870)" <david.wilson@...a.gov>
To:     Guenter Roeck <linux@...ck-us.net>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>
Subject: Re: [EXTERNAL] Re: Potential issue with smb word operations for
 tmp461 device in tmp401 driver

Hi Guenter,

Like last time, I updated several files in my platform's v5.4 files and applied your three patches. 
>From what I can tell, I'm having no problems with the tmp461's basic support in the modified lm90 driver.

Thanks again for looking into this,
David


From: Guenter Roeck <groeck7@...il.com> on behalf of Guenter Roeck <linux@...ck-us.net>
Sent: Thursday, October 21, 2021 3:07 AM
To: Wilson, David T. (GSFC-5870) <david.wilson@...a.gov>
Cc: linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>; linux-hwmon@...r.kernel.org <linux-hwmon@...r.kernel.org>
Subject: Re: [EXTERNAL] Re: Potential issue with smb word operations for tmp461 device in tmp401 driver 
 
Hi David,

On 10/20/21 10:37 PM, Wilson, David T. (GSFC-5870) wrote:
> Guenter,
> 
> I updated several files and applied the lm90.c and tmp401.c patches. It should be noted that my target platform only supports v5.4, so
> I had to update additional files (i.e. hwmon.c/.h) in order to successfully compile and apply the patch to lm90.c, which could suggest
> that some of my findings may have been due to poor updating of files.
> 
> In regards to testing, I got temp1_input and temp2_input values that look good from a glance and that steadily increased after booting
> my target platform. There were three areas of possible concern.
> 1. The lm90.c patch includes  "+#define TMP461_REG_DFC 0x23". If this define is referring to the address of the digital filter control,
>      then the value should be 0x24, since 0x23 is the n-Factor Correction register

Good catch. That is a bug. Thanks a lot for noticing.

> 2. The values I recorded had a resolution of 0.125 deg resolution (e.g., 30625, 25250, etc...). However, the datasheet for the tmp461
>      notes that the resolution is 0.0625 deg for local and remote channels, so I was wondering if that would also be supported in lm90.c

You are correct, the lm90 driver assumes a 0.125 deg resolution. I'll see what I can do about that.

> 3. For temp1_min and temp2_min, I used "cat temp1_min" to get the value -128000. Assuming that is -128.0 deg, I don't believe the
>      tmp461 should be returning a value underneath -64 deg based on the temp data format if I'm understanding the datasheet correctly.
> 

It is odd, but the tmp461 datasheet does list a default value of 0x80 for the low limit registers.
In 2-complement format, that does translate to -128 degrees C. That doesn't really make much sense,
but it is what the chip reports, and I hesitate to change that.

Thanks,
Guenter

> Thanks,
> David
> 
> From: Guenter Roeck <groeck7@...il.com> on behalf of Guenter Roeck <linux@...ck-us.net>
> Sent: Tuesday, October 19, 2021 10:18 AM
> To: Wilson, David T. (GSFC-5870) <david.wilson@...a.gov>
> Cc: linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>; linux-hwmon@...r.kernel.org <linux-hwmon@...r.kernel.org>
> Subject: Re: [EXTERNAL] Re: Potential issue with smb word operations for tmp461 device in tmp401 driver
>   
> David,
> 
> On 10/18/21 10:42 PM, Wilson, David T. (GSFC-5870) wrote:
>> Guenter,
>>
>> I've tried testing the patch by checking out the most recent commit's version for tmp401.c and applying the patch. However, the temperature values seem to be to low <1000 for each read.
>>
>> If I'm understanding the driver code correctly, I believe there's two places of interest that might explain the low values
>> 1. In tmp401_update_device_reg16, the final assignment to data->temp[j][i] overrides the patch's assignment to data->temp[j][i] in the tmp461 branch
>> 2. In SENSOR_DEVICE_ATTR_2_RW, the temp2_offset is at (6, 1), but the newly added LSB array does not contain the LSB address at (6, 1)
>>
> Thanks a lot for the information.
> 
>> Regarding your most recent email, I will try the lm90 by changing the driver tree entry to "ti,tmp451" and I'll let you know if that works for me.
>>
> I just sent two patches, one to remove tmp461 support from the tmp401 driver, and one to add
> tmp461 support to the lm90 driver. Turns out tmp461 is almost identical to tmp451, which was
> already supported by the lm90 driver. Adding support for tmp461 to the lm90 driver makes
> much more sense than trying to fix the tmp401 driver.
> 
> I module tested the lm90 driver for both tmp451 and tmp461 (I was able to find
> register dumps for both chips), so I am reasonably sure that it works.
> 
> Thanks,
> Guenter
> 
>> Thanks,
>> David
>>
>> From: Guenter Roeck <groeck7@...il.com> on behalf of Guenter Roeck <linux@...ck-us.net>
>> Sent: Tuesday, October 19, 2021 12:56 AM
>> To: Wilson, David T. (GSFC-5870) <david.wilson@...a.gov>
>> Cc: linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>; linux-hwmon@...r.kernel.org <linux-hwmon@...r.kernel.org>
>> Subject: [EXTERNAL] Re: Potential issue with smb word operations for tmp461 device in tmp401 driver
>>     
>> David,
>>
>> On 10/15/21 10:43 AM, Wilson, David T. (GSFC-5870) wrote:
>>> Hi,
>>>
>>> I am reporting what I believe is a potential issue in the tmp401 driver for the tmp461 device specifically. I am new to reporting issues, so I apologize in advance if I've provided insufficient information for an issue report.
>>>
>>> The problem I'm encountering is that when I use the tmp401 linux driver to read temperature values from the tmp461, all of the read temperature values end with 996 (e.g. 33996, 38996, etc...).
>>>
>>> Looking further into the tmp401 commit messages, I see that the driver was changed to use smb word operations instead of separate byte operations. Although the other supported devices (i.e. tmp432, etc...) are noted to support 16-bit read operations in their respective datasheets, I see no indications of 16-bit read support in the tmp461 datasheet, which is supported by my inquiry in the TI forums (https://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fe2e.ti.com%2Fsupport%2Fsensors-group%2Fsensors%2Ff%2Fsensors-forum%2F1044935%2Ftmp461-linux-driver-support-and-16-bit-temperature-register-reads&amp;data=04%7C01%7Cdavid.wilson%40nasa.gov%7C2e3728d5f2e74810482108d994618683%7C7005d45845be48ae8140d43da96dd17b%7C0%7C0%7C637703968869859896%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3n9QPCDAzD0Qoy16KVtTzVNAe42KBgFdGbZr3naFWko%3D&amp;reserved=0).
>>>
>>> Reverting the driver to the commit before the smb word change, I am then able to read temperature values that do not end only with 996. As a result, I believe that the tmp461 support may be partially broken by the switch to smb word operations.
>>>
>>
>> can you try to instantiate the lm90 driver (instead of the tmp401 driver)
>> and let me know if it works for you ? If your system uses devicetree,
>> you might have to select "ti,tmp451" instead of "ti,tmp461".
>>
>> Thanks,
>> Guenter
>>

Powered by blists - more mailing lists