From: Patrick M. <mo...@os...> - 2002-03-26 20:38:21
|
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 |