[PATCH v2 0/2] ChromeOS Embedded controller hwmon driver

[PATCH v2 0/2] ChromeOS Embedded controller hwmon driver

[Thomas Weißschuh]

Add a hwmon driver that reports fan and temperature readings from the
ChromeOS Embedded controller.

There was an earlier effort in 2017 to add such a driver [0], but there
was no followup after v1.
The new driver is complete reimplementation based on newer APIs and with
more features (temp sensor names).

It only works on LPC-connected ECs, as only those implement direct
memory-map access.
For other busses the data would need to be read with a command.
Adding some helpers was discussed in the previous patchset [1].

The EC protocols also support reading and writing fan curves but that is
not implemented.

Tested on a Framework 13 AMD, Firmware 3.05.

[0] https://lore.kernel.org/all/1491602410-31518-1-git-send-email-moritz.fischer@ettus.com/
[1] https://lore.kernel.org/all/ac61bfca-bfa0-143b-c9ca-365b8026ce8d@roeck-us.net/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v2:
- drop unnecessary range checks (Guenter)
- only validate thermal_version during probing
- reorder some variable declarations
- validate thermal_version directly in cros_ec_hwmon_probe (Mario)
- drop return value from probe_temp_sensors as it can't fail anymore
- fail with -ENODEV if cmd_readmem is missing to avoid spurious warnings
- Link to v1: https://lore.kernel.org/r/20240507-cros_ec-hwmon-v1-0-2c47c5ce8e85@weissschuh.net

---
Thomas Weißschuh (2):
      hwmon: add ChromeOS EC driver
      mfd: cros_ec: Register hardware monitoring subdevice

 Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
 Documentation/hwmon/index.rst         |   1 +
 MAINTAINERS                           |   8 +
 drivers/hwmon/Kconfig                 |  11 ++
 drivers/hwmon/Makefile                |   1 +
 drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
 drivers/mfd/cros_ec_dev.c             |   1 +
 7 files changed, 317 insertions(+)
---
base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
change-id: 20240506-cros_ec-hwmon-24634b07cf6f

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Thomas Weißschuh]

The ChromeOS Embedded Controller exposes fan speed and temperature
readings.
Expose this data through the hwmon subsystem.

The driver is designed to be probed via the cros_ec mfd device.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
 Documentation/hwmon/index.rst         |   1 +
 MAINTAINERS                           |   8 +
 drivers/hwmon/Kconfig                 |  11 ++
 drivers/hwmon/Makefile                |   1 +
 drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
 6 files changed, 316 insertions(+)

diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
new file mode 100644
index 000000000000..aeb88c79d11b
--- /dev/null
+++ b/Documentation/hwmon/cros_ec_hwmon.rst
@@ -0,0 +1,26 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver cros_ec_hwmon
+===========================
+
+Supported chips:
+
+  * ChromeOS embedded controllers connected via LPC
+
+    Prefix: 'cros_ec'
+
+    Addresses scanned: -
+
+Author:
+
+  - Thomas Weißschuh <linux@weissschuh.net>
+
+Description
+-----------
+
+This driver implements support for hardware monitoring commands exposed by the
+ChromeOS embedded controller used in Chromebooks and other devices.
+
+The channel labels exposed via hwmon are retrieved from the EC itself.
+
+Fan and temperature readings are supported.
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 1ca7a4fe1f8f..355a83e66928 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -57,6 +57,7 @@ Hardware Monitoring Kernel Drivers
    coretemp
    corsair-cpro
    corsair-psu
+   cros_ec_hwmon
    da9052
    da9055
    dell-smm-hwmon
diff --git a/MAINTAINERS b/MAINTAINERS
index c23fda1aa1f0..aa5689169eca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4988,6 +4988,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
 F:	sound/soc/codecs/cros_ec_codec.*
 
+CHROMEOS EC HARDWARE MONITORING
+M:	Thomas Weißschuh <thomas@weissschuh.net>
+L:	chrome-platform@lists.linux.dev
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/chros_ec_hwmon.rst
+F:	drivers/hwmon/cros_ec_hwmon.c
+
 CHROMEOS EC SUBDRIVERS
 M:	Benson Leung <bleung@chromium.org>
 R:	Guenter Roeck <groeck@chromium.org>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 83945397b6eb..c1284d42697f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -506,6 +506,17 @@ config SENSORS_CORSAIR_PSU
 	  This driver can also be built as a module. If so, the module
 	  will be called corsair-psu.
 
+config SENSORS_CROS_EC
+	tristate "ChromeOS Embedded Controller sensors"
+	depends on MFD_CROS_EC_DEV
+	default MFD_CROS_EC_DEV
+	help
+	  If you say yes here you get support for ChromeOS Embedded Controller
+	  sensors.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called cros_ec_hwmon.
+
 config SENSORS_DRIVETEMP
 	tristate "Hard disk drives with temperature sensors"
 	depends on SCSI && ATA
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 5c31808f6378..8519a6b36c00 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
 obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o
+obj-$(CONFIG_SENSORS_CROS_EC)	+= cros_ec_hwmon.o
 obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
 obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
 obj-$(CONFIG_SENSORS_DELL_SMM)	+= dell-smm-hwmon.o
diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
new file mode 100644
index 000000000000..d59d39df2ac4
--- /dev/null
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -0,0 +1,269 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  ChromesOS EC driver for hwmon
+ *
+ *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
+ */
+
+#include <linux/device.h>
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/units.h>
+
+#define DRV_NAME	"cros-ec-hwmon"
+
+struct cros_ec_hwmon_priv {
+	struct cros_ec_device *cros_ec;
+	u8 thermal_version;
+	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
+};
+
+static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
+{
+	u16 data;
+	int ret;
+
+	ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
+	if (ret < 0)
+		return ret;
+
+	data = le16_to_cpu(data);
+
+	if (data == EC_FAN_SPEED_NOT_PRESENT)
+		return -ENODEV;
+
+	*speed = data;
+	return 0;
+}
+
+static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version,
+				   u8 index, u8 *data)
+{
+	unsigned int offset;
+	int ret;
+
+	if (index < EC_TEMP_SENSOR_ENTRIES)
+		offset = EC_MEMMAP_TEMP_SENSOR + index;
+	else
+		offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES;
+
+	ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data);
+	if (ret < 0)
+		return ret;
+
+	if (*data == EC_TEMP_SENSOR_NOT_PRESENT ||
+	    *data == EC_TEMP_SENSOR_ERROR ||
+	    *data == EC_TEMP_SENSOR_NOT_POWERED ||
+	    *data == EC_TEMP_SENSOR_NOT_CALIBRATED)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *val)
+{
+	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+	int ret = -ENODATA;
+	u16 speed;
+	u8 temp;
+
+	if (type == hwmon_fan) {
+		ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
+		if (ret == 0)
+			*val = speed;
+	} else if (type == hwmon_temp) {
+		ret = cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, channel, &temp);
+		if (ret == 0)
+			*val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
+	}
+
+	return ret;
+}
+
+static int cros_ec_hwmon_get_temp_sensor_info(struct cros_ec_device *cros_ec, u8 id,
+					      struct ec_response_temp_sensor_get_info *resp)
+{
+	int ret;
+	struct {
+		struct cros_ec_command msg;
+		union {
+			struct ec_params_temp_sensor_get_info req;
+			struct ec_response_temp_sensor_get_info resp;
+		} __packed data;
+	} __packed buf = {
+		.msg = {
+			.version = 0,
+			.command = EC_CMD_TEMP_SENSOR_GET_INFO,
+			.insize  = sizeof(buf.data.resp),
+			.outsize = sizeof(buf.data.req),
+		},
+		.data.req.id = id,
+	};
+
+	ret = cros_ec_cmd_xfer_status(cros_ec, &buf.msg);
+	if (ret < 0)
+		return ret;
+
+	*resp = buf.data.resp;
+	return 0;
+}
+
+static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+				     u32 attr, int channel, const char **str)
+{
+	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+
+	if (type == hwmon_temp && attr == hwmon_temp_label) {
+		*str = priv->temp_sensor_names[channel];
+		return 0;
+	}
+
+	return -ENODATA;
+}
+
+static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
+					u32 attr, int channel)
+{
+	const struct cros_ec_hwmon_priv *priv = data;
+	u16 speed;
+
+	if (type == hwmon_fan) {
+		if (cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed) == 0)
+			return 0444;
+	} else if (type == hwmon_temp) {
+		if (priv->temp_sensor_names[channel])
+			return 0444;
+	}
+
+	return 0;
+}
+
+static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT,
+			   HWMON_F_INPUT),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL),
+	NULL
+};
+
+static const struct hwmon_ops cros_ec_hwmon_ops = {
+	.read = cros_ec_hwmon_read,
+	.read_string = cros_ec_hwmon_read_string,
+	.is_visible = cros_ec_hwmon_is_visible,
+};
+
+static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
+	.ops = &cros_ec_hwmon_ops,
+	.info = cros_ec_hwmon_info,
+};
+
+static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv)
+{
+	struct ec_response_temp_sensor_get_info info;
+	size_t candidates, i;
+	int ret;
+	u8 temp;
+
+	if (priv->thermal_version < 2)
+		candidates = EC_TEMP_SENSOR_ENTRIES;
+	else
+		candidates = ARRAY_SIZE(priv->temp_sensor_names);
+
+	for (i = 0; i < candidates; i++) {
+		if (cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, i, &temp) != 0)
+			continue;
+
+		ret = cros_ec_hwmon_get_temp_sensor_info(priv->cros_ec, i, &info);
+		if (ret < 0)
+			continue;
+
+		priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%*s",
+							    (int)sizeof(info.sensor_name),
+							    info.sensor_name);
+	}
+}
+
+static int cros_ec_hwmon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
+	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
+	struct cros_ec_hwmon_priv *priv;
+	struct device *hwmon_dev;
+	int ret;
+
+	/* Not every platform supports direct reads */
+	if (!cros_ec->cmd_readmem)
+		return -ENODEV;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->cros_ec = cros_ec;
+
+	ret = priv->cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_THERMAL_VERSION,
+					 1, &priv->thermal_version);
+	if (ret < 0)
+		return ret;
+
+	/* Covers both fan and temp sensors */
+	if (!priv->thermal_version)
+		return -ENODEV;
+
+	cros_ec_hwmon_probe_temp_sensors(dev, priv);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
+							 &cros_ec_hwmon_chip_info, NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct platform_device_id cros_ec_hwmon_id[] = {
+	{ DRV_NAME, 0 },
+	{ }
+};
+
+static struct platform_driver cros_ec_hwmon_driver = {
+	.driver.name	= DRV_NAME,
+	.probe		= cros_ec_hwmon_probe,
+	.id_table	= cros_ec_hwmon_id,
+};
+module_platform_driver(cros_ec_hwmon_driver);
+
+MODULE_DEVICE_TABLE(platform, cros_ec_hwmon_id);
+MODULE_DESCRIPTION("ChromeOS EC Hardware Monitoring Driver");
+MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
+MODULE_LICENSE("GPL");

