Messages in this thread | | | From | Eugene Shalygin <> | Date | Thu, 7 Oct 2021 01:32:14 +0200 | Subject | Re: [PATCH v2 2/3] hwmon: (asus_wmi_ec_sensors) Support B550 Asus WMI. |
| |
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
| |