[RFC]convert ACPI driver model to Linux driver model - takes 2

[RFC]convert ACPI driver model to Linux driver model - takes 2

[Shaohua Li]

Hi,
This is the updated patch for the topic. In this version, I completely
convert devices in ACPI bus into Linux driver model. Now the device
driver for ACPI bus device should be a 'struct acpi_phys_driver'. Old
style driver (struct acpi_driver) now can't work for ACPI bus devices.
Here I still regard devices listed in DSDT but not belonging to any
physical bus as ACPI bus devices.
For back compatibility, old style driver model still applies to non-ACPI
bus devices. In the implementation, we register an old style driver
(acpi_dummy) for each acpi bus device, whose goal is to enumerate acpi
bus devices and prevent new old style driver binding with the devices. 
I tried to convert one ACPI driver (link device driver) to the new
model. It's quite easy.
I'm looking forward to your comments/suggestions.

Thanks,
Shaohua
---

 linux-2.6.14-rc3-root/drivers/acpi/scan.c     |  231 +++++++++++++++++++++++++-
 linux-2.6.14-rc3-root/include/acpi/acpi_bus.h |   34 +++
 2 files changed, 263 insertions(+), 2 deletions(-)

diff -puN drivers/acpi/scan.c~acpi_bus drivers/acpi/scan.c
--- linux-2.6.14-rc3/drivers/acpi/scan.c~acpi_bus	2005-10-11 14:13:26.000000000 +0800
+++ linux-2.6.14-rc3-root/drivers/acpi/scan.c	2005-10-13 13:59:08.000000000 +0800
@@ -112,6 +112,234 @@ static struct kset acpi_namespace_kset =
 	.hotplug_ops = &namespace_hotplug_ops,
 };
 
