[PATCH 0/4] Convert ACPI fan driver to platform driver

[PATCH 0/4] Convert ACPI fan driver to platform driver

[Aaron Lu]

This patchset converts ACPI fan driver to platform driver. Patch 1-3 are
cleanups for existing fan driver and patch 4 does the convertion.

Tested on harris beach.
Apply on top of Rafael's linux-next branch.

Aaron Lu (4):
  ACPI / fan: remove unused macro for debug
  ACPI / fan: remove no need check for device pointer
  ACPI / fan: use acpi_device_xxx_power instead of acpi_bus equivelant
  ACPI / fan: convert to platform driver

 drivers/acpi/acpi_platform.c |  3 ++
 drivers/acpi/device_pm.c     |  1 +
 drivers/acpi/fan.c           | 88 ++++++++++++++++----------------------------
 drivers/acpi/internal.h      |  2 -
 include/acpi/acpi_bus.h      |  1 +
 5 files changed, 36 insertions(+), 59 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] ACPI / fan: remove unused macro for debug

[Aaron Lu]

The _COMPONENT and ACPI_MODULE_NAME(name) is used for ACPI_DEBUG_PRINT,
since there is no such use in fan module we can remove them.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/fan.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 1fb62900f32a..9638d28513e8 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -36,9 +36,6 @@
 #define ACPI_FAN_CLASS			"fan"
 #define ACPI_FAN_FILE_STATE		"state"
 
-#define _COMPONENT		ACPI_FAN_COMPONENT
-ACPI_MODULE_NAME("fan");
-
 MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_DESCRIPTION("ACPI Fan Driver");
 MODULE_LICENSE("GPL");
-- 
1.8.3.1

[PATCH 2/4] ACPI / fan: remove no need check for device pointer

[Aaron Lu]

The device pointer will not be NULL in the PM callback and ACPI driver's
add/remove callback, so checking NULL for them isn't necessary.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/fan.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 9638d28513e8..4a624a35f07d 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -124,9 +124,6 @@ static int acpi_fan_add(struct acpi_device *device)
 	int result = 0;
 	struct thermal_cooling_device *cdev;
 
-	if (!device)
-		return -EINVAL;
-
 	strcpy(acpi_device_name(device), "Fan");
 	strcpy(acpi_device_class(device), ACPI_FAN_CLASS);
 