-- 
2.45.0

[PATCH v2 2/2] mfd: cros_ec: Register hardware monitoring subdevice

[Thomas Weißschuh]

Add ChromeOS EC-based hardware monitoring as EC subdevice.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/mfd/cros_ec_dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index a52d59cc2b1e..ca30ca25fbf8 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -131,6 +131,7 @@ static const struct mfd_cell cros_ec_platform_cells[] = {
 	{ .name = "cros-ec-chardev", },
 	{ .name = "cros-ec-debugfs", },
 	{ .name = "cros-ec-sysfs", },
+	{ .name = "cros-ec-hwmon", },
 };
 
 static const struct mfd_cell cros_ec_pchg_cells[] = {

-- 
2.45.0

Re: [PATCH v2 0/2] ChromeOS Embedded controller hwmon driver

[Mario Limonciello]

On 5/7/2024 11:29, Thomas Weißschuh wrote:
> Add a hwmon driver that reports fan and temperature readings from the
> ChromeOS Embedded controller.
> 
> There was an earlier effort in 2017 to add such a driver [0], but there
> was no followup after v1.
> The new driver is complete reimplementation based on newer APIs and with
> more features (temp sensor names).
> 
> It only works on LPC-connected ECs, as only those implement direct
> memory-map access.
> For other busses the data would need to be read with a command.
> Adding some helpers was discussed in the previous patchset [1].
> 
> The EC protocols also support reading and writing fan curves but that is
> not implemented.
> 
> Tested on a Framework 13 AMD, Firmware 3.05.
> 
> [0] https://lore.kernel.org/all/1491602410-31518-1-git-send-email-moritz.fischer@ettus.com/
> [1] https://lore.kernel.org/all/ac61bfca-bfa0-143b-c9ca-365b8026ce8d@roeck-us.net/
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Changes in v2:
> - drop unnecessary range checks (Guenter)
> - only validate thermal_version during probing
> - reorder some variable declarations
> - validate thermal_version directly in cros_ec_hwmon_probe (Mario)
> - drop return value from probe_temp_sensors as it can't fail anymore
> - fail with -ENODEV if cmd_readmem is missing to avoid spurious warnings
> - Link to v1: https://lore.kernel.org/r/20240507-cros_ec-hwmon-v1-0-2c47c5ce8e85@weissschuh.net
> 
> ---
> Thomas Weißschuh (2):
>        hwmon: add ChromeOS EC driver
>        mfd: cros_ec: Register hardware monitoring subdevice
> 
>   Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
>   Documentation/hwmon/index.rst         |   1 +
>   MAINTAINERS                           |   8 +
>   drivers/hwmon/Kconfig                 |  11 ++
>   drivers/hwmon/Makefile                |   1 +
>   drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
>   drivers/mfd/cros_ec_dev.c             |   1 +
>   7 files changed, 317 insertions(+)
> ---
> base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
> change-id: 20240506-cros_ec-hwmon-24634b07cf6f
> 
> Best regards,

That was fast!  The series looks good to me, thanks.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Guenter Roeck]

On Tue, May 7, 2024 at 10:29 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> The ChromeOS Embedded Controller exposes fan speed and temperature
> readings.
> Expose this data through the hwmon subsystem.
>
> The driver is designed to be probed via the cros_ec mfd device.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
>  Documentation/hwmon/index.rst         |   1 +
>  MAINTAINERS                           |   8 +
>  drivers/hwmon/Kconfig                 |  11 ++
>  drivers/hwmon/Makefile                |   1 +
>  drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
>  6 files changed, 316 insertions(+)
>
> diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
> new file mode 100644
> index 000000000000..aeb88c79d11b
> --- /dev/null
> +++ b/Documentation/hwmon/cros_ec_hwmon.rst
> @@ -0,0 +1,26 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver cros_ec_hwmon
> +===========================
> +
> +Supported chips:
> +
> +  * ChromeOS embedded controllers connected via LPC
> +
> +    Prefix: 'cros_ec'
> +
> +    Addresses scanned: -
> +
> +Author:
> +
> +  - Thomas Weißschuh <linux@weissschuh.net>
> +
> +Description
> +-----------
> +
> +This driver implements support for hardware monitoring commands exposed by the
> +ChromeOS embedded controller used in Chromebooks and other devices.
> +
> +The channel labels exposed via hwmon are retrieved from the EC itself.
> +
> +Fan and temperature readings are supported.
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 1ca7a4fe1f8f..355a83e66928 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -57,6 +57,7 @@ Hardware Monitoring Kernel Drivers
>     coretemp
>     corsair-cpro
>     corsair-psu
> +   cros_ec_hwmon
>     da9052
>     da9055
>     dell-smm-hwmon
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c23fda1aa1f0..aa5689169eca 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4988,6 +4988,14 @@ S:       Maintained
>  F:     Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
>  F:     sound/soc/codecs/cros_ec_codec.*
>
> +CHROMEOS EC HARDWARE MONITORING
> +M:     Thomas Weißschuh <thomas@weissschuh.net>
> +L:     chrome-platform@lists.linux.dev
> +L:     linux-hwmon@vger.kernel.org
> +S:     Maintained
> +F:     Documentation/hwmon/chros_ec_hwmon.rst
> +F:     drivers/hwmon/cros_ec_hwmon.c
> +
>  CHROMEOS EC SUBDRIVERS
>  M:     Benson Leung <bleung@chromium.org>
>  R:     Guenter Roeck <groeck@chromium.org>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 83945397b6eb..c1284d42697f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -506,6 +506,17 @@ config SENSORS_CORSAIR_PSU
>           This driver can also be built as a module. If so, the module
>           will be called corsair-psu.
>
> +config SENSORS_CROS_EC
> +       tristate "ChromeOS Embedded Controller sensors"
> +       depends on MFD_CROS_EC_DEV
> +       default MFD_CROS_EC_DEV
> +       help
> +         If you say yes here you get support for ChromeOS Embedded Controller
> +         sensors.
> +
> +         This driver can also be built as a module. If so, the module
> +         will be called cros_ec_hwmon.
> +
>  config SENSORS_DRIVETEMP
>         tristate "Hard disk drives with temperature sensors"
>         depends on SCSI && ATA
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 5c31808f6378..8519a6b36c00 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o
>  obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
>  obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
>  obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o
> +obj-$(CONFIG_SENSORS_CROS_EC)  += cros_ec_hwmon.o
>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
>  obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
>  obj-$(CONFIG_SENSORS_DELL_SMM) += dell-smm-hwmon.o
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> new file mode 100644
> index 000000000000..d59d39df2ac4
> --- /dev/null
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  ChromesOS EC driver for hwmon
> + *
> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/units.h>
> +
> +#define DRV_NAME       "cros-ec-hwmon"
> +
> +struct cros_ec_hwmon_priv {
> +       struct cros_ec_device *cros_ec;
> +       u8 thermal_version;
> +       const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> +};
> +
> +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> +{
> +       u16 data;
> +       int ret;
> +
> +       ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
> +       if (ret < 0)
> +               return ret;
> +
> +       data = le16_to_cpu(data);
> +
> +       if (data == EC_FAN_SPEED_NOT_PRESENT)
> +               return -ENODEV;
> +
I don't have time for a full review right now, but this looks wrong:
If a fan is not present, its sysfs attribute should not be instantiated.
Returning -ENODEV here is most definitely wrong.

> +       *speed = data;
> +       return 0;
> +}
> +
> +static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version,
> +                                  u8 index, u8 *data)
> +{
> +       unsigned int offset;
> +       int ret;
> +
> +       if (index < EC_TEMP_SENSOR_ENTRIES)
> +               offset = EC_MEMMAP_TEMP_SENSOR + index;
> +       else
> +               offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES;
> +
> +       ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (*data == EC_TEMP_SENSOR_NOT_PRESENT ||
> +           *data == EC_TEMP_SENSOR_ERROR ||
> +           *data == EC_TEMP_SENSOR_NOT_POWERED ||
> +           *data == EC_TEMP_SENSOR_NOT_CALIBRATED)
> +               return -ENODEV;
> +
Same as above.

Guenter

> +       return 0;
> +}
> +
> +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +                             u32 attr, int channel, long *val)
> +{
> +       struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +       int ret = -ENODATA;
> +       u16 speed;
> +       u8 temp;
> +
> +       if (type == hwmon_fan) {
> +               ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> +               if (ret == 0)
> +                       *val = speed;
> +       } else if (type == hwmon_temp) {
> +               ret = cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, channel, &temp);
> +               if (ret == 0)
> +                       *val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
> +       }
> +
> +       return ret;
> +}
> +
> +static int cros_ec_hwmon_get_temp_sensor_info(struct cros_ec_device *cros_ec, u8 id,
> +                                             struct ec_response_temp_sensor_get_info *resp)
> +{
> +       int ret;
> +       struct {
> +               struct cros_ec_command msg;
> +               union {
> +                       struct ec_params_temp_sensor_get_info req;
> +                       struct ec_response_temp_sensor_get_info resp;
> +               } __packed data;
> +       } __packed buf = {
> +               .msg = {
> +                       .version = 0,
> +                       .command = EC_CMD_TEMP_SENSOR_GET_INFO,
> +                       .insize  = sizeof(buf.data.resp),
> +                       .outsize = sizeof(buf.data.req),
> +               },
> +               .data.req.id = id,
> +       };
> +
> +       ret = cros_ec_cmd_xfer_status(cros_ec, &buf.msg);
> +       if (ret < 0)
> +               return ret;
> +
> +       *resp = buf.data.resp;
> +       return 0;
> +}
> +
> +static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> +                                    u32 attr, int channel, const char **str)
> +{
> +       struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +
> +       if (type == hwmon_temp && attr == hwmon_temp_label) {
> +               *str = priv->temp_sensor_names[channel];
> +               return 0;
> +       }
> +
> +       return -ENODATA;
> +}
> +
> +static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> +                                       u32 attr, int channel)
> +{
> +       const struct cros_ec_hwmon_priv *priv = data;
> +       u16 speed;
> +
> +       if (type == hwmon_fan) {
> +               if (cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed) == 0)
> +                       return 0444;
> +       } else if (type == hwmon_temp) {
> +               if (priv->temp_sensor_names[channel])
> +                       return 0444;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
> +       HWMON_CHANNEL_INFO(fan,
> +                          HWMON_F_INPUT,
> +                          HWMON_F_INPUT,
> +                          HWMON_F_INPUT,
> +                          HWMON_F_INPUT),
> +       HWMON_CHANNEL_INFO(temp,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL,
> +                          HWMON_T_INPUT | HWMON_T_LABEL),
> +       NULL
> +};
> +
> +static const struct hwmon_ops cros_ec_hwmon_ops = {
> +       .read = cros_ec_hwmon_read,
> +       .read_string = cros_ec_hwmon_read_string,
> +       .is_visible = cros_ec_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
> +       .ops = &cros_ec_hwmon_ops,
> +       .info = cros_ec_hwmon_info,
> +};
> +
> +static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv)
> +{
> +       struct ec_response_temp_sensor_get_info info;
> +       size_t candidates, i;
> +       int ret;
> +       u8 temp;
> +
> +       if (priv->thermal_version < 2)
> +               candidates = EC_TEMP_SENSOR_ENTRIES;
> +       else
> +               candidates = ARRAY_SIZE(priv->temp_sensor_names);
> +
> +       for (i = 0; i < candidates; i++) {
> +               if (cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, i, &temp) != 0)
> +                       continue;
> +
> +               ret = cros_ec_hwmon_get_temp_sensor_info(priv->cros_ec, i, &info);
> +               if (ret < 0)
> +                       continue;
> +
> +               priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%*s",
> +                                                           (int)sizeof(info.sensor_name),
> +                                                           info.sensor_name);
> +       }
> +}
> +
> +static int cros_ec_hwmon_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> +       struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +       struct cros_ec_hwmon_priv *priv;
> +       struct device *hwmon_dev;
> +       int ret;
> +
> +       /* Not every platform supports direct reads */
> +       if (!cros_ec->cmd_readmem)
> +               return -ENODEV;
> +
> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->cros_ec = cros_ec;
> +
> +       ret = priv->cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_THERMAL_VERSION,
> +                                        1, &priv->thermal_version);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Covers both fan and temp sensors */
> +       if (!priv->thermal_version)
> +               return -ENODEV;
> +
> +       cros_ec_hwmon_probe_temp_sensors(dev, priv);
> +
> +       hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
> +                                                        &cros_ec_hwmon_chip_info, NULL);
> +
> +       return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct platform_device_id cros_ec_hwmon_id[] = {
> +       { DRV_NAME, 0 },
> +       { }
> +};
> +
> +static struct platform_driver cros_ec_hwmon_driver = {
> +       .driver.name    = DRV_NAME,
> +       .probe          = cros_ec_hwmon_probe,
> +       .id_table       = cros_ec_hwmon_id,
> +};
> +module_platform_driver(cros_ec_hwmon_driver);
> +
> +MODULE_DEVICE_TABLE(platform, cros_ec_hwmon_id);
> +MODULE_DESCRIPTION("ChromeOS EC Hardware Monitoring Driver");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
> +MODULE_LICENSE("GPL");
>
> --
> 2.45.0
>

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Thomas Weißschuh]

