lkml.org 
[lkml]   [2012]   [Feb]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/3 v4] mfd: Add LPC driver for Intel ICH chipsets
    Hi Aaron,

    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 yesterday
    faster than I could review v3.

    > ---
    > drivers/mfd/Kconfig | 9 +
    > drivers/mfd/Makefile | 1 +
    > drivers/mfd/lpc_ich.c | 525 +++++++++++++++++++++++++++++++++++++++++++
    > include/linux/mfd/lpc_ich.h | 32 +++
    > 4 files changed, 567 insertions(+), 0 deletions(-)
    > create mode 100644 drivers/mfd/lpc_ich.c
    > create mode 100644 include/linux/mfd/lpc_ich.h
    >
    > (...)
    > +static struct mfd_cell lpc_ich_cells[] = {
    > + [LPC_GPIO] = {
    > + .name = "gpio_ich",
    > + .num_resources = ARRAY_SIZE(gpio_ich_res),
    > + .resources = gpio_ich_res,

    I think you should set ignore_resource_conflicts here too. Your code is
    already checking for ACPI resource conflicts, so there is no point in
    having mfd-core check again. This is not only redundant, this also
    makes the kernel log harder to read as the warnings are printed
    multiple times.

    > (...)
    > +static void lpc_ich_restore_config_space(struct pci_dev *dev)
    > +{
    > + if (lpc_ich_acpi_save >= 0)
    > + pci_write_config_byte(dev, ACPICTRL, lpc_ich_acpi_save);
    > + if (lpc_ich_gpio_save >= 0)
    > + pci_write_config_byte(dev, GPIOCTRL, lpc_ich_gpio_save);
    > +
    > + lpc_ich_acpi_save = -1;
    > + lpc_ich_gpio_save = -1;
    > +}

    A minor optimization is possible here, by including the "save = -1"
    statements inside their respective conditional.

    > +
    > +static void lpc_ich_finalize_cell(struct mfd_cell *cell,
    > + const struct pci_device_id *id)

    Called from a __devinit function so could be made __devinit too.

    > +{
    > + cell->id = id->driver_data;
    > + cell->platform_data = &lpc_chipset_info[id->driver_data];
    > + cell->pdata_size = sizeof(struct lpc_ich_info);
    > +}
    > +
    > +static int __devinit lpc_ich_probe(struct pci_dev *dev,
    > + const struct pci_device_id *id)
    > +{
    > + u32 base_addr_cfg;
    > + u32 base_addr;
    > + u8 reg_save;
    > + int ret;
    > + bool cell_added = false;
    > + bool acpi_conflict = false;
    > +
    > + /* Setup power management base register */
    > + pci_read_config_dword(dev, ACPIBASE, &base_addr_cfg);
    > + base_addr = base_addr_cfg & 0x0000ff80;
    > + if (!base_addr) {
    > + dev_err(&dev->dev, "I/O space for ACPI uninitialized\n");
    > + goto pm_done;
    > + }
    > +
    > + gpio_ich_res[ICH_RES_GPE0].start = base_addr + ACPIBASE_GPE_OFF;
    > + gpio_ich_res[ICH_RES_GPE0].end = base_addr + ACPIBASE_GPE_END;
    > + ret = acpi_check_resource_conflict(&gpio_ich_res[ICH_RES_GPE0]);
    > + if (ret) {
    > + /* this isn't necessarily fatal for the GPIO */
    > + gpio_ich_res[ICH_RES_GPE0].start = 0;
    > + gpio_ich_res[ICH_RES_GPE0].end = 0;

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

    0000-0cf7 : PCI Bus 0000:00
    0000-001f : dma1
    0000-0000 : gpio_ich.32 <-- HERE
    0020-0021 : pic1

    This is not clean and could cause a conflict on its own. So I don't
    think 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 enabling
    the LPC ACPI space if you are never going to access it. Not that it
    should really make a difference in practice, I presume that if ACPI is
    using the resource, the LPC ACPI space is already enabled...

    > + }
    > +
    > + /* Enable LPC ACPI space */
    > + pci_read_config_byte(dev, ACPICTRL, &reg_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. This means that
    resource 0 is mandatory and resource 1 is optional. All you have to do
    then 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 I
    mentioned earlier.

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

    > + acpi_conflict = true;
    > + goto gpio_reg;
    > + }
    > +
    > + /* Enable LPC GPIO space */
    > + pci_read_config_byte(dev, GPIOCTRL, &reg_save);
    > + pci_write_config_byte(dev, GPIOCTRL, reg_save | 0x10);
    > + lpc_ich_gpio_save = reg_save;
    > +
    > +gpio_reg:

    Shouldn't this label be named gpio_done for consistency? Probably a
    moot point given my remark above anyway.

    > + 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.

    > +
    > + /*
    > + * We only care if at least one or none of the cells registered
    > + * successfully.
    > + */
    > + if (!cell_added) {
    > + lpc_ich_restore_config_space(dev);
    > + return -ENODEV;
    > + }
    > +
    > + return 0;
    > +}

    --
    Jean Delvare


    \
     
     \ /
      Last update: 2012-02-18 18:35    [W:0.033 / U:0.212 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site