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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday, January 29, 2013 11:20:44 AM Yasuaki Ishimatsu wrote:
> 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?

Ah.  By mistake. :-)

Thanks for poiting this out!

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.