+/* ACPI phys bus */
+static int acpi_phys_bus_match(struct device *dev, struct device_driver *drv)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+	struct acpi_phys_driver *phys_drv = to_acpi_phys_drv(drv);
+	return !acpi_match_ids(phys_dev->acpi_dev, phys_drv->ids);
+}
+
+static int acpi_phys_bus_hotplug(struct device *dev, char **envp,
+	int num_envp, char *buffer, int buffer_size)
+{
+	int i = 0;
+	struct acpi_device *acpi_dev = to_acpi_phys_dev(dev)->acpi_dev;
+	char *scratch;
+	int length = 0;
+	struct acpi_compatible_id_list *cid_list;
+
+	scratch = buffer;
+	if (acpi_dev->flags.hardware_id) {
+		envp[i++] = scratch;
+		length += scnprintf(scratch, buffer_size - length, "HWID=%s",
+			acpi_dev->pnp.hardware_id);
+		if ((buffer_size - length <=0) || (i > num_envp))
+			return -ENOMEM;
+		++length;
+		scratch += length;
+	}
+	if (acpi_dev->flags.compatible_ids) {
+		int j;
+		cid_list = acpi_dev->pnp.cid_list;
+
+		/* multiple _CID entries */
+		for (j = 0; j < cid_list->count; j++)
+		{
+			envp[i++] = scratch;
+			length += scnprintf(scratch, buffer_size - length,
+				"COMPTID=%s", cid_list->id[j].value);
+			if ((buffer_size - length <=0) || (i > num_envp))
+				return -ENOMEM;
+			++length;
+			scratch += length;
+		}
+	}
+	envp[i] = NULL;
+
+	return 0;
+}
+
+static int acpi_phys_bus_suspend(struct device * dev, pm_message_t state)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+
+	if (!phys_dev->driver || !phys_dev->driver->suspend)
+		return 0;
+	return phys_dev->driver->suspend(phys_dev, state);
+}
+
+static int acpi_phys_bus_resume(struct device * dev)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+
+	if (!phys_dev->driver || !phys_dev->driver->resume)
+		return 0;
+	return phys_dev->driver->resume(phys_dev);
+}
+
+static struct bus_type acpi_phys_bus_type = {
+	.name = "acpi",
+	.match = acpi_phys_bus_match,
+	.hotplug = acpi_phys_bus_hotplug,
+	.suspend = acpi_phys_bus_suspend,
+	.resume = acpi_phys_bus_resume,
+};
+
+static LIST_HEAD(acpi_phys_devices);
+static DEFINE_SPINLOCK(acpi_phys_bus_lock);
+static struct acpi_phys_device *phys_root;
+
+static void acpi_phys_device_release(struct device *dev)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+	kfree(phys_dev);
+}
+
+static int acpi_phys_bus_add(struct acpi_device *acpi_dev)
+{
+	struct acpi_phys_device *phys_dev;
+
+	phys_dev = kzalloc(sizeof(struct acpi_phys_device), GFP_KERNEL);
+	if (!phys_dev)
+		return -ENOMEM;
+
+	spin_lock(&acpi_phys_bus_lock);
+	list_add_tail(&phys_dev->node, &acpi_phys_devices);
+	spin_unlock(&acpi_phys_bus_lock);
+
+	phys_dev->acpi_dev = acpi_dev;
+	phys_dev->phys_dev.parent = &phys_root->phys_dev;
+	phys_dev->phys_dev.bus = &acpi_phys_bus_type;
+	device_initialize(&phys_dev->phys_dev);
+	sprintf(phys_dev->phys_dev.bus_id, "%s", acpi_dev->pnp.bus_id);
+	phys_dev->phys_dev.release = acpi_phys_device_release;
+	device_add(&phys_dev->phys_dev);
+	return 0;
+}
+
+static int acpi_phys_bus_remove(struct acpi_device *acpi_dev, int type)
+{
+	struct acpi_phys_device *phys_dev, *tmp, *find = NULL;
+
+	spin_lock(&acpi_phys_bus_lock);
+	list_for_each_entry_safe(phys_dev, tmp, &acpi_phys_devices, node) {
+		if (phys_dev->acpi_dev == acpi_dev) {
+			find = phys_dev;
+			list_del(&find->node);
+			break;
+		}
+	}
+	spin_unlock(&acpi_phys_bus_lock);
+	if (find)
+		device_unregister(&find->phys_dev);
+	return 0;
+}
+
+/* this driver is linking acpi device with acpi phys device */
+static struct acpi_driver dummy_driver = {
+	.name = "acpi_dummy",
+	/* List all ids of ACPI phys devices */
+	.ids =
+//		"PNP0C09," /* EC */
+//		"ACPI0003," /* AC */
+//		"PNP0C0F," /* Link */
+//		"PNP0C0A," /* battery */
+//		"PNP0C0C,PNP0C0E,PNP0C0D,ACPI_FPB,ACPI_FSB," /* button */
+//		"PNP0C0B," /* fan */
+//		"PNP0A03," /* PCI root */
+//		ACPI_POWER_HID"," /* power */
+//		ACPI_PROCESSOR_HID"," /* CPU */
+//		ACPI_THERMAL_HID"," /* thermal */
+		"",
+	.ops = {
+		.add = acpi_phys_bus_add,
+		.remove = acpi_phys_bus_remove,
+	},
+};
+
+static int __init acpi_phys_bus_init(void)
+{
+	bus_register(&acpi_phys_bus_type);
+
+	phys_root = kzalloc(sizeof(struct acpi_phys_device), GFP_KERNEL);
+	if (!phys_root)
+		return -ENOMEM;
+
+	phys_root->acpi_dev = acpi_root;
+	phys_root->phys_dev.release = acpi_phys_device_release;
+	sprintf(phys_root->phys_dev.bus_id, "%s", acpi_root->pnp.bus_id);
+	device_register(&phys_root->phys_dev);
+
+	/* enumerate devices */
+	acpi_bus_register_driver(&dummy_driver);
+	return 0;
+}
+
+
+static int acpi_phys_device_probe(struct device *dev)
+{
+	int error = -ENODEV;
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+	struct acpi_phys_driver *phys_drv = to_acpi_phys_drv(dev->driver);
+
+	get_device(dev);
+	if (!phys_dev->driver && phys_drv->probe) {
+		error = phys_drv->probe(phys_dev);
+		if (error >= 0)
+			phys_dev->driver = phys_drv;
+	}
+	if (error < 0)
+		put_device(dev);
+	return error;
+}
+
+static int acpi_phys_device_remove(struct device * dev)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+	struct acpi_phys_driver *phys_drv = phys_dev->driver;
+
+	if (phys_drv) {
+		if (phys_drv->remove)
+			phys_drv->remove(phys_dev);
+		phys_dev->driver = NULL;
+	}
+	put_device(dev);
+	return 0;
+}
+
+static void acpi_phys_device_shutdown(struct device *dev)
+{
+	struct acpi_phys_device *phys_dev = to_acpi_phys_dev(dev);
+
+	if (!phys_dev->driver || !phys_dev->driver->shutdown)
+		return;
+
+	phys_dev->driver->shutdown(phys_dev);
+}
+
+int acpi_phys_driver_register(struct acpi_phys_driver *drv)
+{
+	int error;
+	/*FIXME: add to a list */
+	drv->phys_drv.name = drv->name;
+	drv->phys_drv.bus = &acpi_phys_bus_type;
+	drv->phys_drv.probe = acpi_phys_device_probe;
+	drv->phys_drv.remove = acpi_phys_device_remove;
+	drv->phys_drv.shutdown = acpi_phys_device_shutdown;
+	drv->phys_drv.owner = drv->owner;
+	error = driver_register(&drv->phys_drv);
+	return error;
+}
+EXPORT_SYMBOL(acpi_phys_driver_register);
+
+void acpi_phys_driver_unregister(struct acpi_phys_driver *drv)
+{
+	driver_unregister(&drv->phys_drv);
+}
+EXPORT_SYMBOL(acpi_phys_driver_unregister);
+/*-----------------------------------*/
+
 static void acpi_device_register(struct acpi_device *device,
 				 struct acpi_device *parent)
 {
@@ -1400,8 +1628,9 @@ static int __init acpi_scan_init(void)
 
 	if (result)
 		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);
+	acpi_phys_bus_init();
 
-      Done:
+Done:
 	return_VALUE(result);
 }
 
diff -puN include/acpi/acpi_bus.h~acpi_bus include/acpi/acpi_bus.h
--- linux-2.6.14-rc3/include/acpi/acpi_bus.h~acpi_bus	2005-10-11 14:13:26.000000000 +0800
+++ linux-2.6.14-rc3-root/include/acpi/acpi_bus.h	2005-10-13 10:43:21.000000000 +0800
@@ -28,6 +28,7 @@
 
 #include <linux/kobject.h>
 
+#include <linux/device.h>
 #include <acpi/acpi.h>
 
 #define PREFIX			"ACPI: "
