linux-kernel - Re: [PATCH 2/2] platform/x86: portwell-ec: Add hwmon support for voltage and temperature

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 for Android: free password hash cracker in your pocket

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

Message-ID: <9d5d40ed-db62-4d0a-9807-82b231135b0b@portwell.com.tw>
Date: Wed, 9 Jul 2025 16:17:34 +0800
From: Yen-Chi Huang <jesse.huang@...twell.com.tw>
To: ilpo.jarvinen@...ux.intel.com
Cc: linux@...ck-us.net, hansg@...nel.org, jdelvare@...e.com,
 linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 linux-hwmon@...r.kernel.org
Subject: Re: [PATCH 2/2] platform/x86: portwell-ec: Add hwmon support for
 voltage and temperature

Hi Ilpo,

Thank you for the detailed and insightful review.

I realized that I had misunderstood some of your points in my earlier reply.
Sorry about that, and thank you for the clarifications.

On 7/3/2025 5:43 PM, Ilpo Jarvinen wrote:
> On Thu, 3 Jul 2025, Yen-Chi Huang wrote:
>> On 6/27/2025 7:34 PM, Ilpo Jarvinen wrote:
>>> On Fri, 27 Jun 2025, jesse huang wrote:
>>
>>>> +static const struct pwec_hwmon_data pwec_nano_hwmon_in[] = {
>>>> +	{ "Vcore", 0x20, 0x21, 3000 },
[...]
>>>
>>> Those registers appear to be always consecutive so it looks unnecessary to 
>>> store both.
>>
>> Some ECs use little-endian while others use big-endian register ordering.
>>
>> To maintain flexibility and support future boards with different endianness,
>> both registers are stored explicitly.
> 
> When do we expect to see patches to support those other boards? I think 
> the endianness should be only added then, unless the patch is really 
> around the corner.
> 
> Besides, wouldn't it make more sense to record the endianness instead if 
> the registers are always next to each other anyway? Do we expect there's 
> need to handle disjoint parts?

The `msb_reg` in `struct pwec_hwmon_data` will be removed,
and the rest of the code will be updated accordingly in patch v2.

>> To support multiple boards with different sensor configurations in a scalable way,
>> the hwmon data is structured as board-specific arrays.
[...]
> 
> 
> I was just asking why you need to place them into an array and not just 
> have a separate struct for each board variation as is the usual pattern. 
[...]
> ...Those can be put directly into driver_data without the intermediate 
> array. So why is the array necessary?

The pwec_board_data[] array will be replaced with a standalone
`pwec_board_data_nano` struct.
Patch v2 will include the corresponding changes.

>>>> +		if (channel < data->hwmon_temp_num) {
>>>> +			*val = pwec_read(data->hwmon_temp_data[channel].lsb_reg) * 1000;
>>>
>>> linux/units.h ?
>>
>> "1000" will be replaced with MILLI in the next patch.
> 
> As this seems temperature related(?), there's also DEGREE specific define 
> which would be preferred over that unitless define (if applicable, of 
> course).

The literal `1000` will be replaced with `MILLIDEGREE_PER_DEGREE` in patch v2.

Best regards,
Yen-Chi Huang