Re: [PATCH v1 11/12] ACPI: platform: Add macro for acpi platform driver

[Hans de Goede <hansg@kernel.org>]

Hi Slawomir,

On 30-Aug-25 7:34 AM, Slawomir Rosek wrote:
> Introduce module_acpi_platform_driver() macro to simplify dynamic
> enumeration of ACPI device objects on the platform bus by loadable
> modules. Move common code from the intel-hid and intel-vbtn drivers
> to the ACPI platform core.
> 
> Signed-off-by: Slawomir Rosek <srosek@google.com>

Thank you for your interesting patch.

> ---
>  drivers/acpi/acpi_platform.c      | 27 ++++++++++++++++++++
>  drivers/platform/x86/intel/hid.c  | 41 +------------------------------
>  drivers/platform/x86/intel/vbtn.c | 30 +---------------------
>  include/linux/platform_device.h   | 17 +++++++++++++
>  4 files changed, 46 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
> index 48d15dd785f6..adf32ffa6be6 100644
> --- a/drivers/acpi/acpi_platform.c
> +++ b/drivers/acpi/acpi_platform.c
> @@ -190,6 +190,33 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
>  }
>  EXPORT_SYMBOL_GPL(acpi_create_platform_device);
>  
> +static acpi_status
> +__acpi_platform_driver_register_cb(acpi_handle handle, u32 lvl,
> +				void *context, void **rv)
> +{
> +	const struct acpi_device_id *ids = context;
> +	struct acpi_device *dev = acpi_fetch_acpi_dev(handle);
> +
> +	if (dev && acpi_match_device_ids(dev, ids) == 0)
> +		if (!IS_ERR_OR_NULL(acpi_create_platform_device(dev, NULL))) {
> +			dev_info(&dev->dev,
> +				 "created platform device\n");
> +		}
> +
> +	return AE_OK;
> +}
> +
> +int __acpi_platform_driver_register(struct platform_driver *drv,
> +				struct module *owner)
> +{
> +	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
> +			    __acpi_platform_driver_register_cb, NULL,
> +			    (void *)drv->driver.acpi_match_table, NULL);
> +
> +	return __platform_driver_register(drv, owner);
> +}
> +EXPORT_SYMBOL_GPL(__acpi_platform_driver_register);
> +
>  void __init acpi_platform_init(void)
>  {
>  	acpi_reconfig_notifier_register(&acpi_platform_notifier);
> diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/intel/hid.c
> index f25a427cccda..e2e0fc95e177 100644
> --- a/drivers/platform/x86/intel/hid.c
> +++ b/drivers/platform/x86/intel/hid.c
> @@ -766,43 +766,4 @@ static struct platform_driver intel_hid_pl_driver = {
>  	.remove = intel_hid_remove,
>  };
>  
> -/*
> - * Unfortunately, some laptops provide a _HID="INT33D5" device with
> - * _CID="PNP0C02".  This causes the pnpacpi scan driver to claim the
> - * ACPI node, so no platform device will be created.  The pnpacpi
> - * driver rejects this device in subsequent processing, so no physical
> - * node is created at all.
> - *
> - * As a workaround until the ACPI core figures out how to handle
> - * this corner case, manually ask the ACPI platform device code to
> - * claim the ACPI node.
> - */

This comment contains useful info, please preserve the comment changing
the last paragraph to:

 * As a workaround until the ACPI core figures out how to handle
 * this corner case, manually ask the ACPI platform device code to
 * claim the ACPI node by using module_acpi_platform_driver()
 * instead of the regular module_platform_driver().

> -static acpi_status __init
> -check_acpi_dev(acpi_handle handle, u32 lvl, void *context, void **rv)
> -{
> -	const struct acpi_device_id *ids = context;
> -	struct acpi_device *dev = acpi_fetch_acpi_dev(handle);
> -
> -	if (dev && acpi_match_device_ids(dev, ids) == 0)
> -		if (!IS_ERR_OR_NULL(acpi_create_platform_device(dev, NULL)))
> -			dev_info(&dev->dev,
> -				 "intel-hid: created platform device\n");
> -
> -	return AE_OK;
> -}
> -
> -static int __init intel_hid_init(void)
> -{
> -	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> -			    ACPI_UINT32_MAX, check_acpi_dev, NULL,
> -			    (void *)intel_hid_ids, NULL);
> -
> -	return platform_driver_register(&intel_hid_pl_driver);
> -}
> -module_init(intel_hid_init);
> -
> -static void __exit intel_hid_exit(void)
> -{
> -	platform_driver_unregister(&intel_hid_pl_driver);
> -}
> -module_exit(intel_hid_exit);
> +module_acpi_platform_driver(intel_hid_pl_driver);
> diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
> index 232cd12e3c9f..42932479de35 100644
> --- a/drivers/platform/x86/intel/vbtn.c
> +++ b/drivers/platform/x86/intel/vbtn.c

...

> -static int __init intel_vbtn_init(void)
> -{
> -	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
> -			    ACPI_UINT32_MAX, check_acpi_dev, NULL,
> -			    (void *)intel_vbtn_ids, NULL);

Too bad there is no comment here. I wonder if this is necessary
at all, or if this was just copy & pasted from the intel/hid.c
driver.

git blame is not really helpful here, the acpi_walk_namespace()
was added in 332e081225fc2 ("intel-vbtn: new driver for Intel Virtual
Button").

So it looks like this is just copy paste and maybe a regular
module_platform_driver() will be sufficient here. But changing
behavior like that is out of scope for this patch-set, so please
keep using module_acpi_platform_driver()

Otherwise this looks good to me.

Regards,

Hans