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

[Yen-Chi Huang <jesse.huang@portwell.com.tw>]

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