Re: [PATCH v5 09/10] acpi/nfit: Move handler installing logic to driver

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:55 AM Wilczynski, Michal
<michal.wilczynski@xxxxxxxxx> wrote:
>
>
>
> On 6/29/2023 6:18 PM, Rafael J. Wysocki wrote:
> > On Fri, Jun 16, 2023 at 6:51 PM Michal Wilczynski
> > <michal.wilczynski@xxxxxxxxx> wrote:
> >> Currently logic for installing notifications from ACPI devices is
> >> implemented using notify callback in struct acpi_driver. Preparations
> >> are being made to replace acpi_driver with more generic struct
> >> platform_driver, which doesn't contain notify callback. Furthermore
> >> as of now handlers are being called indirectly through
> >> acpi_notify_device(), which decreases performance.
> >>
> >> Call acpi_dev_install_notify_handler() at the end of .add() callback.
> >> Call acpi_dev_remove_notify_handler() at the beginning of .remove()
> >> callback. Change arguments passed to the notify function to match with
> >> what's required by acpi_install_notify_handler(). Remove .notify
> >> callback initialization in acpi_driver.
> >>
> >> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >> Signed-off-by: Michal Wilczynski <michal.wilczynski@xxxxxxxxx>
> >> ---
> >>  drivers/acpi/nfit/core.c | 24 ++++++++++++++++++------
> >>  1 file changed, 18 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> >> index 95930e9d776c..a281bdfee8a0 100644
> >> --- a/drivers/acpi/nfit/core.c
> >> +++ b/drivers/acpi/nfit/core.c
> >> @@ -3312,11 +3312,13 @@ void acpi_nfit_shutdown(void *data)
> >>  }
> >>  EXPORT_SYMBOL_GPL(acpi_nfit_shutdown);
> >>
> >> -static void acpi_nfit_notify(struct acpi_device *adev, u32 event)
> >> +static void acpi_nfit_notify(acpi_handle handle, u32 event, void *data)
> >>  {
> >> -       device_lock(&adev->dev);
> >> -       __acpi_nfit_notify(&adev->dev, adev->handle, event);
> >> -       device_unlock(&adev->dev);
> > It's totally not necessary to rename the ACPI device variable here.
> >
> > Just add
> >
> > struct acpi_device *adev = data;
> >
> > to this function.
>
> Sure, is adev a preferred name for acpi_device ?

In new code, it is.

In the existing code, it depends.  If you do a one-line change, it is
better to retain the original naming (for the sake of clarity of the
change itself).  If you rearrange it completely, you may as well
change the names while at it.  And there is a spectrum in between.

>  I've seen a mix of different naming
> in drivers, some use device, adev, acpi_dev and so on. I suppose it's not a big deal, but
> it would be good to know.

Personally, I prefer adev, but this isn't a very strong preference.

Using "device" as a name of a struct acpi_device object (or a pointer
to one of these for that matter) is slightly misleading IMV, because
those things represent AML entities rather than actual hardware.