Hi Guenter,

thanks for your quick responses!

On 2024-05-07 14:55:44+0000, Guenter Roeck wrote:
> On Tue, May 7, 2024 at 10:29 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > The ChromeOS Embedded Controller exposes fan speed and temperature
> > readings.
> > Expose this data through the hwmon subsystem.
> >
> > The driver is designed to be probed via the cros_ec mfd device.
> >
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
> >  Documentation/hwmon/index.rst         |   1 +
> >  MAINTAINERS                           |   8 +
> >  drivers/hwmon/Kconfig                 |  11 ++
> >  drivers/hwmon/Makefile                |   1 +
> >  drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
> >  6 files changed, 316 insertions(+)
> >

[..]

> > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > new file mode 100644
> > index 000000000000..d59d39df2ac4
> > --- /dev/null
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -0,0 +1,269 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *  ChromesOS EC driver for hwmon
> > + *
> > + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +#include <linux/units.h>
> > +
> > +#define DRV_NAME       "cros-ec-hwmon"
> > +
> > +struct cros_ec_hwmon_priv {
> > +       struct cros_ec_device *cros_ec;
> > +       u8 thermal_version;
> > +       const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> > +};
> > +
> > +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> > +{
> > +       u16 data;
> > +       int ret;
> > +
> > +       ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       data = le16_to_cpu(data);
> > +
> > +       if (data == EC_FAN_SPEED_NOT_PRESENT)
> > +               return -ENODEV;
> > +
> I don't have time for a full review right now, but this looks wrong:
> If a fan is not present, its sysfs attribute should not be instantiated.
> Returning -ENODEV here is most definitely wrong.

This is also called from cros_ec_hwmon_is_visible(),

I think the special value should be handled in the read function
anyways, although it should never happen after probing.

Otherwise the magic, nonsensical value will be shown to the user as
sensor reading.

The sysfs hiding works.
Out of 4 fans and 24 temp sensors known by the driver,
on my device only 1 fan and 4 temp sensors exist and are probed.

If you prefer another error code please let me know.

Please let me know if I got something wrong.

> > +       *speed = data;
> > +       return 0;
> > +}
> > +
> > +static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version,
> > +                                  u8 index, u8 *data)
> > +{
> > +       unsigned int offset;
> > +       int ret;
> > +
> > +       if (index < EC_TEMP_SENSOR_ENTRIES)
> > +               offset = EC_MEMMAP_TEMP_SENSOR + index;
> > +       else
> > +               offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES;
> > +
> > +       ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (*data == EC_TEMP_SENSOR_NOT_PRESENT ||
> > +           *data == EC_TEMP_SENSOR_ERROR ||
> > +           *data == EC_TEMP_SENSOR_NOT_POWERED ||
> > +           *data == EC_TEMP_SENSOR_NOT_CALIBRATED)
> > +               return -ENODEV;
> > +
> Same as above.

cros_ec_hwmon_probe()
  cros_ec_hwmon_probe_temp_sensors()
    cros_ec_hwmon_read_temp()

if _read_temp() fails here, the sensor name remains NULL and
is_visible() will hide that sensor.
      
As for the general reasoning, see above.

[..]


Thanks,
Thomas

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Stephen Horvath]

Hi Thomas,

I was the one to implement fan monitoring/control into Dustin's driver, 
and just had a quick comment for your driver:

