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