@@ -300,6 +301,38 @@ struct acpi_device {
 
 #define acpi_driver_data(d)	((d)->driver_data)
 
+/* acpi bus type for real ACPI device */
+struct acpi_phys_device {
+	struct list_head node;
+	struct acpi_device *acpi_dev;
+	struct device phys_dev;
+	struct acpi_phys_driver *driver;
+	void *driver_data;
+};
+
+#define acpi_phys_driver_data(d) ((d)->driver_data)
+
+struct acpi_phys_driver {
+//	struct list_head node;
+	char name[80];
+	char class[80];
+	struct module *owner;
+	char *ids;
+	int (*probe)(struct acpi_phys_device *dev);
+	void (*remove) (struct acpi_phys_device *dev);
+	int  (*suspend) (struct acpi_phys_device *dev, pm_message_t state);
+	int  (*resume) (struct acpi_phys_device *dev);
+	void (*shutdown) (struct acpi_phys_device *dev);
+
+	struct device_driver    phys_drv;
+};
+
+#define to_acpi_phys_dev(n) container_of(n, struct acpi_phys_device, phys_dev)
+#define to_acpi_phys_drv(n) container_of(n, struct acpi_phys_driver, phys_drv)
+int acpi_phys_driver_register(struct acpi_phys_driver *drv);
+void acpi_phys_driver_unregister(struct acpi_phys_driver *drv);
+
+
 /*
  * Events
  * ------
@@ -339,7 +372,6 @@ void acpi_remove_dir(struct acpi_device 
 /*
  * Bind physical devices with ACPI devices
  */
-#include <linux/device.h>
 struct acpi_bus_type {
 	struct list_head list;
 	struct bus_type *bus;
_




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Bjorn Helgaas]

On Thursday 13 October 2005 12:37 am, Shaohua Li wrote:
> This is the updated patch for the topic. In this version, I completely
> convert devices in ACPI bus into Linux driver model.

This looks like a nice piece of work.

> Now the device 
> driver for ACPI bus device should be a 'struct acpi_phys_driver'. Old
> style driver (struct acpi_driver) now can't work for ACPI bus devices.

You now have both 'acpi_driver' and 'acpi_phys_driver' (and
'acpi_device' and 'acpi_phys_device').  What exactly is the
distinction?  Is the distinction necessary?  Can you just stuff
the generic device stuff into 'struct acpi_device' itself?

> I tried to convert one ACPI driver (link device driver) to the new
> model. It's quite easy.

I'd like to see the link device driver patch, just to help
me understand all this.

> +struct acpi_phys_driver {
> +//	struct list_head node;
> +	char name[80];
> +	char class[80];

Shouldn't these be "char *" instead of "char []"?

> +	struct module *owner;
> +	char *ids;
> +	int (*probe)(struct acpi_phys_device *dev);
> +	void (*remove) (struct acpi_phys_device *dev);
> +	int  (*suspend) (struct acpi_phys_device *dev, pm_message_t state);
> +	int  (*resume) (struct acpi_phys_device *dev);
> +	void (*shutdown) (struct acpi_phys_device *dev);
> +
> +	struct device_driver    phys_drv;

I think you should name this item "driver", not "phys_drv".


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Shaohua Li]

[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]

On Fri, 2005-10-14 at 04:43 +0800, Bjorn Helgaas wrote:
> On Thursday 13 October 2005 12:37 am, Shaohua Li wrote: 
> > This is the updated patch for the topic. In this version, I
> completely 
> > convert devices in ACPI bus into Linux driver model.
> 
> This looks like a nice piece of work.
> 
> > Now the device  
> > driver for ACPI bus device should be a 'struct acpi_phys_driver'.
> Old 
> > style driver (struct acpi_driver) now can't work for ACPI bus
> devices.
> 
> You now have both 'acpi_driver' and 'acpi_phys_driver' (and 
> 'acpi_device' and 'acpi_phys_device').  What exactly is the 
> distinction?  Is the distinction necessary?  Can you just stuff 
> the generic device stuff into 'struct acpi_device' itself?
Devices in ACPI bus use acpi_phys_driver, other devices use acpi_driver.
Converting acpi_device use the generic device stuff might be difficult,
since many devices in DSDT aren't ACPI bus devices. The distinction is
to do smooth change. That is non ACPI bus devices still can register old
style driver. On the future, we might remove the old style driver model.
Adam suggested non-acpi bus device directly use ACPI services instead of
register a 'acpi_driver', which might be worthy tring.

> 
> > I tried to convert one ACPI driver (link device driver) to the new 
> > model. It's quite easy.
> 
> I'd like to see the link device driver patch, just to help 
> me understand all this.
Ok, please see attached patch.

> > +struct acpi_phys_driver { 
> > +//   struct list_head node; 
> > +     char name[80]; 
> > +     char class[80];
> 
> Shouldn't these be "char *" instead of "char []"?
I'll do code cleanup, if people agree the basic idea.
> 
> > +     struct module *owner; 
> > +     char *ids; 
> > +     int (*probe)(struct acpi_phys_device *dev); 
> > +     void (*remove) (struct acpi_phys_device *dev); 
> > +     int  (*suspend) (struct acpi_phys_device *dev, pm_message_t
> state); 
> > +     int  (*resume) (struct acpi_phys_device *dev); 
> > +     void (*shutdown) (struct acpi_phys_device *dev); 
> > + 
> > +     struct device_driver    phys_drv;
> 
> I think you should name this item "driver", not "phys_drv".
Ditto.

Thanks,
Shaohua

[-- Attachment #2: pci_link_convert.patch --]
[-- Type: text/x-patch, Size: 5635 bytes --]



---

 linux-2.6.14-rc3-root/drivers/acpi/osl.c      |    6 -
 linux-2.6.14-rc3-root/drivers/acpi/pci_link.c |  106 +++++++-------------------
 2 files changed, 33 insertions(+), 79 deletions(-)

diff -puN drivers/acpi/osl.c~pci_link_convert drivers/acpi/osl.c
--- linux-2.6.14-rc3/drivers/acpi/osl.c~pci_link_convert	2005-10-12 16:46:57.000000000 +0800
+++ linux-2.6.14-rc3-root/drivers/acpi/osl.c	2005-10-12 16:46:57.000000000 +0800
@@ -136,13 +136,9 @@ void acpi_os_vprintf(const char *fmt, va
 #endif
 }
 
-extern int acpi_in_resume;
 void *acpi_os_allocate(acpi_size size)
 {
-	if (acpi_in_resume)
-		return kmalloc(size, GFP_ATOMIC);
-	else
-		return kmalloc(size, GFP_KERNEL);
+	return kmalloc(size, GFP_KERNEL);
 }
 
 void acpi_os_free(void *ptr)
diff -puN drivers/acpi/pci_link.c~pci_link_convert drivers/acpi/pci_link.c
--- linux-2.6.14-rc3/drivers/acpi/pci_link.c~pci_link_convert	2005-10-12 16:46:57.000000000 +0800
+++ linux-2.6.14-rc3-root/drivers/acpi/pci_link.c	2005-10-12 16:46:57.000000000 +0800
@@ -51,18 +51,6 @@ ACPI_MODULE_NAME("pci_link")
 #define ACPI_PCI_LINK_FILE_INFO		"info"
 #define ACPI_PCI_LINK_FILE_STATUS	"state"
 #define ACPI_PCI_LINK_MAX_POSSIBLE 16
-static int acpi_pci_link_add(struct acpi_device *device);
-static int acpi_pci_link_remove(struct acpi_device *device, int type);
-
-static struct acpi_driver acpi_pci_link_driver = {
-	.name = ACPI_PCI_LINK_DRIVER_NAME,
-	.class = ACPI_PCI_LINK_CLASS,
-	.ids = ACPI_PCI_LINK_HID,
-	.ops = {
-		.add = acpi_pci_link_add,
-		.remove = acpi_pci_link_remove,
-		},
-};
 
 /*
  * If a link is initialized, we never change its active and initialized
@@ -722,14 +710,14 @@ int acpi_pci_link_free_irq(acpi_handle h
                                  Driver Interface
    -------------------------------------------------------------------------- */
 
-static int acpi_pci_link_add(struct acpi_device *device)
+static int __acpi_pci_link_add(struct acpi_device *device)
 {
 	int result = 0;
 	struct acpi_pci_link *link = NULL;
 	int i = 0;
 	int found = 0;
 
-	ACPI_FUNCTION_TRACE("acpi_pci_link_add");
+	ACPI_FUNCTION_TRACE("__acpi_pci_link_add");
 
 	if (!device)
 		return_VALUE(-EINVAL);
@@ -788,47 +776,11 @@ static int acpi_pci_link_add(struct acpi
 	return_VALUE(result);
 }
 
-static int acpi_pci_link_resume(struct acpi_pci_link *link)
+static int __acpi_pci_link_remove(struct acpi_device *device, int type)
 {
-	ACPI_FUNCTION_TRACE("acpi_pci_link_resume");
-
-	if (link->refcnt && link->irq.active && link->irq.initialized)
-		return_VALUE(acpi_pci_link_set(link, link->irq.active));
-	else
-		return_VALUE(0);
-}
-
-/*
- * FIXME: this is a workaround to avoid nasty warning.  It will be removed
- * after every device calls pci_disable_device in .resume.
- */
-int acpi_in_resume;
-static int irqrouter_resume(struct sys_device *dev)
-{
-	struct list_head *node = NULL;
 	struct acpi_pci_link *link = NULL;
 
-	ACPI_FUNCTION_TRACE("irqrouter_resume");
-
-	acpi_in_resume = 1;
-	list_for_each(node, &acpi_link.entries) {
-		link = list_entry(node, struct acpi_pci_link, node);
-		if (!link) {
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
-					  "Invalid link context\n"));
-			continue;
-		}
-		acpi_pci_link_resume(link);
-	}
-	acpi_in_resume = 0;
-	return_VALUE(0);
-}
-
-static int acpi_pci_link_remove(struct acpi_device *device, int type)
-{
-	struct acpi_pci_link *link = NULL;
-
-	ACPI_FUNCTION_TRACE("acpi_pci_link_remove");
+	ACPI_FUNCTION_TRACE("__acpi_pci_link_remove");
 
 	if (!device || !acpi_driver_data(device))
 		return_VALUE(-EINVAL);
@@ -932,34 +884,40 @@ int __init acpi_irq_balance_set(char *st
 
 __setup("acpi_irq_balance", acpi_irq_balance_set);
 
-/* FIXME: we will remove this interface after all drivers call pci_disable_device */
-static struct sysdev_class irqrouter_sysdev_class = {
-	set_kset_name("irqrouter"),
-	.resume = irqrouter_resume,
-};
-
-static struct sys_device device_irqrouter = {
-	.id = 0,
-	.cls = &irqrouter_sysdev_class,
-};
-
-static int __init irqrouter_init_sysfs(void)
+static int acpi_pci_link_probe(struct acpi_phys_device *dev)
 {
-	int error;
+	struct acpi_device *acpi_dev = dev->acpi_dev;
+	return __acpi_pci_link_add(acpi_dev);
+}
 
-	ACPI_FUNCTION_TRACE("irqrouter_init_sysfs");
+static void acpi_pci_link_remove(struct acpi_phys_device *dev)
+{
+	struct acpi_device *acpi_dev = dev->acpi_dev;
+	__acpi_pci_link_remove(acpi_dev, 0);
+}
 
-	if (acpi_disabled || acpi_noirq)
-		return_VALUE(0);
+static int acpi_pci_link_resume(struct acpi_phys_device *dev)
+{
+	struct acpi_device *acpi_dev = dev->acpi_dev;
+	struct acpi_pci_link *link = NULL;
 
-	error = sysdev_class_register(&irqrouter_sysdev_class);
-	if (!error)
-		error = sysdev_register(&device_irqrouter);
+	if (!acpi_driver_data(acpi_dev))
+		return -EINVAL;
+	link = (struct acpi_pci_link *)acpi_driver_data(acpi_dev);
 
-	return_VALUE(error);
+	if (link->refcnt && link->irq.active && link->irq.initialized)
+		return acpi_pci_link_set(link, link->irq.active);
+	return 0;
 }
 
-device_initcall(irqrouter_init_sysfs);
+static struct acpi_phys_driver acpi_pci_phys_link_driver = {
+	.name = ACPI_PCI_LINK_DRIVER_NAME,
+	.class = ACPI_PCI_LINK_CLASS,
+	.ids = ACPI_PCI_LINK_HID,
+	.probe = acpi_pci_link_probe,
+	.remove = acpi_pci_link_remove,
+	.resume = acpi_pci_link_resume,
+};
 
 static int __init acpi_pci_link_init(void)
 {
@@ -971,7 +929,7 @@ static int __init acpi_pci_link_init(voi
 	acpi_link.count = 0;
 	INIT_LIST_HEAD(&acpi_link.entries);
 
-	if (acpi_bus_register_driver(&acpi_pci_link_driver) < 0)
+	if (acpi_phys_driver_register(&acpi_pci_phys_link_driver) < 0)
 		return_VALUE(-ENODEV);
 
 	return_VALUE(0);
_

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Bjorn Helgaas]

> On Fri, 2005-10-14 at 04:43 +0800, Bjorn Helgaas wrote:
>> You now have both 'acpi_driver' and 'acpi_phys_driver' (and
>> 'acpi_device' and 'acpi_phys_device').  What exactly is the
>> distinction?  Is the distinction necessary?  Can you just stuff
>> the generic device stuff into 'struct acpi_device' itself?

> Devices in ACPI bus use acpi_phys_driver, other devices use acpi_driver.

I'm sorry to be dense, but this doesn't help me.  What do you
mean by "devices in ACPI bus" as opposed to other ACPI devices?

Let's say I want to write a driver for something in ACPI, like
a fan.  How do I decide whether to use acpi_phys_driver or
acpi_driver?

Do you mean that I can use either one, and that you want to
migrate drivers from acpi_driver to acpi_phys_driver over time?

If that's the case, I think we should end up with acpi_driver,
not acpi_phys_driver, because "phys" doesn't tell me anything
useful.  And there aren't very many ACPI drivers, so I don't
really see the problem with just moving them all at once.




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Bjorn Helgaas]

On Thursday 13 October 2005 12:37 am, Shaohua Li wrote:
> This is the updated patch for the topic. In this version, I completely
> convert devices in ACPI bus into Linux driver model. Now the device
> driver for ACPI bus device should be a 'struct acpi_phys_driver'. Old
> style driver (struct acpi_driver) now can't work for ACPI bus devices.

    - I don't like names that include "acpi_bus" because there
      really is no "bus".  If PCI, which really DOES have a bus,
      doesn't need it, ACPI surely doesn't.

    - Suspend/resume/probe/remove/shutdown are device-oriented
      interfaces and should be named with "device", not "bus".

    - We should not end up with two interfaces to register ACPI
      drivers.  ACPI is confusing enough as it is.  Having both
      acpi_phys_driver_register() and acpi_bus_register_driver()
      will be a disaster.  (Keeping both temporarily for during
      a transition is OK, but one should be explicitly deprecated.)

> +			length += scnprintf(scratch, buffer_size - length,
> +				"COMPTID=%s", cid_list->id[j].value);

This requires a sequence number (COMPTID%d) to support multiple
_CIDs, doesn't it?

> +static struct bus_type acpi_phys_bus_type = {
> +	.name = "acpi",
> +	.match = acpi_phys_bus_match,
> +	.hotplug = acpi_phys_bus_hotplug,

I think this should be just "acpi_hotplug".

> +	.suspend = acpi_phys_bus_suspend,
> +	.resume = acpi_phys_bus_resume,

And these should be "acpi_device_suspend" and "acpi_device_resume".

> +int acpi_phys_driver_register(struct acpi_phys_driver *drv)

The majority of driver registration interfaces use
"register_driver", not "driver_register".  So I think
this should be "acpi_register_driver", and you should
deprecate "acpi_bus_register_driver".  I still don't
see any fundamental reason for having two ways to
register ACPI drivers.

> +{
> +	int error;
> +	/*FIXME: add to a list */
> +	drv->phys_drv.name = drv->name;
> +	drv->phys_drv.bus = &acpi_phys_bus_type;
> +	drv->phys_drv.probe = acpi_phys_device_probe;
> +	drv->phys_drv.remove = acpi_phys_device_remove;
> +	drv->phys_drv.shutdown = acpi_phys_device_shutdown;

I think these should be "acpi_bus_type", "acpi_device_probe",
"acpi_device_remove", and "acpi_device_shutdown".


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

RE: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Yu, Luming]

>+static int __init acpi_phys_bus_init(void)
>+{
>+	bus_register(&acpi_phys_bus_type);
>+
>+	phys_root = kzalloc(sizeof(struct acpi_phys_device), 
>GFP_KERNEL);
>+	if (!phys_root)
>+		return -ENOMEM;
>+
>+	phys_root->acpi_dev = acpi_root;
>+	phys_root->phys_dev.release = acpi_phys_device_release;
>+	sprintf(phys_root->phys_dev.bus_id, "%s", 
>acpi_root->pnp.bus_id);
>+	device_register(&phys_root->phys_dev);
>+
>+	/* enumerate devices */
>+	acpi_bus_register_driver(&dummy_driver);
>+	return 0;
>+}

The structure is in a mess. why not cut bus.c?

I like this:

acpi_bus_init(void)
{
...
bus_register(&acpi_bus_type);
...
driver_register(&acpi_bus_driver);
...
}



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

RE: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Shaohua Li]

On Thu, 2005-10-13 at 15:45 +0800, Yu, Luming wrote:
> >+static int __init acpi_phys_bus_init(void)
> >+{
> >+	bus_register(&acpi_phys_bus_type);
> >+
> >+	phys_root = kzalloc(sizeof(struct acpi_phys_device), 
> >GFP_KERNEL);
> >+	if (!phys_root)
> >+		return -ENOMEM;
> >+
> >+	phys_root->acpi_dev = acpi_root;
> >+	phys_root->phys_dev.release = acpi_phys_device_release;
> >+	sprintf(phys_root->phys_dev.bus_id, "%s", 
> >acpi_root->pnp.bus_id);
> >+	device_register(&phys_root->phys_dev);
> >+
> >+	/* enumerate devices */
> >+	acpi_bus_register_driver(&dummy_driver);
> >+	return 0;
> >+}
> 
> The structure is in a mess. why not cut bus.c?
> 
> I like this:
> 
> acpi_bus_init(void)
> {
> ...
> bus_register(&acpi_bus_type);
> ...
> driver_register(&acpi_bus_driver);
> ...
> }
I'm sorry, but I don't understand what do you mean. Can you clarify it?
Also, please focus on the basic idea of how to convert ACPI driver model
in current stage. It appears people have quite different opinions about
the topic. The code is just to show my idea (a RFC) not for merging, so
please ignore possible errors (Not everybody agrees with introducing
ACPI bus, actually). It's my bad not explain this.

Thanks,
Shaohua



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[mika.penttila-9Aww8k/80nUxHbG02/KK1g]

>+/* this driver is linking acpi device with acpi phys device */
>+static struct acpi_driver dummy_driver = {
>+ .name = "acpi_dummy",
>+ /* List all ids of ACPI phys devices */
>+ .ids =
>+// "PNP0C09," /* EC */
>+// "ACPI0003," /* AC */
>+// "PNP0C0F," /* Link */
>+// "PNP0C0A," /* battery */
>+// "PNP0C0C,PNP0C0E,PNP0C0D,ACPI_FPB,ACPI_FSB," /* button */
>+// "PNP0C0B," /* fan */
>+// "PNP0A03," /* PCI root */
>+// ACPI_POWER_HID"," /* power */
>+// ACPI_PROCESSOR_HID"," /* CPU */
>+// ACPI_THERMAL_HID"," /* thermal */
>+ "",
>+ .ops = {
>+ .add = acpi_phys_bus_add,
>+ .remove = acpi_phys_bus_remove,
>+ },
>+};

Are those ids meant to be commented out in the dummy driver?

--Mika




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Shaohua Li]

On Thu, 2005-10-13 at 11:18 +0300, mika.penttila-9Aww8k/80nUxHbG02/KK1g@public.gmane.org wrote:
> >+/* this driver is linking acpi device with acpi phys device */
> >+static struct acpi_driver dummy_driver = {
> >+ .name = "acpi_dummy",
> >+ /* List all ids of ACPI phys devices */
> >+ .ids =
> >+// "PNP0C09," /* EC */
> >+// "ACPI0003," /* AC */
> >+// "PNP0C0F," /* Link */
> >+// "PNP0C0A," /* battery */
> >+// "PNP0C0C,PNP0C0E,PNP0C0D,ACPI_FPB,ACPI_FSB," /* button */
> >+// "PNP0C0B," /* fan */
> >+// "PNP0A03," /* PCI root */
> >+// ACPI_POWER_HID"," /* power */
> >+// ACPI_PROCESSOR_HID"," /* CPU */
> >+// ACPI_THERMAL_HID"," /* thermal */
> >+ "",
> >+ .ops = {
> >+ .add = acpi_phys_bus_add,
> >+ .remove = acpi_phys_bus_remove,
> >+ },
> >+};
> 
> Are those ids meant to be commented out in the dummy driver?
The patch is just to show my idea not for merging, so there is some
debug info. Please ignore them currently. I'm sorry I didn't mention
this in previous email.

Thanks,
Shaohua



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

RE: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Li, Shaohua]

Hi,
>
>> On Fri, 2005-10-14 at 04:43 +0800, Bjorn Helgaas wrote:
>>> You now have both 'acpi_driver' and 'acpi_phys_driver' (and
>>> 'acpi_device' and 'acpi_phys_device').  What exactly is the
>>> distinction?  Is the distinction necessary?  Can you just stuff
>>> the generic device stuff into 'struct acpi_device' itself?
>
>> Devices in ACPI bus use acpi_phys_driver, other devices use
acpi_driver.
>
>I'm sorry to be dense, but this doesn't help me.  What do you
>mean by "devices in ACPI bus" as opposed to other ACPI devices?
>
>Let's say I want to write a driver for something in ACPI, like
>a fan.  How do I decide whether to use acpi_phys_driver or
>acpi_driver?
>
>Do you mean that I can use either one, and that you want to
>migrate drivers from acpi_driver to acpi_phys_driver over time?
>
In my mind, the devices which don't belong to other physical buses (such
as PCI, PNP) are ACPI bus devices. These devices are:
+//		"PNP0C09," /* EC */
+//		"ACPI0003," /* AC */
+//		"PNP0C0F," /* Link */
+//		"PNP0C0A," /* battery */
+//		"PNP0C0C,PNP0C0E,PNP0C0D,ACPI_FPB,ACPI_FSB," /* button
*/
+//		"PNP0C0B," /* fan */
+//		"PNP0A03," /* PCI root */
+//		ACPI_POWER_HID"," /* power */
+//		ACPI_PROCESSOR_HID"," /* CPU */
+//		ACPI_THERMAL_HID"," /* thermal */
The devices only can use acpi_phys_driver. Other devices still use
acpi_driver.

>If that's the case, I think we should end up with acpi_driver,
>not acpi_phys_driver, because "phys" doesn't tell me anything
>useful.  And there aren't very many ACPI drivers, so I don't
>really see the problem with just moving them all at once.
I agree we should just have acpi_driver. Non ACPI bus devices should
directly use ACPI services instead of registering an acpi_driver. The
issue is there are some ACPI drivers whose devices are hard to catalog,
such as video, hotkey. Anyway, the change should be slow.
The other issue is some ACPI drivers such as pci_root.c processor.c now
support hotplug. They require two call backs (.add, .start). The
callbacks are called in different sequence at the hotplug time and boot
time. Linux driver model provides .probe callback, so it's hard to
convert pci_root.c and processor.c.
For the acpi_device/acpi_phys_device, do you think we should just have
acpi_device as well?

Thanks,
Shaohua


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Matthew Wilcox]

