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/[email protected]/
[1] https://lore.kernel.org/all/[email protected]/
To: Jean Delvare <[email protected]>
To: Guenter Roeck <[email protected]>
To: Benson Leung <[email protected]>
To: Lee Jones <[email protected]>
To: Tzung-Bi Shih <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Dustin Howett <[email protected]>
Cc: Mario Limonciello <[email protected]>
Cc: Moritz Fischer <[email protected]>
Cc: Stephen Horvath <[email protected]>
Cc: Rajas Paranjpe <[email protected]>
Changes in v3:
- Drop Mario's Reviewed-by tag, as the code has changed
- Introduce cros_ec_cmd_readmem() for non-LPC compatibility
- Report fault state for fans and temp sensors
- Avoid adding unnecessary space characters to channel label
- Drop thermal_version from priv data
- Read fans during probing only once
- Don't include linux/kernel.h
- Move _read_temp_sensor_info to similar functions
- Insert MFD entry alphabetically
- Link to v2: https://lore.kernel.org/r/[email protected]
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/[email protected]
---
Thomas Weißschuh (3):
platform/chrome: cros_ec_proto: Introduce cros_ec_cmd_readmem()
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 | 291 ++++++++++++++++++++++++++++
drivers/mfd/cros_ec_dev.c | 1 +
drivers/platform/chrome/cros_ec_proto.c | 27 +++
include/linux/platform_data/cros_ec_proto.h | 2 +
9 files changed, 368 insertions(+)
---
base-commit: 2bfcfd584ff5ccc8bb7acde19b42570414bf880b
change-id: 20240506-cros_ec-hwmon-24634b07cf6f
Best regards,
--
Thomas Weißschuh <[email protected]>
To read from the EC memory different mechanism are possible.
ECs connected via LPC expose their memory via a ->cmd_readmem operation.
Other protocols require the usage of EC_CMD_READ_MEMMAP, which on the
other hand is not implemented by LPC ECs.
Provide a helper that automatically selects the correct mechanism.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 27 +++++++++++++++++++++++++++
include/linux/platform_data/cros_ec_proto.h | 2 ++
2 files changed, 29 insertions(+)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 945b1b15a04c..4b48fa1fe3e0 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -1035,3 +1035,30 @@ int cros_ec_cmd(struct cros_ec_device *ec_dev,
return ret;
}
EXPORT_SYMBOL_GPL(cros_ec_cmd);
+
+/**
+ * cros_ec_cmd_readmem - Read from EC memory.
+ *
+ * @ec_dev: EC device
+ * @offset: Is within EC_LPC_ADDR_MEMMAP region.
+ * @size: Number of bytes to read. zero means "read a string" (including
+ * the trailing '\0').
+ * Caller must ensure that the buffer is large enough for the
+ * result when reading a string.
+ * @dest: EC command output data
+ *
+ * Return: >= 0 on success, negative error number on failure.
+ */
+int cros_ec_cmd_readmem(struct cros_ec_device *ec_dev, u8 offset, u8 size, void *dest)
+{
+ struct ec_params_read_memmap params;
+
+ if (ec_dev->cmd_readmem)
+ return ec_dev->cmd_readmem(ec_dev, offset, size, dest);
+
+ params.offset = offset;
+ params.size = size;
+ return cros_ec_cmd(ec_dev, 0, EC_CMD_READ_MEMMAP,
+ ¶ms, sizeof(params), dest, size);
+}
+EXPORT_SYMBOL_GPL(cros_ec_cmd_readmem);
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 8865e350c12a..1ddc52603f9a 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -261,6 +261,8 @@ int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
int cros_ec_cmd(struct cros_ec_device *ec_dev, unsigned int version, int command, const void *outdata,
size_t outsize, void *indata, size_t insize);
+int cros_ec_cmd_readmem(struct cros_ec_device *ec_dev, u8 offset, u8 size, void *dest);
+
/**
* cros_ec_get_time_ns() - Return time in ns.
*
--
2.45.1
Add ChromeOS EC-based hardware monitoring as EC subdevice.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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..1262d1f8d954 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -130,6 +130,7 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
static const struct mfd_cell cros_ec_platform_cells[] = {
{ .name = "cros-ec-chardev", },
{ .name = "cros-ec-debugfs", },
+ { .name = "cros-ec-hwmon", },
{ .name = "cros-ec-sysfs", },
};
--
2.45.1
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 <[email protected]>
---
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 | 291 ++++++++++++++++++++++++++++++++++
6 files changed, 338 insertions(+)
diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
new file mode 100644
index 000000000000..47ecae983bdb
--- /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.
+
+ Prefix: 'cros_ec'
+
+ Addresses scanned: -
+
+Author:
+
+ - Thomas Weißschuh <[email protected]>
+
+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 03d313af469a..342ea5deba24 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -58,6 +58,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 d6c90161c7bf..0c85aa5073e7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5135,6 +5135,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 <[email protected]>
+L: [email protected]
+L: [email protected]
+S: Maintained
+F: Documentation/hwmon/chros_ec_hwmon.rst
+F: drivers/hwmon/cros_ec_hwmon.c
+
CHROMEOS EC SUBDRIVERS
M: Benson Leung <[email protected]>
R: Guenter Roeck <[email protected]>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e14ae18a973b..702dc45ea405 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 e3f25475d1f0..4fb14dd1eafd 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..2d9c1f4ec09a
--- /dev/null
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ChromesOS EC driver for hwmon
+ *
+ * Copyright (C) 2024 Thomas Weißschuh <[email protected]>
+ */
+
+#include <linux/device.h>
+#include <linux/hwmon.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/types.h>
+#include <linux/units.h>
+
+#define DRV_NAME "cros-ec-hwmon"
+
+struct cros_ec_hwmon_priv {
+ struct cros_ec_device *cros_ec;
+ const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
+ u8 usable_fans;
+};
+
+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);
+ *speed = data;
+
+ if (data == EC_FAN_SPEED_NOT_PRESENT || data == EC_FAN_SPEED_STALLED)
+ return -EIO;
+ return 0;
+}
+
+static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, 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 -EIO;
+
+ return 0;
+}
+
+static int cros_ec_hwmon_read_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(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 = 0;
+ u8 temp = 0;
+
+ if (type == hwmon_fan && attr == hwmon_fan_input) {
+ ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
+ if (ret == 0)
+ *val = speed;
+ } else if (type == hwmon_fan && attr == hwmon_fan_fault) {
+ ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
+ if (ret == -EIO && speed == EC_FAN_SPEED_STALLED)
+ ret = 0;
+ if (ret == 0)
+ *val = speed == EC_FAN_SPEED_STALLED;
+ } else if (type == hwmon_temp && attr == hwmon_temp_input) {
+ ret = cros_ec_hwmon_read_temp(priv->cros_ec, channel, &temp);
+ if (ret == 0)
+ *val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
+ } else if (type == hwmon_temp && attr == hwmon_temp_fault) {
+ ret = cros_ec_hwmon_read_temp(priv->cros_ec, channel, &temp);
+ if (ret == -EIO && speed == EC_TEMP_SENSOR_ERROR)
+ ret = 0;
+ if (ret == 0)
+ *val = temp == EC_TEMP_SENSOR_ERROR;
+ }
+
+ return ret;
+}
+
+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;
+
+ if (type == hwmon_fan) {
+ if (priv->usable_fans & BIT(channel))
+ 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_FAULT,
+ HWMON_F_INPUT | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_FAULT),
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
+ HWMON_T_INPUT | HWMON_T_FAULT | 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,
+ u8 thermal_version)
+{
+ struct ec_response_temp_sensor_get_info info;
+ size_t candidates, i, sensor_name_size;
+ int ret;
+ u8 temp;
+
+ if (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, i, &temp) != 0)
+ continue;
+
+ ret = cros_ec_hwmon_read_temp_sensor_info(priv->cros_ec, i, &info);
+ if (ret < 0)
+ continue;
+
+ sensor_name_size = strnlen(info.sensor_name, sizeof(info.sensor_name));
+ priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%*s",
+ (int)sensor_name_size,
+ info.sensor_name);
+ }
+}
+
+static void cros_ec_hwmon_probe_fans(struct device *dev, struct cros_ec_hwmon_priv *priv)
+{
+ u16 speed;
+ size_t i;
+ int ret;
+
+ for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
+ speed = EC_FAN_SPEED_NOT_PRESENT;
+ ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, i, &speed);
+ if (ret == 0 || speed != EC_FAN_SPEED_NOT_PRESENT)
+ priv->usable_fans |= BIT(i);
+ }
+}
+
+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;
+ u8 thermal_version;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ ret = cros_ec_cmd_readmem(cros_ec, EC_MEMMAP_THERMAL_VERSION, 1, &thermal_version);
+ if (ret < 0)
+ return ret;
+
+ /* Covers both fan and temp sensors */
+ if (thermal_version == 0)
+ return -ENODEV;
+
+ priv->cros_ec = cros_ec;
+
+ cros_ec_hwmon_probe_temp_sensors(dev, priv, thermal_version);
+ cros_ec_hwmon_probe_fans(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 <[email protected]");
+MODULE_LICENSE("GPL");
--
2.45.1
On Mon, May 27, 2024 at 10:58:31PM +0200, Thomas Wei?schuh wrote:
> +/**
> + * cros_ec_cmd_readmem - Read from EC memory.
> + *
> + * @ec_dev: EC device
> + * @offset: Is within EC_LPC_ADDR_MEMMAP region.
> + * @size: Number of bytes to read. zero means "read a string" (including
> + * the trailing '\0').
The behavior is cros_ec_lpc_readmem() only but not for cros_ec_cmd().
On Mon, May 27, 2024 at 10:58:32PM +0200, Thomas Wei?schuh wrote:
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
[...]
> + * ChromesOS EC driver for hwmon
s/ChromesOS/ChromeOS/.
> +struct cros_ec_hwmon_priv {
> + struct cros_ec_device *cros_ec;
> + const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> + u8 usable_fans;
> +};
> +
> +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);
> + *speed = data;
> +
> + if (data == EC_FAN_SPEED_NOT_PRESENT || data == EC_FAN_SPEED_STALLED)
> + return -EIO;
`data` could be eliminated; use `*speed` instead.
> +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
[...]
> + u16 speed = 0;
> + u8 temp = 0;
They don't need to initialize.
> + if (type == hwmon_fan && attr == hwmon_fan_input) {
> + ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> + if (ret == 0)
> + *val = speed;
> + } else if (type == hwmon_fan && attr == hwmon_fan_fault) {
> + ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> + if (ret == -EIO && speed == EC_FAN_SPEED_STALLED)
> + ret = 0;
> + if (ret == 0)
> + *val = speed == EC_FAN_SPEED_STALLED;
> + } else if (type == hwmon_temp && attr == hwmon_temp_input) {
> + ret = cros_ec_hwmon_read_temp(priv->cros_ec, channel, &temp);
> + if (ret == 0)
> + *val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
> + } else if (type == hwmon_temp && attr == hwmon_temp_fault) {
> + ret = cros_ec_hwmon_read_temp(priv->cros_ec, channel, &temp);
> + if (ret == -EIO && speed == EC_TEMP_SENSOR_ERROR)
> + ret = 0;
> + if (ret == 0)
> + *val = temp == EC_TEMP_SENSOR_ERROR;
> + }
Refactoring them by switch .. case .. may improve the readability.
On 2024-05-28 06:39:25+0000, Tzung-Bi Shih wrote:
> On Mon, May 27, 2024 at 10:58:32PM +0200, Thomas Weißschuh wrote:
> > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> [...]
> > + * ChromesOS EC driver for hwmon
>
> s/ChromesOS/ChromeOS/.
Ack. Copy-and-paste...
> > +struct cros_ec_hwmon_priv {
> > + struct cros_ec_device *cros_ec;
> > + const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> > + u8 usable_fans;
> > +};
> > +
> > +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);
> > + *speed = data;
> > +
> > + if (data == EC_FAN_SPEED_NOT_PRESENT || data == EC_FAN_SPEED_STALLED)
> > + return -EIO;
>
> `data` could be eliminated; use `*speed` instead.
Then the le16 value would need to be written directly to the out
parameter. The current usage relies on *speed (sometimes) being set even
if ret != 0.
(See next response block)
>
> > +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> [...]
> > + u16 speed = 0;
> > + u8 temp = 0;
>
> They don't need to initialize.
They need to.
The logic
if (ret == -EIO && speed == EC_FAN_SPEED_STALLED)
ret = 0;
relies on -EIO and a write to speed from cros_ec_hwmon_read_fan_speed().
But if cros_ec_cmd_readmem() already returns -EIO, then speed would be
uninitialized.
I'll see if I can make this clearer somehow.
>
> > + if (type == hwmon_fan && attr == hwmon_fan_input) {
> > + ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> > + if (ret == 0)
> > + *val = speed;
> > + } else if (type == hwmon_fan && attr == hwmon_fan_fault) {
> > + ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> > + if (ret == -EIO && speed == EC_FAN_SPEED_STALLED)
> > + ret = 0;
> > + if (ret == 0)
> > + *val = speed == EC_FAN_SPEED_STALLED;
> > + } else if (type == hwmon_temp && attr == hwmon_temp_input) {
> > + ret = cros_ec_hwmon_read_temp(priv->cros_ec, channel, &temp);
> > + if (ret == 0)
> > + *val = kelvin_to_millicelsius((((long)temp) + EC_TEMP_SENSOR_OFFSET));
> > + } else if (type == hwmon_temp && attr == hwmon_temp_fault) {
> > + ret = cros_ec_hwmon_read_temp(priv->cros_ec, channel, &temp);
> > + if (ret == -EIO && speed == EC_TEMP_SENSOR_ERROR)
> > + ret = 0;
> > + if (ret == 0)
> > + *val = temp == EC_TEMP_SENSOR_ERROR;
> > + }
>
> Refactoring them by switch .. case .. may improve the readability.
It would introduce another level of indentation, which I tried to avoid.
But some vertical whitespace would be useful indeed.
On 2024-05-28 06:39:03+0000, Tzung-Bi Shih wrote:
> On Mon, May 27, 2024 at 10:58:31PM +0200, Thomas Weißschuh wrote:
> > +/**
> > + * cros_ec_cmd_readmem - Read from EC memory.
> > + *
> > + * @ec_dev: EC device
> > + * @offset: Is within EC_LPC_ADDR_MEMMAP region.
> > + * @size: Number of bytes to read. zero means "read a string" (including
> > + * the trailing '\0').
>
> The behavior is cros_ec_lpc_readmem() only but not for cros_ec_cmd().
Indeed.
I thought I checked for this specifically, but got it wrong.
I'll drop the docs and add a
if (!size)
return -EINVAL;
to make sure nobody starts relying on that behaviour when writing a
driver against an LPC EC.
On Tue, May 28, 2024 at 09:09:28AM +0200, Thomas Wei?schuh wrote:
> On 2024-05-28 06:39:25+0000, Tzung-Bi Shih wrote:
> > On Mon, May 27, 2024 at 10:58:32PM +0200, Thomas Wei?schuh wrote:
> > > +struct cros_ec_hwmon_priv {
> > > + struct cros_ec_device *cros_ec;
> > > + const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
> > > + u8 usable_fans;
> > > +};
> > > +
> > > +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);
> > > + *speed = data;
> > > +
> > > + if (data == EC_FAN_SPEED_NOT_PRESENT || data == EC_FAN_SPEED_STALLED)
> > > + return -EIO;
> >
> > `data` could be eliminated; use `*speed` instead.
>
> Then the le16 value would need to be written directly to the out
> parameter. The current usage relies on *speed (sometimes) being set even
> if ret != 0.
>
> (See next response block)
>
> >
> > > +static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > > + u32 attr, int channel, long *val)
> > > +{
> > [...]
> > > + u16 speed = 0;
> > > + u8 temp = 0;
> >
> > They don't need to initialize.
>
> They need to.
>
> The logic
>
> if (ret == -EIO && speed == EC_FAN_SPEED_STALLED)
> ret = 0;
>
> relies on -EIO and a write to speed from cros_ec_hwmon_read_fan_speed().
>
> But if cros_ec_cmd_readmem() already returns -EIO, then speed would be
> uninitialized.
I see.