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

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

[jesse huang]

Integrates Vcore, VDIMM, 3.3V, 5V, 12V voltage and system temperature
monitoring into the driver via the hwmon subsystem, enabling
standardized reporting via tools like lm-sensors.

Signed-off-by: Yen-Chi Huang <jesse.huang@portwell.com.tw>
---
 drivers/platform/x86/portwell-ec.c | 188 ++++++++++++++++++++++++++++-
 1 file changed, 186 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
index a68522aaa3fa..79597b4b5559 100644
--- a/drivers/platform/x86/portwell-ec.c
+++ b/drivers/platform/x86/portwell-ec.c
@@ -33,6 +33,10 @@
 #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>
 
 #define PORTWELL_EC_IOSPACE              0xe300
 #define PORTWELL_EC_IOSPACE_LEN          SZ_256
@@ -52,16 +56,59 @@
 #define PORTWELL_EC_FW_VENDOR_LENGTH     3
 #define PORTWELL_EC_FW_VENDOR_NAME       "PWG"
 
+#define PORTWELL_EC_ADC_MAX              1023
+
 static bool force;
 module_param(force, bool, 0444);
 MODULE_PARM_DESC(force, "Force loading EC driver without checking DMI boardname");
 
+enum pwec_board_id {
+	PWEC_BOARD_NANO6064,
+	PWEC_BOARD_ID_MAX
+};
+
+struct pwec_hwmon_data {
+	const char *label;
+	u8 lsb_reg;
+	u8 msb_reg;
+	u32 scale;
+};
+
+struct pwec_data {
+	const struct pwec_hwmon_data *hwmon_in_data;
+	int hwmon_in_num;
+	const struct pwec_hwmon_data *hwmon_temp_data;
+	int hwmon_temp_num;
+};
+
+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 },
+};
+
+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),
+	},
+};
+
 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[PWEC_BOARD_NANO6064],
 	},
 	{ }
 };
@@ -79,6 +126,19 @@ static u8 pwec_read(u8 address)
 	return inb(PORTWELL_EC_IOSPACE + address);
 }
 
+static u16 pwec_read16_stable(u8 lsb_reg, u8 msb_reg)
+{
+	u8 lsb, msb, old_msb;
+
+	do {
+		old_msb = pwec_read(msb_reg);
+		lsb = pwec_read(lsb_reg);
+		msb = pwec_read(msb_reg);
+	} while (msb != old_msb);
+
+	return (msb << 8) | lsb;
+}
+
 /* GPIO functions */
 
 static int pwec_gpio_get(struct gpio_chip *chip, unsigned int offset)
@@ -204,6 +264,122 @@ static struct watchdog_device ec_wdt_dev = {
 	.max_timeout = PORTWELL_WDT_EC_MAX_COUNT_SECOND,
 };
 
