Date:   Thu, 7 Oct 2021 18:46:44 +0300
From:   Denis Pauk <pauk.denis@...il.com>
To:     Eugene Shalygin <eugene.shalygin@...il.com>
Cc:     andy.shevchenko@...il.com, Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2 2/3] hwmon: (asus_wmi_ec_sensors) Support B550 Asus
 WMI.

Hi Eugene, 

On Thu, 7 Oct 2021 01:32:14 +0200
Eugene Shalygin <eugene.shalygin@...il.com> wrote:

> On Thu, 7 Oct 2021 at 00:25, Denis Pauk <pauk.denis@...il.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.
Thanks, I will check.
> 
> > + * EC provided:  
> provides
Thanks, I will check.
> 
> > + * 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"?
> 
Thanks, I will check.
> > +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.
> 
Do you have such fix in your repository?
> > +/**
> > + * 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?
> 
Its platform data and used to share board_id with probe.

> > +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.
I have tried to apply Andy's idea. And it looks it does not
provide benefits. Andy, what do you think? Maybe I understand it in
wrong way. 
> > +
> > +       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.
> 
I have tried to apply Andy's idea. And it looks it does not
provide benefits. Andy, what do you think? Maybe I understand it in
wrong way.
> > +}
> > +
> > +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?
> 
Looks as it was in original patch, I will look.
> > +
> > +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.
> 
Thank you, I will look.
 
> > +               if (si->addr.size == 1)  
> Maybe switch(si->addr.size)?
> 
Thank you, I will check.
> > +                       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?
> 
It looked complicated to support two kind of WMI interfaces with UUID.
As we split big support module to two separate - I will look to such
change also.

> Thank you,
> Eugene

Best regards,
     Denis.

Powered by blists - more mailing lists