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

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

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

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.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 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

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

[Mark Pearson]

Hi Rafael

On Tue, Mar 24, 2026, at 4:08 PM, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>
> 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?

>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  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@squebb.ca>
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Mark

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@squebb.ca> wrote:
>
> Hi Rafael
>
> On Tue, Mar 24, 2026, at 4:08 PM, Rafael J. Wysocki wrote:
> > From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >  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@squebb.ca>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>

Thank you!

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

[Ilpo Järvinen]

On Tue, 24 Mar 2026 21:08:01 +0100, Rafael J. Wysocki wrote:

> 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().
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo-next branch. Note it will show up in the public
platform-drivers-x86/review-ilpo-next branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86: thinkpad_acpi: Drop ACPI driver registration
      commit: 955165c3e537668b9a5a6eb26397281b88143002

--
 i.