On Fri, Oct 14, 2005 at 05:35:44PM +0800, Li, Shaohua wrote:
> The other issue is some ACPI drivers such as pci_root.c processor.c now
> support hotplug. They require two call backs (.add, .start). The
> callbacks are called in different sequence at the hotplug time and boot
> time. Linux driver model provides .probe callback, so it's hard to
> convert pci_root.c and processor.c.

Why do we need to have devices separately added and started?  What
prevents us from doing it all in one callback?  I can't quite follow
what the current code's doing, much less figure out why it's doing it.


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Shaohua Li]

On Fri, 2005-10-14 at 06:47 -0600, Matthew Wilcox wrote:
> On Fri, Oct 14, 2005 at 05:35:44PM +0800, Li, Shaohua wrote:
> > The other issue is some ACPI drivers such as pci_root.c processor.c now
> > support hotplug. They require two call backs (.add, .start). The
> > callbacks are called in different sequence at the hotplug time and boot
> > time. Linux driver model provides .probe callback, so it's hard to
> > convert pci_root.c and processor.c.
> 
> Why do we need to have devices separately added and started?  What
> prevents us from doing it all in one callback?  I can't quite follow
> what the current code's doing, much less figure out why it's doing it.
Current processor and pci root hotplug hope to do something between .add
and .start for hotplug, such as allocating resources for hotpluged PCI
devices, after the the devices are scanned (.add) and before binding the
devices to their drivers (.start).  The idea is from Rajesh (CCed in the
list), he can give us more details.

