Re: [PATCH 1/3] hwmon: (asus-ec-sensors) add driver for ASUS EC
From: Andy Shevchenko
Date: Thu Dec 16 2021 - 16:28:38 EST
On Thu, Dec 16, 2021 at 10:53 PM Eugene Shalygin
<eugene.shalygin@xxxxxxxxx> wrote:
>
> This driver provides the same data as the asus_wmi_ec_sensors driver
> (and gets it from the same source) but does not use WMI, polling
> the ACPI EC directly.
>
> That provides two enhancements: sensor reading became quicker (on some
> systems or kernel configuration it took almost a full second to read
> all the sensors, that transfers less than 15 bytes of data), the driver
> became more fexible. The driver now relies on ACPI mutex to lock access
flexible
> to the EC, in the same way as the WMI DSDT code does.
How do you know that this way there is no race with any of ACPI code?
...
> +config SENSORS_ASUS_EC
> + tristate "ASUS EC Sensors"
> + depends on ACPI
No need to duplicate. See (1) below.
> + help
> + If you say yes here you get support for the ACPI embedded controller
> + hardware monitoring interface found in ASUS motherboards. The driver
> + currently supports B550/X570 boards, although other ASUS boards might
> + provide this monitoring interface as well.
> +
> + This driver can also be built as a module. If so, the module
> + will be called asus_ec_sensors.
> +
> endif # ACPI
(1)
...
> +/*
> + * HWMON driver for ASUS motherboards that publish some sensor values
> + * via the embedded controller registers
Missed grammatical period.
> + *
> + */
...
> +#define ASUS_EC_BANK_REGISTER 0xff /* Writing to this EC register switches EC bank */
Can you put comment before the definition?
...
> +#define SENSOR_LABEL_LEN 0x10
Why in hex?
Missed blank line here.
> +/*
> + * Arbitrary set max. allowed bank number. Required for sorting banks and
> + * currently is overkill with just 2 banks used at max, but for the sake
> + * of alignment let's set it to a higher value
Check grammar everywhere, again missed period (at least).
> + */
...
> +#define ACPI_DELAY_MSEC_LOCK 500 /* Wait for 0.5 s max. to acquire the lock */
_LOCK_DELAY_MS and drop useless comment
I think I gave the very same comments before. Maybe you can check the
reviews of another driver?
...
> +#define MAKE_SENSOR_ADDRESS(size, bank, index) \
> + { \
> + .value = (size << 16) + (bank << 8) + index \
Leave comma here and everywhere else in the structure entries.
> + }
Besides that, would it be better to have it defined as a compound literal?
...
> +enum known_ec_sensor {
> + SENSOR_TEMP_CHIPSET = 1 << 0, /* chipset temperature [℃] */
> + SENSOR_TEMP_CPU = 1 << 1, /* CPU temperature [℃] */
> + SENSOR_TEMP_MB = 1 << 2, /* motherboard temperature [℃] */
> + SENSOR_TEMP_T_SENSOR = 1 << 3, /* "T_Sensor" temperature sensor reading [℃] */
> + SENSOR_TEMP_VRM = 1 << 4, /* VRM temperature [℃] */
> + SENSOR_FAN_CPU_OPT = 1 << 5, /* CPU_Opt fan [RPM] */
> + SENSOR_FAN_VRM_HS = 1 << 6, /* VRM heat sink fan [RPM] */
> + SENSOR_FAN_CHIPSET = 1 << 7, /* chipset fan [RPM] */
> + SENSOR_FAN_WATER_FLOW = 1 << 8, /* water flow sensor reading [RPM] */
> + SENSOR_CURR_CPU = 1 << 9, /* CPU current [A] */
> + SENSOR_TEMP_WATER_IN = 1 << 10, /* "Water_In" temperature sensor reading [℃] */
> + SENSOR_TEMP_WATER_OUT = 1 << 11, /* "Water_Out" temperature sensor reading [℃] */
Perhaps kernel doc and use of BIT()?
> + SENSOR_END
> +};
...
> +static const struct ec_sensor_info known_ec_sensors[] = {
> + EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a), /* SENSOR_TEMP_CHIPSET */
> + EC_SENSOR("CPU", hwmon_temp, 1, 0x00, 0x3b), /* SENSOR_TEMP_CPU */
> + EC_SENSOR("Motherboard", hwmon_temp, 1, 0x00, 0x3c), /* SENSOR_TEMP_MB */
> + EC_SENSOR("T_Sensor", hwmon_temp, 1, 0x00, 0x3d), /* SENSOR_TEMP_T_SENSOR */
> + EC_SENSOR("VRM", hwmon_temp, 1, 0x00, 0x3e), /* SENSOR_TEMP_VRM */
> + EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xb0), /* SENSOR_FAN_CPU_OPT */
> + EC_SENSOR("VRM HS", hwmon_fan, 2, 0x00, 0xb2), /* SENSOR_FAN_VRM_HS */
> + EC_SENSOR("Chipset", hwmon_fan, 2, 0x00, 0xb4), /* SENSOR_FAN_CHIPSET */
> + EC_SENSOR("Water_Flow", hwmon_fan, 2, 0x00, 0xbc), /* SENSOR_FAN_WATER_FLOW */
> + EC_SENSOR("CPU", hwmon_curr, 1, 0x00, 0xf4), /* SENSOR_CURR_CPU */
> + EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00), /* SENSOR_TEMP_WATER_IN */
> + EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x01), /* SENSOR_TEMP_WATER_OUT */
Instead of comments, use form of
[FOO] = BAR(...),
> +};
...
> +static struct asus_ec_board_info board_P_X570_P = {
> + .sensors = SENSOR_TEMP_CHIPSET | SENSOR_TEMP_CPU | SENSOR_TEMP_MB | SENSOR_TEMP_VRM
> + | SENSOR_FAN_CHIPSET,
It's a bit long and better to leave ' |' on the previous line(s).
> + .acpi_mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX
+ Comma.
> +};
Ditto for all other similar cases.
...
> +#define DMI_EXACT_MATCH_BOARD(vendor, name, sensors) { \
> + .matches = { \
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR, vendor), \
> + DMI_EXACT_MATCH(DMI_BOARD_NAME, name), \
> + }, \
> + .driver_data = sensors \
+ Comma.
> + }
...
> +struct ec_sensors_data {
> + const struct asus_ec_board_info *board;
> + struct ec_sensor *sensors;
> + /* EC registers to read from */
> + u16 *registers;
> + u8 *read_buffer;
> + /* sorted list of unique register banks */
> + u8 banks[ASUS_EC_MAX_BANK];
> + unsigned long last_updated; /* in jiffies */
> + acpi_handle aml_mutex;
> + u8 nr_sensors; /* number of board EC sensors */
> + /* number of EC registers to read (sensor might span more than 1 register) */
> + u8 nr_registers;
> + u8 nr_banks; /* number of unique register banks */
> +};
Kernel doc?
...
> +static u8 register_bank(u16 reg)
> +{
> + return (reg & 0xff00) >> 8;
' & 0xff00' part is redundant.
> +}
...
> +static struct ec_sensors_data *get_sensor_data(struct device *pdev)
> +{
> + return dev_get_drvdata(pdev);
> +}
Useless wrapper. It adds no value.
...
> + unsigned int i;
> +
> + for (i = 0; i < ec->nr_sensors; ++i) {
> + if (get_sensor_info(ec, i)->type == type) {
> + if (channel == 0)
> + return i;
> + --channel;
What's wrong with post-decrement, and I think I already commented on this.
So, I stopped here until you go and enforce all comments given against
previous incarnation of this driver.
> + }
> + }
> + return -ENOENT;
> +}
...
> + for (i = 1; i < SENSOR_END; i <<= 1) {
> + if ((i & ec->board->sensors) == 0
> + continue;
Interesting way of NIH for_each_set_bit().
...
> + for (j = 0; j < si->addr.components.size; ++j, ++register_idx) {
Why pre-increments?
> + ec->registers[register_idx] =
> + (si->addr.components.bank << 8) +
> + si->addr.components.index + j;
> + }
> + }
> +}
...
> + acpi_handle res;
> + acpi_status status = acpi_get_handle(
> + NULL, (acpi_string)state->board->acpi_mutex_path, &res);
It looks awful (indentation), Have you run checkpatch?
> + if (ACPI_FAILURE(status)) {
> + dev_err(dev, "Could not get hardware access guard mutex: error %d", status);
> + return NULL;
> + }
...
> +static struct hwmon_chip_info asus_wmi_chip_info = {
> + .ops = &asus_ec_hwmon_ops,
> + .info = NULL,
Redundant.
> +};
--
With Best Regards,
Andy Shevchenko