+/* HWMON functions */
+
+static umode_t pwec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
+		u32 attr, int channel)
+{
+	const struct pwec_data *d = data;
+
+	switch (type) {
+	case hwmon_temp:
+		if (channel < d->hwmon_temp_num)
+			return 0444;
+		break;
+	case hwmon_in:
+		if (channel < d->hwmon_in_num)
+			return 0444;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int pwec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			   u32 attr, int channel, long *val)
+{
+	struct pwec_data *data = dev_get_drvdata(dev);
+	u16 tmp;
+
+	switch (type) {
+	case hwmon_temp:
+		if (channel < data->hwmon_temp_num) {
+			*val = pwec_read(data->hwmon_temp_data[channel].lsb_reg) * 1000;
+			return 0;
+		}
+		break;
+	case hwmon_in:
+		if (channel < data->hwmon_in_num) {
+			tmp = pwec_read16_stable(data->hwmon_in_data[channel].lsb_reg,
+						 data->hwmon_in_data[channel].msb_reg);
+			*val = (data->hwmon_in_data[channel].scale * tmp) / PORTWELL_EC_ADC_MAX;
+			return 0;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int pwec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+				  u32 attr, int channel, const char **str)
+{
+	struct pwec_data *data = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_temp:
+		if (channel < data->hwmon_temp_num) {
+			*str = data->hwmon_temp_data[channel].label;
+			return 0;
+		}
+		break;
+	case hwmon_in:
+		if (channel < data->hwmon_in_num) {
+			*str = data->hwmon_in_data[channel].label;
+			return 0;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_channel_info *pwec_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL),
+	NULL
+};
+
+static const struct hwmon_ops pwec_hwmon_ops = {
+	.is_visible = pwec_hwmon_is_visible,
+	.read = pwec_hwmon_read,
+	.read_string = pwec_hwmon_read_string,
+};
+
+static const struct hwmon_chip_info pwec_chip_info = {
+	.ops = &pwec_hwmon_ops,
+	.info = pwec_hwmon_info,
+};
+
+static int pwec_hwmon_init(struct device *dev)
+{
+	struct pwec_data *data = dev_get_platdata(dev);
+	void *hwmon;
+	int ret;
+
+	if (!IS_REACHABLE(CONFIG_HWMON))
+		return 0;
+
+	hwmon = devm_hwmon_device_register_with_info(dev, "portwell_ec", data, &pwec_chip_info,
+						     NULL);
+	ret = PTR_ERR_OR_ZERO(hwmon);
+	if (ret)
+		dev_err(dev, "Failed to register hwmon_dev: %d\n", ret);
+
+	return ret;
+}
+
 static int pwec_firmware_vendor_check(void)
 {
 	u8 buf[PORTWELL_EC_FW_VENDOR_LENGTH + 1];
@@ -242,6 +418,10 @@ static int pwec_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = pwec_hwmon_init(&pdev->dev);
+	if (ret < 0)
+		return ret;
+
 	return 0;
 }
 
@@ -274,11 +454,14 @@ static struct platform_device *pwec_dev;
 
 static int __init pwec_init(void)
 {
+	const struct dmi_system_id *match;
 	int ret;
 
-	if (!dmi_check_system(pwec_dmi_table)) {
+	match = dmi_first_match(pwec_dmi_table);
+	if (!match) {
 		if (!force)
 			return -ENODEV;
+		match = &pwec_dmi_table[0];
 		pr_warn("force load portwell-ec without DMI check\n");
 	}
 
@@ -286,7 +469,8 @@ static int __init pwec_init(void)
 	if (ret)
 		return ret;
 
-	pwec_dev = platform_device_register_simple("portwell-ec", -1, NULL, 0);
+	pwec_dev = platform_device_register_data(NULL, "portwell-ec", -1, match->driver_data,
+						 sizeof(struct pwec_data));
 	if (IS_ERR(pwec_dev)) {
 		platform_driver_unregister(&pwec_driver);
 		return PTR_ERR(pwec_dev);
-- 
2.34.1

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

[Ilpo Järvinen]

On Fri, 27 Jun 2025, jesse huang wrote:

> Integrates Vcore, VDIMM, 3.3V, 5V, 12V voltage and system temperature
> monitoring into the driver via the hwmon subsystem, enabling
> standardized reporting via tools like lm-sensors.
> 
> Signed-off-by: Yen-Chi Huang <jesse.huang@portwell.com.tw>
> ---
>  drivers/platform/x86/portwell-ec.c | 188 ++++++++++++++++++++++++++++-
>  1 file changed, 186 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
> index a68522aaa3fa..79597b4b5559 100644
> --- a/drivers/platform/x86/portwell-ec.c
> +++ b/drivers/platform/x86/portwell-ec.c
> @@ -33,6 +33,10 @@
>  #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.

>  #define PORTWELL_EC_IOSPACE              0xe300
>  #define PORTWELL_EC_IOSPACE_LEN          SZ_256
> @@ -52,16 +56,59 @@
>  #define PORTWELL_EC_FW_VENDOR_LENGTH     3
>  #define PORTWELL_EC_FW_VENDOR_NAME       "PWG"
>  
> +#define PORTWELL_EC_ADC_MAX              1023
> +
>  static bool force;
>  module_param(force, bool, 0444);
>  MODULE_PARM_DESC(force, "Force loading EC driver without checking DMI boardname");
>  
> +enum pwec_board_id {
> +	PWEC_BOARD_NANO6064,
> +	PWEC_BOARD_ID_MAX
> +};
> +
> +struct pwec_hwmon_data {
> +	const char *label;
> +	u8 lsb_reg;
> +	u8 msb_reg;
> +	u32 scale;
> +};
> +
> +struct pwec_data {
> +	const struct pwec_hwmon_data *hwmon_in_data;
> +	int hwmon_in_num;
> +	const struct pwec_hwmon_data *hwmon_temp_data;
> +	int hwmon_temp_num;
> +};
> +
> +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.

> +};
> +
> +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?

> +
>  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[PWEC_BOARD_NANO6064],
>  	},
>  	{ }
>  };
> @@ -79,6 +126,19 @@ static u8 pwec_read(u8 address)
>  	return inb(PORTWELL_EC_IOSPACE + address);
>  }
>  
> +static u16 pwec_read16_stable(u8 lsb_reg, u8 msb_reg)
> +{
> +	u8 lsb, msb, old_msb;
> +
> +	do {
> +		old_msb = pwec_read(msb_reg);
> +		lsb = pwec_read(lsb_reg);
> +		msb = pwec_read(msb_reg);
> +	} while (msb != old_msb);
> +
> +	return (msb << 8) | lsb;
> +}
> +
>  /* GPIO functions */
>  
>  static int pwec_gpio_get(struct gpio_chip *chip, unsigned int offset)
> @@ -204,6 +264,122 @@ static struct watchdog_device ec_wdt_dev = {
>  	.max_timeout = PORTWELL_WDT_EC_MAX_COUNT_SECOND,
>  };
>  
> +/* HWMON functions */
> +
> +static umode_t pwec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> +		u32 attr, int channel)
> +{
> +	const struct pwec_data *d = data;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		if (channel < d->hwmon_temp_num)
> +			return 0444;
> +		break;
> +	case hwmon_in:
> +		if (channel < d->hwmon_in_num)
> +			return 0444;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pwec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	struct pwec_data *data = dev_get_drvdata(dev);
> +	u16 tmp;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		if (channel < data->hwmon_temp_num) {
> +			*val = pwec_read(data->hwmon_temp_data[channel].lsb_reg) * 1000;

linux/units.h ?

> +			return 0;
> +		}
> +		break;
> +	case hwmon_in:
> +		if (channel < data->hwmon_in_num) {
> +			tmp = pwec_read16_stable(data->hwmon_in_data[channel].lsb_reg,
> +						 data->hwmon_in_data[channel].msb_reg);
> +			*val = (data->hwmon_in_data[channel].scale * tmp) / PORTWELL_EC_ADC_MAX;
> +			return 0;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int pwec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> +				  u32 attr, int channel, const char **str)
> +{
> +	struct pwec_data *data = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		if (channel < data->hwmon_temp_num) {
> +			*str = data->hwmon_temp_data[channel].label;
> +			return 0;
> +		}
> +		break;
> +	case hwmon_in:
> +		if (channel < data->hwmon_in_num) {
> +			*str = data->hwmon_in_data[channel].label;
> +			return 0;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct hwmon_channel_info *pwec_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_ops pwec_hwmon_ops = {
> +	.is_visible = pwec_hwmon_is_visible,
> +	.read = pwec_hwmon_read,
> +	.read_string = pwec_hwmon_read_string,
> +};
> +
> +static const struct hwmon_chip_info pwec_chip_info = {
> +	.ops = &pwec_hwmon_ops,
> +	.info = pwec_hwmon_info,
> +};
> +
> +static int pwec_hwmon_init(struct device *dev)
> +{
> +	struct pwec_data *data = dev_get_platdata(dev);
> +	void *hwmon;
> +	int ret;
> +
> +	if (!IS_REACHABLE(CONFIG_HWMON))
> +		return 0;
> +
> +	hwmon = devm_hwmon_device_register_with_info(dev, "portwell_ec", data, &pwec_chip_info,
> +						     NULL);
> +	ret = PTR_ERR_OR_ZERO(hwmon);
> +	if (ret)
> +		dev_err(dev, "Failed to register hwmon_dev: %d\n", ret);
> +
> +	return ret;
> +}
> +
>  static int pwec_firmware_vendor_check(void)
>  {
>  	u8 buf[PORTWELL_EC_FW_VENDOR_LENGTH + 1];
> @@ -242,6 +418,10 @@ static int pwec_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = pwec_hwmon_init(&pdev->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	return 0;
>  }
>  
> @@ -274,11 +454,14 @@ static struct platform_device *pwec_dev;
>  
>  static int __init pwec_init(void)
>  {
> +	const struct dmi_system_id *match;
>  	int ret;
>  
> -	if (!dmi_check_system(pwec_dmi_table)) {
> +	match = dmi_first_match(pwec_dmi_table);
> +	if (!match) {
>  		if (!force)
>  			return -ENODEV;
> +		match = &pwec_dmi_table[0];
>  		pr_warn("force load portwell-ec without DMI check\n");
>  	}
>  
> @@ -286,7 +469,8 @@ static int __init pwec_init(void)
>  	if (ret)
>  		return ret;
>  
> -	pwec_dev = platform_device_register_simple("portwell-ec", -1, NULL, 0);
> +	pwec_dev = platform_device_register_data(NULL, "portwell-ec", -1, match->driver_data,
> +						 sizeof(struct pwec_data));
>  	if (IS_ERR(pwec_dev)) {
>  		platform_driver_unregister(&pwec_driver);
>  		return PTR_ERR(pwec_dev);
> 

-- 
 i.

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

[Guenter Roeck]

On Fri, Jun 27, 2025 at 05:15:01PM +0800, jesse huang wrote:
> Integrates Vcore, VDIMM, 3.3V, 5V, 12V voltage and system temperature
> monitoring into the driver via the hwmon subsystem, enabling
> standardized reporting via tools like lm-sensors.
> 
> Signed-off-by: Yen-Chi Huang <jesse.huang@portwell.com.tw>
> ---
>  drivers/platform/x86/portwell-ec.c | 188 ++++++++++++++++++++++++++++-
>  1 file changed, 186 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
> index a68522aaa3fa..79597b4b5559 100644
> --- a/drivers/platform/x86/portwell-ec.c
> +++ b/drivers/platform/x86/portwell-ec.c
> @@ -33,6 +33,10 @@
>  #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>

Two unnecessary include files.

Guenter

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

[Yen-Chi Huang]

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

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

[Ilpo Järvinen]

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.

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

[Yen-Chi Huang]

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