Thanks,
Shaohua



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Rajesh Shah]

On Tue, Oct 18, 2005 at 09:30:34AM +0800, Shaohua Li wrote:
> On Fri, 2005-10-14 at 06:47 -0600, Matthew Wilcox wrote:
> > On Fri, Oct 14, 2005 at 05:35:44PM +0800, Li, Shaohua wrote:
> > > The other issue is some ACPI drivers such as pci_root.c processor.c now
> > > support hotplug. They require two call backs (.add, .start). The
> > > callbacks are called in different sequence at the hotplug time and boot
> > > time. Linux driver model provides .probe callback, so it's hard to
> > > convert pci_root.c and processor.c.
> > 
> > Why do we need to have devices separately added and started?  What
> > prevents us from doing it all in one callback?  I can't quite follow
> > what the current code's doing, much less figure out why it's doing it.
> Current processor and pci root hotplug hope to do something between .add
> and .start for hotplug, such as allocating resources for hotpluged PCI
> devices, after the the devices are scanned (.add) and before binding the
> devices to their drivers (.start).  The idea is from Rajesh (CCed in the
> list), he can give us more details.
> 
The current code supports IO root bridge hotplug by separating pci
scan from pci start. The current flow is that we get an ACPI
interrupt and notify event on a hot-added root bridge that
transfers control to acpiphp. The acpi hotplug driver then adds
the root bridge to the ACPI device list, which invokes the .add
callback in pci_root.c. That code causes the pci scan of the
IO hierarchy hot-added and returns back to the acpiphp hotplug
driver. The hotplug driver then configures the hierarchy and
starts the devices via the acpi .start callback. The boot flow
uses the .add and .start callbacks too, except there's no
configuration step in the middle since we depend on BIOS to do
this for bridges/devices present at boot..