On 8/5/24 02:29, Thomas Weißschuh wrote:
> The ChromeOS Embedded Controller exposes fan speed and temperature
> readings.
> Expose this data through the hwmon subsystem.
> 
> The driver is designed to be probed via the cros_ec mfd device.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
>   Documentation/hwmon/index.rst         |   1 +
>   MAINTAINERS                           |   8 +
>   drivers/hwmon/Kconfig                 |  11 ++
>   drivers/hwmon/Makefile                |   1 +
>   drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
>   6 files changed, 316 insertions(+)
> 
> diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
> new file mode 100644
> index 000000000000..aeb88c79d11b
> --- /dev/null
> +++ b/Documentation/hwmon/cros_ec_hwmon.rst
> @@ -0,0 +1,26 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver cros_ec_hwmon
> +===========================
> +
> +Supported chips:
> +
> +  * ChromeOS embedded controllers connected via LPC
> +
> +    Prefix: 'cros_ec'
> +
> +    Addresses scanned: -
> +
> +Author:
> +
> +  - Thomas Weißschuh <linux@weissschuh.net>
> +
> +Description
> +-----------
> +
> +This driver implements support for hardware monitoring commands exposed by the
> +ChromeOS embedded controller used in Chromebooks and other devices.
> +
> +The channel labels exposed via hwmon are retrieved from the EC itself.
> +
> +Fan and temperature readings are supported.
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 1ca7a4fe1f8f..355a83e66928 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -57,6 +57,7 @@ Hardware Monitoring Kernel Drivers
>      coretemp
>      corsair-cpro
>      corsair-psu
> +   cros_ec_hwmon
>      da9052
>      da9055
>      dell-smm-hwmon
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c23fda1aa1f0..aa5689169eca 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4988,6 +4988,14 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/sound/google,cros-ec-codec.yaml
>   F:	sound/soc/codecs/cros_ec_codec.*
>   
> +CHROMEOS EC HARDWARE MONITORING
> +M:	Thomas Weißschuh <thomas@weissschuh.net>
> +L:	chrome-platform@lists.linux.dev
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/chros_ec_hwmon.rst
> +F:	drivers/hwmon/cros_ec_hwmon.c
> +
>   CHROMEOS EC SUBDRIVERS
>   M:	Benson Leung <bleung@chromium.org>
>   R:	Guenter Roeck <groeck@chromium.org>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 83945397b6eb..c1284d42697f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -506,6 +506,17 @@ config SENSORS_CORSAIR_PSU
>   	  This driver can also be built as a module. If so, the module
>   	  will be called corsair-psu.
>   
> +config SENSORS_CROS_EC
> +	tristate "ChromeOS Embedded Controller sensors"
> +	depends on MFD_CROS_EC_DEV
> +	default MFD_CROS_EC_DEV
> +	help
> +	  If you say yes here you get support for ChromeOS Embedded Controller
> +	  sensors.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called cros_ec_hwmon.
> +
>   config SENSORS_DRIVETEMP
>   	tristate "Hard disk drives with temperature sensors"
>   	depends on SCSI && ATA
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 5c31808f6378..8519a6b36c00 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o
>   obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
>   obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
>   obj-$(CONFIG_SENSORS_CORSAIR_PSU) += corsair-psu.o
> +obj-$(CONFIG_SENSORS_CROS_EC)	+= cros_ec_hwmon.o
>   obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
>   obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
>   obj-$(CONFIG_SENSORS_DELL_SMM)	+= dell-smm-hwmon.o
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> new file mode 100644
> index 000000000000..d59d39df2ac4
> --- /dev/null
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  ChromesOS EC driver for hwmon
> + *
> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/units.h>
> +
> +#define DRV_NAME	"cros-ec-hwmon"
> +
> +struct cros_ec_hwmon_priv {
> +	struct cros_ec_device *cros_ec;
> +	u8 thermal_version;
> +	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> +};
> +
> +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> +{
> +	u16 data;
> +	int ret;
> +
> +	ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
> +	if (ret < 0)
> +		return ret;
> +
> +	data = le16_to_cpu(data);
> +
> +	if (data == EC_FAN_SPEED_NOT_PRESENT)
> +		return -ENODEV;
> +

Don't forget it can also return `EC_FAN_SPEED_STALLED`.
Like Guenter, I also don't like returning `-ENODEV`, but I don't have a 
problem with checking for `EC_FAN_SPEED_NOT_PRESENT` in case it was 
removed since init or something.
My approach was to return the speed as `0`, since the fan probably isn't 
spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and 
HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
No idea if this is correct though.

> +	*speed = data;
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 thermal_version,
> +				   u8 index, u8 *data)
> +{
> +	unsigned int offset;
> +	int ret;
> +
> +	if (index < EC_TEMP_SENSOR_ENTRIES)
> +		offset = EC_MEMMAP_TEMP_SENSOR + index;
> +	else
> +		offset = EC_MEMMAP_TEMP_SENSOR_B + index - EC_TEMP_SENSOR_ENTRIES;
> +
> +	ret = cros_ec->cmd_readmem(cros_ec, offset, 1, data);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (*data == EC_TEMP_SENSOR_NOT_PRESENT ||
> +	    *data == EC_TEMP_SENSOR_ERROR ||
> +	    *data == EC_TEMP_SENSOR_NOT_POWERED ||
> +	    *data == EC_TEMP_SENSOR_NOT_CALIBRATED)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *val)
> +{
> +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +	int ret = -ENODATA;
> +	u16 speed;
> +	u8 temp;
> +
> +	if (type == hwmon_fan) {
> +		ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> +		if (ret == 0)
> +			*val = speed;
> +	} else if (type == hwmon_temp) {
> +		ret = cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, channel, &temp);
> +		if (ret == 0)
> +			*val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
> +	}
> +
> +	return ret;
> +}
> +
> +static int cros_ec_hwmon_get_temp_sensor_info(struct cros_ec_device *cros_ec, u8 id,
> +					      struct ec_response_temp_sensor_get_info *resp)
> +{
> +	int ret;
> +	struct {
> +		struct cros_ec_command msg;
> +		union {
> +			struct ec_params_temp_sensor_get_info req;
> +			struct ec_response_temp_sensor_get_info resp;
> +		} __packed data;
> +	} __packed buf = {
> +		.msg = {
> +			.version = 0,
> +			.command = EC_CMD_TEMP_SENSOR_GET_INFO,
> +			.insize  = sizeof(buf.data.resp),
> +			.outsize = sizeof(buf.data.req),
> +		},
> +		.data.req.id = id,
> +	};
> +
> +	ret = cros_ec_cmd_xfer_status(cros_ec, &buf.msg);
> +	if (ret < 0)
> +		return ret;
> +
> +	*resp = buf.data.resp;
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> +				     u32 attr, int channel, const char **str)
> +{
> +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_label) {
> +		*str = priv->temp_sensor_names[channel];
> +		return 0;
> +	}
> +
> +	return -ENODATA;
> +}
> +
> +static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> +					u32 attr, int channel)
> +{
> +	const struct cros_ec_hwmon_priv *priv = data;
> +	u16 speed;
> +
> +	if (type == hwmon_fan) {
> +		if (cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed) == 0)
> +			return 0444;
> +	} else if (type == hwmon_temp) {
> +		if (priv->temp_sensor_names[channel])
> +			return 0444;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT,
> +			   HWMON_F_INPUT),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_ops cros_ec_hwmon_ops = {
> +	.read = cros_ec_hwmon_read,
> +	.read_string = cros_ec_hwmon_read_string,
> +	.is_visible = cros_ec_hwmon_is_visible,
> +};
> +
> +static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
> +	.ops = &cros_ec_hwmon_ops,
> +	.info = cros_ec_hwmon_info,
> +};
> +
> +static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv)
> +{
> +	struct ec_response_temp_sensor_get_info info;
> +	size_t candidates, i;
> +	int ret;
> +	u8 temp;
> +
> +	if (priv->thermal_version < 2)
> +		candidates = EC_TEMP_SENSOR_ENTRIES;
> +	else
> +		candidates = ARRAY_SIZE(priv->temp_sensor_names);
> +
> +	for (i = 0; i < candidates; i++) {
> +		if (cros_ec_hwmon_read_temp(priv->cros_ec, priv->thermal_version, i, &temp) != 0)
> +			continue;
> +
> +		ret = cros_ec_hwmon_get_temp_sensor_info(priv->cros_ec, i, &info);
> +		if (ret < 0)
> +			continue;
> +
> +		priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%*s",
> +							    (int)sizeof(info.sensor_name),
> +							    info.sensor_name);
> +	}
> +}
> +
> +static int cros_ec_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +	struct cros_ec_hwmon_priv *priv;
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	/* Not every platform supports direct reads */
> +	if (!cros_ec->cmd_readmem)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->cros_ec = cros_ec;
> +
> +	ret = priv->cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_THERMAL_VERSION,
> +					 1, &priv->thermal_version);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Covers both fan and temp sensors */
> +	if (!priv->thermal_version)
> +		return -ENODEV;
> +
> +	cros_ec_hwmon_probe_temp_sensors(dev, priv);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
> +							 &cros_ec_hwmon_chip_info, NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct platform_device_id cros_ec_hwmon_id[] = {
> +	{ DRV_NAME, 0 },
> +	{ }
> +};
> +
> +static struct platform_driver cros_ec_hwmon_driver = {
> +	.driver.name	= DRV_NAME,
> +	.probe		= cros_ec_hwmon_probe,
> +	.id_table	= cros_ec_hwmon_id,
> +};
> +module_platform_driver(cros_ec_hwmon_driver);
> +
> +MODULE_DEVICE_TABLE(platform, cros_ec_hwmon_id);
> +MODULE_DESCRIPTION("ChromeOS EC Hardware Monitoring Driver");
> +MODULE_AUTHOR("Thomas Weißschuh <linux@weissschuh.net");
> +MODULE_LICENSE("GPL");
> 

But feel free to ignore me if I'm completly wrong about this, since I 
really don't have much experience with kernel dev.

Thanks,
Steve

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Thomas Weißschuh]

Hi Stephen,

On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
> I was the one to implement fan monitoring/control into Dustin's driver, and
> just had a quick comment for your driver:
> 
> On 8/5/24 02:29, Thomas Weißschuh wrote:
> > The ChromeOS Embedded Controller exposes fan speed and temperature
> > readings.
> > Expose this data through the hwmon subsystem.
> > 
> > The driver is designed to be probed via the cros_ec mfd device.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >   Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
> >   Documentation/hwmon/index.rst         |   1 +
> >   MAINTAINERS                           |   8 +
> >   drivers/hwmon/Kconfig                 |  11 ++
> >   drivers/hwmon/Makefile                |   1 +
> >   drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
> >   6 files changed, 316 insertions(+)
> > 

<snip>

> > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > new file mode 100644
> > index 000000000000..d59d39df2ac4
> > --- /dev/null
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -0,0 +1,269 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *  ChromesOS EC driver for hwmon
> > + *
> > + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +#include <linux/units.h>
> > +
> > +#define DRV_NAME	"cros-ec-hwmon"
> > +
> > +struct cros_ec_hwmon_priv {
> > +	struct cros_ec_device *cros_ec;
> > +	u8 thermal_version;
> > +	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> > +};
> > +
> > +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> > +{
> > +	u16 data;
> > +	int ret;
> > +
> > +	ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	data = le16_to_cpu(data);
> > +
> > +	if (data == EC_FAN_SPEED_NOT_PRESENT)
> > +		return -ENODEV;
> > +
> 
> Don't forget it can also return `EC_FAN_SPEED_STALLED`.

Thanks for the hint. I'll need to think about how to handle this better.

> Like Guenter, I also don't like returning `-ENODEV`, but I don't have a
> problem with checking for `EC_FAN_SPEED_NOT_PRESENT` in case it was removed
> since init or something.

Ok.

> My approach was to return the speed as `0`, since the fan probably isn't
> spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
> HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
> No idea if this is correct though.

I'm not a fan of returning a speed of 0 in case of errors.
Rather -EIO which can't be mistaken.
Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which should never happen)
and also for EC_FAN_SPEED_STALLED.
And EC_FAN_SPEED_STALLED also sets HWMON_F_FAULT.
HWMON_F_ALARM doesn't seem right to me.

