This addition adds in the ability for the system to scan
the EC chip in the Lenovo ThinkStation systems to get the
current fan RPM speeds the Maximum speed value for each
fan also provides the CPU, DIMM other thermal statuses
Signed-off-by: David Ober <[email protected]>
Signed-off-by: David Ober <[email protected]>
---
Changes in V4
removed internal name references and checks
added in DMI checks to verify the device is a on supported system
added in IO memory region protection
---
---
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/lenovo-ec-sensors.c | 615 ++++++++++++++++++++++++++++++
3 files changed, 626 insertions(+)
create mode 100644 drivers/hwmon/lenovo-ec-sensors.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ec38c8892158..a4bb403bd4ad 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -862,6 +862,16 @@ config SENSORS_LAN966X
This driver can also be built as a module. If so, the module
will be called lan966x-hwmon.
+config SENSORS_LENOVO_EC
+ tristate "Sensor reader for Lenovo ThinkStations"
+ depends on X86
+ help
+ If you say yes here you get support for LENOVO
+ EC Sensor data on newer ThinkStation systems
+
+ This driver can also be built as a module. If so, the module
+ will be called lenovo_ec_sensors.
+
config SENSORS_LINEAGE
tristate "Lineage Compact Power Line Power Entry Module"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 4ac9452b5430..aa3c2dc390ec 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -104,6 +104,7 @@ obj-$(CONFIG_SENSORS_JC42) += jc42.o
obj-$(CONFIG_SENSORS_K8TEMP) += k8temp.o
obj-$(CONFIG_SENSORS_K10TEMP) += k10temp.o
obj-$(CONFIG_SENSORS_LAN966X) += lan966x-hwmon.o
+obj-$(CONFIG_SENSORS_LENOVO_EC) += lenovo-ec-sensors.o
obj-$(CONFIG_SENSORS_LINEAGE) += lineage-pem.o
obj-$(CONFIG_SENSORS_LOCHNAGAR) += lochnagar-hwmon.o
obj-$(CONFIG_SENSORS_LM63) += lm63.o
diff --git a/drivers/hwmon/lenovo-ec-sensors.c b/drivers/hwmon/lenovo-ec-sensors.c
new file mode 100644
index 000000000000..0fc32bc11365
--- /dev/null
+++ b/drivers/hwmon/lenovo-ec-sensors.c
@@ -0,0 +1,615 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * HWMON driver for Lenovo ThinkStation based workstations
+ * via the embedded controller registers
+ *
+ * Copyright (C) 2023 David Ober (Lenovo) <[email protected]>
+ *
+ * EC provides:
+ * - CPU temperature
+ * - DIMM temperature
+ * - Chassis zone temperatures
+ * - CPU fan RPM
+ * - DIMM fan RPM
+ * - Chassis fans RPM
+ */
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/dmi.h>
+#include <linux/hwmon.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/units.h>
+
+#define MCHP_SING_IDX 0x0000
+#define MCHP_EMI0_APPLICATION_ID 0x090C
+#define MCHP_EMI0_EC_ADDRESS_LSB 0x0902
+#define MCHP_EMI0_EC_ADDRESS_MSB 0x0903
+#define MCHP_EMI0_EC_DATA_BYTE0 0x0904
+#define MCHP_EMI0_EC_DATA_BYTE1 0x0905
+#define MCHP_EMI0_EC_DATA_BYTE2 0x0906
+#define MCHP_EMI0_EC_DATA_BYTE3 0x0907
+#define IO_REGION_START 0x0900
+#define IO_REGION_LENGTH 0xD
+
+#define io_write8(a, b) outb_p(b, a)
+#define io_read8(a) inb_p(a)
+
+static inline uint8_t
+get_ec_reg(unsigned char page, unsigned char index)
+{
+ u8 onebyte;
+ unsigned short m_index;
+ unsigned short phy_index = page * 256 + index;
+
+ io_write8(MCHP_EMI0_APPLICATION_ID, 0x01);
+
+ m_index = phy_index & 0x7FFC;
+ io_write8(MCHP_EMI0_EC_ADDRESS_LSB, m_index);
+ io_write8(MCHP_EMI0_EC_ADDRESS_MSB, m_index >> 8);
+
+ onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE0 + (phy_index & 3));
+
+ io_write8(MCHP_EMI0_APPLICATION_ID, 0x01); /* write 0x01 again to clean */
+ return onebyte;
+}
+
+enum systems {
+ LENOVO_PX,
+ LENOVO_P7,
+ LENOVO_P5,
+ LENOVO_P8,
+};
+
+static int px_temp_map[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15};
+
+static const char * const lenovo_px_ec_temp_label[] = {
+ "CPU1",
+ "CPU2",
+ "R_DIMM1",
+ "L_DIMM1",
+ "R_DIMM2",
+ "L_DIMM2",
+ "PCH",
+ "M2_R",
+ "M2_Z1R",
+ "M2_Z2R",
+ "PCI_Z1",
+ "PCI_Z2",
+ "PCI_Z3",
+ "PCI_Z4",
+ "AMB",
+};
+
+static int gen_temp_map[] = {0, 2, 3, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15};
+
+static const char * const lenovo_gen_ec_temp_label[] = {
+ "CPU1",
+ "R_DIMM",
+ "L_DIMM",
+ "PCH",
+ "M2_R",
+ "M2_Z1R",
+ "M2_Z2R",
+ "PCI_Z1",
+ "PCI_Z2",
+ "PCI_Z3",
+ "PCI_Z4",
+ "AMB",
+};
+
+static int px_fan_map[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15};
+
+static const char * const px_ec_fan_label[] = {
+ "CPU1_Fan",
+ "CPU2_Fan",
+ "Front_Fan1-1",
+ "Front_Fan1-2",
+ "Front_Fan2",
+ "Front_Fan3",
+ "MEM_Fan1",
+ "MEM_Fan2",
+ "Rear_Fan1",
+ "Rear_Fan2",
+ "Flex_Bay_Fan1",
+ "Flex_Bay_Fan2",
+ "Flex_Bay_Fan2",
+ "PSU_HDD_Fan",
+ "PSU1_Fan",
+ "PSU2_Fan",
+};
+
+static int p7_fan_map[] = {0, 2, 3, 4, 5, 6, 7, 8, 10, 11, 14};
+
+static const char * const p7_ec_fan_label[] = {
+ "CPU1_Fan",
+ "HP_CPU_Fan1",
+ "HP_CPU_Fan2",
+ "PCIE1_4_Fan",
+ "PCIE5_7_Fan",
+ "MEM_Fan1",
+ "MEM_Fan2",
+ "Rear_Fan1",
+ "BCB_Fan",
+ "Flex_Bay_Fan",
+ "PSU_Fan",
+};
+
+static int p5_fan_map[] = {0, 5, 6, 7, 8, 10, 11, 14};
+
+static const char * const p5_ec_fan_label[] = {
+ "CPU_Fan",
+ "HDD_Fan",
+ "Duct_Fan1",
+ "MEM_Fan",
+ "Rear_Fan",
+ "Front_Fan",
+ "Flex_Bay_Fan",
+ "PSU_Fan",
+};
+
+static int p8_fan_map[] = {0. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 14};
+
+static const char * const p8_ec_fan_label[] = {
+ "CPU1_Fan",
+ "CPU2_Fan",
+ "HP_CPU_Fan1",
+ "HP_CPU_Fan2",
+ "PCIE1_4_Fan",
+ "PCIE5_7_Fan",
+ "DIMM1_Fan1",
+ "DIMM1_Fan2",
+ "DIMM2_Fan1",
+ "DIMM2_Fan2",
+ "Rear_Fan",
+ "HDD_Bay_Fan",
+ "Flex_Bay_Fan",
+ "PSU_Fan",
+};
+
+struct ec_sensors_data {
+ struct mutex mec_mutex; /* lock for sensor data access */
+ /*int platform_id;*/
+ const char *const *fan_labels;
+ const char *const *temp_labels;
+ const int *fan_map;
+ const int *temp_map;
+};
+
+static int
+lenovo_ec_do_read_temp(struct ec_sensors_data *data, u32 attr, int channel, long *val)
+{
+ u8 lsb;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ mutex_lock(&data->mec_mutex);
+ lsb = get_ec_reg(2, 0x81 + channel);
+ mutex_unlock(&data->mec_mutex);
+ if (lsb <= 0x40)
+ return -ENODATA;
+ *val = (lsb - 0x40) * 1000;
+ return 0;
+ default:
+ break;
+ }
+ return -EOPNOTSUPP;
+}
+
+static int
+lenovo_ec_do_read_fan(struct ec_sensors_data *data, u32 attr, int channel, long *val)
+{
+ u8 lsb, msb;
+
+ channel *= 2;
+ switch (attr) {
+ case hwmon_fan_input:
+ mutex_lock(&data->mec_mutex);
+ lsb = get_ec_reg(4, 0x20 + channel);
+ msb = get_ec_reg(4, 0x21 + channel);
+ mutex_unlock(&data->mec_mutex);
+ *val = (msb << 8) + lsb;
+ return 0;
+ case hwmon_fan_max:
+ mutex_lock(&data->mec_mutex);
+ lsb = get_ec_reg(4, 0x40 + channel);
+ msb = get_ec_reg(4, 0x41 + channel);
+ mutex_unlock(&data->mec_mutex);
+ *val = (msb << 8) + lsb;
+ return 0;
+ case hwmon_fan_min:
+ case hwmon_fan_div:
+ case hwmon_fan_alarm:
+ break;
+ default:
+ break;
+ }
+ return -EOPNOTSUPP;
+}
+
+static int
+lenovo_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **str)
+{
+ struct ec_sensors_data *state = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_temp:
+ *str = state->temp_labels[channel];
+ break;
+ case hwmon_fan:
+ *str = state->fan_labels[channel];
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int
+lenovo_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct ec_sensors_data *data = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_temp:
+ return lenovo_ec_do_read_temp(data, attr, data->temp_map[channel], val);
+ case hwmon_fan:
+ return lenovo_ec_do_read_fan(data, attr, data->fan_map[channel], val);
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static umode_t
+lenovo_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (type) {
+ case hwmon_temp:
+ if (attr == hwmon_temp_input || attr == hwmon_temp_label)
+ return 0444;
+ break;
+ case hwmon_fan:
+ if (attr == hwmon_fan_input || attr == hwmon_fan_max || attr == hwmon_fan_label)
+ return 0444;
+ break;
+ default:
+ return 0;
+ }
+ return 0;
+}
+
+static const struct hwmon_channel_info *lenovo_ec_hwmon_info_px[] = {
+ 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_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX),
+ NULL
+};
+
+static const struct hwmon_channel_info *lenovo_ec_hwmon_info_p8[] = {
+ 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_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX),
+ NULL
+};
+
+static const struct hwmon_channel_info *lenovo_ec_hwmon_info_p7[] = {
+ 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_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX),
+ NULL
+};
+
+static const struct hwmon_channel_info *lenovo_ec_hwmon_info_p5[] = {
+ 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_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX,
+ HWMON_F_INPUT | HWMON_F_LABEL | HWMON_F_MAX),
+ NULL
+};
+
+static const struct hwmon_ops lenovo_ec_hwmon_ops = {
+ .is_visible = lenovo_ec_hwmon_is_visible,
+ .read = lenovo_ec_hwmon_read,
+ .read_string = lenovo_ec_hwmon_read_string,
+};
+
+static struct hwmon_chip_info lenovo_ec_chip_info = {
+ .ops = &lenovo_ec_hwmon_ops,
+};
+
+static const struct dmi_system_id thinkstation_dmi_table[] = {
+ {
+ .ident = "LENOVO_PX",
+ .driver_data = (void *)(long)LENOVO_PX,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "30EU"),
+ },
+ },
+ {
+ .ident = "LENOVO_PX",
+ .driver_data = (void *)(long)LENOVO_PX,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "30EV"),
+ },
+ },
+ {
+ .ident = "LENOVO_P7",
+ .driver_data = (void *)(long)LENOVO_P7,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "30F2"),
+ },
+ },
+ {
+ .ident = "LENOVO_P7",
+ .driver_data = (void *)(long)LENOVO_P7,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "30F3"),
+ },
+ },
+ {
+ .ident = "LENOVO_P5",
+ .driver_data = (void *)(long)LENOVO_P5,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "30G9"),
+ },
+ },
+ {
+ .ident = "LENOVO_P5",
+ .driver_data = (void *)(long)LENOVO_P5,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "30GA"),
+ },
+ },
+ {
+ .ident = "LENOVO_P8",
+ .driver_data = (void *)(long)LENOVO_P8,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "30HH"),
+ },
+ },
+ {
+ .ident = "LENOVO_P8",
+ .driver_data = (void *)(long)LENOVO_P8,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "30HJ"),
+ },
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(dmi, thinkstation_dmi_table);
+
+static int lenovo_ec_probe(struct platform_device *pdev)
+{
+ struct device *hwdev;
+ struct ec_sensors_data *ec_data;
+ const struct hwmon_chip_info *chip_info;
+ struct device *dev = &pdev->dev;
+ const struct dmi_system_id *dmi_id;
+ int app_id;
+
+ ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data), GFP_KERNEL);
+ if (!ec_data) {
+ release_region(IO_REGION_START, IO_REGION_LENGTH);
+ return -ENOMEM;
+ }
+
+ dev_set_drvdata(dev, ec_data);
+
+ chip_info = &lenovo_ec_chip_info;
+
+ mutex_init(&ec_data->mec_mutex);
+
+ mutex_lock(&ec_data->mec_mutex);
+ app_id = io_read8(MCHP_EMI0_APPLICATION_ID);
+ if (app_id) /* check EMI Application ID Value */
+ io_write8(MCHP_EMI0_APPLICATION_ID, app_id); /* set EMI Application ID to 0 */
+ io_write8(MCHP_EMI0_EC_ADDRESS_LSB, MCHP_SING_IDX);
+ io_write8(MCHP_EMI0_EC_ADDRESS_MSB, MCHP_SING_IDX >> 8);
+ mutex_unlock(&ec_data->mec_mutex);
+
+ if ((io_read8(MCHP_EMI0_EC_DATA_BYTE0) != 'M') &&
+ (io_read8(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
+ (io_read8(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
+ (io_read8(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
+ release_region(IO_REGION_START, IO_REGION_LENGTH);
+ return -ENODEV;
+ }
+
+ dmi_id = dmi_first_match(thinkstation_dmi_table);
+
+ switch ((long)dmi_id->driver_data) {
+ case 0:
+ ec_data->fan_labels = px_ec_fan_label;
+ ec_data->temp_labels = lenovo_px_ec_temp_label;
+ ec_data->fan_map = px_fan_map;
+ ec_data->temp_map = px_temp_map;
+ lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_px;
+ break;
+ case 1:
+ ec_data->fan_labels = p7_ec_fan_label;
+ ec_data->temp_labels = lenovo_gen_ec_temp_label;
+ ec_data->fan_map = p7_fan_map;
+ ec_data->temp_map = gen_temp_map;
+ lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p7;
+ break;
+ case 2:
+ ec_data->fan_labels = p5_ec_fan_label;
+ ec_data->temp_labels = lenovo_gen_ec_temp_label;
+ ec_data->fan_map = p5_fan_map;
+ ec_data->temp_map = gen_temp_map;
+ lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p5;
+ break;
+ case 3:
+ ec_data->fan_labels = p8_ec_fan_label;
+ ec_data->temp_labels = lenovo_gen_ec_temp_label;
+ ec_data->fan_map = p8_fan_map;
+ ec_data->temp_map = gen_temp_map;
+ lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
+ break;
+ default:
+ release_region(IO_REGION_START, IO_REGION_LENGTH);
+ return -ENODEV;
+ }
+
+ hwdev = devm_hwmon_device_register_with_info(dev, "lenovo_ec",
+ ec_data,
+ chip_info, NULL);
+
+ return PTR_ERR_OR_ZERO(hwdev);
+}
+
+static struct platform_driver lenovo_ec_sensors_platform_driver = {
+ .driver = {
+ .name = "lenovo-ec-sensors",
+ },
+ .probe = lenovo_ec_probe,
+};
+
+static struct platform_device *lenovo_ec_sensors_platform_device;
+
+static int __init lenovo_ec_init(void)
+{
+ if (!dmi_check_system(thinkstation_dmi_table))
+ return -ENODEV;
+
+ if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
+ pr_err(":request fail\n");
+ return -EIO;
+ }
+
+ lenovo_ec_sensors_platform_device =
+ platform_create_bundle(&lenovo_ec_sensors_platform_driver,
+ lenovo_ec_probe, NULL, 0, NULL, 0);
+
+ if (IS_ERR(lenovo_ec_sensors_platform_device)) {
+ release_region(IO_REGION_START, IO_REGION_LENGTH);
+ return PTR_ERR(lenovo_ec_sensors_platform_device);
+ }
+
+ return 0;
+}
+
+static void __exit lenovo_ec_exit(void)
+{
+ release_region(IO_REGION_START, IO_REGION_LENGTH);
+ platform_device_unregister(lenovo_ec_sensors_platform_device);
+ platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
+}
+
+module_init(lenovo_ec_init);
+module_exit(lenovo_ec_exit);
+
+MODULE_AUTHOR("David Ober <[email protected]>");
+MODULE_DESCRIPTION("HWMON driver for sensors accesssible via EC in LENOVO motherboards");
+MODULE_LICENSE("GPL");
--
2.34.1
On Fri, Mar 15, 2024 at 07:58:10AM -0400, David Ober wrote:
> This addition adds in the ability for the system to scan
> the EC chip in the Lenovo ThinkStation systems to get the
> current fan RPM speeds the Maximum speed value for each
> fan also provides the CPU, DIMM other thermal statuses
>
> Signed-off-by: David Ober <[email protected]>
> Signed-off-by: David Ober <[email protected]>
Applied to hwmon-next.
Please note that I'll push the branch after the commit window closed.
Thanks,
Guenter
On Wed, Mar 20, 2024 at 10:59:34AM -0700, Guenter Roeck wrote:
> On Fri, Mar 15, 2024 at 07:58:10AM -0400, David Ober wrote:
> > This addition adds in the ability for the system to scan
> > the EC chip in the Lenovo ThinkStation systems to get the
> > current fan RPM speeds the Maximum speed value for each
> > fan also provides the CPU, DIMM other thermal statuses
> >
> > Signed-off-by: David Ober <[email protected]>
> > Signed-off-by: David Ober <[email protected]>
>
> Applied to hwmon-next.
>
> Please note that I'll push the branch after the commit window closed.
Hmm... Was it even compiled?
lenovo-ec-sensors.c:154:31: error: expected '}'
static int p8_fan_map[] = {0. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 14};
^
lenovo-ec-sensors.c:154:27: note: to match this '{'
static int p8_fan_map[] = {0. 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 14};
^
--
With Best Regards,
Andy Shevchenko
On 3/25/24 05:29, Andy Shevchenko wrote:
> On Fri, Mar 15, 2024 at 07:58:10AM -0400, David Ober wrote:
>> This addition adds in the ability for the system to scan
>> the EC chip in the Lenovo ThinkStation systems to get the
>> current fan RPM speeds the Maximum speed value for each
>> fan also provides the CPU, DIMM other thermal statuses
>
> Besides the compilation error, see other remarks below.
>
Thanks a lot for the detailed review. Dropped the driver
for now.
Guenter
On Fri, Mar 15, 2024 at 07:58:10AM -0400, David Ober wrote:
> This addition adds in the ability for the system to scan
> the EC chip in the Lenovo ThinkStation systems to get the
> current fan RPM speeds the Maximum speed value for each
> fan also provides the CPU, DIMM other thermal statuses
Besides the compilation error, see other remarks below.
..
> Signed-off-by: David Ober <[email protected]>
> Signed-off-by: David Ober <[email protected]>
Can you leave only one of these?
> +config SENSORS_LENOVO_EC
> + tristate "Sensor reader for Lenovo ThinkStations"
> + depends on X86
> + help
> + If you say yes here you get support for LENOVO
> + EC Sensor data on newer ThinkStation systems
> +
> + This driver can also be built as a module. If so, the module
> + will be called lenovo_ec_sensors.
..
> + * Copyright (C) 2023 David Ober (Lenovo) <[email protected]>
2024?
..
Use IWYU principle (include what you use). See below what I have noticed
(may not be the full list of missing inclusions).
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
+ device.h
> +#include <linux/dmi.h>
+ err.h
> +#include <linux/hwmon.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
How is this one being used, please?
> +#include <linux/module.h>
+ mutex.h
> +#include <linux/platform_device.h>
+ types.h
> +#include <linux/units.h>
..
> +#define io_write8(a, b) outb_p(b, a)
> +#define io_read8(a) inb_p(a)
First of all, these are too generic, second, what's the point? If you
wish to make useful macros, put a pointer to the private data, for
example, from which you can get the address.
..
> +static inline uint8_t
uint8_t...
> +get_ec_reg(unsigned char page, unsigned char index)
> +{
> + u8 onebyte;
u8... Just in a few closer lines :-(
Can you be consistent with a kernel types?
> + unsigned short m_index;
> + unsigned short phy_index = page * 256 + index;
> +
> + io_write8(MCHP_EMI0_APPLICATION_ID, 0x01);
> +
> + m_index = phy_index & 0x7FFC;
GENMASK() (you will need bits.h for this)
> + io_write8(MCHP_EMI0_EC_ADDRESS_LSB, m_index);
> + io_write8(MCHP_EMI0_EC_ADDRESS_MSB, m_index >> 8);
Can the 16-bit write suffice?
> + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE0 + (phy_index & 3));
GENMASK()
> + io_write8(MCHP_EMI0_APPLICATION_ID, 0x01); /* write 0x01 again to clean */
> + return onebyte;
> +}
..
> +struct ec_sensors_data {
> + struct mutex mec_mutex; /* lock for sensor data access */
> + /*int platform_id;*/
Huh?! Please remove, if no use for it.
> + const char *const *fan_labels;
> + const char *const *temp_labels;
> + const int *fan_map;
> + const int *temp_map;
> +};
..
> +static int
> +lenovo_ec_do_read_fan(struct ec_sensors_data *data, u32 attr, int channel, long *val)
> +{
> + u8 lsb, msb;
> +
> + channel *= 2;
> + switch (attr) {
> + case hwmon_fan_input:
> + mutex_lock(&data->mec_mutex);
> + lsb = get_ec_reg(4, 0x20 + channel);
> + msb = get_ec_reg(4, 0x21 + channel);
> + mutex_unlock(&data->mec_mutex);
> + *val = (msb << 8) + lsb;
> + return 0;
> + case hwmon_fan_max:
> + mutex_lock(&data->mec_mutex);
> + lsb = get_ec_reg(4, 0x40 + channel);
> + msb = get_ec_reg(4, 0x41 + channel);
> + mutex_unlock(&data->mec_mutex);
> + *val = (msb << 8) + lsb;
> + return 0;
> + case hwmon_fan_min:
> + case hwmon_fan_div:
> + case hwmon_fan_alarm:
> + break;
> + default:
> + break;
> + }
> + return -EOPNOTSUPP;
Combine with the above and return only once this from the default case.
> +}
..
> +static int
> +lenovo_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **str)
> +{
> + struct ec_sensors_data *state = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + *str = state->temp_labels[channel];
> + break;
In the other function you returned directly. Keep the style consistent,
please.
> + case hwmon_fan:
> + *str = state->fan_labels[channel];
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
..
> +static int
> +lenovo_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct ec_sensors_data *data = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + return lenovo_ec_do_read_temp(data, attr, data->temp_map[channel], val);
> + case hwmon_fan:
> + return lenovo_ec_do_read_fan(data, attr, data->fan_map[channel], val);
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
Dead code.
> +}
..
> +static umode_t
> +lenovo_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + switch (type) {
> + case hwmon_temp:
> + if (attr == hwmon_temp_input || attr == hwmon_temp_label)
> + return 0444;
> + break;
> + case hwmon_fan:
> + if (attr == hwmon_fan_input || attr == hwmon_fan_max || attr == hwmon_fan_label)
> + return 0444;
> + break;
> + default:
> + return 0;
> + }
> + return 0;
Same comment about the style.
> +}
..
> +static int lenovo_ec_probe(struct platform_device *pdev)
> +{
> + struct device *hwdev;
> + struct ec_sensors_data *ec_data;
> + const struct hwmon_chip_info *chip_info;
> + struct device *dev = &pdev->dev;
> + const struct dmi_system_id *dmi_id;
> + int app_id;
> +
> + ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data), GFP_KERNEL);
> + if (!ec_data) {
> + release_region(IO_REGION_START, IO_REGION_LENGTH);
This is weird. Please, either move request region to the probe, or drop
these calls here. Obviously you haven't checked the bind-unbind-bind
cycle with error injection.
> + return -ENOMEM;
> + }
> +
> + dev_set_drvdata(dev, ec_data);
> +
> + chip_info = &lenovo_ec_chip_info;
> +
> + mutex_init(&ec_data->mec_mutex);
> +
> + mutex_lock(&ec_data->mec_mutex);
> + app_id = io_read8(MCHP_EMI0_APPLICATION_ID);
> + if (app_id) /* check EMI Application ID Value */
> + io_write8(MCHP_EMI0_APPLICATION_ID, app_id); /* set EMI Application ID to 0 */
> + io_write8(MCHP_EMI0_EC_ADDRESS_LSB, MCHP_SING_IDX);
> + io_write8(MCHP_EMI0_EC_ADDRESS_MSB, MCHP_SING_IDX >> 8);
> + mutex_unlock(&ec_data->mec_mutex);
> +
> + if ((io_read8(MCHP_EMI0_EC_DATA_BYTE0) != 'M') &&
> + (io_read8(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
> + (io_read8(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
> + (io_read8(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
> + release_region(IO_REGION_START, IO_REGION_LENGTH);
> + return -ENODEV;
> + }
> +
> + dmi_id = dmi_first_match(thinkstation_dmi_table);
> +
> + switch ((long)dmi_id->driver_data) {
> + case 0:
> + ec_data->fan_labels = px_ec_fan_label;
> + ec_data->temp_labels = lenovo_px_ec_temp_label;
> + ec_data->fan_map = px_fan_map;
> + ec_data->temp_map = px_temp_map;
> + lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_px;
> + break;
> + case 1:
> + ec_data->fan_labels = p7_ec_fan_label;
> + ec_data->temp_labels = lenovo_gen_ec_temp_label;
> + ec_data->fan_map = p7_fan_map;
> + ec_data->temp_map = gen_temp_map;
> + lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p7;
> + break;
> + case 2:
> + ec_data->fan_labels = p5_ec_fan_label;
> + ec_data->temp_labels = lenovo_gen_ec_temp_label;
> + ec_data->fan_map = p5_fan_map;
> + ec_data->temp_map = gen_temp_map;
> + lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p5;
> + break;
> + case 3:
> + ec_data->fan_labels = p8_ec_fan_label;
> + ec_data->temp_labels = lenovo_gen_ec_temp_label;
> + ec_data->fan_map = p8_fan_map;
> + ec_data->temp_map = gen_temp_map;
> + lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
> + break;
> + default:
> + release_region(IO_REGION_START, IO_REGION_LENGTH);
> + return -ENODEV;
> + }
> +
> + hwdev = devm_hwmon_device_register_with_info(dev, "lenovo_ec",
> + ec_data,
> + chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwdev);
> +}
..
> + if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
> + pr_err(":request fail\n");
I haven't noticed pr_fmt(). How can the user distinguish this from
something similar from another place?
> + return -EIO;
> + }
..
> +static void __exit lenovo_ec_exit(void)
> +{
> + release_region(IO_REGION_START, IO_REGION_LENGTH);
> + platform_device_unregister(lenovo_ec_sensors_platform_device);
> + platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
> +}
> +
Unneeded blank line (see also below).
> +module_init(lenovo_ec_init);
> +module_exit(lenovo_ec_exit);
Move each of them closer to the respective callback implementation.
--
With Best Regards,
Andy Shevchenko
Andy
Sorry about the compile break comma got changed to period when I was adding in the spaces checkpatch forced me to put in between the values and since I was only adding spaces did not think to check compile
Thanks for the comments I will fix these and resubmit soon.
David
-----Original Message-----
From: Andy Shevchenko <[email protected]>
Sent: Monday, March 25, 2024 8:30 AM
To: David Ober <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; David Ober <[email protected]>; Mark Pearson <[email protected]>
Subject: [External] Re: [PATCH v4] hwmon:Add EC Chip driver for Lenovo ThinkStation motherboards
On Fri, Mar 15, 2024 at 07:58:10AM -0400, David Ober wrote:
> This addition adds in the ability for the system to scan the EC chip
> in the Lenovo ThinkStation systems to get the current fan RPM speeds
> the Maximum speed value for each fan also provides the CPU, DIMM other
> thermal statuses
Besides the compilation error, see other remarks below.
..
> Signed-off-by: David Ober <[email protected]>
> Signed-off-by: David Ober <[email protected]>
Can you leave only one of these?
> +config SENSORS_LENOVO_EC
> + tristate "Sensor reader for Lenovo ThinkStations"
> + depends on X86
> + help
> + If you say yes here you get support for LENOVO
> + EC Sensor data on newer ThinkStation systems
> +
> + This driver can also be built as a module. If so, the module
> + will be called lenovo_ec_sensors.
..
> + * Copyright (C) 2023 David Ober (Lenovo) <[email protected]>
2024?
..
Use IWYU principle (include what you use). See below what I have noticed (may not be the full list of missing inclusions).
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
+ device.h
> +#include <linux/dmi.h>
+ err.h
> +#include <linux/hwmon.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
How is this one being used, please?
> +#include <linux/module.h>
+ mutex.h
> +#include <linux/platform_device.h>
+ types.h
> +#include <linux/units.h>
..
> +#define io_write8(a, b) outb_p(b, a)
> +#define io_read8(a) inb_p(a)
First of all, these are too generic, second, what's the point? If you wish to make useful macros, put a pointer to the private data, for example, from which you can get the address.
..
> +static inline uint8_t
uint8_t...
> +get_ec_reg(unsigned char page, unsigned char index) {
> + u8 onebyte;
u8... Just in a few closer lines :-(
Can you be consistent with a kernel types?
> + unsigned short m_index;
> + unsigned short phy_index = page * 256 + index;
> +
> + io_write8(MCHP_EMI0_APPLICATION_ID, 0x01);
> +
> + m_index = phy_index & 0x7FFC;
GENMASK() (you will need bits.h for this)
> + io_write8(MCHP_EMI0_EC_ADDRESS_LSB, m_index);
> + io_write8(MCHP_EMI0_EC_ADDRESS_MSB, m_index >> 8);
Can the 16-bit write suffice?
> + onebyte = io_read8(MCHP_EMI0_EC_DATA_BYTE0 + (phy_index & 3));
GENMASK()
> + io_write8(MCHP_EMI0_APPLICATION_ID, 0x01); /* write 0x01 again to clean */
> + return onebyte;
> +}
..
> +struct ec_sensors_data {
> + struct mutex mec_mutex; /* lock for sensor data access */
> + /*int platform_id;*/
Huh?! Please remove, if no use for it.
> + const char *const *fan_labels;
> + const char *const *temp_labels;
> + const int *fan_map;
> + const int *temp_map;
> +};
..
> +static int
> +lenovo_ec_do_read_fan(struct ec_sensors_data *data, u32 attr, int
> +channel, long *val) {
> + u8 lsb, msb;
> +
> + channel *= 2;
> + switch (attr) {
> + case hwmon_fan_input:
> + mutex_lock(&data->mec_mutex);
> + lsb = get_ec_reg(4, 0x20 + channel);
> + msb = get_ec_reg(4, 0x21 + channel);
> + mutex_unlock(&data->mec_mutex);
> + *val = (msb << 8) + lsb;
> + return 0;
> + case hwmon_fan_max:
> + mutex_lock(&data->mec_mutex);
> + lsb = get_ec_reg(4, 0x40 + channel);
> + msb = get_ec_reg(4, 0x41 + channel);
> + mutex_unlock(&data->mec_mutex);
> + *val = (msb << 8) + lsb;
> + return 0;
> + case hwmon_fan_min:
> + case hwmon_fan_div:
> + case hwmon_fan_alarm:
> + break;
> + default:
> + break;
> + }
> + return -EOPNOTSUPP;
Combine with the above and return only once this from the default case.
> +}
..
> +static int
> +lenovo_ec_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **str) {
> + struct ec_sensors_data *state = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + *str = state->temp_labels[channel];
> + break;
In the other function you returned directly. Keep the style consistent, please.
> + case hwmon_fan:
> + *str = state->fan_labels[channel];
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
..
> +static int
> +lenovo_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val) {
> + struct ec_sensors_data *data = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + return lenovo_ec_do_read_temp(data, attr, data->temp_map[channel], val);
> + case hwmon_fan:
> + return lenovo_ec_do_read_fan(data, attr, data->fan_map[channel], val);
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
Dead code.
> +}
..
> +static umode_t
> +lenovo_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + switch (type) {
> + case hwmon_temp:
> + if (attr == hwmon_temp_input || attr == hwmon_temp_label)
> + return 0444;
> + break;
> + case hwmon_fan:
> + if (attr == hwmon_fan_input || attr == hwmon_fan_max || attr == hwmon_fan_label)
> + return 0444;
> + break;
> + default:
> + return 0;
> + }
> + return 0;
Same comment about the style.
> +}
..
> +static int lenovo_ec_probe(struct platform_device *pdev) {
> + struct device *hwdev;
> + struct ec_sensors_data *ec_data;
> + const struct hwmon_chip_info *chip_info;
> + struct device *dev = &pdev->dev;
> + const struct dmi_system_id *dmi_id;
> + int app_id;
> +
> + ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data), GFP_KERNEL);
> + if (!ec_data) {
> + release_region(IO_REGION_START, IO_REGION_LENGTH);
This is weird. Please, either move request region to the probe, or drop these calls here. Obviously you haven't checked the bind-unbind-bind cycle with error injection.
> + return -ENOMEM;
> + }
> +
> + dev_set_drvdata(dev, ec_data);
> +
> + chip_info = &lenovo_ec_chip_info;
> +
> + mutex_init(&ec_data->mec_mutex);
> +
> + mutex_lock(&ec_data->mec_mutex);
> + app_id = io_read8(MCHP_EMI0_APPLICATION_ID);
> + if (app_id) /* check EMI Application ID Value */
> + io_write8(MCHP_EMI0_APPLICATION_ID, app_id); /* set EMI Application ID to 0 */
> + io_write8(MCHP_EMI0_EC_ADDRESS_LSB, MCHP_SING_IDX);
> + io_write8(MCHP_EMI0_EC_ADDRESS_MSB, MCHP_SING_IDX >> 8);
> + mutex_unlock(&ec_data->mec_mutex);
> +
> + if ((io_read8(MCHP_EMI0_EC_DATA_BYTE0) != 'M') &&
> + (io_read8(MCHP_EMI0_EC_DATA_BYTE1) != 'C') &&
> + (io_read8(MCHP_EMI0_EC_DATA_BYTE2) != 'H') &&
> + (io_read8(MCHP_EMI0_EC_DATA_BYTE3) != 'P')) {
> + release_region(IO_REGION_START, IO_REGION_LENGTH);
> + return -ENODEV;
> + }
> +
> + dmi_id = dmi_first_match(thinkstation_dmi_table);
> +
> + switch ((long)dmi_id->driver_data) {
> + case 0:
> + ec_data->fan_labels = px_ec_fan_label;
> + ec_data->temp_labels = lenovo_px_ec_temp_label;
> + ec_data->fan_map = px_fan_map;
> + ec_data->temp_map = px_temp_map;
> + lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_px;
> + break;
> + case 1:
> + ec_data->fan_labels = p7_ec_fan_label;
> + ec_data->temp_labels = lenovo_gen_ec_temp_label;
> + ec_data->fan_map = p7_fan_map;
> + ec_data->temp_map = gen_temp_map;
> + lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p7;
> + break;
> + case 2:
> + ec_data->fan_labels = p5_ec_fan_label;
> + ec_data->temp_labels = lenovo_gen_ec_temp_label;
> + ec_data->fan_map = p5_fan_map;
> + ec_data->temp_map = gen_temp_map;
> + lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p5;
> + break;
> + case 3:
> + ec_data->fan_labels = p8_ec_fan_label;
> + ec_data->temp_labels = lenovo_gen_ec_temp_label;
> + ec_data->fan_map = p8_fan_map;
> + ec_data->temp_map = gen_temp_map;
> + lenovo_ec_chip_info.info = lenovo_ec_hwmon_info_p8;
> + break;
> + default:
> + release_region(IO_REGION_START, IO_REGION_LENGTH);
> + return -ENODEV;
> + }
> +
> + hwdev = devm_hwmon_device_register_with_info(dev, "lenovo_ec",
> + ec_data,
> + chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwdev);
> +}
..
> + if (!request_region(IO_REGION_START, IO_REGION_LENGTH, "LNV-WKS")) {
> + pr_err(":request fail\n");
I haven't noticed pr_fmt(). How can the user distinguish this from something similar from another place?
> + return -EIO;
> + }
..
> +static void __exit lenovo_ec_exit(void) {
> + release_region(IO_REGION_START, IO_REGION_LENGTH);
> + platform_device_unregister(lenovo_ec_sensors_platform_device);
> + platform_driver_unregister(&lenovo_ec_sensors_platform_driver);
> +}
> +
Unneeded blank line (see also below).
> +module_init(lenovo_ec_init);
> +module_exit(lenovo_ec_exit);
Move each of them closer to the respective callback implementation.
--
With Best Regards,
Andy Shevchenko