[ACPI] Re: driverfs support for fans

[Patrick M. <mo...@os...>]

Hi Dominik,

On Tue, 26 Mar 2002, Dominik Brodowski wrote:

> Hi Paul, Hi Pat!
> 
> Attached is some preliminary driverfs support for acpi_fan.c. Both of you 
> probably won't like some aspects of it:

One favor I would ask: could you please include the patch inline, 
especially when submitting it for review. It makes it much easier to 
comment on. 

> x Parent of fan devices is device_sys of Pavel Machek's latest patchset (so 
> you need that to get this patch compiled), when Pat's 
> "register_sys_device"-patch is available using that should be straightforward 
> (see #1 in "TBD").

The patch is appended. I sent Linus email this morning.

> x All fan devices are included in /devices/sys/, and so ACPI-2.0-specific 
> fans for special parts of the systems appear there, too. I suggest that in 
> future fans that are "below" any other device will simply have that device as 
> parent, and fans that are "system-wide" will be devices of the system bus 
> (see #2 in TBD).
> 

--- acpi_bus.h.orig	Tue Mar 26 19:20:05 2002
+++ acpi_bus.h	Tue Mar 26 19:19:24 2002
@@ -274,6 +274,7 @@
 	void			*driver_data;
 #ifdef CONFIG_LDM
 	struct device		dev;
+	struct device_driver	device_driver;
 #endif
 };

***
This isn't necessary. There is an entry in struct device for this. 
 

--- acpi_fan.c.orig	Tue Mar 26 17:08:44 2002
+++ acpi_fan.c	Tue Mar 26 19:48:53 2002
@@ -54,8 +54,15 @@
 				},
 };
 
+#ifdef CONFIG_LDM
+static unsigned char acpi_fan_saved_state = 0;
+#endif
+
 struct acpi_fan {
 	acpi_handle		handle;
+#ifdef CONFIG_LDM
+	struct acpi_device     *parent;
+#endif
 };

***
Boy, these #ifdefs are getting out of hand.... ;)
 
 
+int acpi_fan_ldm_init (struct acpi_device *device, int *state) 
+{
+	int                    result = 0;
+	
+	/* driver wrapping */
+
+//	device->device_driver.probe = acpi_fan_ldm_probe;
+//	device->device_driver.remove = acpi_fan_ldm_remove;
+	device->device_driver.suspend = acpi_fan_ldm_suspend;
+	device->device_driver.resume = acpi_fan_ldm_resume;

*** 
You should do something like:

struct device_driver fan_device_driver = {
	probe:		acpi_fan_ldm_probe,
	remove:		acpi_fan_ldm_remove,
	suspend:	acpi_fan_ldm_suspend,
	resume:		acpi_fan_ldm_resume.
};

...
	device->dev.driver = &fan_device_driver;

+	strncpy(device->dev.bus_id, acpi_device_bid(device),sizeof(acpi_bus_id));

***
One thing I noticed when booting the latest kernel was the poor selection 
of bus_ids in the ACPI driverfs code. STUDLY CAPS are distasteful and 
hard to read. It is cosmetic, and not such a big deal coming from the 
Windows world. But, it is inconsistent with the rest of the driverfs 
entries, and most Linux filenames in general.

Second of all, most of the names violate the concept of "bus ID" 
completely. The names I see are _device_ IDs, as in type. Very few 
actually are the address on the bus of the device in question. (Which is 
what the field is intended for in this context). Period.

Thirdly, and what Dominik mentioned previously, there are multiple entries 
in the device tree for devices. The most notable are the PCI devices. 
Please, coordinate yourselves with the rest of the system. Please, do it 
once, and do it right (the first time). 

	-pat