But if Guenter has a preference, that will do, too.

> 
> > +	*speed = data;
> > +	return 0;
> > +}
> > +

<snip>

> But feel free to ignore me if I'm completly wrong about this, since I really
> don't have much experience with kernel dev.

Thanks for your feedback!

Would you mind if I Cc you on further revisions?


Thomas

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Stephen Horvath]

Hi Thomas,

On 28/5/24 05:24, Thomas Weißschuh wrote:
> Hi Stephen,
> 
> On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
>> I was the one to implement fan monitoring/control into Dustin's driver, and
>> just had a quick comment for your driver:
>>
>> On 8/5/24 02:29, Thomas Weißschuh wrote:
>>> The ChromeOS Embedded Controller exposes fan speed and temperature
>>> readings.
>>> Expose this data through the hwmon subsystem.
>>>
>>> The driver is designed to be probed via the cros_ec mfd device.
>>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> ---
>>>    Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
>>>    Documentation/hwmon/index.rst         |   1 +
>>>    MAINTAINERS                           |   8 +
>>>    drivers/hwmon/Kconfig                 |  11 ++
>>>    drivers/hwmon/Makefile                |   1 +
>>>    drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
>>>    6 files changed, 316 insertions(+)
>>>
> 
> <snip>
> 
>>> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
>>> new file mode 100644
>>> index 000000000000..d59d39df2ac4
>>> --- /dev/null
>>> +++ b/drivers/hwmon/cros_ec_hwmon.c
>>> @@ -0,0 +1,269 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + *  ChromesOS EC driver for hwmon
>>> + *
>>> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/platform_data/cros_ec_commands.h>
>>> +#include <linux/platform_data/cros_ec_proto.h>
>>> +#include <linux/units.h>
>>> +
>>> +#define DRV_NAME	"cros-ec-hwmon"
>>> +
>>> +struct cros_ec_hwmon_priv {
>>> +	struct cros_ec_device *cros_ec;
>>> +	u8 thermal_version;
>>> +	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
>>> +};
>>> +
>>> +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
>>> +{
>>> +	u16 data;
>>> +	int ret;
>>> +
>>> +	ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	data = le16_to_cpu(data);
>>> +
>>> +	if (data == EC_FAN_SPEED_NOT_PRESENT)
>>> +		return -ENODEV;
>>> +
>>
>> Don't forget it can also return `EC_FAN_SPEED_STALLED`.
> 
> Thanks for the hint. I'll need to think about how to handle this better.
> 
>> Like Guenter, I also don't like returning `-ENODEV`, but I don't have a
>> problem with checking for `EC_FAN_SPEED_NOT_PRESENT` in case it was removed
>> since init or something.
> 
> Ok.
> 
>> My approach was to return the speed as `0`, since the fan probably isn't
>> spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
>> HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
>> No idea if this is correct though.
> 
> I'm not a fan of returning a speed of 0 in case of errors.
> Rather -EIO which can't be mistaken.
> Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which should never happen)
> and also for EC_FAN_SPEED_STALLED.

Yeah, that's pretty reasonable.

> And EC_FAN_SPEED_STALLED also sets HWMON_F_FAULT.
> HWMON_F_ALARM doesn't seem right to me.

Fair enough, I thought I copied the behaviour off another driver, but I 
can't find which one, so maybe I just made it up. I do agree with you 
though.

> But if Guenter has a preference, that will do, too.

Of course!

>>
>>> +	*speed = data;
>>> +	return 0;
>>> +}
>>> +
> 
> <snip>
> 
>> But feel free to ignore me if I'm completly wrong about this, since I really
>> don't have much experience with kernel dev.
> 
> Thanks for your feedback!
> 
> Would you mind if I Cc you on further revisions?

Sure, I don't mind at all!
To be honest, I wouldn't mind a Cc on the other Framework related stuff 
too, but I don't mind either way.

Thanks,
Steve

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Guenter Roeck]

On 5/27/24 17:15, Stephen Horvath wrote:
> Hi Thomas,
> 
> On 28/5/24 05:24, Thomas Weißschuh wrote:
>> Hi Stephen,
>>
>> On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
>>> I was the one to implement fan monitoring/control into Dustin's driver, and
>>> just had a quick comment for your driver:
>>>
>>> On 8/5/24 02:29, Thomas Weißschuh wrote:
>>>> The ChromeOS Embedded Controller exposes fan speed and temperature
>>>> readings.
>>>> Expose this data through the hwmon subsystem.
>>>>
>>>> The driver is designed to be probed via the cros_ec mfd device.
>>>>
>>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>> ---
>>>>    Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
>>>>    Documentation/hwmon/index.rst         |   1 +
>>>>    MAINTAINERS                           |   8 +
>>>>    drivers/hwmon/Kconfig                 |  11 ++
>>>>    drivers/hwmon/Makefile                |   1 +
>>>>    drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
>>>>    6 files changed, 316 insertions(+)
>>>>
>>
>> <snip>
>>
>>>> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
>>>> new file mode 100644
>>>> index 000000000000..d59d39df2ac4
>>>> --- /dev/null
>>>> +++ b/drivers/hwmon/cros_ec_hwmon.c
>>>> @@ -0,0 +1,269 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>> +/*
>>>> + *  ChromesOS EC driver for hwmon
>>>> + *
>>>> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/hwmon.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/platform_data/cros_ec_commands.h>
>>>> +#include <linux/platform_data/cros_ec_proto.h>
>>>> +#include <linux/units.h>
>>>> +
>>>> +#define DRV_NAME    "cros-ec-hwmon"
>>>> +
>>>> +struct cros_ec_hwmon_priv {
>>>> +    struct cros_ec_device *cros_ec;
>>>> +    u8 thermal_version;
>>>> +    const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
>>>> +};
>>>> +
>>>> +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
>>>> +{
>>>> +    u16 data;
>>>> +    int ret;
>>>> +
>>>> +    ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    data = le16_to_cpu(data);
>>>> +
>>>> +    if (data == EC_FAN_SPEED_NOT_PRESENT)
>>>> +        return -ENODEV;
>>>> +
>>>
>>> Don't forget it can also return `EC_FAN_SPEED_STALLED`.
>>
>> Thanks for the hint. I'll need to think about how to handle this better.
>>
>>> Like Guenter, I also don't like returning `-ENODEV`, but I don't have a
>>> problem with checking for `EC_FAN_SPEED_NOT_PRESENT` in case it was removed
>>> since init or something.
>>

That won't happen. Chromebooks are not servers, where one might be able to
replace a fan tray while the system is running.

>> Ok.
>>
>>> My approach was to return the speed as `0`, since the fan probably isn't
>>> spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
>>> HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
>>> No idea if this is correct though.
>>
>> I'm not a fan of returning a speed of 0 in case of errors.
>> Rather -EIO which can't be mistaken.
>> Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which should never happen)
>> and also for EC_FAN_SPEED_STALLED.
> 
> Yeah, that's pretty reasonable.
> 

-EIO is an i/o error. I have trouble reconciling that with
EC_FAN_SPEED_NOT_PRESENT or EC_FAN_SPEED_STALLED.

Looking into the EC source code [1], I see:

EC_FAN_SPEED_NOT_PRESENT means that the fan is not present.
That should return -ENODEV in the above code, but only for
the purpose of making the attribute invisible.

EC_FAN_SPEED_STALLED means exactly that, i.e., that the fan
is present but not turning. The EC code does not expect that
to happen and generates a thermal event in case it does.
Given that, it does make sense to set the fault flag.
The actual fan speed value should then be reported as 0 or
possibly -ENODATA. It should _not_ generate any other error
because that would trip up the "sensors" command for no
good reason.

Guenter

---
[1] https://chromium.googlesource.com/chromiumos/platform/ec

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Thomas Weißschuh]

On 2024-05-28 08:50:49+0000, Guenter Roeck wrote:
> On 5/27/24 17:15, Stephen Horvath wrote:
> > On 28/5/24 05:24, Thomas Weißschuh wrote:
> > > On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
> > > > I was the one to implement fan monitoring/control into Dustin's driver, and
> > > > just had a quick comment for your driver:
> > > > 
> > > > On 8/5/24 02:29, Thomas Weißschuh wrote:
> > > > > The ChromeOS Embedded Controller exposes fan speed and temperature
> > > > > readings.
> > > > > Expose this data through the hwmon subsystem.
> > > > > 
> > > > > The driver is designed to be probed via the cros_ec mfd device.
> > > > > 
> > > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > > ---
> > > > >    Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
> > > > >    Documentation/hwmon/index.rst         |   1 +
> > > > >    MAINTAINERS                           |   8 +
> > > > >    drivers/hwmon/Kconfig                 |  11 ++
> > > > >    drivers/hwmon/Makefile                |   1 +
> > > > >    drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
> > > > >    6 files changed, 316 insertions(+)
> > > > > 
> > > 
> > > <snip>
> > > 
> > > > > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > > > > new file mode 100644
> > > > > index 000000000000..d59d39df2ac4
> > > > > --- /dev/null
> > > > > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > > > > @@ -0,0 +1,269 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > > +/*
> > > > > + *  ChromesOS EC driver for hwmon
> > > > > + *
> > > > > + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
> > > > > + */
> > > > > +
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/hwmon.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/mod_devicetable.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/platform_data/cros_ec_commands.h>
> > > > > +#include <linux/platform_data/cros_ec_proto.h>
> > > > > +#include <linux/units.h>
> > > > > +
> > > > > +#define DRV_NAME    "cros-ec-hwmon"
> > > > > +
> > > > > +struct cros_ec_hwmon_priv {
> > > > > +    struct cros_ec_device *cros_ec;
> > > > > +    u8 thermal_version;
> > > > > +    const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> > > > > +};
> > > > > +
> > > > > +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> > > > > +{
> > > > > +    u16 data;
> > > > > +    int ret;
> > > > > +
> > > > > +    ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
> > > > > +    if (ret < 0)
> > > > > +        return ret;
> > > > > +
> > > > > +    data = le16_to_cpu(data);
> > > > > +
> > > > > +    if (data == EC_FAN_SPEED_NOT_PRESENT)
> > > > > +        return -ENODEV;
> > > > > +
> > > > 
> > > > Don't forget it can also return `EC_FAN_SPEED_STALLED`.
> > > 
> > > Thanks for the hint. I'll need to think about how to handle this better.
> > > 
> > > > Like Guenter, I also don't like returning `-ENODEV`, but I don't have a
> > > > problem with checking for `EC_FAN_SPEED_NOT_PRESENT` in case it was removed
> > > > since init or something.
> > > 
> 
> That won't happen. Chromebooks are not servers, where one might be able to
> replace a fan tray while the system is running.

In one of my testruns this actually happened.
When running on battery, one specific of the CPU sensors sporadically
returned EC_FAN_SPEED_NOT_PRESENT.

> > > Ok.
> > > 
> > > > My approach was to return the speed as `0`, since the fan probably isn't
> > > > spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
> > > > HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
> > > > No idea if this is correct though.
> > > 
> > > I'm not a fan of returning a speed of 0 in case of errors.
> > > Rather -EIO which can't be mistaken.
> > > Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which should never happen)
> > > and also for EC_FAN_SPEED_STALLED.
> > 
> > Yeah, that's pretty reasonable.
> > 
> 
> -EIO is an i/o error. I have trouble reconciling that with
> EC_FAN_SPEED_NOT_PRESENT or EC_FAN_SPEED_STALLED.
> 
> Looking into the EC source code [1], I see:
> 
> EC_FAN_SPEED_NOT_PRESENT means that the fan is not present.
> That should return -ENODEV in the above code, but only for
> the purpose of making the attribute invisible.
> 
> EC_FAN_SPEED_STALLED means exactly that, i.e., that the fan
> is present but not turning. The EC code does not expect that
> to happen and generates a thermal event in case it does.
> Given that, it does make sense to set the fault flag.
> The actual fan speed value should then be reported as 0 or
> possibly -ENODATA. It should _not_ generate any other error
> because that would trip up the "sensors" command for no
> good reason.

