linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] platform/x86: portwell-ec: Add watchdog suspend/resume and hwmon
@ 2025-07-14 10:52 Yen-Chi Huang
  2025-07-15  3:51 ` [PATCH v2 1/2] platform/x86: portwell-ec: Add suspend/resume support for watchdog jesse huang
  2025-07-15  3:56 ` [PATCH v2 2/2] platform/x86: portwell-ec: Add hwmon support for voltage and temperature Yen-Chi Huang
  0 siblings, 2 replies; 5+ messages in thread
From: Yen-Chi Huang @ 2025-07-14 10:52 UTC (permalink / raw)
  To: hdegoede, ilpo.jarvinen, jdelvare, linux, wim
  Cc: linux-kernel, platform-driver-x86, linux-hwmon, linux-watchdog,
	jay.chen, jesse.huang

Add suspend/resume support for watchdog and hwmon monitoring functionality
to the Portwell EC driver, enabling better power management and sensor
reporting.

Tested on Portwell NANO-6064.

--
V2:

- Added watchdog mailing list to Cc.

Patch 1/2:
  - unchanged

Patch 2/2:
  - Removed `msb_reg` from `struct pwec_hwmon_data`
  - Updated `pwec_read16_stable()` to assume MSB follows LSB
  - Moved `hwmon_channel_info` to per-board data and assigned it to `.info` at runtime
  - Replaced the `pwec_board_data[]` array with a standalone struct
  - Replaced literal `1000` with `MILLIDEGREE_PER_DEGREE`
  - Removed unused include and sorted header includes

Yen-Chi Huang (2):
  platform/x86: portwell-ec: Add suspend/resume support for watchdog
  platform/x86: portwell-ec: Add hwmon support for voltage and temperature

 drivers/platform/x86/portwell-ec.c | 196 ++++++++++++++++++++++++++++-
 1 file changed, 194 insertions(+), 2 deletions(-)

-- 
2.34.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/2] platform/x86: portwell-ec: Add suspend/resume support for watchdog
  2025-07-14 10:52 [PATCH v2 0/2] platform/x86: portwell-ec: Add watchdog suspend/resume and hwmon Yen-Chi Huang
@ 2025-07-15  3:51 ` jesse huang
  2025-07-15  3:56 ` [PATCH v2 2/2] platform/x86: portwell-ec: Add hwmon support for voltage and temperature Yen-Chi Huang
  1 sibling, 0 replies; 5+ messages in thread
From: jesse huang @ 2025-07-15  3:51 UTC (permalink / raw)
  To: hdegoede, ilpo.jarvinen, jdelvare, linux, wim
  Cc: linux-kernel, platform-driver-x86, linux-hwmon, linux-watchdog,
	jay.chen

Portwell EC does not disable the watchdog during suspend. To avoid unwanted
resets, this patch adds suspend and resume callbacks (pwec_suspend() and
pwec_resume()) to the driver.

The watchdog is stopped in pwec_suspend() and restarted in pwec_resume() if
it was active before suspend.

Signed-off-by: Yen-Chi Huang <jesse.huang@portwell.com.tw>
---

[Re-sending to fix message threading, no content changes since v2.]
---
 drivers/platform/x86/portwell-ec.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
index 8b788822237b..a68522aaa3fa 100644
--- a/drivers/platform/x86/portwell-ec.c
+++ b/drivers/platform/x86/portwell-ec.c
@@ -245,11 +245,29 @@ static int pwec_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int pwec_suspend(struct platform_device *pdev, pm_message_t message)
+{
+	if (watchdog_active(&ec_wdt_dev))
+		return pwec_wdt_stop(&ec_wdt_dev);
+
+	return 0;
+}
+
+static int pwec_resume(struct platform_device *pdev)
+{
+	if (watchdog_active(&ec_wdt_dev))
+		return pwec_wdt_start(&ec_wdt_dev);
+
+	return 0;
+}
+
 static struct platform_driver pwec_driver = {
 	.driver = {
 		.name = "portwell-ec",
 	},
 	.probe = pwec_probe,
+	.suspend = pm_ptr(pwec_suspend),
+	.resume = pm_ptr(pwec_resume),
 };
 
 static struct platform_device *pwec_dev;
