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 <dober@xxxxxxxxxx>
> Signed-off-by: David Ober <dober6023@xxxxxxxxx>
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) <dober@xxxxxxxxxx>
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
