Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 16 Sep 2021 11:34:50 +0300 | Subject | Re: [PATCH v6 3/3] hwmon: (nct6775) Support access via Asus WMI. |
| |
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
| |