-- 
2.34.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] platform/x86: portwell-ec: Add hwmon support for voltage and temperature
  2025-07-14 10:52 [PATCH v2 0/2] platform/x86: portwell-ec: Add watchdog suspend/resume and hwmon Yen-Chi Huang
  2025-07-15  3:51 ` [PATCH v2 1/2] platform/x86: portwell-ec: Add suspend/resume support for watchdog jesse huang
@ 2025-07-15  3:56 ` Yen-Chi Huang
  2025-07-21 13:56   ` Ilpo Järvinen
  1 sibling, 1 reply; 5+ messages in thread
From: Yen-Chi Huang @ 2025-07-15  3:56 UTC (permalink / raw)
  To: hdegoede, ilpo.jarvinen, jdelvare, linux, wim
  Cc: linux-kernel, platform-driver-x86, linux-hwmon, linux-watchdog,
	jay.chen

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

[Re-sending to fix message threading, no content changes since v2.]
---
 drivers/platform/x86/portwell-ec.c | 178 ++++++++++++++++++++++++++++-
 1 file changed, 176 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
index a68522aaa3fa..ac113aaf8bb0 100644
--- a/drivers/platform/x86/portwell-ec.c
+++ b/drivers/platform/x86/portwell-ec.c
@@ -25,6 +25,7 @@
 #include <linux/bitfield.h>
 #include <linux/dmi.h>
 #include <linux/gpio/driver.h>
+#include <linux/hwmon.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
@@ -52,16 +53,64 @@
 #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");
 
+struct pwec_hwmon_data {
+	const char *label;
+	u8 lsb_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;
+	const struct hwmon_channel_info * const *hwmon_info;
+};
+
+static const struct pwec_hwmon_data pwec_nano_hwmon_in[] = {
+	{ "Vcore", 0x20, 3000 },
+	{ "VDIMM", 0x32, 3000 },
+	{ "3.3V",  0x22, 6000 },
+	{ "5V",    0x24, 9600 },
+	{ "12V",   0x30, 19800 },
+};
+
+static const struct pwec_hwmon_data pwec_nano_hwmon_temp[] = {
+	{ "System Temperature", 0x02, 0 },
+};
+
+static const struct hwmon_channel_info *pwec_nano_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 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
+};
+
 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,
 	},
 	{ }
 };
@@ -79,6 +128,19 @@ static u8 pwec_read(u8 address)
 	return inb(PORTWELL_EC_IOSPACE + address);
 }
 
+static u16 pwec_read16_stable(u8 lsb_reg)
+{
+	u8 lsb, msb, old_msb;
+
+	do {
+		old_msb = pwec_read(lsb_reg+1);
+		lsb = pwec_read(lsb_reg);
+		msb = pwec_read(lsb_reg+1);
+	} while (msb != old_msb);
+
+	return (msb << 8) | lsb;
+}
+
 /* GPIO functions */
 
 static int pwec_gpio_get(struct gpio_chip *chip, unsigned int offset)
@@ -204,6 +266,110 @@ 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);
+			*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_ops pwec_hwmon_ops = {
+	.is_visible = pwec_hwmon_is_visible,
+	.read = pwec_hwmon_read,
+	.read_string = pwec_hwmon_read_string,
+};
+
+static struct hwmon_chip_info pwec_chip_info = {
+	.ops = &pwec_hwmon_ops,
+};
+
+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;
+
+	pwec_chip_info.info = data->hwmon_info;
+	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 +408,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 +444,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 +459,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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] platform/x86: portwell-ec: Add hwmon support for voltage and temperature
  2025-07-15  3:56 ` [PATCH v2 2/2] platform/x86: portwell-ec: Add hwmon support for voltage and temperature Yen-Chi Huang
@ 2025-07-21 13:56   ` Ilpo Järvinen
  2025-07-24  8:34     ` Yen-Chi Huang
  0 siblings, 1 reply; 5+ messages in thread
From: Ilpo Järvinen @ 2025-07-21 13:56 UTC (permalink / raw)
  To: Yen-Chi Huang
  Cc: Hans de Goede, jdelvare, linux, wim, LKML, platform-driver-x86,
	linux-hwmon, linux-watchdog, jay.chen

