On Wed, Sep 15, 2021 at 11:45 PM Denis Pauk <pauk.denis@xxxxxxxxx> 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