I haven't quite kept up with the discussions around the new
ACPI driver model. However, I have no objections to changes
in this area, as long as we have the ability to keep pci
device scan and start as separate, with a configuration
step in the middle.

Rajesh



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Bjorn Helgaas]

On Friday 14 October 2005 3:35 am, Li, Shaohua wrote:
> >> On Fri, 2005-10-14 at 04:43 +0800, Bjorn Helgaas wrote:
> >I'm sorry to be dense, but this doesn't help me.  What do you
> >mean by "devices in ACPI bus" as opposed to other ACPI devices?
> >...
> In my mind, the devices which don't belong to other physical buses (such
> as PCI, PNP) are ACPI bus devices.

PCI is a physical bus.  It defines wires and electrical things
that happen on those wires.  Neither PNP nor ACPI is a physical
bus.  So every device in the ACPI namespace seems like an "ACPI
bus device".

> These devices are: 
> +//		"PNP0C09," /* EC */
> ...
> +//		ACPI_THERMAL_HID"," /* thermal */
> The devices only can use acpi_phys_driver. Other devices still use
> acpi_driver.

What is the rule by which you determine that the above devices
should use acpi_phys_driver, and all other devices should use
acpi_driver?

> >If that's the case, I think we should end up with acpi_driver,
> >not acpi_phys_driver, because "phys" doesn't tell me anything
> >useful.  And there aren't very many ACPI drivers, so I don't
> >really see the problem with just moving them all at once.
> I agree we should just have acpi_driver. Non ACPI bus devices should
> directly use ACPI services instead of registering an acpi_driver.

