From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753655AbaIBLyZ (ORCPT ); Tue, 2 Sep 2014 07:54:25 -0400 Received: from mail-ie0-f182.google.com ([209.85.223.182]:55091 "EHLO mail-ie0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750722AbaIBLyX (ORCPT ); Tue, 2 Sep 2014 07:54:23 -0400 Date: Tue, 2 Sep 2014 12:54:17 +0100 From: Lee Jones To: Andy Shevchenko Cc: Bjorn Helgaas , linux-kernel@vger.kernel.org, Chang Rebecca Swee Fun Subject: Re: [PATCH v2 1/4] mfd: lpc_sch: reduce duplicate code and improve manageability Message-ID: <20140902115417.GG17117@lee--X1> References: <1409654722-6570-1-git-send-email-andriy.shevchenko@linux.intel.com> <1409654722-6570-2-git-send-email-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1409654722-6570-2-git-send-email-andriy.shevchenko@linux.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 02 Sep 2014, Andy Shevchenko wrote: > This patch refactors the driver to use helper functions instead of > copy'n'pasted pieces of code. > > It also introduces an additional struct to hold a chipset info. The chipset > info will be used to store features that are supported by specific processor or > chipset. LPC_SCH supports SMBUS, GPIO and WDT features. As this code base might > expand further to support more processors, this implementation will help to > keep code base clean and manageable. > > The patch is partially based on the work done by Chang Rebecca Swee Fun. > > Tested-by: Chang Rebecca Swee Fun > Signed-off-by: Andy Shevchenko > --- > drivers/mfd/lpc_sch.c | 181 +++++++++++++++++++++++++++----------------------- > 1 file changed, 99 insertions(+), 82 deletions(-) Applied, thanks. > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c > index 4ee7550..bde070a 100644 > --- a/drivers/mfd/lpc_sch.c > +++ b/drivers/mfd/lpc_sch.c > @@ -40,120 +40,137 @@ > #define WDTBASE 0x84 > #define WDT_IO_SIZE 64 > > -static struct resource smbus_sch_resource = { > - .flags = IORESOURCE_IO, > +enum sch_chipsets { > + LPC_SCH = 0, /* Intel Poulsbo SCH */ > + LPC_ITC, /* Intel Tunnel Creek */ > + LPC_CENTERTON, /* Intel Centerton */ > }; > > -static struct resource gpio_sch_resource = { > - .flags = IORESOURCE_IO, > +struct lpc_sch_info { > + unsigned int io_size_smbus; > + unsigned int io_size_gpio; > + unsigned int io_size_wdt; > }; > > -static struct resource wdt_sch_resource = { > - .flags = IORESOURCE_IO, > -}; > - > -static struct mfd_cell lpc_sch_cells[3]; > - > -static struct mfd_cell isch_smbus_cell = { > - .name = "isch_smbus", > - .num_resources = 1, > - .resources = &smbus_sch_resource, > - .ignore_resource_conflicts = true, > -}; > - > -static struct mfd_cell sch_gpio_cell = { > - .name = "sch_gpio", > - .num_resources = 1, > - .resources = &gpio_sch_resource, > - .ignore_resource_conflicts = true, > -}; > - > -static struct mfd_cell wdt_sch_cell = { > - .name = "ie6xx_wdt", > - .num_resources = 1, > - .resources = &wdt_sch_resource, > - .ignore_resource_conflicts = true, > +static struct lpc_sch_info sch_chipset_info[] = { > + [LPC_SCH] = { > + .io_size_smbus = SMBUS_IO_SIZE, > + .io_size_gpio = GPIO_IO_SIZE, > + }, > + [LPC_ITC] = { > + .io_size_smbus = SMBUS_IO_SIZE, > + .io_size_gpio = GPIO_IO_SIZE, > + .io_size_wdt = WDT_IO_SIZE, > + }, > + [LPC_CENTERTON] = { > + .io_size_smbus = SMBUS_IO_SIZE, > + .io_size_gpio = GPIO_IO_SIZE_CENTERTON, > + .io_size_wdt = WDT_IO_SIZE, > + }, > }; > > static const struct pci_device_id lpc_sch_ids[] = { > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC) }, > - { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB) }, > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_SCH_LPC), LPC_SCH }, > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ITC_LPC), LPC_ITC }, > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_CENTERTON_ILB), LPC_CENTERTON }, > { 0, } > }; > MODULE_DEVICE_TABLE(pci, lpc_sch_ids); > > -static int lpc_sch_probe(struct pci_dev *dev, > - const struct pci_device_id *id) > +#define LPC_NO_RESOURCE 1 > +#define LPC_SKIP_RESOURCE 2 > + > +static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name, > + struct resource *res, int size) > { > unsigned int base_addr_cfg; > unsigned short base_addr; > - int i, cells = 0; > - int ret; > > - pci_read_config_dword(dev, SMBASE, &base_addr_cfg); > + if (size == 0) > + return LPC_NO_RESOURCE; > + > + pci_read_config_dword(pdev, where, &base_addr_cfg); > base_addr = 0; > if (!(base_addr_cfg & (1 << 31))) > - dev_warn(&dev->dev, "Decode of the SMBus I/O range disabled\n"); > + dev_warn(&pdev->dev, "Decode of the %s I/O range disabled\n", > + name); > else > base_addr = (unsigned short)base_addr_cfg; > > if (base_addr == 0) { > - dev_warn(&dev->dev, "I/O space for SMBus uninitialized\n"); > - } else { > - lpc_sch_cells[cells++] = isch_smbus_cell; > - smbus_sch_resource.start = base_addr; > - smbus_sch_resource.end = base_addr + SMBUS_IO_SIZE - 1; > + dev_warn(&pdev->dev, "I/O space for %s uninitialized\n", name); > + return LPC_SKIP_RESOURCE; > } > > - pci_read_config_dword(dev, GPIOBASE, &base_addr_cfg); > - base_addr = 0; > - if (!(base_addr_cfg & (1 << 31))) > - dev_warn(&dev->dev, "Decode of the GPIO I/O range disabled\n"); > - else > - base_addr = (unsigned short)base_addr_cfg; > + res->start = base_addr; > + res->end = base_addr + size - 1; > + res->flags = IORESOURCE_IO; > > - if (base_addr == 0) { > - dev_warn(&dev->dev, "I/O space for GPIO uninitialized\n"); > - } else { > - lpc_sch_cells[cells++] = sch_gpio_cell; > - gpio_sch_resource.start = base_addr; > - if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB) > - gpio_sch_resource.end = base_addr + GPIO_IO_SIZE_CENTERTON - 1; > - else > - gpio_sch_resource.end = base_addr + GPIO_IO_SIZE - 1; > - } > + return 0; > +} > > - if (id->device == PCI_DEVICE_ID_INTEL_ITC_LPC > - || id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB) { > - pci_read_config_dword(dev, WDTBASE, &base_addr_cfg); > - base_addr = 0; > - if (!(base_addr_cfg & (1 << 31))) > - dev_warn(&dev->dev, "Decode of the WDT I/O range disabled\n"); > - else > - base_addr = (unsigned short)base_addr_cfg; > - if (base_addr == 0) > - dev_warn(&dev->dev, "I/O space for WDT uninitialized\n"); > - else { > - lpc_sch_cells[cells++] = wdt_sch_cell; > - wdt_sch_resource.start = base_addr; > - wdt_sch_resource.end = base_addr + WDT_IO_SIZE - 1; > - } > - } > +static int lpc_sch_populate_cell(struct pci_dev *pdev, int where, > + const char *name, int size, int id, > + struct mfd_cell *cell) > +{ > + struct resource *res; > + int ret; > > - if (WARN_ON(cells > ARRAY_SIZE(lpc_sch_cells))) { > - dev_err(&dev->dev, "Cell count exceeds array size"); > - return -ENODEV; > - } > + res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL); > + if (!res) > + return -ENOMEM; > + > + ret = lpc_sch_get_io(pdev, where, name, res, size); > + if (ret) > + return ret; > + > + memset(cell, 0, sizeof(*cell)); > + > + cell->name = name; > + cell->resources = res; > + cell->num_resources = 1; > + cell->ignore_resource_conflicts = true; > + cell->id = id; > + > + return 0; > +} > + > +static int lpc_sch_probe(struct pci_dev *dev, const struct pci_device_id *id) > +{ > + struct mfd_cell lpc_sch_cells[3]; > + struct lpc_sch_info *info = &sch_chipset_info[id->driver_data]; > + unsigned int cells = 0; > + int ret; > + > + ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", > + info->io_size_smbus, > + id->device, &lpc_sch_cells[cells]); > + if (ret < 0) > + return ret; > + if (ret == 0) > + cells++; > + > + ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio", > + info->io_size_gpio, > + id->device, &lpc_sch_cells[cells]); > + if (ret < 0) > + return ret; > + if (ret == 0) > + cells++; > + > + ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt", > + info->io_size_wdt, > + id->device, &lpc_sch_cells[cells]); > + if (ret < 0) > + return ret; > + if (ret == 0) > + cells++; > > if (cells == 0) { > dev_err(&dev->dev, "All decode registers disabled.\n"); > return -ENODEV; > } > > - for (i = 0; i < cells; i++) > - lpc_sch_cells[i].id = id->device; > - > ret = mfd_add_devices(&dev->dev, 0, lpc_sch_cells, cells, NULL, 0, NULL); > if (ret) > mfd_remove_devices(&dev->dev); -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog