Re: [PATCH 2/3] hwmon: (nct6775) Implement custom lock by ACPI mutex.

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

Dear Andy, Denis, and Günter,

Denis worked on my code with the first attempt to read EC sensors from
ASUS motherboards and submitted it as a driver named
"asus_wmi_ec_sensors", which is not in hwmon-next. Now he adds the
ACPI lock feature to support other motherboards, and I have another
iteration of the EC sensor driver under development (needs some
polishment) that utilizes the same concept (ACPI lock instead of WMI
methods), which is smaller, cleaner and faster than the WMI-based one.
I'm going to submit it to the mainline too. I think it should replace
the WMI one. In anticipation of that, can we change the name of the
accepted driver (asus_wmi_ec_sensors -> asus_ec_sensors) now, in order
to not confuse users in the next version and to remove implementation
detail from the module name? The drivers provide indistinguishable
data to HWMON.

Regards,
Eugene

On Wed, 24 Nov 2021 at 17:11, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > +       if (ACPI_FAILURE(acpi_acquire_mutex(data->acpi_wmi_mutex, NULL, ASUSWMI_DELAY_MSEC_LOCK)))
>
>
> On Mon, Nov 22, 2021 at 11:29 PM Denis Pauk <pauk.denis@gmail.com> wrote:
>
> No period in the Subject.
>
> > Use ACPI lock when board has separate lock for monitoring IO.
>
> the board
> a separate
>
> ...
>
> > +#define ASUSWMI_DELAY_MSEC_LOCK                500     /* Wait 0.5 s max. to get the lock */
>
> Units are the last in the names, hence (also check the comment's
> location and English)
>
> /* Wait for up to 0.5 s to acquire the lock */
> #define ASUSWMI_LOCK_TIMEOUT_MS                500
>
> ...
>
> > -       struct mutex update_lock;
> > +       struct mutex update_lock;       /* non ACPI lock */
> > +       acpi_handle acpi_wmi_mutex;     /* ACPI lock */
>
> Couldn't it be an anonymous union?
>
> ...
>
> > +static int nct6775_wmi_lock(struct nct6775_data *data)
> > +{
> > +       if (ACPI_FAILURE(acpi_acquire_mutex(data->acpi_wmi_mutex, NULL, ASUSWMI_DELAY_MSEC_LOCK)))
>
> Please, use a temporary variable here and in the similar cases.
>
>   acpi_status status;
>
>   status = acpi_acquire_mutex(data->acpi_wmi_mutex, NULL,
> ASUSWMI_LOCK_TIMEOUT_MS));
>   if (ACPI_FAILURE(status))
>
> > +               return -EIO;
> > +
> > +       return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko