Re: [PATCH v2 1/2] hwmon: add ChromeOS EC driver
From: Stephen Horvath
Date: Mon May 27 2024 - 20:15:30 EST
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@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxx>
+ */
+
+#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