Messages in this thread | | | From | Eugene Shalygin <> | Date | Fri, 17 Dec 2021 01:07:41 +0100 | Subject | Re: [PATCH 1/3] hwmon: (asus-ec-sensors) add driver for ASUS EC |
| |
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
| |