Re: [PATCH 3/3] hwmon: (asus_wmi_sensors) Support access via Asus WMI.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Oct 3, 2021 at 12:10 AM Denis Pauk <pauk.denis@xxxxxxxxx> wrote:
>
> Linux HWMON sensors driver for ASUS motherboards to read
> sensors from the embedded controller.
>
> Many ASUS motherboards do not publish all the available
> sensors via the Super I/O chip but the missing ones are
> available through the embedded controller (EC) registers.
>
> This driver implements reading those sensor data via the
> WMI method BREC, which is known to be present in all ASUS
> motherboards based on the AMD 500 series chipsets (and
> probably is available in other models too). The driver
> needs to know exact register addresses for the sensors and
> thus support for each motherboard has to be added explicitly.
>
> Supported motherboards:
> * ROG CROSSHAIR VIII HERO
> * ROG CROSSHAIR VIII DARK HERO
> * ROG CROSSHAIR VIII FORMULA
> * ROG STRIX X570-E GAMING
> * ROG STRIX B550-E GAMING

...

> +config SENSORS_ASUS_WMI
> +       tristate "ASUS WMI"

Provide a better short description here.

> +       depends on X86

COMPILE_TEST ?

> +       help
> +         If you say yes here you get support for the ACPI hardware
> +         monitoring interface found in many ASUS motherboards. This
> +         driver will provide readings of fans, voltages and temperatures
> +         through the system firmware.
> +
> +         This driver can also be built as a module. If so, the module
> +         will be called asus_wmi_sensors.

...

> +/*

> + * HWMON driver for ASUS motherboards that publish some sensor values
> + * via the embedded controller registers

I would drop the word 'some' here and...

> + *
> + * Copyright (C) 2021 Eugene Shalygin <eugene.shalygin@xxxxxxxxx>
> + * Copyright (C) 2018-2019 Ed Brindley <kernel@xxxxxxxxxxxxx>

...add a bit more elaborative text here to explain what values are
provided and what aren't.

> + */
...

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Any real need for this?

...

> +#define DRVNAME "asus_wmi_sensors"

Can you use this directly?

...

> +#include <linux/dmi.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <linux/wmi.h>

Sorted?

...

> +struct asus_wmi_ec_info {
> +       struct asus_wmi_ec_sensor_info sensors[ASUSWMI_SENSORS_MAX];
> +       /* UTF-16 string to pass to BRxx() WMI function */
> +       char read_arg[((ASUS_WMI_BLOCK_READ_REGISTERS_MAX * 4) + 1) * 2];
> +       u8 read_buffer[ASUS_WMI_BLOCK_READ_REGISTERS_MAX];
> +       u8 nr_sensors; /* number of board EC sensors */
> +       /* number of EC registers to read (sensor might span more than 1 register) */
> +       u8 nr_registers;
> +       unsigned long last_updated; /* in jiffies */
> +};

Convert those comments to a proper kernel doc format?

...

> +/*
> + * The next four functions converts to/from BRxx string argument format
> + * The format of the string is as follows:
> + * The string consists of two-byte UTF-16 characters
> + * The value of the very first byte int the string is equal to the total length
> + * of the next string in bytes, thus excluding the first two-byte character
> + * The rest of the string encodes pairs of (bank, index) pairs, where both
> + * values are byte-long (0x00 to 0xFF)
> + * Numbers are encoded as UTF-16 hex values
> + */

All above can probably use existing functions?
https://elixir.bootlin.com/linux/latest/source/fs/nls/nls_base.c#L132

...

> +static void asus_wmi_ec_make_block_read_query(struct asus_wmi_ec_info *ec)
> +{
> +       u16 registers[ASUS_EC_KNOWN_EC_REGISTERS];
> +       u8 i, j, register_idx = 0;

> +       /* if we can get values for all the registers in a single query,
> +        * the query will not change from call to call
> +        */

/*
 * Wrong multi-line style is in
 * use. Compare to this comment.
 */

> +       if (ec->nr_registers <= ASUS_WMI_BLOCK_READ_REGISTERS_MAX &&
> +           ec->read_arg[0] > 0) {
> +               /* no need to update */
> +               return;
> +       }
> +
> +       for (i = 0; i < ec->nr_sensors; ++i) {
> +               for (j = 0; j < ec->sensors[i].addr.addr.size;

> +                    ++j, ++register_idx) {

Here and in plenty other places, why _pre_-increment (or
_pre_-decrement in some cases)?

> +                       registers[register_idx] =
> +                               (ec->sensors[i].addr.addr.bank << 8) +
> +                               ec->sensors[i].addr.addr.index + j;
> +               }
> +       }
> +
> +       asus_wmi_ec_encode_registers(registers, ec->nr_registers, ec->read_arg);
> +}
> +
> +static int asus_wmi_ec_block_read(u32 method_id, const char *query, u8 *out)
> +{
> +       struct acpi_buffer input;

> +       struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER,
> +                                     NULL }; // TODO use pre-allocated buffer

