linux-kernel - Re: [PATCH v3 09/11] hwmon: (amc6821) Convert to use regmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b4218ace-4aae-4fe3-ad27-dd983087d1f7@cherry.de>
Date: Mon, 8 Jul 2024 12:37:59 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Farouk Bouabid <farouk.bouabid@...rry.de>
Subject: Re: [PATCH v3 09/11] hwmon: (amc6821) Convert to use regmap

Hi Guenter,

On 7/5/24 4:28 PM, Guenter Roeck wrote:
> On 7/5/24 03:59, Quentin Schulz wrote:
>> Hi Guenter,
>>
>> On 7/4/24 7:52 PM, Guenter Roeck wrote:
[...]
>>> +    err = regmap_read(regmap,
>>> +              channel ? AMC6821_REG_RTEMP_FAN_CTRL : 
>>> AMC6821_REG_LTEMP_FAN_CTRL,
>>> +              &regval);
>>> +    if (err)
>>> +      return err;
>>> +    temps[1] = (regval & 0xF8) >> 1;
>>
>> I think we want to use AMC6821_TEMP_LIMIT_MASK here instead of 0xF8?
>>
>> I guess we could also use FIELD_GET?
>>
> 
> Yes. The value in the register is in °C * 4, so that is going to be
>      temps[1] = FIELD_GET(regval, AMC6821_TEMP_LIMIT_MASK) * 4;
> which improves readability and should also clarify the units a bit
> better.
> 
> Note hat
>      (regval & 0xF8) >> 1;
> resulted in the temperature in °C (shift right 1 instead of 3).
> 

Yes, it actually took me a while to figure out why this 1b shift was 
necessary as it didn't match what I got from the datasheet, but the 
formula was actually (>>3) * 4. Former because the register starts at 
bit 3, so we need to right-shift by three bits to have the actual value. 
Then multiply by 4 because a bit in the register means 4°C.

So yes, much more readable with this instead :)

[...]

>>> +        /*
>>> +         * Passive cooling temperature. Range limit against low limit
>>> +         * of both channels.
>>> +         */
>>> +        val = DIV_ROUND_CLOSEST(clamp_val(val, 0, 63000), 1000);
>>
>> This was already in the original code, but I think 64°C should be 
>> doable as well? The datasheet says:
>>
>> """
>> The PSV ranges from 0°C to +64°C.
>> """
>>
> 
> Yes, but I am sure the datasheet is wrong here. The register has 6 
> active bits,
> which means the highest possible value is 0x3f or 63.
> 
>> And there's a PSV8 bit we can write, meaning we can do (1 << 8) with a 
>> step of 4°C which gives us 64°C? In a separate commit though, to not 
>> mix too many fixes into one, making it easier for people to identify 
>> and possibly revert them if necessary.
>>
> Not sure I understand. Can you clarify ?
> 
> Temperature bit assignments in the datasheet are confusing. PSV3
> means full degrees C, PSV8 means 32 degrees C. That is all in one register.
> On the other side, L-TEMP0 reflects _4_ degrees C.
> 
> Am I missing something ?
> 

No, my brain came up with its own math. Register value TEMPERATURE 
MONITORING section all seems to be 1°C increments (except 
Temp-DATA-LByte since it represents 0.125°C increments for a few select 
registers.

Thanks for taking the time to explain!
Quentin