Maybe it would help me understand this if you gave an example of
a non ACPI bus device that should just use ACPI services instead
of registering a driver.

> The issue is there are some ACPI drivers whose devices are hard to
> catalog, such as video, hotkey.

I see that the ACPI video driver does not claim by _HID or _CID,
but by a special match function.  But what is the problem?
driver_probe_device() supports an optional match function, so
it looks like ACPI video could be done in that model.

Hotkey does look strange.  When it registers, it has no list of
_HIDs to claim.  But hotkeys cause ACPI notify events, which go
to a specific object in the ACPI namespace.  So it looks like
it has a way via /proc to tell the driver which devices to
claim.

But this doesn't seem so much different than a PCI driver that
can be told dynamically to claim new PCI IDs.  Could a similar
mechanism be used here?

> For the acpi_device/acpi_phys_device, do you think we should just have
> acpi_device as well?

Yes, unless you can explain why we need both.


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [RFC]convert ACPI driver model to Linux driver model - takes 2

[Shaohua Li]

On Fri, 2005-10-14 at 09:54 -0600, Bjorn Helgaas wrote:
> On Friday 14 October 2005 3:35 am, Li, Shaohua wrote:
> > >> On Fri, 2005-10-14 at 04:43 +0800, Bjorn Helgaas wrote:
> > >I'm sorry to be dense, but this doesn't help me.  What do you
> > >mean by "devices in ACPI bus" as opposed to other ACPI devices?
> > >...
> > In my mind, the devices which don't belong to other physical buses (such
> > as PCI, PNP) are ACPI bus devices.
> 
> PCI is a physical bus.  It defines wires and electrical things
> that happen on those wires.  Neither PNP nor ACPI is a physical
> bus.  So every device in the ACPI namespace seems like an "ACPI
> bus device".
We already have an interface for PNP drivers, it might be better to not
include PNP devices into "ACPI bus devices". PNP devices basically are
ISA bus devices. For other devices in ACPI namespace, lets see an
example (the list is from T40 laptop):
        Device (LNKA)
        Device (MEM)
        Device (LID)
        Device (SLPB)
        Device (PCI0)
            Device (LPC)
                Device (MOU)
                Device (FDC)
                    Device (FDD0)
                Device (UART)
                Device (LPT)
                Device (ECP)
                Device (FIR)
                Device (EC)
                    Device (BAT0)
                    Device (BAT1)
                    Device (AC)
                    Device (HKEY)
            Device (AGP)
                Device (VID)
                    Device (LCD0)
                    Device (CRT0)
                    Device (TV0)
                    Device (DVI0)
            Device (PCI1)
                Device (CBS0)
                Device (CBS1)
                Device (DOCK)
                    Device (IDE1)
                        Device (PRIM)
                            Device (MSTR)
                    Device (CBS2)
                    Device (CBS3)
            Device (IDE0)
                Device (PRIM)
                    Device (MSTR)
                Device (SCND)
                    Device (MSTR)
            Device (USB0)
                Device (URTH)
                    Device (UPDK)
            Device (USB7)
                Device (URTH)
                    Device (UPDK)
            Device (AC9M)

The devices have PCI peers such as LPC, AGP, PCI1, IDE0, USB0, USB7,
AC9M should belong to PCI bus. Such devices can directly use ACPI
services or they can register an 'acpi_driver'.
The same reason, PRIM, PRIM.MSTR, SCND, SCND.MSTR should belong to IDE
bus.
URTH, UPDK might belong to USB bus.
Most devices under LPC are PNP devices. But EC should not be, as PNP
interface can't provide sufficient service, such as handle GPE.
Other devices such LID, SLPB are 'ACPI bus devices'. PCI0 is a little
strange. There is physical PCI device 0:0.0 sometimes, sometimes it's
just a fake device. I regard it as 'acpi bus devices' here.

> > These devices are: 
> > +//		"PNP0C09," /* EC */
> > ...
> > +//		ACPI_THERMAL_HID"," /* thermal */
> > The devices only can use acpi_phys_driver. Other devices still use
> > acpi_driver.
> 
> What is the rule by which you determine that the above devices
> should use acpi_phys_driver, and all other devices should use
> acpi_driver?
See above.
> 
> > >If that's the case, I think we should end up with acpi_driver,
> > >not acpi_phys_driver, because "phys" doesn't tell me anything
> > >useful.  And there aren't very many ACPI drivers, so I don't
> > >really see the problem with just moving them all at once.
> > I agree we should just have acpi_driver. Non ACPI bus devices should
> > directly use ACPI services instead of registering an acpi_driver.
> 
> Maybe it would help me understand this if you gave an example of
> a non ACPI bus device that should just use ACPI services instead
> of registering a driver.
An example is a PCI device. It belongs to PCI bus, but it might have a
peer in ACPI namespace. The PCI driver could call some ACPI methods such
as _PSW for wakeup.

> 
> > The issue is there are some ACPI drivers whose devices are hard to
> > catalog, such as video, hotkey.
> 
> I see that the ACPI video driver does not claim by _HID or _CID,
> but by a special match function.  But what is the problem?
> driver_probe_device() supports an optional match function, so
> it looks like ACPI video could be done in that model.
ok. I can add a .match callback in acpi_phys_driver.
> 
> Hotkey does look strange.  When it registers, it has no list of
> _HIDs to claim.  But hotkeys cause ACPI notify events, which go
> to a specific object in the ACPI namespace.  So it looks like
> it has a way via /proc to tell the driver which devices to
> claim.
> 
> But this doesn't seem so much different than a PCI driver that
> can be told dynamically to claim new PCI IDs.  Could a similar
> mechanism be used here?
Might we can use the similar method for video.

But we still have trouble with other devices such as pci_root and
processor. There also have some other devices which register acpi_driver
in IA64, I don't the type of such devices. And Len hopes the change can
be smoothly. 

Thanks,
Shaohua



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl