Re: [PATCH v1] platform/x86: thinkpad_acpi: Drop ACPI driver registration

[Rafael J. Wysocki]

On Sat, Mar 28, 2026 at 2:29 AM Mark Pearson <mpearson-lenovo@xxxxxxxxx> wrote:
>
> Hi Rafael
>
> On Tue, Mar 24, 2026, at 4:08 PM, Rafael J. Wysocki wrote:
> > From: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
> >
> > There is no point in registering an ACPI driver that only has an empty
> > .add() callback, which is done by the thinkpad_acpi driver, since
> > after binding to an ACPI device it only sits there and does nothing.
> >
> > That binding only effectively causes the ACPI device's reference count
> > to increase, but that can be achieved by using acpi_get_acpi_dev()
> > instead of acpi_fetch_acpi_dev() in setup_acpi_notify(), and doing
> > the corresponding cleanup in ibm_exit().
> >
> > Update the code accordingly and get rid of the non-functional ACPI
> > driver.
> >
> > No intentional functional impact beyond altering sysfs content.
>
> Just curious - where would I see changes?

In /sys/bus/acpi/drivers/ - no thinkpad_* any more (quite obviously)
and no "driver" link in the sysfs directory of the ACPI device the
driver used to bind to.  That should be it IIRC.

> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >  drivers/platform/x86/lenovo/thinkpad_acpi.c | 62 ++-------------------
> >  1 file changed, 4 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > index 8982d92dfd97..9e1614754cd7 100644
> > --- a/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > +++ b/drivers/platform/x86/lenovo/thinkpad_acpi.c
> > @@ -299,7 +299,6 @@ struct ibm_struct;
> >
> >  struct tp_acpi_drv_struct {
> >       const struct acpi_device_id *hid;
> > -     struct acpi_driver *driver;
> >
> >       void (*notify) (struct ibm_struct *, u32);
> >       acpi_handle *handle;
> > @@ -322,7 +321,6 @@ struct ibm_struct {
> >       struct tp_acpi_drv_struct *acpi;
> >
> >       struct {
> > -             u8 acpi_driver_registered:1;
> >               u8 acpi_notify_installed:1;
> >               u8 proc_created:1;
> >               u8 init_called:1;
> > @@ -832,9 +830,9 @@ static int __init setup_acpi_notify(struct ibm_struct *ibm)
> >       vdbg_printk(TPACPI_DBG_INIT,
> >               "setting up ACPI notify for %s\n", ibm->name);
> >
> > -     ibm->acpi->device = acpi_fetch_acpi_dev(*ibm->acpi->handle);
> > +     ibm->acpi->device = acpi_get_acpi_dev(*ibm->acpi->handle);
> >       if (!ibm->acpi->device) {
> > -             pr_err("acpi_fetch_acpi_dev(%s) failed\n", ibm->name);
> > +             pr_err("acpi_get_acpi_dev(%s) failed\n", ibm->name);
> >               return -ENODEV;
> >       }
> >
> > @@ -859,44 +857,6 @@ static int __init setup_acpi_notify(struct ibm_struct *ibm)
> >       return 0;
> >  }
> >
> > -static int __init tpacpi_device_add(struct acpi_device *device)
> > -{
> > -     return 0;
> > -}
> > -
> > -static int __init register_tpacpi_subdriver(struct ibm_struct *ibm)
> > -{
> > -     int rc;
> > -
> > -     dbg_printk(TPACPI_DBG_INIT,
> > -             "registering %s as an ACPI driver\n", ibm->name);
> > -
> > -     BUG_ON(!ibm->acpi);
> > -
> > -     ibm->acpi->driver = kzalloc_obj(struct acpi_driver);
> > -     if (!ibm->acpi->driver) {
> > -             pr_err("failed to allocate memory for ibm->acpi->driver\n");
> > -             return -ENOMEM;
> > -     }
> > -
> > -     sprintf(ibm->acpi->driver->name, "%s_%s", TPACPI_NAME, ibm->name);
> > -     ibm->acpi->driver->ids = ibm->acpi->hid;
> > -
> > -     ibm->acpi->driver->ops.add = &tpacpi_device_add;
> > -
> > -     rc = acpi_bus_register_driver(ibm->acpi->driver);
> > -     if (rc < 0) {
> > -             pr_err("acpi_bus_register_driver(%s) failed: %d\n",
> > -                    ibm->name, rc);
> > -             kfree(ibm->acpi->driver);
> > -             ibm->acpi->driver = NULL;
> > -     } else if (!rc)
> > -             ibm->flags.acpi_driver_registered = 1;
> > -
> > -     return rc;
> > -}
> > -
> > -
> >  /****************************************************************************
> >   ****************************************************************************
> >   *
> > @@ -11532,6 +11492,8 @@ static void ibm_exit(struct ibm_struct *ibm)
> >               acpi_remove_notify_handler(*ibm->acpi->handle,
> >                                          ibm->acpi->type,
> >                                          dispatch_acpi_notify);
> > +             ibm->acpi->device->driver_data = NULL;
> > +             acpi_dev_put(ibm->acpi->device);
> >               ibm->flags.acpi_notify_installed = 0;
> >       }
> >
> > @@ -11542,16 +11504,6 @@ static void ibm_exit(struct ibm_struct *ibm)
> >               ibm->flags.proc_created = 0;
> >       }
> >
> > -     if (ibm->flags.acpi_driver_registered) {
> > -             dbg_printk(TPACPI_DBG_EXIT,
> > -                     "%s: acpi_bus_unregister_driver\n", ibm->name);
> > -             BUG_ON(!ibm->acpi);
> > -             acpi_bus_unregister_driver(ibm->acpi->driver);
> > -             kfree(ibm->acpi->driver);
> > -             ibm->acpi->driver = NULL;
> > -             ibm->flags.acpi_driver_registered = 0;
> > -     }
> > -
> >       if (ibm->flags.init_called && ibm->exit) {
> >               ibm->exit();
> >               ibm->flags.init_called = 0;
> > @@ -11587,12 +11539,6 @@ static int __init ibm_init(struct
> > ibm_init_struct *iibm)
> >       }
> >
> >       if (ibm->acpi) {
> > -             if (ibm->acpi->hid) {
> > -                     ret = register_tpacpi_subdriver(ibm);
> > -                     if (ret)
> > -                             goto err_out;
> > -             }
> > -
> >               if (ibm->acpi->notify) {
> >                       ret = setup_acpi_notify(ibm);
> >                       if (ret == -ENODEV) {
> > --
> > 2.51.0
>
> Changes seem good to me (but not an expert).
> I did try them out on a system (X1 Carbon 13) and didn't find any problems. Let me know if there's anything in particular I should look out for or check and happy to do that.
>
> Tested-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
> Reviewed-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>

Thank you!