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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 3 Jul 2025, Yen-Chi Huang wrote:

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

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?

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

I understand this. :-)

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. 
(For boards which can share the struct, the variable name is usually just 
according to the firstly introduced board.)

So you'd have e.g.

static const struct pwec_hwmon_data pwec_board_data_nano6064 = {
	.hwmon_in_data = ...,
	...
};

Then when you have something else, you add another:

static const struct pwec_hwmon_data pwec_board_data_xx = {
	...
};

...Those can be put directly into driver_data without the intermediate 
array. So why is the array necessary?

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

-- 
 i.