| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140902115417.GG17117@lee--X1>
Date: Tue, 2 Sep 2014 12:54:17 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
Chang Rebecca Swee Fun <rebecca.swee.fun.chang@...el.com>
Subject: Re: [PATCH v2 1/4] mfd: lpc_sch: reduce duplicate code and improve
manageability
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 <rebecca.swee.fun.chang@...el.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists