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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <62e4e647-9eb4-4329-89f4-6b2b897ba15b@portwell.com.tw>
Date: Thu, 3 Jul 2025 17:13:53 +0800
From: Yen-Chi Huang <jesse.huang@...twell.com.tw>
To: Guenter Roeck <linux@...ck-us.net>,
 Ilpo Jarvinen <ilpo.jarvinen@...ux.intel.com>
Cc: 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 and Guenter,

Thank you both for the review and suggestions.  
Apologies for the missed cleanup in the includes.

---

On 6/27/2025 7:34 PM, Ilpo Jarvinen wrote:
> On Fri, 27 Jun 2025, jesse huang wrote:
>>  #include <linux/sizes.h>
>>  #include <linux/string.h>
>>  #include <linux/watchdog.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/hwmon-vid.h>
>> +#include <linux/err.h>
> 
> You forgot to add these according to alphabetical order.

The include order will be fixed in the next patch.

>> +static const struct pwec_hwmon_data pwec_nano_hwmon_in[] = {
>> +	{ "Vcore", 0x20, 0x21, 3000 },
>> +	{ "VDIMM", 0x32, 0x33, 3000 },
>> +	{ "3.3V",  0x22, 0x23, 6000 },
>> +	{ "5V",    0x24, 0x25, 9600 },
>> +	{ "12V",   0x30, 0x31, 19800 },
> 
> 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.

>> +static const struct pwec_hwmon_data pwec_nano_hwmon_temp[] = {
>> +	{ "System Temperature", 0x02, 0, 0 },
>> +};
>> +
>> +static const struct pwec_data pwec_board_data[] = {
>> +	[PWEC_BOARD_NANO6064] = {
>> +		.hwmon_in_data = pwec_nano_hwmon_in,
>> +		.hwmon_in_num = ARRAY_SIZE(pwec_nano_hwmon_in),
>> +		.hwmon_temp_data = pwec_nano_hwmon_temp,
>> +		.hwmon_temp_num = ARRAY_SIZE(pwec_nano_hwmon_temp),
>> +	},
>> +};
> 
> What's advantage of having these in an array?

To support multiple boards with different sensor configurations in a scalable way,
the hwmon data is structured as board-specific arrays.

I intend to store the hwmon configuration in the driver_data field of the dmi_system_id table.
This allows each board to carry its own sensor definitions, making it easier to add support for
new boards without modifying the driver logic. Since the number of sensors may vary, the *_num
fields in pwec_data are used to validate the index range in hwmon_ops callbacks, ensuring only
valid sensors are accessed.

>> +		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.

---

On 6/27/2025 9:28 PM, Guenter Roeck wrote:
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/hwmon-vid.h>
> 
> Two unnecessary include files.
> 
> Guenter

The dummy includes will be removed in the next patch.

Best regards,  
Yen-Chi Huang