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

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

On Wed, Sep 15, 2021 at 11:45 PM Denis Pauk <pauk.denis@gmail.com> wrote:
>
> Support accessing the NCT677x via Asus WMI functions.
>
> On mainboards that support this way of accessing the chip, the driver will
> usually not work without this option since in these mainboards, ACPI will
> mark the I/O port as used.
>
> Code uses ACPI firmware interface to commucate with sensors with ASUS

communicate

> motherboards:
> * PRIME B460-PLUS,
> * ROG CROSSHAIR VIII IMPACT,
> * ROG STRIX B550-E GAMING,
> * ROG STRIX B550-F GAMING,
> * ROG STRIX B550-F GAMING (WI-FI),
> * ROG STRIX Z490-I GAMING,
> * TUF GAMING B550M-PLUS,
> * TUF GAMING B550M-PLUS (WI-FI),
> * TUF GAMING B550-PLUS,
> * TUF GAMING X570-PLUS,
> * TUF GAMING X570-PRO (WI-FI).

...

> +static int asuswmi_evaluate_method(u32 method_id, u8 bank, u8 reg, u8 val, u32 *retval)
> +{

> +#if IS_ENABLED(CONFIG_ACPI_WMI)

The idea behind IS_ENABLED() macro is that it may be used in C
conditionals, like

if (IS_ENABLED(...))

> +       u32 args = bank | (reg << 8) | (val << 16);
> +       struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
> +       struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +       acpi_status status;
> +       union acpi_object *obj;
> +       u32 tmp = 0;

> +       obj = output.pointer;
> +       if (obj && obj->type == ACPI_TYPE_INTEGER)
> +               tmp = obj->integer.value;
> +
> +       if (retval)
> +               *retval = tmp;
> +
> +       kfree(obj);

> +       if (tmp == ASUSWMI_UNSUPPORTED_METHOD)

This is uninitialized tmp in case when no obj, or obj is of the wrong type.

> +               return -ENODEV;
> +       return 0;
> +#else
> +       return -EOPNOTSUPP;
> +#endif
> +}

...

> +static u16 nct6775_wmi_read_value(struct nct6775_data *data, u16 reg)
> +{
> +       int res, word_sized = is_word_sized(data, reg);
> +       u8 tmp;
> +
> +       nct6775_wmi_set_bank(data, reg);
> +
> +       nct6775_asuswmi_read(data->bank, reg, &tmp);

> +       res = (tmp & 0xff);

Too many parentheses.
tnp is u8, ' & 0xff' is redundant.

> +       if (word_sized) {
> +               nct6775_asuswmi_read(data->bank, (reg & 0xff) + 1, &tmp);
> +               res = (res << 8) + (tmp & 0xff);

Ditto.

> +       }
> +       return res;
> +}
> +
> +static int nct6775_wmi_write_value(struct nct6775_data *data, u16 reg, u16 value)
> +{
> +       int word_sized = is_word_sized(data, reg);
> +
> +       nct6775_wmi_set_bank(data, reg);
> +
> +       if (word_sized) {

> +               nct6775_asuswmi_write(data->bank, (reg & 0xff),

Too many parentheses

> +                                     (value >> 8) & 0xff);

' & 0xff' part is redundant.

> +               nct6775_asuswmi_write(data->bank, (reg & 0xff) + 1,
> +                                     value & 0xff);
> +       } else {
> +               nct6775_asuswmi_write(data->bank, (reg & 0xff), value);
> +       }
> +
> +       return 0;
> +}

...

> +               err = match_string(asus_wmi_boards, ARRAY_SIZE(asus_wmi_boards),
> +                                  board_name);

> +               if (err != -EINVAL) {

I believe I commented on this in the way as

if (err >= 0)

The rationale behind is that you shouldn't really care what kind of
error codes the API may return.

> +                       /* if reading chip id via WMI succeeds, use WMI */
> +                       if (!nct6775_asuswmi_read(0, NCT6775_PORT_CHIPID, &tmp)) {
> +                               pr_info("Using Asus WMI to access %#x chip.\n", tmp);
> +                               access = access_asuswmi;
> +                       } else {
> +                               pr_err("Can't read ChipID by Asus WMI.\n");
> +                       }
> +               }

-- 
With Best Regards,
Andy Shevchenko