On Fri, 17 Feb 2012 17:28:23 -0600 (CST), Aaron Sierra wrote:> This driver currently creates resources for use by a forthcoming ICH> chipset GPIO driver. It could be expanded to created the resources for> converting the esb2rom (mtd) and iTCO_wdt (wdt), and potentially more,> drivers to use the mfd model.> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Sorry I have some more comments. You resent the patch series yesterdayfaster than I could review v3.

I think you should set ignore_resource_conflicts here too. Your code isalready checking for ACPI resource conflicts, so there is no point inhaving mfd-core check again. This is not only redundant, this alsomakes the kernel log harder to read as the warnings are printedmultiple times.

Is it really sufficient to disable the resource? I see that you handlethis case properly in the gpio-ich driver, however there's also theplatform subsystem which needs to be considered. The above will causeplatform_device_add_resources (called by mfd_add_device) to register anI/O resource at address 0, size 1. I can see it in /proc/ioports:

This is not clean and could cause a conflict on its own. So I don'tthink this is the right approach. See below for a possible solution.

> + acpi_conflict = true;

Don't you want to jump to pm_done here? There's no point in enablingthe LPC ACPI space if you are never going to access it. Not that itshould really make a difference in practice, I presume that if ACPI isusing the resource, the LPC ACPI space is already enabled...

I don't quite get how this can be non-fatal, given that the gpio-ichdriver's probe function will return -ENODEV in this case. So if thisresource is mandatory, let's make it exactly that. This means thatresource 0 is mandatory and resource 1 is optional. All you have to dothen is:* Don't register the mfd device at all if GPIO resource is unavailable.* If ACPI resource is unavailable, set num_resources to 1.

That should work, and this solves the ghost resource problem Imentioned earlier.

Yet a completely different approach would be to delegate the ACPIresource conflict checking to the gpio-ich subdriver. I suspect we mayend up doing that anyway, as requesting the whole I/O range when weonly need subsets thereof is likely to cause ACPI resource conflicts ontoo many systems for the driver to be useful in practice. This is abigger change though and I would understand if you are reluctant to doit as this point of the review cycle. This can be changed later and Ivolunteer to take care of it (I need it for my Asus Z8NA-D6 board.)

I'm not sure if it really makes sense to report this. ACPI resourceconflicts are already reported quite loudly by the acpi core. Andpassing acpi_enforce_resources=lax blindly isn't quite recommended, soI'm not sure if we really want to mention it here, it might do moreharm than help.