linux-kernel - Re: [PATCH v2 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] [day] [month] [year] [list]

Message-ID: <e5acefed-dc7d-490d-a795-230fdf3b312f@portwell.com.tw>
Date: Thu, 24 Jul 2025 16:34:50 +0800
From: Yen-Chi Huang <jesse.huang@...twell.com.tw>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>, jdelvare@...e.com,
 linux@...ck-us.net, wim@...ux-watchdog.org,
 LKML <linux-kernel@...r.kernel.org>, platform-driver-x86@...r.kernel.org,
 linux-hwmon@...r.kernel.org, linux-watchdog@...r.kernel.org,
 jay.chen@...onical.com
Subject: Re: [PATCH v2 2/2] platform/x86: portwell-ec: Add hwmon support for
 voltage and temperature

Hi Ilpo,

Thanks for the thorough review, and sorry for the basic issues.

On 7/21/2025 9:56 PM, Ilpo Jarvinen wrote:
> On Tue, 15 Jul 2025, Yen-Chi Huang wrote:
> 
>> +static const struct pwec_data pwec_board_data_nano = {
>> +	.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),
>> +	.hwmon_info = pwec_nano_hwmon_info
> 
> Please add comma to any that is not a real terminator so that a future 
> changes won't need to add the comma if more fields get added.

Will fix in patch v3.

>>  static const struct dmi_system_id pwec_dmi_table[] = {
>>  	{
>>  		.ident = "NANO-6064 series",
>>  		.matches = {
>>  			DMI_MATCH(DMI_BOARD_NAME, "NANO-6064"),
>>  		},
>> +		.driver_data = (void *)&pwec_board_data_nano,
> 
> Casting a pointer to void * is not required.

`pwec_board_data_nano` is declared `const`, so dropping the cast produces:

	warning: initialization discards 'const' qualifier from pointer target type

Hence the explicit `(void *)` cast is needed to avoid the warning while
keeping the data read-only.

>> +		old_msb = pwec_read(lsb_reg+1);
> 
> Please add spaces around + as per the coding style guidance.
> 
>> +		lsb = pwec_read(lsb_reg);
>> +		msb = pwec_read(lsb_reg+1);
> 
> Ditto.

Will fix in patch v3.

>> +	switch (type) {
>> +	case hwmon_temp:
>> +		if (channel < d->hwmon_temp_num)
>> +			return 0444;
>> +		break;
> 
> I'd suggest you change these to:
> 
> 		return channel < d->hwmon_temp_num ? 0444 : 0;
> 
>> +	case hwmon_in:
>> +		if (channel < d->hwmon_in_num)
>> +			return 0444;
>> +		break;
>> +	default:
>> +		break;
> 
> ...and this to direct return 0; to simplify the code flow.

Will update as suggested in patch v3.

>> +	switch (type) {
>> +	case hwmon_temp:
>> +		if (channel < data->hwmon_temp_num) {
>> +			*val = pwec_read(data->hwmon_temp_data[channel].lsb_reg) * 1000;
> 
> There might have been some problem in preparing this series as literal 
> 1000 is still there despite your cover letter suggesting it was changed?
> 
> Please check the other expected changes as well, on a glance they seemed 
> to be in place but it has been a while since I've looked on this patch.

Will replace the literal with `MILLIDEGREE_PER_DEGREE` from
`<linux/units.h>` and double-check all other expected changes in v3.

Best regards,
Yen-Chi Huang