Re: [PATCH 1/3] hwmon: (asus-ec-sensors) add driver for ASUS EC

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

On Thu, 16 Dec 2021 at 22:28, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > 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?

Because this mutex is exactly what the ACPI code uses to avoid races.

> _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?

I understand your frustration, sorry. In all those similar reviews I
must have missed some emails. I'll fix what I can.

> > +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(...),

Was unable do that because the SENSOR_ enum is a flag enum. But given
this suggestion and the one about bit foreach loop, I will convert the
enum to bitmap.

> 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.

I missed these ones, sorry.

> > +       for (i = 1; i < SENSOR_END; i <<= 1) {
> > +               if ((i & ec->board->sensors) == 0
> > +                       continue;
>
> Interesting way of NIH for_each_set_bit().

Will convert to bitmap.

> > +       acpi_status status = acpi_get_handle(
> > +               NULL, (acpi_string)state->board->acpi_mutex_path, &res);
>
> It looks awful (indentation), Have you run checkpatch?

Yes, but some warnings remained.

Thanks,
Eugene