On Fri, 22 Aug 2014, Andy Shevchenko wrote:

> Introduce additional struct to hold chipset info. This 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.
> 
> Signed-off-by: Chang Rebecca Swee Fun <[email protected]>
> Tested-by: Chang Rebecca Swee Fun <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
>  drivers/mfd/lpc_sch.c | 65 
> ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index 0f01ef0..c4eb359 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -40,10 +40,39 @@
>  #define WDTBASE              0x84
>  #define WDT_IO_SIZE  64
>  
> +enum sch_chipsets {
> +     LPC_SCH = 0,            /* Intel Poulsbo SCH */
> +     LPC_ITC,                /* Intel Tunnel Creek */
> +     LPC_CENTERTON,          /* Intel Centerton */
> +};
> +
> +struct lpc_sch_info {
> +     unsigned int io_size_smbus;
> +     unsigned int io_size_gpio;
> +     unsigned int io_size_wdt;
> +};
> +
> +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);
> @@ -54,6 +83,9 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, 
> const char *name,
>       unsigned int base_addr_cfg;
>       unsigned short base_addr;
>  
> +     if (size == 0)
> +             return -EINVAL;
> +
>       pci_read_config_dword(pdev, where, &base_addr_cfg);
>       base_addr = 0;
>       if (!(base_addr_cfg & (1 << 31)))
> @@ -104,32 +136,27 @@ static int lpc_sch_probe(struct pci_dev *dev,
>                        const struct pci_device_id *id)
>  {
>       struct mfd_cell lpc_sch_cells[3];
> -     int size, cells = 0;
> +     struct lpc_sch_info *info = &sch_chipset_info[id->driver_data];
> +     int cells = 0;
>       int ret;
>  
> -     ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> +     ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
> +                                 info->io_size_smbus,
>                                   id->device, &lpc_sch_cells[cells]);
>       if (!ret)
>               cells++;
>  
> -     if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
> -             size = GPIO_IO_SIZE_CENTERTON;
> -     else
> -             size = GPIO_IO_SIZE;
> -
> -     ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio", size,
> +     ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
> +                                 info->io_size_gpio,
>                                   id->device, &lpc_sch_cells[cells]);
>       if (!ret)
>               cells++;
>  
> -     if (id->device == PCI_DEVICE_ID_INTEL_ITC_LPC
> -         || id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB) {
> -             ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> -                                         WDT_IO_SIZE,
> -                                         id->device, &lpc_sch_cells[cells]);
> -             if (!ret)
> -                     cells++;
> -     }
> +     ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> +                                 info->io_size_wdt,
> +                                 id->device, &lpc_sch_cells[cells]);
> +     if (!ret)
> +             cells++;

The first patch would look a great deal cleaner if it had these
changes in too.  Unless you have a really good reason not to, please
consider squashing them.

>       if (cells == 0) {
>               dev_err(&dev->dev, "All decode registers disabled.\n");

-- 
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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to