@@ -170,14 +167,7 @@ end:
 
 static int acpi_fan_remove(struct acpi_device *device)
 {
-	struct thermal_cooling_device *cdev;
-
-	if (!device)
-		return -EINVAL;
-
-	cdev =  acpi_driver_data(device);
-	if (!cdev)
-		return -EINVAL;
+	struct thermal_cooling_device *cdev = acpi_driver_data(device);
 
 	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
 	sysfs_remove_link(&cdev->device.kobj, "device");
@@ -189,9 +179,6 @@ static int acpi_fan_remove(struct acpi_device *device)
 #ifdef CONFIG_PM_SLEEP
 static int acpi_fan_suspend(struct device *dev)
 {
-	if (!dev)
-		return -EINVAL;
-
 	acpi_bus_set_power(to_acpi_device(dev)->handle, ACPI_STATE_D0);
 
 	return AE_OK;
@@ -201,9 +188,6 @@ static int acpi_fan_resume(struct device *dev)
 {
 	int result;
 
-	if (!dev)
-		return -EINVAL;
-
 	result = acpi_bus_update_power(to_acpi_device(dev)->handle, NULL);
 	if (result)
 		printk(KERN_ERR PREFIX "Error updating fan power state\n");
-- 
1.8.3.1

[PATCH 3/4] ACPI / fan: use acpi_device_xxx_power instead of acpi_bus equivelant

[Aaron Lu]

When we have the acpi_device pointer, there is no need to pass the
device's handle to the acpi_bus_xxx_power functions to get/set/update
the device's power state, instead, use the acpi_device_xxx_power
functions directly.

To make this happen for fan module, export acpi_device_update_power.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/device_pm.c |  1 +
 drivers/acpi/fan.c       | 10 +++++-----
 drivers/acpi/internal.h  |  2 --
 include/acpi/acpi_bus.h  |  1 +
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index d49f1e464703..d85b88f88979 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -343,6 +343,7 @@ int acpi_device_update_power(struct acpi_device *device, int *state_p)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(acpi_device_update_power);
 
 int acpi_bus_update_power(acpi_handle handle, int *state_p)
 {
diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 4a624a35f07d..8683bed14dac 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -85,7 +85,7 @@ static int fan_get_cur_state(struct thermal_cooling_device *cdev, unsigned long
 	if (!device)
 		return -EINVAL;
 
-	result = acpi_bus_update_power(device->handle, &acpi_state);
+	result = acpi_device_update_power(device, &acpi_state);
 	if (result)
 		return result;
 
@@ -103,7 +103,7 @@ fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
 	if (!device || (state != 0 && state != 1))
 		return -EINVAL;
 
-	result = acpi_bus_set_power(device->handle,
+	result = acpi_device_set_power(device,
 				state ? ACPI_STATE_D0 : ACPI_STATE_D3_COLD);
 
 	return result;
@@ -127,7 +127,7 @@ static int acpi_fan_add(struct acpi_device *device)
 	strcpy(acpi_device_name(device), "Fan");
 	strcpy(acpi_device_class(device), ACPI_FAN_CLASS);
 
-	result = acpi_bus_update_power(device->handle, NULL);
+	result = acpi_device_update_power(device, NULL);
 	if (result) {
 		printk(KERN_ERR PREFIX "Setting initial power state\n");
 		goto end;
@@ -179,7 +179,7 @@ static int acpi_fan_remove(struct acpi_device *device)
 #ifdef CONFIG_PM_SLEEP
 static int acpi_fan_suspend(struct device *dev)
 {
-	acpi_bus_set_power(to_acpi_device(dev)->handle, ACPI_STATE_D0);
+	acpi_device_set_power(to_acpi_device(dev), ACPI_STATE_D0);
 
 	return AE_OK;
 }
@@ -188,7 +188,7 @@ static int acpi_fan_resume(struct device *dev)
 {
 	int result;
 
-	result = acpi_bus_update_power(to_acpi_device(dev)->handle, NULL);
+	result = acpi_device_update_power(to_acpi_device(dev), NULL);
 	if (result)
 		printk(KERN_ERR PREFIX "Error updating fan power state\n");
 
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index b125fdb0b30c..d13e66cac8e1 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -106,8 +106,6 @@ int acpi_power_get_inferred_state(struct acpi_device *device, int *state);
 int acpi_power_on_resources(struct acpi_device *device, int state);
 int acpi_power_transition(struct acpi_device *device, int state);
 
-int acpi_device_update_power(struct acpi_device *device, int *state_p);
-
 int acpi_wakeup_device_init(void);
 void acpi_early_processor_set_pdc(void);
 
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 7135fe3d6daa..4b0df6e6bd86 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -363,6 +363,7 @@ int acpi_device_set_power(struct acpi_device *device, int state);
 int acpi_bus_init_power(struct acpi_device *device);
 int acpi_device_fix_up_power(struct acpi_device *device);
 int acpi_bus_update_power(acpi_handle handle, int *state_p);
+int acpi_device_update_power(struct acpi_device *device, int *state_p);
 bool acpi_bus_power_manageable(acpi_handle handle);
 
 #ifdef CONFIG_PM
-- 
1.8.3.1

[PATCH 4/4] ACPI / fan: convert to platform driver

[Aaron Lu]

Convert ACPI fan driver to a platform driver for the purpose of phasing
out ACPI bus.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/acpi/acpi_platform.c |  3 +++
 drivers/acpi/fan.c           | 63 ++++++++++++++++++++------------------------
 2 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c
index dbfe49e5fd63..69e5317c7753 100644
--- a/drivers/acpi/acpi_platform.c
+++ b/drivers/acpi/acpi_platform.c
@@ -37,6 +37,9 @@ static const struct acpi_device_id acpi_platform_device_ids[] = {
 	{ "INT33C8" },
 	{ "80860F28" },
 
+	/* ACPI fan device */
+	{ "PNP0C0B" },
+
 	{ }
 };
 
diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
index 8683bed14dac..78b054ee245c 100644
--- a/drivers/acpi/fan.c
+++ b/drivers/acpi/fan.c
@@ -30,18 +30,14 @@
 #include <asm/uaccess.h>
 #include <linux/thermal.h>
 #include <linux/acpi.h>
-
-#define PREFIX "ACPI: "
-
-#define ACPI_FAN_CLASS			"fan"
-#define ACPI_FAN_FILE_STATE		"state"
+#include <linux/platform_device.h>
 
 MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_DESCRIPTION("ACPI Fan Driver");
 MODULE_LICENSE("GPL");
 
-static int acpi_fan_add(struct acpi_device *device);
-static int acpi_fan_remove(struct acpi_device *device);
+static int acpi_fan_probe(struct platform_device *pdev);
+static int acpi_fan_remove(struct platform_device *pdev);
 
 static const struct acpi_device_id fan_device_ids[] = {
 	{"PNP0C0B", 0},
@@ -55,15 +51,14 @@ static int acpi_fan_resume(struct device *dev);
 #endif
 static SIMPLE_DEV_PM_OPS(acpi_fan_pm, acpi_fan_suspend, acpi_fan_resume);
 
-static struct acpi_driver acpi_fan_driver = {
-	.name = "fan",
-	.class = ACPI_FAN_CLASS,
-	.ids = fan_device_ids,
-	.ops = {
-		.add = acpi_fan_add,
-		.remove = acpi_fan_remove,
-		},
-	.drv.pm = &acpi_fan_pm,
+static struct platform_driver acpi_fan_driver = {
+	.probe = acpi_fan_probe,
+	.remove = acpi_fan_remove,
+	.driver = {
+		.name = "acpi-fan",
+		.acpi_match_table = fan_device_ids,
+		.pm = &acpi_fan_pm,
+	},
 };
 
 /* thermal cooling device callbacks */
@@ -119,17 +114,15 @@ static const struct thermal_cooling_device_ops fan_cooling_ops = {
                                  Driver Interface
    -------------------------------------------------------------------------- */
 
-static int acpi_fan_add(struct acpi_device *device)
+static int acpi_fan_probe(struct platform_device *pdev)
 {
 	int result = 0;
 	struct thermal_cooling_device *cdev;
-
-	strcpy(acpi_device_name(device), "Fan");
-	strcpy(acpi_device_class(device), ACPI_FAN_CLASS);
+	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
 
 	result = acpi_device_update_power(device, NULL);
 	if (result) {
-		printk(KERN_ERR PREFIX "Setting initial power state\n");
+		dev_err(&pdev->dev, "Setting initial power state\n");
 		goto end;
 	}
 
@@ -140,24 +133,24 @@ static int acpi_fan_add(struct acpi_device *device)
 		goto end;
 	}
 
-	dev_dbg(&device->dev, "registered as cooling_device%d\n", cdev->id);
+	dev_dbg(&pdev->dev, "registered as cooling_device%d\n", cdev->id);
 
-	device->driver_data = cdev;
-	result = sysfs_create_link(&device->dev.kobj,
+	platform_set_drvdata(pdev, cdev);
+	result = sysfs_create_link(&pdev->dev.kobj,
 				   &cdev->device.kobj,
 				   "thermal_cooling");
 	if (result)
-		dev_err(&device->dev, "Failed to create sysfs link "
+		dev_err(&pdev->dev, "Failed to create sysfs link "
 			"'thermal_cooling'\n");
 
 	result = sysfs_create_link(&cdev->device.kobj,
-				   &device->dev.kobj,
+				   &pdev->dev.kobj,
 				   "device");
 	if (result)
-		dev_err(&device->dev, "Failed to create sysfs link "
+		dev_err(&pdev->dev, "Failed to create sysfs link "
 			"'device'\n");
 
-	printk(KERN_INFO PREFIX "%s [%s] (%s)\n",
+	dev_info(&pdev->dev, "%s [%s] (%s)\n",
 	       acpi_device_name(device), acpi_device_bid(device),
 	       !device->power.state ? "on" : "off");
 
@@ -165,11 +158,11 @@ end:
 	return result;
 }
 
-static int acpi_fan_remove(struct acpi_device *device)
+static int acpi_fan_remove(struct platform_device *pdev)
 {
-	struct thermal_cooling_device *cdev = acpi_driver_data(device);
+	struct thermal_cooling_device *cdev = platform_get_drvdata(pdev);
 
-	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
+	sysfs_remove_link(&pdev->dev.kobj, "thermal_cooling");
 	sysfs_remove_link(&cdev->device.kobj, "device");
 	thermal_cooling_device_unregister(cdev);
 
@@ -179,7 +172,7 @@ static int acpi_fan_remove(struct acpi_device *device)
 #ifdef CONFIG_PM_SLEEP
 static int acpi_fan_suspend(struct device *dev)
 {
-	acpi_device_set_power(to_acpi_device(dev), ACPI_STATE_D0);
+	acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0);
 
 	return AE_OK;
 }
@@ -188,12 +181,12 @@ static int acpi_fan_resume(struct device *dev)
 {
 	int result;
 
-	result = acpi_device_update_power(to_acpi_device(dev), NULL);
+	result = acpi_device_update_power(ACPI_COMPANION(dev), NULL);
 	if (result)
-		printk(KERN_ERR PREFIX "Error updating fan power state\n");
+		dev_err(dev, "Error updating fan power state\n");
 
 	return result;
 }
 #endif
 
-module_acpi_driver(acpi_fan_driver);
+module_platform_driver(acpi_fan_driver);
-- 
1.8.3.1

Re: [PATCH 0/4] Convert ACPI fan driver to platform driver

[Rafael J. Wysocki]

On Tuesday, December 03, 2013 04:28:28 PM Aaron Lu wrote:
> This patchset converts ACPI fan driver to platform driver. Patch 1-3 are
> cleanups for existing fan driver and patch 4 does the convertion.
> 
> Tested on harris beach.
> Apply on top of Rafael's linux-next branch.
> 
> Aaron Lu (4):
>   ACPI / fan: remove unused macro for debug
>   ACPI / fan: remove no need check for device pointer
>   ACPI / fan: use acpi_device_xxx_power instead of acpi_bus equivelant
>   ACPI / fan: convert to platform driver
> 
>  drivers/acpi/acpi_platform.c |  3 ++
>  drivers/acpi/device_pm.c     |  1 +
>  drivers/acpi/fan.c           | 88 ++++++++++++++++----------------------------
>  drivers/acpi/internal.h      |  2 -
>  include/acpi/acpi_bus.h      |  1 +
>  5 files changed, 36 insertions(+), 59 deletions(-)

Unfortunately, we need to postpone these conversions, because Matthew Garrett
has problems with adding more entries to acpi_platform_device_ids[].  He seems
to be concerned that that list will grow indefinitely and will become difficult
to maintain eventually.

For this reason, he would prefer it if we did the following:
- Figure out the list of ACPI device IDs we need to create PNP devices for
  via ACPI PNP.
- Make ACPI PNP create PNP devices for these IDs only and make the ACPI core create
  platform devices for all "unassigned" ACPI device objects by default.
- Do the conversions at that point.

I'm slightly worried that we'll encounter ordering issues while doing that, but
this is the only way forward I can see without going straight against the
Matthew's objections, which I'd prefer to avoid.

Thanks,
Rafael

Re: [PATCH 0/4] Convert ACPI fan driver to platform driver

[Rafael J. Wysocki]

Cc: +Matthew (sorry, omitted by mistake previously)

On Thursday, December 05, 2013 12:07:31 AM Rafael J. Wysocki wrote:
> On Tuesday, December 03, 2013 04:28:28 PM Aaron Lu wrote:
> > This patchset converts ACPI fan driver to platform driver. Patch 1-3 are
> > cleanups for existing fan driver and patch 4 does the convertion.
> > 
> > Tested on harris beach.
> > Apply on top of Rafael's linux-next branch.
> > 
> > Aaron Lu (4):
> >   ACPI / fan: remove unused macro for debug
> >   ACPI / fan: remove no need check for device pointer
> >   ACPI / fan: use acpi_device_xxx_power instead of acpi_bus equivelant
> >   ACPI / fan: convert to platform driver
> > 
> >  drivers/acpi/acpi_platform.c |  3 ++
> >  drivers/acpi/device_pm.c     |  1 +
> >  drivers/acpi/fan.c           | 88 ++++++++++++++++----------------------------
> >  drivers/acpi/internal.h      |  2 -
> >  include/acpi/acpi_bus.h      |  1 +
> >  5 files changed, 36 insertions(+), 59 deletions(-)
> 
> Unfortunately, we need to postpone these conversions, because Matthew Garrett
> has problems with adding more entries to acpi_platform_device_ids[].  He seems
> to be concerned that that list will grow indefinitely and will become difficult
> to maintain eventually.
> 
> For this reason, he would prefer it if we did the following:
> - Figure out the list of ACPI device IDs we need to create PNP devices for
>   via ACPI PNP.
> - Make ACPI PNP create PNP devices for these IDs only and make the ACPI core create
>   platform devices for all "unassigned" ACPI device objects by default.
> - Do the conversions at that point.
> 
> I'm slightly worried that we'll encounter ordering issues while doing that, but
> this is the only way forward I can see without going straight against the
> Matthew's objections, which I'd prefer to avoid.
> 
> Thanks,
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 0/4] Convert ACPI fan driver to platform driver

[Zhang, Rui]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4692 bytes --]



> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Thursday, December 05, 2013 7:10 AM
> To: Lu, Aaron
> Cc: Zhang, Rui; linux-acpi@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lan, Tianyu; Brown, Len; Matthew Garrett
> Subject: Re: [PATCH 0/4] Convert ACPI fan driver to platform driver
> Importance: High
> 
> Cc: +Matthew (sorry, omitted by mistake previously)
> 
> On Thursday, December 05, 2013 12:07:31 AM Rafael J. Wysocki wrote:
> > On Tuesday, December 03, 2013 04:28:28 PM Aaron Lu wrote:
> > > This patchset converts ACPI fan driver to platform driver. Patch 1-
> 3
> > > are cleanups for existing fan driver and patch 4 does the
> convertion.
> > >
> > > Tested on harris beach.
> > > Apply on top of Rafael's linux-next branch.
> > >
> > > Aaron Lu (4):
> > >   ACPI / fan: remove unused macro for debug
> > >   ACPI / fan: remove no need check for device pointer
> > >   ACPI / fan: use acpi_device_xxx_power instead of acpi_bus
> equivelant
> > >   ACPI / fan: convert to platform driver
> > >
> > >  drivers/acpi/acpi_platform.c |  3 ++
> > >  drivers/acpi/device_pm.c     |  1 +
> > >  drivers/acpi/fan.c           | 88 ++++++++++++++++----------------
> ------------
> > >  drivers/acpi/internal.h      |  2 -
> > >  include/acpi/acpi_bus.h      |  1 +
> > >  5 files changed, 36 insertions(+), 59 deletions(-)
> >
> > Unfortunately, we need to postpone these conversions, because Matthew
> > Garrett has problems with adding more entries to
> > acpi_platform_device_ids[].  He seems to be concerned that that list
> > will grow indefinitely and will become difficult to maintain
> eventually.
> >
> > For this reason, he would prefer it if we did the following:
> > - Figure out the list of ACPI device IDs we need to create PNP
> devices for
> >   via ACPI PNP.

Agreed.
I'd also prefer a whitelist for pnpacpi instead of a backlist.
And this is helpful for us to clean up the ACPI code in PNP bus,
Including pnpacpi code and PNP drivers that actually bind to
ACPI enumerated PNP devices.

Question is how to get this white list?
Can we just collect the pnp_device_id in all the PNP drivers as the first step?

> > - Make ACPI PNP create PNP devices for these IDs only and make the
> ACPI core create
> >   platform devices for all "unassigned" ACPI device objects by
> default.

So the above statement means that an ACPI device will be enumerated
to either PNP bus or platform bus, right?
Then I'm wondering how to handle the following cases,
1. an ACPI device may have both _HID and _CID, if its _CID is in the
   PNP whitelist, does this mean the device will always be enumerated
   to PNP bus?
2. is it possible to see the following ASL code?
   Device (ABCDEFG) /* should be enumerated to platform bus */
   {
     Device (PNPABCD) /* should be enumerated to PNP bus */
     {
     }
   }
   If yes, it seems that we will introduce some ugly parent/child
   relationship in driver core?

> > - Do the conversions at that point.
> >
> > I'm slightly worried that we'll encounter ordering issues while doing
> > that, but this is the only way forward I can see without going
> > straight against the Matthew's objections, which I'd prefer to avoid.
> >
Maybe a stupid question, why can't we enumerate all ACPI devices to
platform bus? Here are some of my thoughts,
1. enumerate all ACPI devices to platform bus, thus our ACPI -> platform bus
   conversion work can continue.
2. workout a white list for PNPACPI (by collecting all the pnp_device_ids)
3. cleanup the PNP drivers.
   If a PNP driver is supposed to bind to ACPI enumerated PNP devices,
   convert it to a platform driver.
   If a PNP driver can probe device enumerated from either PNPBIOS or PNPACPI,
   make it a dual-head driver that can probe both PNP and platform devices.
   If a PNP driver is supposed to bind to PNPBIOS enumerated PNP devices,
   leave it as it is.
   Note: every time we have done the cleanup for a driver,
         we can delete one entry in the PNPACPI whitelist in step 2.
4. after cleanup all the PNP drivers, the whitelist is empty
   and we can remove all the PNPACPI code.

Thanks,
rui
> > Thanks,
> > Rafael
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-kernel" in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/4] Convert ACPI fan driver to platform driver

[Rafael J. Wysocki]

On Thursday, December 05, 2013 01:56:24 PM Zhang, Rui wrote:
> 
> > -----Original Message-----
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Sent: Thursday, December 05, 2013 7:10 AM
> > To: Lu, Aaron
> > Cc: Zhang, Rui; linux-acpi@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Lan, Tianyu; Brown, Len; Matthew Garrett
> > Subject: Re: [PATCH 0/4] Convert ACPI fan driver to platform driver
> > Importance: High
> > 
> > Cc: +Matthew (sorry, omitted by mistake previously)
> > 
> > On Thursday, December 05, 2013 12:07:31 AM Rafael J. Wysocki wrote:
> > > On Tuesday, December 03, 2013 04:28:28 PM Aaron Lu wrote:
> > > > This patchset converts ACPI fan driver to platform driver. Patch 1-
> > 3
> > > > are cleanups for existing fan driver and patch 4 does the
> > convertion.
> > > >
> > > > Tested on harris beach.
> > > > Apply on top of Rafael's linux-next branch.
> > > >
> > > > Aaron Lu (4):
> > > >   ACPI / fan: remove unused macro for debug
> > > >   ACPI / fan: remove no need check for device pointer
> > > >   ACPI / fan: use acpi_device_xxx_power instead of acpi_bus
> > equivelant
> > > >   ACPI / fan: convert to platform driver
> > > >
> > > >  drivers/acpi/acpi_platform.c |  3 ++
> > > >  drivers/acpi/device_pm.c     |  1 +
> > > >  drivers/acpi/fan.c           | 88 ++++++++++++++++----------------
> > ------------
> > > >  drivers/acpi/internal.h      |  2 -
> > > >  include/acpi/acpi_bus.h      |  1 +
> > > >  5 files changed, 36 insertions(+), 59 deletions(-)
> > >
> > > Unfortunately, we need to postpone these conversions, because Matthew
> > > Garrett has problems with adding more entries to
> > > acpi_platform_device_ids[].  He seems to be concerned that that list
> > > will grow indefinitely and will become difficult to maintain
> > eventually.
> > >
> > > For this reason, he would prefer it if we did the following:
> > > - Figure out the list of ACPI device IDs we need to create PNP
> > devices for
> > >   via ACPI PNP.
> 
> Agreed.
> I'd also prefer a whitelist for pnpacpi instead of a backlist.
> And this is helpful for us to clean up the ACPI code in PNP bus,
> Including pnpacpi code and PNP drivers that actually bind to
> ACPI enumerated PNP devices.
> 
> Question is how to get this white list?
> Can we just collect the pnp_device_id in all the PNP drivers as the first step?

Yes, we can.

> > > - Make ACPI PNP create PNP devices for these IDs only and make the
> > ACPI core create
> > >   platform devices for all "unassigned" ACPI device objects by
> > default.
> 
> So the above statement means that an ACPI device will be enumerated
> to either PNP bus or platform bus, right?

No.  Processors, memory, containers etc will have their own device
representations.

> Then I'm wondering how to handle the following cases,
> 1. an ACPI device may have both _HID and _CID, if its _CID is in the
>    PNP whitelist, does this mean the device will always be enumerated
>    to PNP bus?

No.  We need to check _HID first as per the spec.

> 2. is it possible to see the following ASL code?
>    Device (ABCDEFG) /* should be enumerated to platform bus */
>    {
>      Device (PNPABCD) /* should be enumerated to PNP bus */
>      {
>      }
>    }
>    If yes, it seems that we will introduce some ugly parent/child
>    relationship in driver core?

There's no rule that PNP devices cannot be children of platform devices as long
as we handle resources assignments correctly.

> > > - Do the conversions at that point.
> > >
> > > I'm slightly worried that we'll encounter ordering issues while doing
> > > that, but this is the only way forward I can see without going
> > > straight against the Matthew's objections, which I'd prefer to avoid.
> > >
> Maybe a stupid question, why can't we enumerate all ACPI devices to
> platform bus?

First of all, as I said, processors, memory modules and such will have their
own device representations anyway.

Second, if the question why we need *both* platform and PNP, my answer is that
we probably don't need them both in the long run, but we need to carry out
the conversion "nicely".  That is, keep PNP devices used by the existing PNP
drivers for the time being and convert them gradually over time.

> Here are some of my thoughts,
> 1. enumerate all ACPI devices to platform bus, thus our ACPI -> platform bus
>    conversion work can continue.

The objection is against adding more entries to the list of IDs in
acpi_platform.c.  To avoid that we need to create platform devices for
ACPI device nodes by default (that is, if they have no scan handlers attached
etc.).  And for that we need to stop creating PNP devices for them by default
(which we do today).  And for that we need a list of ACPI device IDs for which
we need corresponding PNP devices, because the existing drivers use them (and
we need to continue to create PNP devices for them until we convert the PNP
drivers into platform drivers - if we can do that).

> 2. workout a white list for PNPACPI (by collecting all the pnp_device_ids)
> 3. cleanup the PNP drivers.
>    If a PNP driver is supposed to bind to ACPI enumerated PNP devices,
>    convert it to a platform driver.
>    If a PNP driver can probe device enumerated from either PNPBIOS or PNPACPI,
>    make it a dual-head driver that can probe both PNP and platform devices.
>    If a PNP driver is supposed to bind to PNPBIOS enumerated PNP devices,
>    leave it as it is.
>    Note: every time we have done the cleanup for a driver,
>          we can delete one entry in the PNPACPI whitelist in step 2.
> 4. after cleanup all the PNP drivers, the whitelist is empty
>    and we can remove all the PNPACPI code.

Yes, something like that.

Thanks,
Rafael

Re: [PATCH 0/4] Convert ACPI fan driver to platform driver

[Aaron Lu]

On Thu, Dec 05, 2013 at 12:07:31AM +0100, Rafael J. Wysocki wrote:
> On Tuesday, December 03, 2013 04:28:28 PM Aaron Lu wrote:
> > This patchset converts ACPI fan driver to platform driver. Patch 1-3 are
> > cleanups for existing fan driver and patch 4 does the convertion.
> > 
> > Tested on harris beach.
> > Apply on top of Rafael's linux-next branch.
> > 
> > Aaron Lu (4):
> >   ACPI / fan: remove unused macro for debug
> >   ACPI / fan: remove no need check for device pointer
> >   ACPI / fan: use acpi_device_xxx_power instead of acpi_bus equivelant
> >   ACPI / fan: convert to platform driver
> > 
> >  drivers/acpi/acpi_platform.c |  3 ++
> >  drivers/acpi/device_pm.c     |  1 +
> >  drivers/acpi/fan.c           | 88 ++++++++++++++++----------------------------
> >  drivers/acpi/internal.h      |  2 -
> >  include/acpi/acpi_bus.h      |  1 +
> >  5 files changed, 36 insertions(+), 59 deletions(-)
> 
> Unfortunately, we need to postpone these conversions, because Matthew Garrett
> has problems with adding more entries to acpi_platform_device_ids[].  He seems
> to be concerned that that list will grow indefinitely and will become difficult
> to maintain eventually.
> 
> For this reason, he would prefer it if we did the following:
> - Figure out the list of ACPI device IDs we need to create PNP devices for
>   via ACPI PNP.

I'm not sure how to tell this, is it that as long as the ACPI node has a
PNPxxxx ID we will need to create a PNP device for it? And in this case,
do we only check the _HID or both _HID and _CID?

> - Make ACPI PNP create PNP devices for these IDs only and make the ACPI core create
>   platform devices for all "unassigned" ACPI device objects by default.

Does "unassigned" mean (all ACPI devices) - (ACPI devices that have a
PNP device created already)?

Thanks,
Aaron

> - Do the conversions at that point.
> 
> I'm slightly worried that we'll encounter ordering issues while doing that, but
> this is the only way forward I can see without going straight against the
> Matthew's objections, which I'd prefer to avoid.
> 
> Thanks,
> Rafael
> 

Re: [PATCH 0/4] Convert ACPI fan driver to platform driver

[Rafael J. Wysocki]

On Thursday, December 05, 2013 03:47:35 PM Aaron Lu wrote:
> On Thu, Dec 05, 2013 at 12:07:31AM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, December 03, 2013 04:28:28 PM Aaron Lu wrote:
> > > This patchset converts ACPI fan driver to platform driver. Patch 1-3 are
> > > cleanups for existing fan driver and patch 4 does the convertion.
> > > 
> > > Tested on harris beach.
> > > Apply on top of Rafael's linux-next branch.
> > > 
> > > Aaron Lu (4):
> > >   ACPI / fan: remove unused macro for debug
> > >   ACPI / fan: remove no need check for device pointer
> > >   ACPI / fan: use acpi_device_xxx_power instead of acpi_bus equivelant
> > >   ACPI / fan: convert to platform driver
> > > 
> > >  drivers/acpi/acpi_platform.c |  3 ++
> > >  drivers/acpi/device_pm.c     |  1 +
> > >  drivers/acpi/fan.c           | 88 ++++++++++++++++----------------------------
> > >  drivers/acpi/internal.h      |  2 -
> > >  include/acpi/acpi_bus.h      |  1 +
> > >  5 files changed, 36 insertions(+), 59 deletions(-)
> > 
> > Unfortunately, we need to postpone these conversions, because Matthew Garrett
> > has problems with adding more entries to acpi_platform_device_ids[].  He seems
> > to be concerned that that list will grow indefinitely and will become difficult
> > to maintain eventually.
> > 
> > For this reason, he would prefer it if we did the following:
> > - Figure out the list of ACPI device IDs we need to create PNP devices for
> >   via ACPI PNP.
> 
> I'm not sure how to tell this, is it that as long as the ACPI node has a
> PNPxxxx ID we will need to create a PNP device for it? And in this case,
> do we only check the _HID or both _HID and _CID?

Just check the existing PNP drivers and see what device IDs they use.

> > - Make ACPI PNP create PNP devices for these IDs only and make the ACPI core create
> >   platform devices for all "unassigned" ACPI device objects by default.
> 
> Does "unassigned" mean (all ACPI devices) - (ACPI devices that have a
> PNP device created already)?

(all ACPI devices nodes) - (nodes taken care of by scan handlers + PNP)
(and I *think* we can turn the PNP ACPI driver into a scan handler).

Thanks,
Rafael