One line and drop this TODO, or fulfil it.

> +       acpi_status status;
> +       union acpi_object *obj;
> +
> +       /* the first byte of the BRxx() argument string has to be the string size */
> +       input.length = (acpi_size)query[0] + 2;

> +       input.pointer = (void *)query;

Redundant casting. Or is this due to const qualifier?

> +       status = wmi_evaluate_method(ASUSWMI_MONITORING_GUID, 0, method_id, &input,
> +                                    &output);

> +

Here and in plenty of other places, redundant blank lines.

> +       if (ACPI_FAILURE(status)) {

> +               acpi_os_free(output.pointer);

Is it needed?

> +               return -EIO;
> +       }
> +
> +       obj = output.pointer;
> +       if (!obj || obj->type != ACPI_TYPE_BUFFER) {

> +               pr_err("unexpected reply type from ASUS ACPI code");

dev_err()

> +               acpi_os_free(output.pointer);

When !obj, this is not needed.

> +               return -EIO;
> +       }
> +       asus_wmi_ec_decode_reply_buffer(obj->buffer.pointer, out);
> +       acpi_os_free(output.pointer);
> +       return 0;
> +}
> +
> +static int asus_wmi_ec_update_ec_sensors(struct asus_wmi_ec_info *ec)
> +{
> +       struct asus_wmi_ec_sensor_info *si;
> +       u32 value;
> +       int status;
> +       u8 i_sensor, read_reg_ct, i_sensor_register;

Here and in all cases, reversed xmas tree order?

> +       asus_wmi_ec_make_block_read_query(ec);
> +       status = asus_wmi_ec_block_read(ASUSWMI_METHODID_BLOCK_READ_EC,
> +                                       ec->read_arg,
> +                                       ec->read_buffer);
> +       if (status)
> +               return status;
> +
> +       read_reg_ct = 0;
> +       for (i_sensor = 0; i_sensor < ec->nr_sensors; ++i_sensor) {

Why _pre_-increment?

> +               si = &ec->sensors[i_sensor];
> +               value = ec->read_buffer[read_reg_ct++];
> +               for (i_sensor_register = 1;
> +                    i_sensor_register < si->addr.addr.size;

> +                    ++i_sensor_register) {

Why _pre_-increment?

> +                       value <<= 8;
> +                       value += ec->read_buffer[read_reg_ct++];
> +               }

Is it something like get_unaligned_...32()?
Or some ...32_to_cpu() ?

Also provide the corresponding __le32/__be32 types where it's needed.

> +               si->cached_value = value;
> +       }
> +       return 0;
> +}

...

> +static int asus_wmi_ec_scale_sensor_value(u32 value, int data_type)
> +{
> +       switch (data_type) {
> +       case hwmon_curr:
> +       case hwmon_temp:
> +       case hwmon_in:

> +               return value * 1000;

In units.h we have SI multipliers, can you use one to show what is this exactly?

> +       default:
> +               return value;
> +       }
> +}
> +
> +static u8 asus_wmi_ec_find_sensor_index(const struct asus_wmi_ec_info *ec,
> +                                       enum hwmon_sensor_types type, int channel)
> +{
> +       u8 i;

In some cases you are using int i, here u8 i. Be consistent and use, e.g.
unsigned int i;
everywhere for (never been negative) counters.

> +       for (i = 0; i < ec->nr_sensors; ++i) {
> +               if (ec->sensors[i].type == type) {
> +                       if (channel == 0)
> +                               return i;

> +                       --channel;

Why _pre_-decrement?

> +               }
> +       }

> +       return 0xFF;

0xff, but why? Can't you use the proper error code instead?

> +}
> +
> +static int asus_wmi_ec_get_cached_value_or_update(int sensor_index,
> +                                                 struct asus_wmi_sensors *state,
> +                                                 u32 *value)
> +{
> +       int ret;
> +
> +       if (time_after(jiffies, state->ec.last_updated + HZ)) {

> +               ret = asus_wmi_ec_update_ec_sensors(&state->ec);

> +

Redundant blank line.

> +               if (ret) {

> +                       pr_err("asus_wmi_ec_update_ec_sensors() failure\n");

Can you use dev_err()?

> +                       return -EIO;

Why shadowed the real error code? Or is it not an error code there?

> +               }
> +
> +               state->ec.last_updated = jiffies;
> +       }
> +
> +       *value = state->ec.sensors[sensor_index].cached_value;
> +       return 0;
> +}
> +
> +/*
> + * Now follow the functions that implement the hwmon interface
> + */
> +
> +static int asus_wmi_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +                                 u32 attr, int channel, long *val)
> +{
> +       int ret;
> +       u32 value = 0;
> +       struct asus_wmi_sensors *sensor_data = dev_get_drvdata(dev);
> +
> +       u8 sidx = asus_wmi_ec_find_sensor_index(&sensor_data->ec, type, channel);
> +
> +       mutex_lock(&sensor_data->lock);

> +

Seems redundant blank line.

> +       ret = asus_wmi_ec_get_cached_value_or_update(sidx, sensor_data, &value);
> +       mutex_unlock(&sensor_data->lock);

> +       if (!ret)
> +               *val = asus_wmi_ec_scale_sensor_value(value, sensor_data->ec.sensors[sidx].type);
> +
> +       return ret;

Why not the standard pattern?

mutex_lock(...);
ret = ...;
mutex_unlock(...);
if (ret)
  return ret;

*val = ...
return 0;


> +}

