Re: [PATCH v2 2/3] hwmon: (asus_wmi_ec_sensors) Support B550 Asus WMI.

[Eugene Shalygin <eugene.shalygin@gmail.com>]

On Thu, 7 Oct 2021 at 00:25, Denis Pauk <pauk.denis@gmail.com> wrote:
>

> 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

Pro WS X570-ACE is missing from this list.

> + * EC provided:
provides

> + * Chipset temperature,
> + * CPU temperature,
> + * Motherboard temperature,
> + * T_Sensor temperature,
> + * VRM  temperature,
> + * Water In temperature,
> + * Water Out temperature,
> + * CPU Optional Fan,
Hereinafter "CPU Optional Fan RPM"?

> +static const enum known_ec_sensor known_board_sensors[BOARD_MAX][SENSOR_MAX + 1] = {
> +       [BOARD_PW_X570_A] = {
> +               SENSOR_TEMP_CHIPSET, SENSOR_TEMP_CPU, SENSOR_TEMP_MB, SENSOR_TEMP_VRM,
> +               SENSOR_FAN_CHIPSET,

I missed SENSOR_CURR_CPU for a few boards, and unfortunately the
mistake made it here too. Sorry for that.

> +/**
> + * struct asus_wmi_ec_info - sensor info.
> + * @sensors: list of sensors.
> + * @read_arg: UTF-16 string to pass to BRxx() WMI function.
> + * @read_buffer: WMI function output.

This seems to be a bit misleading to me in a sense that the variable
holds decoded output (array of numbers as opposed to array of
characters in the WMI output buffer.

> +struct asus_wmi_data {
> +       int ec_board;
> +};

A leftover?

> +static void asus_wmi_ec_decode_reply_buffer(const u8 *inp, u8 *out)
> +{
> +       unsigned int len = ACPI_MIN(ASUS_WMI_MAX_BUF_LEN, inp[0] / 4);
> +       char buffer[ASUS_WMI_MAX_BUF_LEN * 2];
> +       const char *pos = buffer;
> +       const u8 *data = inp + 2;
> +       unsigned int i;
> +
> +       utf16s_to_utf8s((wchar_t *)data, len * 2,  UTF16_LITTLE_ENDIAN, buffer, len * 2);
Errr... Why is it here? You need the same loop afterwards, just with a
smaller stride.
> +
> +       for (i = 0; i < len; i++, pos += 2)
> +               out[i] = (hex_to_bin(pos[0]) << 4) + hex_to_bin(pos[1]);
> +}
> +
> +static void asus_wmi_ec_encode_registers(u16 *registers, u8 len, char *out)
> +{
> +       char buffer[ASUS_WMI_MAX_BUF_LEN * 2];
> +       char *pos = buffer;
> +       unsigned int i;
> +       u8 byte;
> +
> +       *out++ = len * 8;
> +       *out++ = 0;
> +
> +       for (i = 0; i < len; i++) {
> +               byte = registers[i] >> 8;
> +               *pos = hex_asc_hi(byte);
> +               pos++;
> +               *pos = hex_asc_lo(byte);
> +               pos++;
> +               byte = registers[i];
> +               *pos = hex_asc_hi(byte);
> +               pos++;
> +               *pos = hex_asc_lo(byte);
> +               pos++;
> +       }
> +
> +       utf8s_to_utf16s(buffer, len * 4, UTF16_LITTLE_ENDIAN, (wchar_t *)out, len * 4);
Same here. Just for the sake of calling utf8s_to_utf16s() you need the
same loop plus an additional buffer. I don't get it.

> +}
> +
> +static void asus_wmi_ec_make_block_read_query(struct asus_wmi_ec_info *ec)
> +{
> +       u16 registers[ASUS_WMI_BLOCK_READ_REGISTERS_MAX];
> +       const struct ec_sensor_info *si;
> +       long i, j, register_idx = 0;
long? maybe a simple unsigned or int?

> +
> +static int asus_wmi_ec_update_ec_sensors(struct asus_wmi_ec_info *ec)
> +{
> +       const struct ec_sensor_info *si;
> +       struct ec_sensor *s;
> +
> +       u32 value;
This variable is now redundant.

> +               if (si->addr.size == 1)
Maybe switch(si->addr.size)?

> +                       value = ec->read_buffer[read_reg_ct];
> +               else if (si->addr.size == 2)
> +                       value = get_unaligned_le16(&ec->read_buffer[read_reg_ct]);
> +               else if (si->addr.size == 4)
> +                       value = get_unaligned_le32(&ec->read_buffer[read_reg_ct]);
> +
> +               read_reg_ct += si->addr.size;
> +               s->cached_value = value;
> +       }
> +       return 0;
> +}


> +       mutex_lock(&sensor_data->lock);
The mutex locking/unlocking should be moved inside the
update_ec_sensors(), I guess.

I re-read your answer to my question as to why don't you add module
aliases to the driver, and I have to admit I don't really understand
it. Could you, please, elaborate?

Thank you,
Eugene