Messages in this thread | | | Date | Mon, 1 Sep 2014 10:16:07 +0100 | From | Lee Jones <> | Subject | Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct |
| |
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 <rebecca.swee.fun.chang@intel.com> > Tested-by: Chang Rebecca Swee Fun <rebecca.swee.fun.chang@intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |