* [PATCH 0/2] ChromeOS Embedded controller hwmon driver
@ 2024-05-07 13:57 Thomas Weißschuh
2024-05-07 13:57 ` [PATCH 1/2] hwmon: add ChromeOS EC driver Thomas Weißschuh
2024-05-07 13:57 ` [PATCH 2/2] mfd: cros_ec: Register hardware monitoring subdevice Thomas Weißschuh
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Weißschuh @ 2024-05-07 13:57 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Benson Leung, Lee Jones
Cc: Guenter Roeck, linux-kernel, linux-hwmon, chrome-platform,
Dustin Howett, Mario Limonciello, Moritz Fischer,
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>
---
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 <linux@weissschuh.net>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] hwmon: add ChromeOS EC driver
2024-05-07 13:57 [PATCH 0/2] ChromeOS Embedded controller hwmon driver Thomas Weißschuh
@ 2024-05-07 13:57 ` Thomas Weißschuh
2024-05-07 14:30 ` Mario Limonciello
2024-05-07 13:57 ` [PATCH 2/2] mfd: cros_ec: Register hardware monitoring subdevice Thomas Weißschuh
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Weißschuh @ 2024-05-07 13:57 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Benson Leung, Lee Jones
Cc: Guenter Roeck, linux-kernel, linux-hwmon, chrome-platform,
Dustin Howett, Mario Limonciello, Moritz Fischer,
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 | 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 <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..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 <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)
+{
+ 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 <linux@weissschuh.net");
+MODULE_LICENSE("GPL");
--
2.45.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] mfd: cros_ec: Register hardware monitoring subdevice
2024-05-07 13:57 [PATCH 0/2] ChromeOS Embedded controller hwmon driver Thomas Weißschuh
2024-05-07 13:57 ` [PATCH 1/2] hwmon: add ChromeOS EC driver Thomas Weißschuh
@ 2024-05-07 13:57 ` Thomas Weißschuh
1 sibling, 0 replies; 5+ messages in thread
From: Thomas Weißschuh @ 2024-05-07 13:57 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, Benson Leung, Lee Jones
Cc: Guenter Roeck, linux-kernel, linux-hwmon, chrome-platform,
Dustin Howett, Mario Limonciello, Moritz Fischer,
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hwmon: add ChromeOS EC driver
2024-05-07 13:57 ` [PATCH 1/2] hwmon: add ChromeOS EC driver Thomas Weißschuh
@ 2024-05-07 14:30 ` Mario Limonciello
2024-05-07 15:38 ` Guenter Roeck
0 siblings, 1 reply; 5+ messages in thread
From: Mario Limonciello @ 2024-05-07 14:30 UTC (permalink / raw)
To: Thomas Weißschuh, Jean Delvare, Guenter Roeck, Benson Leung,
Lee Jones
Cc: Guenter Roeck, linux-kernel, linux-hwmon, chrome-platform,
Dustin Howett, Moritz Fischer
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 <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 | 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 <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..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 <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)
> +{
> + 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 <linux@weissschuh.net");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] hwmon: add ChromeOS EC driver
2024-05-07 14:30 ` Mario Limonciello
@ 2024-05-07 15:38 ` Guenter Roeck
0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2024-05-07 15:38 UTC (permalink / raw)
To: Mario Limonciello
Cc: Thomas Weißschuh, Jean Delvare, Guenter Roeck, Benson Leung,
Lee Jones, Guenter Roeck, linux-kernel, linux-hwmon,
chrome-platform, Dustin Howett, Moritz Fischer
On Tue, May 7, 2024 at 8:30 AM Mario Limonciello
<mario.limonciello@amd.com> 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 <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 | 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 <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..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 <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)
> > +{
> > + 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 <linux@weissschuh.net");
> > +MODULE_LICENSE("GPL");
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-05-07 15:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-07 13:57 [PATCH 0/2] ChromeOS Embedded controller hwmon driver Thomas Weißschuh
2024-05-07 13:57 ` [PATCH 1/2] hwmon: add ChromeOS EC driver Thomas Weißschuh
2024-05-07 14:30 ` Mario Limonciello
2024-05-07 15:38 ` Guenter Roeck
2024-05-07 13:57 ` [PATCH 2/2] mfd: cros_ec: Register hardware monitoring subdevice Thomas Weißschuh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).