...

> +static umode_t asus_wmi_ec_hwmon_is_visible(const void *drvdata,
> +                                           enum hwmon_sensor_types type, u32 attr,
> +                                           int channel)
> +{
> +       const struct asus_wmi_sensors *sensor_data = drvdata;

> +       return asus_wmi_ec_find_sensor_index(&sensor_data->ec, type, channel) != 0xFF ?
> +                            0444 :
> +                            0;

This will look better with temporary variables

... index;

index = asus_wmi_ec_find_sensor_index();
return index == 0xff ? 0 : 0444;

(also note small letters for hex value).

> +}
> +
> +static int asus_wmi_hwmon_add_chan_info(struct hwmon_channel_info *asus_wmi_hwmon_chan,
> +                                       struct device *dev, int num,
> +                                       enum hwmon_sensor_types type, u32 config)
> +{
> +       int i;

> +       u32 *cfg = devm_kcalloc(dev, num + 1, sizeof(*cfg), GFP_KERNEL);
> +
> +       if (!cfg)
> +               return -ENOMEM;

Use the following pattern which is slightly better.

u32 *cfg;

cfg = ...;
if (!cfg)
  return -ENOMEM;

> +       asus_wmi_hwmon_chan->type = type;
> +       asus_wmi_hwmon_chan->config = cfg;

> +       for (i = 0; i < num; i++, cfg++)
> +               *cfg = config;

memset32() ?

> +       return 0;
> +}

...