Ack.

Currently I have the following logic (for both fans and temp):

if NOT_PRESENT during probing:
  make the attribute invisible.

if any error during runtime (including NOT_PRESENT):
  return -ENODATA and a FAULT

This should also handle the sporadic NOT_PRESENT failures.

What do you think?

Is there any other feedback to this revision or should I send the next?


Thanks,
Thomas

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Guenter Roeck]

On 5/28/24 09:15, Thomas Weißschuh wrote:
> On 2024-05-28 08:50:49+0000, Guenter Roeck wrote:
>> On 5/27/24 17:15, Stephen Horvath wrote:
>>> On 28/5/24 05:24, Thomas Weißschuh wrote:
>>>> On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
>>>>> I was the one to implement fan monitoring/control into Dustin's driver, and
>>>>> just had a quick comment for your driver:
>>>>>
>>>>> On 8/5/24 02:29, Thomas Weißschuh wrote:
>>>>>> The ChromeOS Embedded Controller exposes fan speed and temperature
>>>>>> readings.
>>>>>> Expose this data through the hwmon subsystem.
>>>>>>
>>>>>> The driver is designed to be probed via the cros_ec mfd device.
>>>>>>
>>>>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>>>> ---
>>>>>>     Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
>>>>>>     Documentation/hwmon/index.rst         |   1 +
>>>>>>     MAINTAINERS                           |   8 +
>>>>>>     drivers/hwmon/Kconfig                 |  11 ++
>>>>>>     drivers/hwmon/Makefile                |   1 +
>>>>>>     drivers/hwmon/cros_ec_hwmon.c         | 269 ++++++++++++++++++++++++++++++++++
>>>>>>     6 files changed, 316 insertions(+)
>>>>>>
>>>>
>>>> <snip>
>>>>
>>>>>> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..d59d39df2ac4
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/hwmon/cros_ec_hwmon.c
>>>>>> @@ -0,0 +1,269 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>>>> +/*
>>>>>> + *  ChromesOS EC driver for hwmon
>>>>>> + *
>>>>>> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/device.h>
>>>>>> +#include <linux/hwmon.h>
>>>>>> +#include <linux/kernel.h>
>>>>>> +#include <linux/mod_devicetable.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +#include <linux/platform_data/cros_ec_commands.h>
>>>>>> +#include <linux/platform_data/cros_ec_proto.h>
>>>>>> +#include <linux/units.h>
>>>>>> +
>>>>>> +#define DRV_NAME    "cros-ec-hwmon"
>>>>>> +
>>>>>> +struct cros_ec_hwmon_priv {
>>>>>> +    struct cros_ec_device *cros_ec;
>>>>>> +    u8 thermal_version;
>>>>>> +    const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
>>>>>> +};
>>>>>> +
>>>>>> +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
>>>>>> +{
>>>>>> +    u16 data;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 2, 2, &data);
>>>>>> +    if (ret < 0)
>>>>>> +        return ret;
>>>>>> +
>>>>>> +    data = le16_to_cpu(data);
>>>>>> +
>>>>>> +    if (data == EC_FAN_SPEED_NOT_PRESENT)
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>
>>>>> Don't forget it can also return `EC_FAN_SPEED_STALLED`.
>>>>
>>>> Thanks for the hint. I'll need to think about how to handle this better.
>>>>
>>>>> Like Guenter, I also don't like returning `-ENODEV`, but I don't have a
>>>>> problem with checking for `EC_FAN_SPEED_NOT_PRESENT` in case it was removed
>>>>> since init or something.
>>>>
>>
>> That won't happen. Chromebooks are not servers, where one might be able to
>> replace a fan tray while the system is running.
> 
> In one of my testruns this actually happened.
> When running on battery, one specific of the CPU sensors sporadically
> returned EC_FAN_SPEED_NOT_PRESENT.
> 

What Chromebook was that ? I can't see the code path in the EC source
that would get me there.

>>>> Ok.
>>>>
>>>>> My approach was to return the speed as `0`, since the fan probably isn't
>>>>> spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
>>>>> HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
>>>>> No idea if this is correct though.
>>>>
>>>> I'm not a fan of returning a speed of 0 in case of errors.
>>>> Rather -EIO which can't be mistaken.
>>>> Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which should never happen)
>>>> and also for EC_FAN_SPEED_STALLED.
>>>
>>> Yeah, that's pretty reasonable.
>>>
>>
>> -EIO is an i/o error. I have trouble reconciling that with
>> EC_FAN_SPEED_NOT_PRESENT or EC_FAN_SPEED_STALLED.
>>
>> Looking into the EC source code [1], I see:
>>
>> EC_FAN_SPEED_NOT_PRESENT means that the fan is not present.
>> That should return -ENODEV in the above code, but only for
>> the purpose of making the attribute invisible.
>>
>> EC_FAN_SPEED_STALLED means exactly that, i.e., that the fan
>> is present but not turning. The EC code does not expect that
>> to happen and generates a thermal event in case it does.
>> Given that, it does make sense to set the fault flag.
>> The actual fan speed value should then be reported as 0 or
>> possibly -ENODATA. It should _not_ generate any other error
>> because that would trip up the "sensors" command for no
>> good reason.
> 
> Ack.
> 
> Currently I have the following logic (for both fans and temp):
> 
> if NOT_PRESENT during probing:
>    make the attribute invisible.
> 
> if any error during runtime (including NOT_PRESENT):
>    return -ENODATA and a FAULT
> 
> This should also handle the sporadic NOT_PRESENT failures.
> 
> What do you think?
> 
> Is there any other feedback to this revision or should I send the next?
> 

No, except I'd really like to know which Chromebook randomly generates
a EC_FAN_SPEED_NOT_PRESENT response because that really looks like a bug.
Also, can you reproduce the problem with the ectool command ?

Thanks,
Guenter

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Stephen Horvath]

Hi Guenter,

On 29/5/24 09:29, Guenter Roeck wrote:
> On 5/28/24 09:15, Thomas Weißschuh wrote:
>> On 2024-05-28 08:50:49+0000, Guenter Roeck wrote:
>>> On 5/27/24 17:15, Stephen Horvath wrote:
>>>> On 28/5/24 05:24, Thomas Weißschuh wrote:
>>>>> On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
>>>>>> I was the one to implement fan monitoring/control into Dustin's 
>>>>>> driver, and
>>>>>> just had a quick comment for your driver:
>>>>>>
>>>>>> On 8/5/24 02:29, Thomas Weißschuh wrote:
>>>>>>> The ChromeOS Embedded Controller exposes fan speed and temperature
>>>>>>> readings.
>>>>>>> Expose this data through the hwmon subsystem.
>>>>>>>
>>>>>>> The driver is designed to be probed via the cros_ec mfd device.
>>>>>>>
>>>>>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>>>>> ---
>>>>>>>     Documentation/hwmon/cros_ec_hwmon.rst |  26 ++++
>>>>>>>     Documentation/hwmon/index.rst         |   1 +
>>>>>>>     MAINTAINERS                           |   8 +
>>>>>>>     drivers/hwmon/Kconfig                 |  11 ++
>>>>>>>     drivers/hwmon/Makefile                |   1 +
>>>>>>>     drivers/hwmon/cros_ec_hwmon.c         | 269 
>>>>>>> ++++++++++++++++++++++++++++++++++
>>>>>>>     6 files changed, 316 insertions(+)
>>>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>>> diff --git a/drivers/hwmon/cros_ec_hwmon.c 
>>>>>>> b/drivers/hwmon/cros_ec_hwmon.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..d59d39df2ac4
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/hwmon/cros_ec_hwmon.c
>>>>>>> @@ -0,0 +1,269 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>>>>>> +/*
>>>>>>> + *  ChromesOS EC driver for hwmon
>>>>>>> + *
>>>>>>> + *  Copyright (C) 2024 Thomas Weißschuh <linux@weissschuh.net>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <linux/device.h>
>>>>>>> +#include <linux/hwmon.h>
>>>>>>> +#include <linux/kernel.h>
>>>>>>> +#include <linux/mod_devicetable.h>
>>>>>>> +#include <linux/module.h>
>>>>>>> +#include <linux/platform_device.h>
>>>>>>> +#include <linux/platform_data/cros_ec_commands.h>
>>>>>>> +#include <linux/platform_data/cros_ec_proto.h>
>>>>>>> +#include <linux/units.h>
>>>>>>> +
>>>>>>> +#define DRV_NAME    "cros-ec-hwmon"
>>>>>>> +
>>>>>>> +struct cros_ec_hwmon_priv {
>>>>>>> +    struct cros_ec_device *cros_ec;
>>>>>>> +    u8 thermal_version;
>>>>>>> +    const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + 
>>>>>>> EC_TEMP_SENSOR_B_ENTRIES];
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device 
>>>>>>> *cros_ec, u8 index, u16 *speed)
>>>>>>> +{
>>>>>>> +    u16 data;
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    ret = cros_ec->cmd_readmem(cros_ec, EC_MEMMAP_FAN + index * 
>>>>>>> 2, 2, &data);
>>>>>>> +    if (ret < 0)
>>>>>>> +        return ret;
>>>>>>> +
>>>>>>> +    data = le16_to_cpu(data);
>>>>>>> +
>>>>>>> +    if (data == EC_FAN_SPEED_NOT_PRESENT)
>>>>>>> +        return -ENODEV;
>>>>>>> +
>>>>>>
>>>>>> Don't forget it can also return `EC_FAN_SPEED_STALLED`.
>>>>>
>>>>> Thanks for the hint. I'll need to think about how to handle this 
>>>>> better.
>>>>>
>>>>>> Like Guenter, I also don't like returning `-ENODEV`, but I don't 
>>>>>> have a
>>>>>> problem with checking for `EC_FAN_SPEED_NOT_PRESENT` in case it 
>>>>>> was removed
>>>>>> since init or something.
>>>>>
>>>
>>> That won't happen. Chromebooks are not servers, where one might be 
>>> able to
>>> replace a fan tray while the system is running.
>>
>> In one of my testruns this actually happened.
>> When running on battery, one specific of the CPU sensors sporadically
>> returned EC_FAN_SPEED_NOT_PRESENT.
>>
> 
> What Chromebook was that ? I can't see the code path in the EC source
> that would get me there.
> 

I believe Thomas and I both have the Framework 13 AMD, the source code 
is here: 
https://github.com/FrameworkComputer/EmbeddedController/tree/lotus-zephyr

The organisation confuses me a little, but Dustin has previous said on 
the framework forums 
(https://community.frame.work/t/what-ec-is-used/38574/2):

"This one is based on the Zephyr port of the ChromeOS EC, and tracks 
mainline more closely. It is in the branch lotus-zephyr.
All of the model-specific code lives in zephyr/program/lotus.
The 13"-specific code lives in a few subdirectories off the main tree 
named azalea."


Also I just unplugged my fan and you are definitely correct, the EC only 
generates EC_FAN_SPEED_NOT_PRESENT for fans it does not have the 
capability to support. Even after a reboot it just returns 0 RPM for an 
unplugged fan. I thought about simulating a stall too, but I was mildly 
scared I was going to break one of the tiny blades.

>>>>> Ok.
>>>>>
>>>>>> My approach was to return the speed as `0`, since the fan probably 
>>>>>> isn't
>>>>>> spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
>>>>>> HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
>>>>>> No idea if this is correct though.
>>>>>
>>>>> I'm not a fan of returning a speed of 0 in case of errors.
>>>>> Rather -EIO which can't be mistaken.
>>>>> Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which should never 
>>>>> happen)
>>>>> and also for EC_FAN_SPEED_STALLED.
>>>>
>>>> Yeah, that's pretty reasonable.
>>>>
>>>
>>> -EIO is an i/o error. I have trouble reconciling that with
>>> EC_FAN_SPEED_NOT_PRESENT or EC_FAN_SPEED_STALLED.
>>>
>>> Looking into the EC source code [1], I see:
>>>
>>> EC_FAN_SPEED_NOT_PRESENT means that the fan is not present.
>>> That should return -ENODEV in the above code, but only for
>>> the purpose of making the attribute invisible.
>>>
>>> EC_FAN_SPEED_STALLED means exactly that, i.e., that the fan
>>> is present but not turning. The EC code does not expect that
>>> to happen and generates a thermal event in case it does.
>>> Given that, it does make sense to set the fault flag.
>>> The actual fan speed value should then be reported as 0 or
>>> possibly -ENODATA. It should _not_ generate any other error
>>> because that would trip up the "sensors" command for no
>>> good reason.
>>
>> Ack.
>>
>> Currently I have the following logic (for both fans and temp):
>>
>> if NOT_PRESENT during probing:
>>    make the attribute invisible.
>>
>> if any error during runtime (including NOT_PRESENT):
>>    return -ENODATA and a FAULT
>>
>> This should also handle the sporadic NOT_PRESENT failures.
>>
>> What do you think?
>>
>> Is there any other feedback to this revision or should I send the next?
>>
> 
> No, except I'd really like to know which Chromebook randomly generates
> a EC_FAN_SPEED_NOT_PRESENT response because that really looks like a bug.
> Also, can you reproduce the problem with the ectool command ?

I have a feeling it was related to the concurrency problems between ACPI 
and the CrOS code that are being fixed in another patch by Ben Walsh, I 
was also seeing some weird behaviour sometimes but I *believe* it was 
fixed by that.

Thanks,
Steve

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Thomas Weißschuh]

On 2024-05-29 10:58:23+0000, Stephen Horvath wrote:
> On 29/5/24 09:29, Guenter Roeck wrote:
> > On 5/28/24 09:15, Thomas Weißschuh wrote:
> > > On 2024-05-28 08:50:49+0000, Guenter Roeck wrote:
> > > > On 5/27/24 17:15, Stephen Horvath wrote:
> > > > > On 28/5/24 05:24, Thomas Weißschuh wrote:
> > > > > > On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
> > > > > > > Don't forget it can also return `EC_FAN_SPEED_STALLED`.

<snip>

> > > > > > 
> > > > > > Thanks for the hint. I'll need to think about how to
> > > > > > handle this better.
> > > > > > 
> > > > > > > Like Guenter, I also don't like returning `-ENODEV`,
> > > > > > > but I don't have a
> > > > > > > problem with checking for `EC_FAN_SPEED_NOT_PRESENT`
> > > > > > > in case it was removed
> > > > > > > since init or something.
> > > > > > 
> > > > 
> > > > That won't happen. Chromebooks are not servers, where one might
> > > > be able to
> > > > replace a fan tray while the system is running.
> > > 
> > > In one of my testruns this actually happened.
> > > When running on battery, one specific of the CPU sensors sporadically
> > > returned EC_FAN_SPEED_NOT_PRESENT.
> > > 
> > 
> > What Chromebook was that ? I can't see the code path in the EC source
> > that would get me there.
> > 
> 
> I believe Thomas and I both have the Framework 13 AMD, the source code is
> here:
> https://github.com/FrameworkComputer/EmbeddedController/tree/lotus-zephyr

Correct.

> The organisation confuses me a little, but Dustin has previous said on the
> framework forums (https://community.frame.work/t/what-ec-is-used/38574/2):
> 
> "This one is based on the Zephyr port of the ChromeOS EC, and tracks
> mainline more closely. It is in the branch lotus-zephyr.
> All of the model-specific code lives in zephyr/program/lotus.
> The 13"-specific code lives in a few subdirectories off the main tree named
> azalea."

The EC code is at [0]:

$ ectool version
RO version:    azalea_v3.4.113353-ec:b4c1fb,os
RW version:    azalea_v3.4.113353-ec:b4c1fb,os
Firmware copy: RO
Build info:    azalea_v3.4.113353-ec:b4c1fb,os:7b88e1,cmsis:4aa3ff 2024-03-26 07:10:22 lotus@ip-172-26-3-226
Tool version:  0.0.1-isolate May  6 2024 none


From the build info I gather it should be commit b4c1fb, which is the
current HEAD of the lotus-zephyr branch.
Lotus is the Framework 16 AMD, which is very similar to Azalea, the
Framework 13 AMD, which I tested this against.
Both share the same codebase.

> Also I just unplugged my fan and you are definitely correct, the EC only
> generates EC_FAN_SPEED_NOT_PRESENT for fans it does not have the capability
> to support. Even after a reboot it just returns 0 RPM for an unplugged fan.
> I thought about simulating a stall too, but I was mildly scared I was going
> to break one of the tiny blades.

I get the error when unplugging *the charger*.

To be more precise:

It does not happen always.
It does not happen instantly on unplugging.
It goes away after a few seconds/minutes.
During the issue, one specific sensor reads 0xffff.

> > > > > > Ok.
> > > > > > 
> > > > > > > My approach was to return the speed as `0`, since
> > > > > > > the fan probably isn't
> > > > > > > spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
> > > > > > > HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
> > > > > > > No idea if this is correct though.
> > > > > > 
> > > > > > I'm not a fan of returning a speed of 0 in case of errors.
> > > > > > Rather -EIO which can't be mistaken.
> > > > > > Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which
> > > > > > should never happen)
> > > > > > and also for EC_FAN_SPEED_STALLED.
> > > > > 
> > > > > Yeah, that's pretty reasonable.
> > > > > 
> > > > 
> > > > -EIO is an i/o error. I have trouble reconciling that with
> > > > EC_FAN_SPEED_NOT_PRESENT or EC_FAN_SPEED_STALLED.
> > > > 
> > > > Looking into the EC source code [1], I see:
> > > > 
> > > > EC_FAN_SPEED_NOT_PRESENT means that the fan is not present.
> > > > That should return -ENODEV in the above code, but only for
> > > > the purpose of making the attribute invisible.
> > > > 
> > > > EC_FAN_SPEED_STALLED means exactly that, i.e., that the fan
> > > > is present but not turning. The EC code does not expect that
> > > > to happen and generates a thermal event in case it does.
> > > > Given that, it does make sense to set the fault flag.
> > > > The actual fan speed value should then be reported as 0 or
> > > > possibly -ENODATA. It should _not_ generate any other error
> > > > because that would trip up the "sensors" command for no
> > > > good reason.
> > > 
> > > Ack.
> > > 
> > > Currently I have the following logic (for both fans and temp):
> > > 
> > > if NOT_PRESENT during probing:
> > >    make the attribute invisible.
> > > 
> > > if any error during runtime (including NOT_PRESENT):
> > >    return -ENODATA and a FAULT
> > > 
> > > This should also handle the sporadic NOT_PRESENT failures.
> > > 
> > > What do you think?
> > > 
> > > Is there any other feedback to this revision or should I send the next?
> > > 
> > 
> > No, except I'd really like to know which Chromebook randomly generates
> > a EC_FAN_SPEED_NOT_PRESENT response because that really looks like a bug.
> > Also, can you reproduce the problem with the ectool command ?

Yes, the ectool command reports the same issue at the same time.

The fan affected was always the sensor cpu@4c, which is
compatible = "amd,sb-tsi".

> I have a feeling it was related to the concurrency problems between ACPI and
> the CrOS code that are being fixed in another patch by Ben Walsh, I was also
> seeing some weird behaviour sometimes but I *believe* it was fixed by that.

I don't think it's this issue.
Ben's series at [1], is for MEC ECs which are the older Intel
Frameworks, not the Framework 13 AMD.

[0] https://github.com/FrameworkComputer/EmbeddedController
[1] https://lore.kernel.org/lkml/20240515055631.5775-1-ben@jubnut.com/

Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver

[Stephen Horvath]

Hi Thomas,

On 29/5/24 16:23, Thomas Weißschuh wrote:
> On 2024-05-29 10:58:23+0000, Stephen Horvath wrote:
>> On 29/5/24 09:29, Guenter Roeck wrote:
>>> On 5/28/24 09:15, Thomas Weißschuh wrote:
>>>> On 2024-05-28 08:50:49+0000, Guenter Roeck wrote:
>>>>> On 5/27/24 17:15, Stephen Horvath wrote:
>>>>>> On 28/5/24 05:24, Thomas Weißschuh wrote:
>>>>>>> On 2024-05-25 09:13:09+0000, Stephen Horvath wrote:
>>>>>>>> Don't forget it can also return `EC_FAN_SPEED_STALLED`.
> 
> <snip>
> 
>>>>>>>
>>>>>>> Thanks for the hint. I'll need to think about how to
>>>>>>> handle this better.
>>>>>>>
>>>>>>>> Like Guenter, I also don't like returning `-ENODEV`,
>>>>>>>> but I don't have a
>>>>>>>> problem with checking for `EC_FAN_SPEED_NOT_PRESENT`
>>>>>>>> in case it was removed
>>>>>>>> since init or something.
>>>>>>>
>>>>>
>>>>> That won't happen. Chromebooks are not servers, where one might
>>>>> be able to
>>>>> replace a fan tray while the system is running.
>>>>
>>>> In one of my testruns this actually happened.
>>>> When running on battery, one specific of the CPU sensors sporadically
>>>> returned EC_FAN_SPEED_NOT_PRESENT.
>>>>
>>>
>>> What Chromebook was that ? I can't see the code path in the EC source
>>> that would get me there.
>>>
>>
>> I believe Thomas and I both have the Framework 13 AMD, the source code is
>> here:
>> https://github.com/FrameworkComputer/EmbeddedController/tree/lotus-zephyr
> 
> Correct.
> 
>> The organisation confuses me a little, but Dustin has previous said on the
>> framework forums (https://community.frame.work/t/what-ec-is-used/38574/2):
>>
>> "This one is based on the Zephyr port of the ChromeOS EC, and tracks
>> mainline more closely. It is in the branch lotus-zephyr.
>> All of the model-specific code lives in zephyr/program/lotus.
>> The 13"-specific code lives in a few subdirectories off the main tree named
>> azalea."
> 
> The EC code is at [0]:
> 
> $ ectool version
> RO version:    azalea_v3.4.113353-ec:b4c1fb,os
> RW version:    azalea_v3.4.113353-ec:b4c1fb,os
> Firmware copy: RO
> Build info:    azalea_v3.4.113353-ec:b4c1fb,os:7b88e1,cmsis:4aa3ff 2024-03-26 07:10:22 lotus@ip-172-26-3-226
> Tool version:  0.0.1-isolate May  6 2024 none

I can confirm mine is the same build too.

>  From the build info I gather it should be commit b4c1fb, which is the
> current HEAD of the lotus-zephyr branch.
> Lotus is the Framework 16 AMD, which is very similar to Azalea, the
> Framework 13 AMD, which I tested this against.
> Both share the same codebase.
> 
>> Also I just unplugged my fan and you are definitely correct, the EC only
>> generates EC_FAN_SPEED_NOT_PRESENT for fans it does not have the capability
>> to support. Even after a reboot it just returns 0 RPM for an unplugged fan.
>> I thought about simulating a stall too, but I was mildly scared I was going
>> to break one of the tiny blades.
> 
> I get the error when unplugging *the charger*.
> 
> To be more precise:
> 
> It does not happen always.
> It does not happen instantly on unplugging.
> It goes away after a few seconds/minutes.
> During the issue, one specific sensor reads 0xffff.
> 

Oh I see, I haven't played around with the temp sensors until now, but I 
can confirm the last temp sensor (cpu@4c / temp4) will randomly (every 
~2-15 seconds) return EC_TEMP_SENSOR_ERROR (0xfe).
Unplugging the charger doesn't seem to have any impact for me.
The related ACPI sensor also says 180.8°C.
I'll probably create an issue or something shortly.

I was mildly confused by 'CPU sensors' and 'EC_FAN_SPEED_NOT_PRESENT' in 
the same sentence, but I'm now assuming you mean the temp sensor?

>>>>>>> Ok.
>>>>>>>
>>>>>>>> My approach was to return the speed as `0`, since
>>>>>>>> the fan probably isn't
>>>>>>>> spinning, but set HWMON_F_FAULT for `EC_FAN_SPEED_NOT_PRESENT` and
>>>>>>>> HWMON_F_ALARM for `EC_FAN_SPEED_STALLED`.
>>>>>>>> No idea if this is correct though.
>>>>>>>
>>>>>>> I'm not a fan of returning a speed of 0 in case of errors.
>>>>>>> Rather -EIO which can't be mistaken.
>>>>>>> Maybe -EIO for both EC_FAN_SPEED_NOT_PRESENT (which
>>>>>>> should never happen)
>>>>>>> and also for EC_FAN_SPEED_STALLED.
>>>>>>
>>>>>> Yeah, that's pretty reasonable.
>>>>>>
>>>>>
>>>>> -EIO is an i/o error. I have trouble reconciling that with
>>>>> EC_FAN_SPEED_NOT_PRESENT or EC_FAN_SPEED_STALLED.
>>>>>
>>>>> Looking into the EC source code [1], I see:
>>>>>
>>>>> EC_FAN_SPEED_NOT_PRESENT means that the fan is not present.
>>>>> That should return -ENODEV in the above code, but only for
>>>>> the purpose of making the attribute invisible.
>>>>>
>>>>> EC_FAN_SPEED_STALLED means exactly that, i.e., that the fan
>>>>> is present but not turning. The EC code does not expect that
>>>>> to happen and generates a thermal event in case it does.
>>>>> Given that, it does make sense to set the fault flag.
>>>>> The actual fan speed value should then be reported as 0 or
>>>>> possibly -ENODATA. It should _not_ generate any other error
>>>>> because that would trip up the "sensors" command for no
>>>>> good reason.
>>>>
>>>> Ack.
>>>>
>>>> Currently I have the following logic (for both fans and temp):
>>>>
>>>> if NOT_PRESENT during probing:
>>>>     make the attribute invisible.
>>>>
>>>> if any error during runtime (including NOT_PRESENT):
>>>>     return -ENODATA and a FAULT
>>>>
>>>> This should also handle the sporadic NOT_PRESENT failures.
>>>>
>>>> What do you think?
>>>>
>>>> Is there any other feedback to this revision or should I send the next?
>>>>
>>>
>>> No, except I'd really like to know which Chromebook randomly generates
>>> a EC_FAN_SPEED_NOT_PRESENT response because that really looks like a bug.
>>> Also, can you reproduce the problem with the ectool command ?
> 
> Yes, the ectool command reports the same issue at the same time.
> 
> The fan affected was always the sensor cpu@4c, which is
> compatible = "amd,sb-tsi".
> 
>> I have a feeling it was related to the concurrency problems between ACPI and
>> the CrOS code that are being fixed in another patch by Ben Walsh, I was also
>> seeing some weird behaviour sometimes but I *believe* it was fixed by that.
> 
> I don't think it's this issue.
> Ben's series at [1], is for MEC ECs which are the older Intel
> Frameworks, not the Framework 13 AMD.

Yeah sorry, I saw it mentioned AMD and threw it into my kernel, I also 
thought it stopped the 'packet too long' messages (for 
EC_CMD_CONSOLE_SNAPSHOT) but it did not.

Thanks,
Steve