Re: [PATCH 1/3] hwmon: (asus-ec-sensors) add driver for ASUS EC

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, Dec 16, 2021 at 10:53 PM Eugene Shalygin
<eugene.shalygin@gmail.com> 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