Re: [PATCH v5 03/10] acpi/ac: Move handler installing logic to driver

[Rafael J. Wysocki]

On Fri, Jun 30, 2023 at 11:41 AM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Fri, Jun 30, 2023 at 11:39 AM Wilczynski, Michal
> <michal.wilczynski@xxxxxxxxx> wrote:
> >
> >
> >
> > On 6/29/2023 5:55 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/ac.c | 33 ++++++++++++++++++++++++---------
> > >>  1 file changed, 24 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> > >> index 1ace70b831cd..207ee3c85bad 100644
> > >> --- a/drivers/acpi/ac.c
> > >> +++ b/drivers/acpi/ac.c
> > >> @@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
> > >>
> > >>  static int acpi_ac_add(struct acpi_device *device);
> > >>  static void acpi_ac_remove(struct acpi_device *device);
> > >> -static void acpi_ac_notify(struct acpi_device *device, u32 event);
> > >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data);
> > >>
> > >>  static const struct acpi_device_id ac_device_ids[] = {
> > >>         {"ACPI0003", 0},
> > >> @@ -54,11 +54,9 @@ static struct acpi_driver acpi_ac_driver = {
> > >>         .name = "ac",
> > >>         .class = ACPI_AC_CLASS,
> > >>         .ids = ac_device_ids,
> > >> -       .flags = ACPI_DRIVER_ALL_NOTIFY_EVENTS,
> > >>         .ops = {
> > >>                 .add = acpi_ac_add,
> > >>                 .remove = acpi_ac_remove,
> > >> -               .notify = acpi_ac_notify,
> > >>                 },
> > >>         .drv.pm = &acpi_ac_pm,
> > >>  };
> > >> @@ -128,9 +126,12 @@ static enum power_supply_property ac_props[] = {
> > >>  };
> > >>
> > >>  /* Driver Model */
> > >> -static void acpi_ac_notify(struct acpi_device *device, u32 event)
> > >> +static void acpi_ac_notify(acpi_handle handle, u32 event, void *data)
> > >>  {
> > >> -       struct acpi_ac *ac = acpi_driver_data(device);
> > > This line doesn't need to be changed.  Just add the device variable
> > > definition above it.
> > >
> > > And the same pattern is present in the other patches in the series.
> >
> > I like the Reverse Christmas Tree, but sure will change that
>
> I do too, but in the cases when it costs 3 extra lines of code I'd
> rather have something simpler.

Besides, moving code around is not strictly related to the functional
changes made by the patch and it kind of hides those changes.  It is
better to move code around in a separate patch if you really want to
do that.