Messages in this thread | | | Date | Tue, 21 Feb 2012 16:21:12 -0600 (CST) | From | Aaron Sierra <> | Subject | Re: [PATCH 1/3 v4] mfd: Add LPC driver for Intel ICH chipsets |
| |
> On Mon, 20 Feb 2012 14:36:27 -0600 (CST), Aaron Sierra wrote: > > > > + } > > > > + > > > > + /* Enable LPC ACPI space */ > > > > + pci_read_config_byte(dev, ACPICTRL, ®_save); > > > > + pci_write_config_byte(dev, ACPICTRL, reg_save | 0x10); > > > > + lpc_ich_acpi_save = reg_save; > > > > + > > > > +pm_done: > > > > + /* Setup GPIO base register */ > > > > + pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg); > > > > + base_addr = base_addr_cfg & 0x0000ff80; > > > > + if (!base_addr) { > > > > + dev_err(&dev->dev, "I/O space for GPIO uninitialized\n"); > > > > + /* GPIO in power-management space may still be available */ > > > > + goto gpio_reg; > > > > + } > > > > + > > > > + gpio_ich_res[ICH_RES_GPIO].start = base_addr; > > > > + gpio_ich_res[ICH_RES_GPIO].end = base_addr + GPIOBASE_IO_SIZE > > > > - > > > > 1; > > > > + ret = > > > > acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPIO]); > > > > + if (ret) { > > > > + /* this isn't necessarily fatal for the GPIO */ > > > > + gpio_ich_res[ICH_RES_GPIO].start = 0; > > > > + gpio_ich_res[ICH_RES_GPIO].end = 0; > > > > > > I don't quite get how this can be non-fatal, given that the > > > gpio-ich driver's probe function will return -ENODEV in this > > > case. So if this resource is mandatory, let's make it exactly > > > that. > > > > The necessity for the ICH_RES_GPIO resource to exist is an issue I > > thought better left to the gpio-ich driver. The way that driver is > > currently written it is mandatory, but it doesn't *have* to be > > written that way. For chipsets that have GPE0 and GPIO space, only > > one needs to be present to have some usable GPIO. > > Ah, OK, I had misunderstood this, I thought GPIO was always > mandatory. > > We're only talking about the ICH6 and 3100, right? I find it > questionable that you even attempt to request and enable the GPE0 > space on all other chips then. This could cause error messages > that are not relevant at all.
You'll see in the V5 patch that I followed your suggestion and now treat the GPIO space as mandatory. The local version I have prepared for V6 only looks for GPE0 space for ICH6 and i3100 devices.
> > Otherwise, the only optimization (fix) I see is that > > ACPIBASE_GPE0_BASE should be 0x28, not 0x20 and gpe0_sts_ofs > > in gpio-ich should be removed. Currently, that portion of > > gpio-ich appears to be broken. > > If you only need the I/O port at offset 0x28, then indeed it might > make sense to only request that one to limit the risk of resource > conflicts.
I addressed this with V5.
> > > > + lpc_ich_finalize_cell(&lpc_ich_cells[LPC_GPIO], id); > > > > + ret = mfd_add_devices(&dev->dev, 0, &lpc_ich_cells[LPC_GPIO], > > > > + 1, NULL, 0); > > > > + if (!ret) > > > > + cell_added = true; > > > > + > > > > + if (acpi_conflict) > > > > + dev_info(&dev->dev, "ACPI resource conflicts found; " > > > > + "consider using acpi_enforce_resources=lax?\n"); > > > > > > I'm not sure if it really makes sense to report this. ACPI > > > resource > > > conflicts are already reported quite loudly by the acpi core. And > > > passing acpi_enforce_resources=lax blindly isn't quite > > > recommended, > > > so I'm not sure if we really want to mention it here, it might do > > > more > > > harm than help. > > > > So the question mark doesn't imply strongly enough that it's not an > > action that should definitely be taken. Would you prefer a warning > > summarizing which drivers are affected by the detected resource > > conflicts or no additional warning at all? > > I wouldn't put any additional warning at all. But maybe that's just > me.
In V5, I added warnings indicating which drivers are affected by conflicts, since it was trivial with the way that I compartmentalized gpio and watchdog mfd cell creation in that version.
-Aaron S.
| |