Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct

Lee Jones
2014-09-01T09:16:15Z

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