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]/
Signed-off-by: Thomas Weißschuh <[email protected]>
---
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 | 279 ++++++++++++++++++++++++++++++++++
drivers/mfd/cros_ec_dev.c | 1 +
7 files changed, 327 insertions(+)
---
base-commit: 2fbe479c0024e1c6b992184a799055e19932aa48
change-id: 20240506-cros_ec-hwmon-24634b07cf6f
Best regards,
--
Thomas Weißschuh <[email protected]>
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..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
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 | 279 ++++++++++++++++++++++++++++++++++
6 files changed, 326 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 <[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 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 <[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 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..9588df202a74
--- /dev/null
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -0,0 +1,279 @@
+// 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/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)
+{
+ int ret;
+ u16 data;
+
+ if (index >= EC_FAN_SPEED_ENTRIES)
+ return -ENODEV;
+
+ 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 (thermal_version < 2 && index >= EC_TEMP_SENSOR_ENTRIES)
+ return -ENODEV;
+
+ if (index >= EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES)
+ return -ENODEV;
+
+ 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);
+ u16 speed;
+ u8 temp;
+ int ret = -ENODATA;
+
+ 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);
+ int ret = -ENODATA;
+
+ if (type == hwmon_temp && attr == hwmon_temp_label) {
+ *str = priv->temp_sensor_names[channel];
+ ret = 0;
+ }
+
+ return ret;
+}
+
+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 int 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 i;
+ u8 temp;
+ int ret;
+
+ ret = priv->cros_ec->cmd_readmem(priv->cros_ec, EC_MEMMAP_THERMAL_VERSION,
+ 1, &priv->thermal_version);
+ if (ret < 0)
+ return ret;
+
+ if (!priv->thermal_version)
+ return 0;
+
+ for (i = 0; i < ARRAY_SIZE(priv->temp_sensor_names); 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);
+ }
+
+ return 0;
+}
+
+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;
+
+ BUILD_BUG_ON(ARRAY_SIZE(priv->temp_sensor_names) != 24);
+
+ /* Not every platform supports direct reads */
+ if (!cros_ec->cmd_readmem)
+ return -ENOTTY;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->cros_ec = cros_ec;
+
+ ret = cros_ec_hwmon_probe_temp_sensors(dev, priv);
+ if (ret < 0)
+ return ret;
+
+ 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.0
On 5/7/2024 08:57, 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 <[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 | 279 ++++++++++++++++++++++++++++++++++
> 6 files changed, 326 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 <[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 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 <[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 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..9588df202a74
> --- /dev/null
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -0,0 +1,279 @@
> +// 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/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)
> +{
> + int ret;
> + u16 data;
> +
> + if (index >= EC_FAN_SPEED_ENTRIES)
> + return -ENODEV;
I could see an argument that this should be -EINVAL as
EC_FAN_SPEED_ENTRIES is hardcoded in the driver and "index" was passed
into the function.
> +
> + 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;
For safety purposes I think you should either do
if (speed)
*speed = data;
Or have a check at the start of the function and return -EINVAL and
throw up a warning if speed was NULL.
> + 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 (thermal_version < 2 && index >= EC_TEMP_SENSOR_ENTRIES)
> + return -ENODEV;
> +
> + if (index >= EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES)
> + return -ENODEV;
Like cros_ec_hwmon_read_fan_speed I have an opinion these should be -EINVAL.
> +
> + 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;
> +
You should make sure that data isn't NULL before doing these checks:
> + 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);
> + u16 speed;
> + u8 temp;
> + int ret = -ENODATA;
I feel it is better practice to add this as a 3rd clause than initialize
the variable at the start of the function.
switch (type) {
case hwmon_fan:
//do stuff
break;
case hwmon_temp:
// dot stuff
break;
default:
ret = -ENODATA;
}
return ret;
> +
> + 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;
Make sure that resp isn't NULL.
> + 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);
> + int ret = -ENODATA;
How about just drop the variable and then do below two comments:
> +
> + if (type == hwmon_temp && attr == hwmon_temp_label) {
> + *str = priv->temp_sensor_names[channel];
> + ret = 0;
return 0;
> + }
> +
> + return ret;
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 int 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 i;
> + u8 temp;
> + int ret;
> +
> + ret = priv->cros_ec->cmd_readmem(priv->cros_ec, EC_MEMMAP_THERMAL_VERSION,
> + 1, &priv->thermal_version);
> + if (ret < 0)
> + return ret;
> +
> + if (!priv->thermal_version)
> + return 0;
I'm a bit confused; why isn't this -ENODEV? I would think that you
don't want to have probe succeed and make devices if there isn't thermal
support in the EC.
> +
> + for (i = 0; i < ARRAY_SIZE(priv->temp_sensor_names); 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);
> + }
> +
> + return 0;
> +}
> +
> +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;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(priv->temp_sensor_names) != 24);
> +
> + /* Not every platform supports direct reads */
> + if (!cros_ec->cmd_readmem)
> + return -ENOTTY;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->cros_ec = cros_ec;
> +
> + ret = cros_ec_hwmon_probe_temp_sensors(dev, priv);
> + if (ret < 0)
> + return ret;
> +
> + 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");
>
On Tue, May 7, 2024 at 8:30 AM Mario Limonciello
<[email protected]> wrote:
>
> On 5/7/2024 08:57, 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 <[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 | 279 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 326 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 <[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 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 <[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 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..9588df202a74
> > --- /dev/null
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -0,0 +1,279 @@
> > +// 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/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)
> > +{
> > + int ret;
> > + u16 data;
> > +
> > + if (index >= EC_FAN_SPEED_ENTRIES)
> > + return -ENODEV;
>
> I could see an argument that this should be -EINVAL as
> EC_FAN_SPEED_ENTRIES is hardcoded in the driver and "index" was passed
> into the function.
>
> > +
> > + 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;
>
> For safety purposes I think you should either do
>
> if (speed)
> *speed = data;
>
NACK. This is a static function. Callers are well known. If speed is
NULL this code deserves to crash.
> Or have a check at the start of the function and return -EINVAL and
> throw up a warning if speed was NULL.
>
> > + 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 (thermal_version < 2 && index >= EC_TEMP_SENSOR_ENTRIES)
> > + return -ENODEV;
> > +
> > + if (index >= EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES)
> > + return -ENODEV;
>
> Like cros_ec_hwmon_read_fan_speed I have an opinion these should be -EINVAL.
>
Again, this is a static function. The caller is in full control of the
parameter range, and those error checks should be unnecessary to start
with.
I am not going to review the rest of the driver. Drop all unnecessary
range checks and resubmit.
Guenter
> > +
> > + 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;
> > +
>
> You should make sure that data isn't NULL before doing these checks:
>
> > + 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);
> > + u16 speed;
> > + u8 temp;
> > + int ret = -ENODATA;
>
> I feel it is better practice to add this as a 3rd clause than initialize
> the variable at the start of the function.
>
> switch (type) {
> case hwmon_fan:
> //do stuff
> break;
> case hwmon_temp:
> // dot stuff
> break;
> default:
> ret = -ENODATA;
> }
>
> return ret;
>
> > +
> > + 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;
>
> Make sure that resp isn't NULL.
>
> > + 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);
> > + int ret = -ENODATA;
>
> How about just drop the variable and then do below two comments:
>
> > +
> > + if (type == hwmon_temp && attr == hwmon_temp_label) {
> > + *str = priv->temp_sensor_names[channel];
> > + ret = 0;
> return 0;
> > + }
> > +
> > + return ret;
> 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 int 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 i;
> > + u8 temp;
> > + int ret;
> > +
> > + ret = priv->cros_ec->cmd_readmem(priv->cros_ec, EC_MEMMAP_THERMAL_VERSION,
> > + 1, &priv->thermal_version);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!priv->thermal_version)
> > + return 0;
>
> I'm a bit confused; why isn't this -ENODEV? I would think that you
> don't want to have probe succeed and make devices if there isn't thermal
> support in the EC.
>
> > +
> > + for (i = 0; i < ARRAY_SIZE(priv->temp_sensor_names); 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);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +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;
> > +
> > + BUILD_BUG_ON(ARRAY_SIZE(priv->temp_sensor_names) != 24);
> > +
> > + /* Not every platform supports direct reads */
> > + if (!cros_ec->cmd_readmem)
> > + return -ENOTTY;
> > +
> > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->cros_ec = cros_ec;
> > +
> > + ret = cros_ec_hwmon_probe_temp_sensors(dev, priv);
> > + if (ret < 0)
> > + return ret;
> > +
> > + 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");
> >
>