On Monday 26 October 2009 07:02:55 pm ykzhao wrote: > In fact you mention two issues about the two patches: > 1: Load a PNP driver for it to register the IPMI system interface. > This is about the first patch. > 2. coding style( for example: comments, the definition about some > variables). > > For the first issue: Before I start the first patch, I consider using > the PNP device driver. But I find that it is so complex because of the > following two points: > 1. One is that we can't register the IPMI system interface if the > boot option of "pnpacpi=off" is added. This will also depend on the PNP > module. This is not a problem. It is perfectly acceptable for the IPMI driver to depend on PNP and PNPACPI in order to claim an ACPI device. If the users boots with "pnpacpi=off", we just won't find an IPMI device. That is the way it works for TPM devices and serial devices described by ACPI, and IPMI should work the same way. > 2. The second is that there exist so many cases about the IPMI > IO/memory resource definition. Maybe there exist both IO/memory resource > definition for one IPMI device. In such case we can't know which should > be selected. At the same time we have similar issues about the interrupt > type. So I decide to parse the IO/memory/interrupt resource > independently. This doesn't make any sense. The fact that an IPMI device might have a variety of IO/memory/IRQ resources is orthogonal to the question of whether you should use pnp_register_driver() or acpi_walk_namespace(). PNPACPI parses the IPMI device resources for every ACPI device, including the IPMI device, before we even know whether there will be a PNP driver for the device. It's much easier to look at the PNP resources and figure out which to use than it is to use acpi_walk_resources() manually. The main point is that ipmi_si_intf.c is a device driver, and it should use the normal driver registration mechanisms. I think it would be simplest and clearest to make a few PNP enhancements so it could use pnp_register_driver(), but even using acpi_bus_register_driver() would be fine. Using acpi_walk_namespace() to do everything by hand is just completely wrong. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
