Message-ID: <20211217000424.41da446e@netbook-debian>
Date:   Fri, 17 Dec 2021 00:04:24 +0200
From:   Denis Pauk <pauk.denis@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Eugene Shalygin <eugene.shalygin@...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 1/3] hwmon: (asus-ec-sensors) add driver for ASUS EC

Hi Eugene,

Have you found some issues with idea of usage ACPI WMI methods as
failback solution, like in case when ASUS will release some BIOS with
different mutex path or different motherboard where will be same WMI
methods but fully different internal logic?
   
On Thu, 16 Dec 2021 23:27:52 +0200
Andy Shevchenko <andy.shevchenko@...il.com> wrote:

> On Thu, Dec 16, 2021 at 10:53 PM Eugene Shalygin
> <eugene.shalygin@...il.com> wrote:
> >
> > This driver provides the same data as the asus_wmi_ec_sensors driver
> > (and gets it from the same source) but does not use WMI, polling
> > the ACPI EC directly.
> >
> > That provides two enhancements: sensor reading became quicker (on
> > some systems or kernel configuration it took almost a full second
> > to read all the sensors, that transfers less than 15 bytes of
> > data), the driver became more fexible. The driver now relies on
> > ACPI mutex to lock access  
> 
> flexible
> 
> > to the EC, in the same way as the WMI DSDT code does.  
> 
> How do you know that this way there is no race with any of ACPI code?
> 
> ...
> 
> > +config SENSORS_ASUS_EC
> > +       tristate "ASUS EC Sensors"  
> 
> > +       depends on ACPI  
> 
> No need to duplicate. See (1) below.
> 
> > +       help
> > +         If you say yes here you get support for the ACPI embedded
> > controller
> > +         hardware monitoring interface found in ASUS motherboards.
> > The driver
> > +         currently supports B550/X570 boards, although other ASUS
> > boards might
> > +         provide this monitoring interface as well.
> > +
> > +         This driver can also be built as a module. If so, the
> > module
> > +         will be called asus_ec_sensors.
> > +
> >  endif # ACPI  
> 
> (1)
> 
> ...
> 
> > +/*
> > + * HWMON driver for ASUS motherboards that publish some sensor
> > values
> > + * via the embedded controller registers  
> 
> Missed grammatical period.
> 
> > + *  
> 
> > + */  
> 
> ...
> 
> > +#define ASUS_EC_BANK_REGISTER 0xff /* Writing to this EC register
> > switches EC bank */  
> 
> Can you put comment before the definition?
> 
> ...
> 
> > +#define SENSOR_LABEL_LEN 0x10  
> 
> Why in hex?
> 
> Missed blank line here.
> 
> > +/*
> > + * Arbitrary set max. allowed bank number. Required for sorting
> > banks and
> > + * currently is overkill with just 2 banks used at max, but for
> > the sake
> > + * of alignment let's set it to a higher value  
> 
> Check grammar everywhere, again missed period (at least).
> 
> > + */  
> 
> ...
> 
> > +#define ACPI_DELAY_MSEC_LOCK   500     /* Wait for 0.5 s max. to
> > acquire the lock */  
> 
> _LOCK_DELAY_MS and drop useless comment
> 
> I think I gave the very same comments before. Maybe you can check the
> reviews of another driver?
> 
> ...
> 
> 
> > +#define MAKE_SENSOR_ADDRESS(size, bank, index)
> >             \
> > +       {
> >            \
> > +               .value = (size << 16) + (bank << 8) + index
> >            \  
> 
> Leave comma here and everywhere else in the structure entries.
> 
> > +       }  
> 
> Besides that, would it be better to have it defined as a compound
> literal?
> 
> ...
> 
> > +enum known_ec_sensor {
> > +       SENSOR_TEMP_CHIPSET     = 1 <<  0, /* chipset temperature
> > [℃] */
> > +       SENSOR_TEMP_CPU         = 1 <<  1, /* CPU temperature [℃] */
> > +       SENSOR_TEMP_MB          = 1 <<  2, /* motherboard
> > temperature [℃] */
> > +       SENSOR_TEMP_T_SENSOR    = 1 <<  3, /* "T_Sensor"
> > temperature sensor reading [℃] */
> > +       SENSOR_TEMP_VRM         = 1 <<  4, /* VRM temperature [℃] */
> > +       SENSOR_FAN_CPU_OPT      = 1 <<  5, /* CPU_Opt fan [RPM] */
> > +       SENSOR_FAN_VRM_HS       = 1 <<  6, /* VRM heat sink fan
> > [RPM] */
> > +       SENSOR_FAN_CHIPSET      = 1 <<  7, /* chipset fan [RPM] */
> > +       SENSOR_FAN_WATER_FLOW   = 1 <<  8, /* water flow sensor
> > reading [RPM] */
> > +       SENSOR_CURR_CPU         = 1 <<  9, /* CPU current [A] */
> > +       SENSOR_TEMP_WATER_IN    = 1 << 10, /* "Water_In"
> > temperature sensor reading [℃] */
> > +       SENSOR_TEMP_WATER_OUT   = 1 << 11, /* "Water_Out"
> > temperature sensor reading [℃] */  
> 
> Perhaps kernel doc and use of BIT()?
> 
> > +       SENSOR_END
> > +};  
> 
> ...
> 
> > +static const struct ec_sensor_info known_ec_sensors[] = {
> > +       EC_SENSOR("Chipset", hwmon_temp, 1, 0x00, 0x3a), /*
> > SENSOR_TEMP_CHIPSET */
> > +       EC_SENSOR("CPU", hwmon_temp, 1, 0x00, 0x3b), /*
> > SENSOR_TEMP_CPU */
> > +       EC_SENSOR("Motherboard", hwmon_temp, 1, 0x00, 0x3c), /*
> > SENSOR_TEMP_MB */
> > +       EC_SENSOR("T_Sensor", hwmon_temp, 1, 0x00, 0x3d), /*
> > SENSOR_TEMP_T_SENSOR */
> > +       EC_SENSOR("VRM", hwmon_temp, 1, 0x00, 0x3e), /*
> > SENSOR_TEMP_VRM */
> > +       EC_SENSOR("CPU_Opt", hwmon_fan, 2, 0x00, 0xb0), /*
> > SENSOR_FAN_CPU_OPT */
> > +       EC_SENSOR("VRM HS", hwmon_fan, 2, 0x00, 0xb2), /*
> > SENSOR_FAN_VRM_HS */
> > +       EC_SENSOR("Chipset", hwmon_fan, 2, 0x00, 0xb4), /*
> > SENSOR_FAN_CHIPSET */
> > +       EC_SENSOR("Water_Flow", hwmon_fan, 2, 0x00, 0xbc), /*
> > SENSOR_FAN_WATER_FLOW */
> > +       EC_SENSOR("CPU", hwmon_curr, 1, 0x00, 0xf4), /*
> > SENSOR_CURR_CPU */
> > +       EC_SENSOR("Water_In", hwmon_temp, 1, 0x01, 0x00), /*
> > SENSOR_TEMP_WATER_IN */
> > +       EC_SENSOR("Water_Out", hwmon_temp, 1, 0x01, 0x01), /*
> > SENSOR_TEMP_WATER_OUT */  
> 
> Instead of comments, use form of
> 
>   [FOO] = BAR(...),
> 
> > +};  
> 
> ...
> 
> > +static struct asus_ec_board_info board_P_X570_P = {
> > +       .sensors = SENSOR_TEMP_CHIPSET | SENSOR_TEMP_CPU |
> > SENSOR_TEMP_MB | SENSOR_TEMP_VRM
> > +                | SENSOR_FAN_CHIPSET,  
> 
> It's a bit long and better to leave ' |' on the previous line(s).
> 
> > +       .acpi_mutex_path = ASUS_HW_ACCESS_MUTEX_ASMX  
> 
> + Comma.
> 
> > +};  
> 
> Ditto for all other similar cases.
> 
> ...
> 
> > +#define DMI_EXACT_MATCH_BOARD(vendor, name, sensors) {
> >             \
> > +               .matches = {
> >            \
> > +                       DMI_EXACT_MATCH(DMI_BOARD_VENDOR, vendor),
> >            \
> > +                       DMI_EXACT_MATCH(DMI_BOARD_NAME, name),
> >            \
> > +               },
> >            \
> > +               .driver_data = sensors
> >            \  
> 
> + Comma.
> 
> > +       }  
> 
> ...
> 
> > +struct ec_sensors_data {
> > +       const struct asus_ec_board_info *board;
> > +       struct ec_sensor *sensors;
> > +       /* EC registers to read from */
> > +       u16 *registers;
> > +       u8 *read_buffer;
> > +       /* sorted list of unique register banks */
> > +       u8 banks[ASUS_EC_MAX_BANK];
> > +       unsigned long last_updated; /* in jiffies */
> > +       acpi_handle aml_mutex;
> > +       u8 nr_sensors; /* number of board EC sensors */
> > +       /* number of EC registers to read (sensor might span more
> > than 1 register) */
> > +       u8 nr_registers;
> > +       u8 nr_banks; /* number of unique register banks */
> > +};  
> 
> Kernel doc?
> 
> ...
> 
> > +static u8 register_bank(u16 reg)
> > +{
> > +       return (reg & 0xff00) >> 8;  
> 
> ' & 0xff00' part is redundant.
> 
> > +}  
> 
> ...
> 
> > +static struct ec_sensors_data *get_sensor_data(struct device *pdev)
> > +{
> > +       return dev_get_drvdata(pdev);
> > +}  
> 
> Useless wrapper. It adds no value.
> 
> ...
> 
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < ec->nr_sensors; ++i) {
> > +               if (get_sensor_info(ec, i)->type == type) {
> > +                       if (channel == 0)
> > +                               return i;  
> 
> > +                       --channel;  
> 
> What's wrong with post-decrement, and I think I already commented on
> this. So, I stopped here until you go and enforce all comments given
> against previous incarnation of this driver.
> 
> > +               }
> > +       }
> > +       return -ENOENT;
> > +}  
> 
> ...
> 
> > +       for (i = 1; i < SENSOR_END; i <<= 1) {
> > +               if ((i & ec->board->sensors) == 0
> > +                       continue;  
> 
> Interesting way of NIH for_each_set_bit().
> 
> ...
> 
> > +               for (j = 0; j < si->addr.components.size; ++j,
> > ++register_idx) {  
> 
> Why pre-increments?
> 
> > +                       ec->registers[register_idx] =
> > +                               (si->addr.components.bank << 8) +
> > +                               si->addr.components.index + j;
> > +               }
> > +       }
> > +}  
> 
> ...
> 
> > +       acpi_handle res;  
> 
> > +       acpi_status status = acpi_get_handle(
> > +               NULL, (acpi_string)state->board->acpi_mutex_path,
> > &res);  
> 
> It looks awful (indentation), Have you run checkpatch?
> 
> > +       if (ACPI_FAILURE(status)) {
> > +               dev_err(dev, "Could not get hardware access guard
> > mutex: error %d", status);
> > +               return NULL;
> > +       }  
> 
> ...
> 
> > +static struct hwmon_chip_info asus_wmi_chip_info = {
> > +       .ops = &asus_ec_hwmon_ops,  
> 
> > +       .info = NULL,  
> 
> Redundant.
> 
> > +};  
> 



Best regards,
             Denis.

Powered by blists - more mailing lists