On Tue, 15 Jul 2025, Yen-Chi 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>
> ---
> 
> [Re-sending to fix message threading, no content changes since v2.]
> ---
>  drivers/platform/x86/portwell-ec.c | 178 ++++++++++++++++++++++++++++-
>  1 file changed, 176 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/portwell-ec.c b/drivers/platform/x86/portwell-ec.c
> index a68522aaa3fa..ac113aaf8bb0 100644
> --- a/drivers/platform/x86/portwell-ec.c
> +++ b/drivers/platform/x86/portwell-ec.c
> @@ -25,6 +25,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/dmi.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/hwmon.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
> @@ -52,16 +53,64 @@
>  #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");
>  
> +struct pwec_hwmon_data {
> +	const char *label;
> +	u8 lsb_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;
> +	const struct hwmon_channel_info * const *hwmon_info;
> +};
> +
> +static const struct pwec_hwmon_data pwec_nano_hwmon_in[] = {
> +	{ "Vcore", 0x20, 3000 },
> +	{ "VDIMM", 0x32, 3000 },
> +	{ "3.3V",  0x22, 6000 },
> +	{ "5V",    0x24, 9600 },
> +	{ "12V",   0x30, 19800 },
> +};
> +
> +static const struct pwec_hwmon_data pwec_nano_hwmon_temp[] = {
> +	{ "System Temperature", 0x02, 0 },
> +};
> +
> +static const struct hwmon_channel_info *pwec_nano_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 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.

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

>  	},
>  	{ }
>  };
> @@ -79,6 +128,19 @@ static u8 pwec_read(u8 address)
>  	return inb(PORTWELL_EC_IOSPACE + address);
>  }
>  
> +static u16 pwec_read16_stable(u8 lsb_reg)
> +{
> +	u8 lsb, msb, old_msb;
> +
> +	do {
> +		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.

> +	} while (msb != old_msb);
> +
> +	return (msb << 8) | lsb;
> +}
> +
>  /* GPIO functions */
>  
>  static int pwec_gpio_get(struct gpio_chip *chip, unsigned int offset)
> @@ -204,6 +266,110 @@ 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;

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.

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

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.

> +			return 0;
> +		}
> +		break;
> +	case hwmon_in:
> +		if (channel < data->hwmon_in_num) {
> +			tmp = pwec_read16_stable(data->hwmon_in_data[channel].lsb_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_ops pwec_hwmon_ops = {
> +	.is_visible = pwec_hwmon_is_visible,
> +	.read = pwec_hwmon_read,
> +	.read_string = pwec_hwmon_read_string,
> +};
> +
> +static struct hwmon_chip_info pwec_chip_info = {
> +	.ops = &pwec_hwmon_ops,
> +};
> +
> +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;
> +
> +	pwec_chip_info.info = data->hwmon_info;
> +	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 +408,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 +444,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 +459,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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] platform/x86: portwell-ec: Add hwmon support for voltage and temperature
  2025-07-21 13:56   ` Ilpo Järvinen
@ 2025-07-24  8:34     ` Yen-Chi Huang
  0 siblings, 0 replies; 5+ messages in thread
From: Yen-Chi Huang @ 2025-07-24  8:34 UTC (permalink / raw)
  To: Ilpo Järvinen
  Cc: Hans de Goede, jdelvare, linux, wim, LKML, platform-driver-x86,
	linux-hwmon, linux-watchdog, jay.chen

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-07-24  8:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-14 10:52 [PATCH v2 0/2] platform/x86: portwell-ec: Add watchdog suspend/resume and hwmon Yen-Chi Huang
2025-07-15  3:51 ` [PATCH v2 1/2] platform/x86: portwell-ec: Add suspend/resume support for watchdog jesse huang
2025-07-15  3:56 ` [PATCH v2 2/2] platform/x86: portwell-ec: Add hwmon support for voltage and temperature Yen-Chi Huang
2025-07-21 13:56   ` Ilpo Järvinen
2025-07-24  8:34     ` Yen-Chi Huang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).