Re: [PATCH 4/4] ACPI / platform: Use struct acpi_scan_handler for creating devices

[Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>]

Hi Rafael,

2013/01/28 22:01, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Currently, the ACPI namespace scanning code creates platform device
> objects for ACPI device nodes whose IDs match the contents of the
> acpi_platform_device_ids[] table.  However, this adds a superfluous
> special case into acpi_bus_device_attach() and makes it more
> difficult to follow than it has to be.  It also will make it more
> difficult to implement removal code for those platform device objects
> in the future.
>
> For the above reasons, introduce a struct acpi_scan_handler object
> for creating platform devices and move the code related to that from
> acpi_bus_device_attach() to the .attach() callback of that object.
> Also move the acpi_platform_device_ids[] table to acpi_platform.c.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/acpi_platform.c |   55 +++++++++++++++++++++++++++++++++++--------
>   drivers/acpi/internal.h      |    7 -----
>   drivers/acpi/scan.c          |   30 -----------------------
>   3 files changed, 47 insertions(+), 45 deletions(-)
>
> Index: test/drivers/acpi/acpi_platform.c
> ===================================================================
> --- test.orig/drivers/acpi/acpi_platform.c
> +++ test/drivers/acpi/acpi_platform.c
> @@ -22,6 +22,30 @@
>
>   ACPI_MODULE_NAME("platform");
>
> +/* Flags for acpi_create_platform_device */
> +#define ACPI_PLATFORM_CLK	BIT(0)
> +
> +/*
> + * The following ACPI IDs are known to be suitable for representing as
> + * platform devices.
> + */
> +static const struct acpi_device_id acpi_platform_device_ids[] = {
> +
> +	{ "PNP0D40" },
> +
> +	/* Haswell LPSS devices */
> +	{ "INT33C0", ACPI_PLATFORM_CLK },
> +	{ "INT33C1", ACPI_PLATFORM_CLK },
> +	{ "INT33C2", ACPI_PLATFORM_CLK },
> +	{ "INT33C3", ACPI_PLATFORM_CLK },
> +	{ "INT33C4", ACPI_PLATFORM_CLK },
> +	{ "INT33C5", ACPI_PLATFORM_CLK },
> +	{ "INT33C6", ACPI_PLATFORM_CLK },
> +	{ "INT33C7", ACPI_PLATFORM_CLK },
> +
> +	{ }
> +};
> +
>   static int acpi_create_platform_clks(struct acpi_device *adev)
>   {
>   	static struct platform_device *pdev;
> @@ -39,8 +63,7 @@ static int acpi_create_platform_clks(str
>   /**
>    * acpi_create_platform_device - Create platform device for ACPI device node
>    * @adev: ACPI device node to create a platform device for.
> - * @flags: ACPI_PLATFORM_* flags that affect the creation of the platform
> - *	   devices.
> + * @id: ACPI device ID used to match @adev.
>    *
>    * Check if the given @adev can be represented as a platform device and, if
>    * that's the case, create and register a platform device, populate its common
> @@ -48,9 +71,10 @@ static int acpi_create_platform_clks(str
>    *
>    * Name of the platform device will be the same as @adev's.
>    */
> -struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
> -						    unsigned long flags)
> +static int acpi_create_platform_device(struct acpi_device *adev,
> +				       const struct acpi_device_id *id)
>   {
> +	unsigned long flags = id->driver_data;
>   	struct platform_device *pdev = NULL;
>   	struct acpi_device *acpi_parent;
>   	struct platform_device_info pdevinfo;
> @@ -59,25 +83,26 @@ struct platform_device *acpi_create_plat
>   	struct resource *resources;
>   	int count;
>

> -	if ((flags & ACPI_PLATFORM_CLK) && acpi_create_platform_clks(adev)) {
> +	if (flags & ACPI_PLATFORM_CLK) {
> +		int ret = acpi_create_platform_clks(adev);
>   		dev_err(&adev->dev, "failed to create clocks\n");
> -		return NULL;
> +		return ret;
>   	}

If (flag & ACPI_PLATFORM_CLK) is true, the acpi_create_platform_device()
always retruns with dev_err() messages. Why?

Thanks,
Yasuaki Ishimatsu

>
>   	/* If the ACPI node already has a physical device attached, skip it. */
>   	if (adev->physical_node_count)
> -		return NULL;
> +		return 0;
>
>   	INIT_LIST_HEAD(&resource_list);
>   	count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
>   	if (count <= 0)
> -		return NULL;
> +		return 0;
>
>   	resources = kmalloc(count * sizeof(struct resource), GFP_KERNEL);
>   	if (!resources) {
>   		dev_err(&adev->dev, "No memory for resources\n");
>   		acpi_dev_free_resource_list(&resource_list);
> -		return NULL;
> +		return -ENOMEM;
>   	}
>   	count = 0;
>   	list_for_each_entry(rentry, &resource_list, node)
> @@ -123,5 +148,15 @@ struct platform_device *acpi_create_plat
>   	}
>
>   	kfree(resources);
> -	return pdev;
> +	return 1;
> +}
> +
> +static struct acpi_scan_handler platform_handler = {
> +	.ids = acpi_platform_device_ids,
> +	.attach = acpi_create_platform_device,
> +};
> +
> +void __init acpi_platform_init(void)
> +{
> +	acpi_scan_add_handler(&platform_handler);
>   }
> Index: test/drivers/acpi/internal.h
> ===================================================================
> --- test.orig/drivers/acpi/internal.h
> +++ test/drivers/acpi/internal.h
> @@ -27,6 +27,7 @@ int init_acpi_device_notify(void);
>   int acpi_scan_init(void);
>   void acpi_pci_root_init(void);
>   void acpi_pci_link_init(void);
> +void acpi_platform_init(void);
>   int acpi_sysfs_init(void);
>   void acpi_csrt_init(void);
>
> @@ -119,10 +120,4 @@ static inline void suspend_nvs_restore(v
>     -------------------------------------------------------------------------- */
>   struct platform_device;
>
> -/* Flags for acpi_create_platform_device */
> -#define ACPI_PLATFORM_CLK	BIT(0)
> -
> -struct platform_device *acpi_create_platform_device(struct acpi_device *adev,
> -						    unsigned long flags);
> -
>   #endif /* _ACPI_INTERNAL_H_ */
> Index: test/drivers/acpi/scan.c
> ===================================================================
> --- test.orig/drivers/acpi/scan.c
> +++ test/drivers/acpi/scan.c
> @@ -29,27 +29,6 @@ extern struct acpi_device *acpi_root;
>
>   static const char *dummy_hid = "device";
>
> -/*
> - * The following ACPI IDs are known to be suitable for representing as
> - * platform devices.
> - */
> -static const struct acpi_device_id acpi_platform_device_ids[] = {
> -
> -	{ "PNP0D40" },
> -
> -	/* Haswell LPSS devices */
> -	{ "INT33C0", ACPI_PLATFORM_CLK },
> -	{ "INT33C1", ACPI_PLATFORM_CLK },
> -	{ "INT33C2", ACPI_PLATFORM_CLK },
> -	{ "INT33C3", ACPI_PLATFORM_CLK },
> -	{ "INT33C4", ACPI_PLATFORM_CLK },
> -	{ "INT33C5", ACPI_PLATFORM_CLK },
> -	{ "INT33C6", ACPI_PLATFORM_CLK },
> -	{ "INT33C7", ACPI_PLATFORM_CLK },
> -
> -	{ }
> -};
> -
>   static LIST_HEAD(acpi_device_list);
>   static LIST_HEAD(acpi_bus_id_list);
>   static DEFINE_MUTEX(acpi_scan_lock);
> @@ -1606,7 +1585,6 @@ static int acpi_scan_attach_handler(stru
>   static acpi_status acpi_bus_device_attach(acpi_handle handle, u32 lvl_not_used,
>   					  void *not_used, void **ret_not_used)
>   {
> -	const struct acpi_device_id *id;
>   	struct acpi_device *device;
>   	unsigned long long sta_not_used;
>   	int ret;
> @@ -1621,13 +1599,6 @@ static acpi_status acpi_bus_device_attac
>   	if (acpi_bus_get_device(handle, &device))
>   		return AE_CTRL_DEPTH;
>
> -	id = __acpi_match_device(device, acpi_platform_device_ids);
> -	if (id) {
> -		/* This is a known good platform device. */
> -		acpi_create_platform_device(device, id->driver_data);
> -		return AE_OK;
> -	}
> -
>   	ret = acpi_scan_attach_handler(device);
>   	if (ret)
>   		return ret > 0 ? AE_OK : AE_CTRL_DEPTH;
> @@ -1775,6 +1746,7 @@ int __init acpi_scan_init(void)
>
>   	acpi_pci_root_init();
>   	acpi_pci_link_init();
> +	acpi_platform_init();
>   	acpi_csrt_init();
>
>   	/*
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>