> +static struct hwmon_chip_info asus_wmi_ec_chip_info = {
> +       .ops = &asus_wmi_ec_hwmon_ops,

> +       .info = NULL,

Redundant assignment.

> +};
> +
> +static int asus_wmi_ec_configure_sensor_setup(struct platform_device *pdev,
> +                                             struct asus_wmi_sensors *sensor_data)
> +{
> +       int i;
> +       int nr_count[HWMON_MAX] = { 0 }, nr_types = 0;
> +       struct device *hwdev;
> +       struct device *dev = &pdev->dev;
> +       struct hwmon_channel_info *asus_wmi_hwmon_chan;
> +       const struct hwmon_channel_info **ptr_asus_wmi_ci;
> +       const struct hwmon_chip_info *chip_info;
> +       const struct asus_wmi_ec_sensor_info *si;
> +       enum hwmon_sensor_types type;

Reversed xmas tree order?

> +       if (sensor_data->ec_board < 0)
> +               return 0;

Is it ever possible?

> +       asus_wmi_ec_fill_board_sensors(&sensor_data->ec, sensor_data->ec_board);
> +
> +       if (!sensor_data->ec.nr_sensors)
> +               return -ENODEV;
> +
> +       for (i = 0; i < sensor_data->ec.nr_sensors; ++i) {
> +               si = &sensor_data->ec.sensors[i];
> +               if (!nr_count[si->type])
> +                       ++nr_types;
> +               ++nr_count[si->type];

What's wrong with post increments?

> +       }

> +       if (nr_count[hwmon_temp])
> +               nr_count[hwmon_chip]++, nr_types++;

This is suspicious code. Refactor to make it easier to understand.

> +       asus_wmi_hwmon_chan = devm_kcalloc(dev, nr_types,
> +                                          sizeof(*asus_wmi_hwmon_chan),
> +                                          GFP_KERNEL);
> +       if (!asus_wmi_hwmon_chan)
> +               return -ENOMEM;
> +
> +       ptr_asus_wmi_ci = devm_kcalloc(dev, nr_types + 1,
> +                                      sizeof(*ptr_asus_wmi_ci), GFP_KERNEL);
> +       if (!ptr_asus_wmi_ci)
> +               return -ENOMEM;
> +
> +       asus_wmi_ec_chip_info.info = ptr_asus_wmi_ci;
> +       chip_info = &asus_wmi_ec_chip_info;
> +
> +       for (type = 0; type < HWMON_MAX; type++) {
> +               if (!nr_count[type])
> +                       continue;
> +
> +               asus_wmi_hwmon_add_chan_info(asus_wmi_hwmon_chan, dev,
> +                                            nr_count[type], type,
> +                                            hwmon_attributes[type]);
> +               *ptr_asus_wmi_ci++ = asus_wmi_hwmon_chan++;
> +       }

> +       pr_info("%s board has %d EC sensors that span %d registers",
> +               asus_wmi_ec_boards_names[sensor_data->ec_board],
> +               sensor_data->ec.nr_sensors,
> +               sensor_data->ec.nr_registers);

First of all, why not dev_info()? Second, do you really need this?
Seems to me like a debug leftover.

> +       hwdev = devm_hwmon_device_register_with_info(dev, "asuswmiecsensors",

No delimiters, like -, _ or spaces?

> +                                                    sensor_data, chip_info, NULL);
> +
> +       return PTR_ERR_OR_ZERO(hwdev);
> +}
> +
> +static int asus_wmi_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct asus_wmi_data *data = dev_get_platdata(dev);
> +       struct asus_wmi_sensors *sensor_data;
> +       int err;
> +
> +       sensor_data = devm_kzalloc(dev, sizeof(struct asus_wmi_sensors),
> +                                  GFP_KERNEL);
> +       if (!sensor_data)
> +               return -ENOMEM;
> +
> +       mutex_init(&sensor_data->lock);
> +       sensor_data->ec_board = data->ec_board;
> +
> +       platform_set_drvdata(pdev, sensor_data);
> +
> +       /* ec init */

> +       err = asus_wmi_ec_configure_sensor_setup(pdev,
> +                                                sensor_data);
> +
> +       return err;

return asus_wmi_ec_configure_sensor_setup(...);

> +}

...

> +static struct platform_driver asus_wmi_sensors_platform_driver = {
> +       .driver = {
> +               .name   = "asus-wmi-sensors",
> +       },
> +       .probe = asus_wmi_probe

+ Comma.

> +};

...

> +static int __init asus_wmi_init(void)
> +{
> +       const char *board_vendor, *board_name;
> +       struct asus_wmi_data data;
> +
> +       data.ec_board = -1;
> +
> +       board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
> +       board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +
> +       if (board_vendor && board_name &&
> +           !strcmp(board_vendor, "ASUSTeK COMPUTER INC.")) {
> +               if (!wmi_has_guid(ASUSWMI_MONITORING_GUID))
> +                       return -ENODEV;
> +
> +               data.ec_board = match_string(asus_wmi_ec_boards_names,
> +                                            ARRAY_SIZE(asus_wmi_ec_boards_names),
> +                                            board_name);
> +       }
> +
> +       /* Nothing to support */
> +       if (data.ec_board < 0)
> +               return -ENODEV;
> +
> +       sensors_pdev = platform_create_bundle(&asus_wmi_sensors_platform_driver,
> +                                             asus_wmi_probe,
> +                                             NULL, 0,
> +                                             &data, sizeof(struct asus_wmi_data));

> +       if (IS_ERR(sensors_pdev))
> +               return PTR_ERR(sensors_pdev);
> +
> +       return 0;

return PTR_ERR_OR_ZERO();

> +}
> +
> +static void __exit asus_wmi_exit(void)
> +{
> +       platform_device_unregister(sensors_pdev);
> +       platform_driver_unregister(&asus_wmi_sensors_platform_driver);
> +}

Can we decouple driver and board code? And put board code to the PDx86 folder?

...

> +MODULE_VERSION("1");

No, the version of the driver is the same as git commit ID. Drop this.

...

> +module_init(asus_wmi_init);
> +module_exit(asus_wmi_exit);

Can you move this to the respective functions?

-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux