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

[Shaohua Li <shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

[-- 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);
_