* [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code
2014-08-22 10:58 [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
@ 2014-08-22 10:58 ` Andy Shevchenko
2014-09-01 9:13 ` Lee Jones
2014-08-22 10:58 ` [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct Andy Shevchenko
` (4 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-08-22 10:58 UTC (permalink / raw)
To: Lee Jones, Bjorn Helgaas, linux-kernel, Samuel Ortiz,
Chang Rebecca Swee Fun
Cc: Andy Shevchenko
This patch refactors the driver to use helper functions instead of
copy'n'pasted pieces of code.
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 | 146 ++++++++++++++++++++++----------------------------
1 file changed, 64 insertions(+), 82 deletions(-)
diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
index 4ee7550..0f01ef0 100644
--- a/drivers/mfd/lpc_sch.c
+++ b/drivers/mfd/lpc_sch.c
@@ -40,41 +40,6 @@
#define WDTBASE 0x84
#define WDT_IO_SIZE 64
-static struct resource smbus_sch_resource = {
- .flags = IORESOURCE_IO,
-};
-
-static struct resource gpio_sch_resource = {
- .flags = IORESOURCE_IO,
-};
-
-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 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) },
@@ -83,67 +48,87 @@ static const struct pci_device_id lpc_sch_ids[] = {
};
MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
-static int lpc_sch_probe(struct pci_dev *dev,
- const struct pci_device_id *id)
+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);
+ 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 -ENODEV;
}
- 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");
+ res->start = base_addr;
+ res->end = base_addr + size - 1;
+ res->flags = IORESOURCE_IO;
+
+ return 0;
+}
+
+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;
+
+ 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];
+ int size, cells = 0;
+ int ret;
+
+ ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
+ id->device, &lpc_sch_cells[cells]);
+ if (!ret)
+ cells++;
+
+ if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
+ size = GPIO_IO_SIZE_CENTERTON;
else
- base_addr = (unsigned short)base_addr_cfg;
+ size = GPIO_IO_SIZE;
- 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;
- }
+ ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio", size,
+ 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) {
- 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;
- }
- }
-
- if (WARN_ON(cells > ARRAY_SIZE(lpc_sch_cells))) {
- dev_err(&dev->dev, "Cell count exceeds array size");
- return -ENODEV;
+ ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
+ WDT_IO_SIZE,
+ id->device, &lpc_sch_cells[cells]);
+ if (!ret)
+ cells++;
}
if (cells == 0) {
@@ -151,9 +136,6 @@ static int lpc_sch_probe(struct pci_dev *dev,
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);
--
2.1.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code
2014-08-22 10:58 ` [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code Andy Shevchenko
@ 2014-09-01 9:13 ` Lee Jones
2014-09-01 10:21 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2014-09-01 9:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> This patch refactors the driver to use helper functions instead of
> copy'n'pasted pieces of code.
>
> 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 | 146 ++++++++++++++++++++++----------------------------
> 1 file changed, 64 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index 4ee7550..0f01ef0 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -40,41 +40,6 @@
[...]
> -static int lpc_sch_probe(struct pci_dev *dev,
> - const struct pci_device_id *id)
> +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);
> + 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 -ENODEV;
If you're going to return an error, you need to use dev_err() above.
> }
>
> - 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");
> + res->start = base_addr;
> + res->end = base_addr + size - 1;
> + res->flags = IORESOURCE_IO;
> +
> + return 0;
> +}
> +
> +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;
> +
> + 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];
> + int size, cells = 0;
> + int ret;
> +
> + ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> + id->device, &lpc_sch_cells[cells]);
> + if (!ret)
> + cells++;
You're masking errors here. You need to return on error.
> + if (id->device == PCI_DEVICE_ID_INTEL_CENTERTON_ILB)
> + size = GPIO_IO_SIZE_CENTERTON;
> else
> - base_addr = (unsigned short)base_addr_cfg;
> + size = GPIO_IO_SIZE;
>
> - 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;
> - }
> + ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio", size,
> + 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) {
> - 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;
> - }
> - }
> -
> - if (WARN_ON(cells > ARRAY_SIZE(lpc_sch_cells))) {
> - dev_err(&dev->dev, "Cell count exceeds array size");
> - return -ENODEV;
> + ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> + WDT_IO_SIZE,
> + id->device, &lpc_sch_cells[cells]);
> + if (!ret)
> + cells++;
> }
>
> if (cells == 0) {
> @@ -151,9 +136,6 @@ static int lpc_sch_probe(struct pci_dev *dev,
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code
2014-09-01 9:13 ` Lee Jones
@ 2014-09-01 10:21 ` Andy Shevchenko
2014-09-01 11:38 ` Lee Jones
0 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-09-01 10:21 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> On Fri, 22 Aug 2014, Andy Shevchenko wrote:
>
> > This patch refactors the driver to use helper functions instead of
> > copy'n'pasted pieces of code.
> >
> > 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 | 146 ++++++++++++++++++++++----------------------------
> > 1 file changed, 64 insertions(+), 82 deletions(-)
> >
> > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > index 4ee7550..0f01ef0 100644
> > --- a/drivers/mfd/lpc_sch.c
> > +++ b/drivers/mfd/lpc_sch.c
> > @@ -40,41 +40,6 @@
>
> [...]
Thanks for review, my answers below.
>
> > -static int lpc_sch_probe(struct pci_dev *dev,
> > - const struct pci_device_id *id)
> > +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);
> > + 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 -ENODEV;
>
> If you're going to return an error, you need to use dev_err() above.
Okay.
> > }
> >
> > - 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");
> > + res->start = base_addr;
> > + res->end = base_addr + size - 1;
> > + res->flags = IORESOURCE_IO;
> > +
> > + return 0;
> > +}
> > +
> > +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;
> > +
> > + 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];
> > + int size, cells = 0;
> > + int ret;
> > +
> > + ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > + id->device, &lpc_sch_cells[cells]);
> > + if (!ret)
> > + cells++;
>
> You're masking errors here. You need to return on error.
No, I don't. It's a local error, I just need to understand if the HW has
or hasn't the IP part we're looking for.
I could, let's say, return true or false, if you prefer, with the above
meaning.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code
2014-09-01 10:21 ` Andy Shevchenko
@ 2014-09-01 11:38 ` Lee Jones
2014-09-01 11:48 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2014-09-01 11:38 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Mon, 01 Sep 2014, Andy Shevchenko wrote:
> On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> >
> > > This patch refactors the driver to use helper functions instead of
> > > copy'n'pasted pieces of code.
> > >
> > > 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 | 146 ++++++++++++++++++++++----------------------------
> > > 1 file changed, 64 insertions(+), 82 deletions(-)
> > >
> > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > index 4ee7550..0f01ef0 100644
> > > --- a/drivers/mfd/lpc_sch.c
> > > +++ b/drivers/mfd/lpc_sch.c
> > > @@ -40,41 +40,6 @@
> >
> > [...]
>
> Thanks for review, my answers below.
>
> >
> > > -static int lpc_sch_probe(struct pci_dev *dev,
> > > - const struct pci_device_id *id)
> > > +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);
> > > + 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 -ENODEV;
> >
> > If you're going to return an error, you need to use dev_err() above.
>
> Okay.
>
> > > }
> > >
> > > - 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");
> > > + res->start = base_addr;
> > > + res->end = base_addr + size - 1;
> > > + res->flags = IORESOURCE_IO;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +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;
> > > +
> > > + 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];
> > > + int size, cells = 0;
> > > + int ret;
> > > +
> > > + ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > > + id->device, &lpc_sch_cells[cells]);
> > > + if (!ret)
> > > + cells++;
> >
> > You're masking errors here. You need to return on error.
>
> No, I don't. It's a local error, I just need to understand if the HW has
> or hasn't the IP part we're looking for.
Yes, you do. -ENOMEM is not a local error, it needs to be returned.
> I could, let's say, return true or false, if you prefer, with the above
> meaning.
I don't see this very often, which makes me wonder if it's the right
thing to do at all; however, if you're going to this then please
return a local state, instead of a Linux Error Code.
#define LCP_SKIP_RESOURCE /* Seems resonable? */
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code
2014-09-01 11:38 ` Lee Jones
@ 2014-09-01 11:48 ` Andy Shevchenko
2014-09-01 13:46 ` Lee Jones
0 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-09-01 11:48 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Mon, 2014-09-01 at 12:38 +0100, Lee Jones wrote:
> On Mon, 01 Sep 2014, Andy Shevchenko wrote:
>
> > On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> > > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> > >
> > > > This patch refactors the driver to use helper functions instead of
> > > > copy'n'pasted pieces of code.
> > > >
> > > > 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 | 146 ++++++++++++++++++++++----------------------------
> > > > 1 file changed, 64 insertions(+), 82 deletions(-)
> > > >
> > > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > > index 4ee7550..0f01ef0 100644
> > > > --- a/drivers/mfd/lpc_sch.c
> > > > +++ b/drivers/mfd/lpc_sch.c
> > > > @@ -40,41 +40,6 @@
> > >
> > > [...]
> >
> > Thanks for review, my answers below.
> >
> > >
> > > > -static int lpc_sch_probe(struct pci_dev *dev,
> > > > - const struct pci_device_id *id)
> > > > +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);
> > > > + 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 -ENODEV;
> > >
> > > If you're going to return an error, you need to use dev_err() above.
> >
> > Okay.
> >
> > > > }
> > > >
> > > > - 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");
> > > > + res->start = base_addr;
> > > > + res->end = base_addr + size - 1;
> > > > + res->flags = IORESOURCE_IO;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +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;
> > > > +
> > > > + 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];
> > > > + int size, cells = 0;
> > > > + int ret;
> > > > +
> > > > + ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > > > + id->device, &lpc_sch_cells[cells]);
> > > > + if (!ret)
> > > > + cells++;
> > >
> > > You're masking errors here. You need to return on error.
> >
> > No, I don't. It's a local error, I just need to understand if the HW has
> > or hasn't the IP part we're looking for.
>
> Yes, you do. -ENOMEM is not a local error, it needs to be returned.
>
> > I could, let's say, return true or false, if you prefer, with the above
> > meaning.
>
> I don't see this very often, which makes me wonder if it's the right
> thing to do at all; however, if you're going to this then please
> return a local state, instead of a Linux Error Code.
Okay, what about 1 — IP is present and we have no error, 0 — no ip, no
error, -err — error that should be returned, would it work for you?
> #define LCP_SKIP_RESOURCE /* Seems resonable? */
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code
2014-09-01 11:48 ` Andy Shevchenko
@ 2014-09-01 13:46 ` Lee Jones
2014-09-01 14:00 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2014-09-01 13:46 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Mon, 01 Sep 2014, Andy Shevchenko wrote:
> On Mon, 2014-09-01 at 12:38 +0100, Lee Jones wrote:
> > On Mon, 01 Sep 2014, Andy Shevchenko wrote:
> >
> > > On Mon, 2014-09-01 at 10:13 +0100, Lee Jones wrote:
> > > > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> > > >
> > > > > This patch refactors the driver to use helper functions instead of
> > > > > copy'n'pasted pieces of code.
> > > > >
> > > > > 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 | 146 ++++++++++++++++++++++----------------------------
> > > > > 1 file changed, 64 insertions(+), 82 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > > > index 4ee7550..0f01ef0 100644
> > > > > --- a/drivers/mfd/lpc_sch.c
> > > > > +++ b/drivers/mfd/lpc_sch.c
> > > > > @@ -40,41 +40,6 @@
> > > >
> > > > [...]
> > >
> > > Thanks for review, my answers below.
> > >
> > > >
> > > > > -static int lpc_sch_probe(struct pci_dev *dev,
> > > > > - const struct pci_device_id *id)
> > > > > +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);
> > > > > + 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 -ENODEV;
> > > >
> > > > If you're going to return an error, you need to use dev_err() above.
> > >
> > > Okay.
> > >
> > > > > }
> > > > >
> > > > > - 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");
> > > > > + res->start = base_addr;
> > > > > + res->end = base_addr + size - 1;
> > > > > + res->flags = IORESOURCE_IO;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +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;
> > > > > +
> > > > > + 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];
> > > > > + int size, cells = 0;
> > > > > + int ret;
> > > > > +
> > > > > + ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus", SMBUS_IO_SIZE,
> > > > > + id->device, &lpc_sch_cells[cells]);
> > > > > + if (!ret)
> > > > > + cells++;
> > > >
> > > > You're masking errors here. You need to return on error.
> > >
> > > No, I don't. It's a local error, I just need to understand if the HW has
> > > or hasn't the IP part we're looking for.
> >
> > Yes, you do. -ENOMEM is not a local error, it needs to be returned.
> >
> > > I could, let's say, return true or false, if you prefer, with the above
> > > meaning.
> >
> > I don't see this very often, which makes me wonder if it's the right
> > thing to do at all; however, if you're going to this then please
> > return a local state, instead of a Linux Error Code.
>
> Okay, what about 1 — IP is present and we have no error, 0 — no ip, no
> error, -err — error that should be returned, would it work for you?
I would suggest -E<error_message> for messages to be passed back, 0
for 'everything found and correct' and >0 (#defined of course) for any
local messages you wish to pass 'IP not found', etc.
> > #define LCP_SKIP_RESOURCE /* Seems resonable? */
>
>
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code
2014-09-01 13:46 ` Lee Jones
@ 2014-09-01 14:00 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2014-09-01 14:00 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Mon, 2014-09-01 at 14:46 +0100, Lee Jones wrote:
[]
> > Okay, what about 1 — IP is present and we have no error, 0 — no ip, no
> > error, -err — error that should be returned, would it work for you?
>
> I would suggest -E<error_message> for messages to be passed back, 0
> for 'everything found and correct' and >0 (#defined of course) for any
> local messages you wish to pass 'IP not found', etc.
>
> > > #define LCP_SKIP_RESOURCE /* Seems resonable? */
Okay, makes sense. Will do this in v2.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct
2014-08-22 10:58 [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
2014-08-22 10:58 ` [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code Andy Shevchenko
@ 2014-08-22 10:58 ` Andy Shevchenko
2014-09-01 9:16 ` Lee Jones
2014-08-22 10:58 ` [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB Andy Shevchenko
` (3 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-08-22 10:58 UTC (permalink / raw)
To: Lee Jones, Bjorn Helgaas, linux-kernel, Samuel Ortiz,
Chang Rebecca Swee Fun
Cc: Andy Shevchenko
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++;
if (cells == 0) {
dev_err(&dev->dev, "All decode registers disabled.\n");
--
2.1.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct
2014-08-22 10:58 ` [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct Andy Shevchenko
@ 2014-09-01 9:16 ` Lee Jones
2014-09-01 10:25 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2014-09-01 9:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct
2014-09-01 9:16 ` Lee Jones
@ 2014-09-01 10:25 ` Andy Shevchenko
2014-09-02 0:17 ` Chang, Rebecca Swee Fun
0 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-09-01 10:25 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Mon, 2014-09-01 at 10:16 +0100, Lee Jones wrote:
> 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>
[]
> 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.
The only reason behind is that this patch (in other form) was written by
Rebecca in the first place. I recommended to clean up before, and
actually did that clean up and amended Rebecca's patch.
So, if Rebecca has now objections I could squash it.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct
2014-09-01 10:25 ` Andy Shevchenko
@ 2014-09-02 0:17 ` Chang, Rebecca Swee Fun
2014-09-02 7:25 ` Lee Jones
0 siblings, 1 reply; 26+ messages in thread
From: Chang, Rebecca Swee Fun @ 2014-09-02 0:17 UTC (permalink / raw)
To: 'Andy Shevchenko', Lee Jones
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1766 bytes --]
> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: 01 September, 2014 6:25 PM
> To: Lee Jones
> Cc: Bjorn Helgaas; linux-kernel@vger.kernel.org; Samuel Ortiz; Chang, Rebecca
> Swee Fun
> Subject: Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with
> chipset info struct
>
> On Mon, 2014-09-01 at 10:16 +0100, Lee Jones wrote:
> > 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>
>
> []
>
>
> > 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.
>
> The only reason behind is that this patch (in other form) was written by
> Rebecca in the first place. I recommended to clean up before, and actually did
> that clean up and amended Rebecca's patch.
>
> So, if Rebecca has now objections I could squash it.
I have no objections. Thanks.
>
>
> --
> Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct
2014-09-02 0:17 ` Chang, Rebecca Swee Fun
@ 2014-09-02 7:25 ` Lee Jones
0 siblings, 0 replies; 26+ messages in thread
From: Lee Jones @ 2014-09-02 7:25 UTC (permalink / raw)
To: Chang, Rebecca Swee Fun
Cc: 'Andy Shevchenko', Bjorn Helgaas, linux-kernel, Samuel Ortiz
On Tue, 02 Sep 2014, Chang, Rebecca Swee Fun wrote:
>
>
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> > Sent: 01 September, 2014 6:25 PM
> > To: Lee Jones
> > Cc: Bjorn Helgaas; linux-kernel@vger.kernel.org; Samuel Ortiz; Chang, Rebecca
> > Swee Fun
> > Subject: Re: [PATCH v1 2/5] mfd: lpc_sch: better code manageability with
> > chipset info struct
> >
> > On Mon, 2014-09-01 at 10:16 +0100, Lee Jones wrote:
> > > 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>
> >
> > []
> >
> >
> > > 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.
> >
> > The only reason behind is that this patch (in other form) was written by
> > Rebecca in the first place. I recommended to clean up before, and actually did
> > that clean up and amended Rebecca's patch.
> >
> > So, if Rebecca has now objections I could squash it.
>
> I have no objections. Thanks.
Thanks Rebecca.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB
2014-08-22 10:58 [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
2014-08-22 10:58 ` [PATCH v1 1/5] mfd: lpc_sch: reduce duplicate code Andy Shevchenko
2014-08-22 10:58 ` [PATCH v1 2/5] mfd: lpc_sch: better code manageability with chipset info struct Andy Shevchenko
@ 2014-08-22 10:58 ` Andy Shevchenko
2014-08-29 14:27 ` Bjorn Helgaas
2014-08-22 10:58 ` [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000 Andy Shevchenko
` (2 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-08-22 10:58 UTC (permalink / raw)
To: Lee Jones, Bjorn Helgaas, linux-kernel, Samuel Ortiz,
Chang Rebecca Swee Fun
Cc: Josef Ahmad, Andy Shevchenko
From: Josef Ahmad <josef.ahmad@intel.com>
This patch adds the PCI id for Intel Quark ILB.
It will be used for GPIO and Multifunction device driver.
Signed-off-by: Josef Ahmad <josef.ahmad@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
include/linux/pci_ids.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 6ed0bb7..4e82195 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2557,6 +2557,7 @@
#define PCI_DEVICE_ID_INTEL_MFD_EMMC0 0x0823
#define PCI_DEVICE_ID_INTEL_MFD_EMMC1 0x0824
#define PCI_DEVICE_ID_INTEL_MRST_SD2 0x084F
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB 0x095E
#define PCI_DEVICE_ID_INTEL_I960 0x0960
#define PCI_DEVICE_ID_INTEL_I960RM 0x0962
#define PCI_DEVICE_ID_INTEL_CENTERTON_ILB 0x0c60
--
2.1.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB
2014-08-22 10:58 ` [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB Andy Shevchenko
@ 2014-08-29 14:27 ` Bjorn Helgaas
2014-09-01 9:14 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Bjorn Helgaas @ 2014-08-29 14:27 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun,
Josef Ahmad
On Fri, Aug 22, 2014 at 4:58 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> From: Josef Ahmad <josef.ahmad@intel.com>
>
> This patch adds the PCI id for Intel Quark ILB.
> It will be used for GPIO and Multifunction device driver.
>
> Signed-off-by: Josef Ahmad <josef.ahmad@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Assuming that this will actually be used in more than one place:
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> include/linux/pci_ids.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 6ed0bb7..4e82195 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2557,6 +2557,7 @@
> #define PCI_DEVICE_ID_INTEL_MFD_EMMC0 0x0823
> #define PCI_DEVICE_ID_INTEL_MFD_EMMC1 0x0824
> #define PCI_DEVICE_ID_INTEL_MRST_SD2 0x084F
> +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB 0x095E
> #define PCI_DEVICE_ID_INTEL_I960 0x0960
> #define PCI_DEVICE_ID_INTEL_I960RM 0x0962
> #define PCI_DEVICE_ID_INTEL_CENTERTON_ILB 0x0c60
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB
2014-08-29 14:27 ` Bjorn Helgaas
@ 2014-09-01 9:14 ` Andy Shevchenko
2014-09-02 0:14 ` Chang, Rebecca Swee Fun
0 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-09-01 9:14 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Lee Jones, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun,
Josef Ahmad
On Fri, 2014-08-29 at 08:27 -0600, Bjorn Helgaas wrote:
> On Fri, Aug 22, 2014 at 4:58 AM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > From: Josef Ahmad <josef.ahmad@intel.com>
> >
> > This patch adds the PCI id for Intel Quark ILB.
> > It will be used for GPIO and Multifunction device driver.
> >
> > Signed-off-by: Josef Ahmad <josef.ahmad@intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Assuming that this will actually be used in more than one place:
Yes, look for the other IDs in the same driver (lpc_sch).
The question is will we have the update for sch_gpio in time to be
included to v3.18? I think Rebecca can answer to this one.
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> > ---
> > include/linux/pci_ids.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > index 6ed0bb7..4e82195 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -2557,6 +2557,7 @@
> > #define PCI_DEVICE_ID_INTEL_MFD_EMMC0 0x0823
> > #define PCI_DEVICE_ID_INTEL_MFD_EMMC1 0x0824
> > #define PCI_DEVICE_ID_INTEL_MRST_SD2 0x084F
> > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB 0x095E
> > #define PCI_DEVICE_ID_INTEL_I960 0x0960
> > #define PCI_DEVICE_ID_INTEL_I960RM 0x0962
> > #define PCI_DEVICE_ID_INTEL_CENTERTON_ILB 0x0c60
> > --
> > 2.1.0
> >
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB
2014-09-01 9:14 ` Andy Shevchenko
@ 2014-09-02 0:14 ` Chang, Rebecca Swee Fun
0 siblings, 0 replies; 26+ messages in thread
From: Chang, Rebecca Swee Fun @ 2014-09-02 0:14 UTC (permalink / raw)
To: 'Andy Shevchenko', Bjorn Helgaas
Cc: Lee Jones, linux-kernel, Samuel Ortiz, Ahmad, Josef
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2313 bytes --]
> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: 01 September, 2014 5:14 PM
> To: Bjorn Helgaas
> Cc: Lee Jones; linux-kernel@vger.kernel.org; Samuel Ortiz; Chang, Rebecca
> Swee Fun; Ahmad, Josef
> Subject: Re: [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB
>
> On Fri, 2014-08-29 at 08:27 -0600, Bjorn Helgaas wrote:
> > On Fri, Aug 22, 2014 at 4:58 AM, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > From: Josef Ahmad <josef.ahmad@intel.com>
> > >
> > > This patch adds the PCI id for Intel Quark ILB.
> > > It will be used for GPIO and Multifunction device driver.
> > >
> > > Signed-off-by: Josef Ahmad <josef.ahmad@intel.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Assuming that this will actually be used in more than one place:
>
> Yes, look for the other IDs in the same driver (lpc_sch).
>
> The question is will we have the update for sch_gpio in time to be included to
> v3.18? I think Rebecca can answer to this one.
Yes, we are working on updating sch_gpio but I can't give you a confirm answer whether it will be included in v3.18. Sch_gpio has been scheduled as my 2nd priority work currently. However, I will try to make it in time if possible.
Rebecca
>
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > > ---
> > > include/linux/pci_ids.h | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index
> > > 6ed0bb7..4e82195 100644
> > > --- a/include/linux/pci_ids.h
> > > +++ b/include/linux/pci_ids.h
> > > @@ -2557,6 +2557,7 @@
> > > #define PCI_DEVICE_ID_INTEL_MFD_EMMC0 0x0823 #define
> > > PCI_DEVICE_ID_INTEL_MFD_EMMC1 0x0824
> > > #define PCI_DEVICE_ID_INTEL_MRST_SD2 0x084F
> > > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB 0x095E
> > > #define PCI_DEVICE_ID_INTEL_I960 0x0960
> > > #define PCI_DEVICE_ID_INTEL_I960RM 0x0962
> > > #define PCI_DEVICE_ID_INTEL_CENTERTON_ILB 0x0c60
> > > --
> > > 2.1.0
> > >
>
>
> --
> Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000
2014-08-22 10:58 [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
` (2 preceding siblings ...)
2014-08-22 10:58 ` [PATCH v1 3/5] pci_ids: add support for Intel Quark ILB Andy Shevchenko
@ 2014-08-22 10:58 ` Andy Shevchenko
2014-09-01 9:22 ` Lee Jones
2014-08-22 10:58 ` [PATCH v1 5/5] mfd: lpc_sch: remove FSF address Andy Shevchenko
2014-08-29 9:57 ` [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
5 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-08-22 10:58 UTC (permalink / raw)
To: Lee Jones, Bjorn Helgaas, linux-kernel, Samuel Ortiz,
Chang Rebecca Swee Fun
Cc: Andy Shevchenko
Intel Quark X1000 SoC supports IRQ based GPIO. This patch will
enable MFD support for Quark X1000 and provide IRQ resources
to Quark X1000 GPIO device driver.
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 | 41 +++++++++++++++++++++++++++++++++++------
1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
index c4eb359..6145a4c 100644
--- a/drivers/mfd/lpc_sch.c
+++ b/drivers/mfd/lpc_sch.c
@@ -37,6 +37,9 @@
#define GPIO_IO_SIZE 64
#define GPIO_IO_SIZE_CENTERTON 128
+/* Intel Quark X1000 GPIO IRQ Number */
+#define GPIO_IRQ_QUARK_X1000 9
+
#define WDTBASE 0x84
#define WDT_IO_SIZE 64
@@ -44,28 +47,37 @@ enum sch_chipsets {
LPC_SCH = 0, /* Intel Poulsbo SCH */
LPC_ITC, /* Intel Tunnel Creek */
LPC_CENTERTON, /* Intel Centerton */
+ LPC_QUARK_X1000, /* Intel Quark X1000 */
};
struct lpc_sch_info {
unsigned int io_size_smbus;
unsigned int io_size_gpio;
unsigned int io_size_wdt;
+ int irq_gpio;
};
static struct lpc_sch_info sch_chipset_info[] = {
[LPC_SCH] = {
.io_size_smbus = SMBUS_IO_SIZE,
.io_size_gpio = GPIO_IO_SIZE,
+ .irq_gpio = -1,
},
[LPC_ITC] = {
.io_size_smbus = SMBUS_IO_SIZE,
.io_size_gpio = GPIO_IO_SIZE,
.io_size_wdt = WDT_IO_SIZE,
+ .irq_gpio = -1,
},
[LPC_CENTERTON] = {
.io_size_smbus = SMBUS_IO_SIZE,
.io_size_gpio = GPIO_IO_SIZE_CENTERTON,
.io_size_wdt = WDT_IO_SIZE,
+ .irq_gpio = -1,
+ },
+ [LPC_QUARK_X1000] = {
+ .io_size_gpio = GPIO_IO_SIZE,
+ .irq_gpio = GPIO_IRQ_QUARK_X1000,
},
};
@@ -73,6 +85,7 @@ static const struct pci_device_id lpc_sch_ids[] = {
{ 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 },
+ { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB), LPC_QUARK_X1000 },
{ 0, }
};
MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
@@ -106,14 +119,26 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
return 0;
}
+static int lpc_sch_get_irq(struct resource *res, int irq)
+{
+ if (irq < 0)
+ return -EINVAL;
+
+ res->start = irq;
+ res->end = irq;
+ res->flags = IORESOURCE_IRQ;
+
+ return 0;
+}
+
static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
- const char *name, int size, int id,
- struct mfd_cell *cell)
+ const char *name, int size, int irq,
+ int id, struct mfd_cell *cell)
{
struct resource *res;
int ret;
- res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
+ res = devm_kcalloc(&pdev->dev, 2, sizeof(*res), GFP_KERNEL);
if (!res)
return -ENOMEM;
@@ -129,6 +154,10 @@ static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
cell->ignore_resource_conflicts = true;
cell->id = id;
+ ret = lpc_sch_get_irq(++res, irq);
+ if (!ret)
+ cell->num_resources++;
+
return 0;
}
@@ -141,19 +170,19 @@ static int lpc_sch_probe(struct pci_dev *dev,
int ret;
ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
- info->io_size_smbus,
+ info->io_size_smbus, -1,
id->device, &lpc_sch_cells[cells]);
if (!ret)
cells++;
ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
- info->io_size_gpio,
+ info->io_size_gpio, info->irq_gpio,
id->device, &lpc_sch_cells[cells]);
if (!ret)
cells++;
ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
- info->io_size_wdt,
+ info->io_size_wdt, -1,
id->device, &lpc_sch_cells[cells]);
if (!ret)
cells++;
--
2.1.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000
2014-08-22 10:58 ` [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000 Andy Shevchenko
@ 2014-09-01 9:22 ` Lee Jones
2014-09-01 10:28 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2014-09-01 9:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> Intel Quark X1000 SoC supports IRQ based GPIO. This patch will
> enable MFD support for Quark X1000 and provide IRQ resources
> to Quark X1000 GPIO device driver.
>
> 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 | 41 +++++++++++++++++++++++++++++++++++------
> 1 file changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index c4eb359..6145a4c 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -37,6 +37,9 @@
> #define GPIO_IO_SIZE 64
> #define GPIO_IO_SIZE_CENTERTON 128
>
> +/* Intel Quark X1000 GPIO IRQ Number */
> +#define GPIO_IRQ_QUARK_X1000 9
> +
> #define WDTBASE 0x84
> #define WDT_IO_SIZE 64
>
> @@ -44,28 +47,37 @@ enum sch_chipsets {
> LPC_SCH = 0, /* Intel Poulsbo SCH */
> LPC_ITC, /* Intel Tunnel Creek */
> LPC_CENTERTON, /* Intel Centerton */
> + LPC_QUARK_X1000, /* Intel Quark X1000 */
> };
>
> struct lpc_sch_info {
> unsigned int io_size_smbus;
> unsigned int io_size_gpio;
> unsigned int io_size_wdt;
> + int irq_gpio;
> };
>
> static struct lpc_sch_info sch_chipset_info[] = {
> [LPC_SCH] = {
> .io_size_smbus = SMBUS_IO_SIZE,
> .io_size_gpio = GPIO_IO_SIZE,
> + .irq_gpio = -1,
> },
> [LPC_ITC] = {
> .io_size_smbus = SMBUS_IO_SIZE,
> .io_size_gpio = GPIO_IO_SIZE,
> .io_size_wdt = WDT_IO_SIZE,
> + .irq_gpio = -1,
> },
> [LPC_CENTERTON] = {
> .io_size_smbus = SMBUS_IO_SIZE,
> .io_size_gpio = GPIO_IO_SIZE_CENTERTON,
> .io_size_wdt = WDT_IO_SIZE,
> + .irq_gpio = -1,
> + },
> + [LPC_QUARK_X1000] = {
> + .io_size_gpio = GPIO_IO_SIZE,
> + .irq_gpio = GPIO_IRQ_QUARK_X1000,
> },
> };
>
> @@ -73,6 +85,7 @@ static const struct pci_device_id lpc_sch_ids[] = {
> { 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 },
> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB), LPC_QUARK_X1000 },
> { 0, }
> };
> MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> @@ -106,14 +119,26 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> return 0;
> }
>
> +static int lpc_sch_get_irq(struct resource *res, int irq)
> +{
> + if (irq < 0)
> + return -EINVAL;
> +
> + res->start = irq;
> + res->end = irq;
> + res->flags = IORESOURCE_IRQ;
> +
> + return 0;
> +}
Why does this need to be a separate function?
I fear that the code will become unnecessarily fragmented, just for the
sake of it.
> static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> - const char *name, int size, int id,
> - struct mfd_cell *cell)
> + const char *name, int size, int irq,
> + int id, struct mfd_cell *cell)
> {
> struct resource *res;
> int ret;
>
> - res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> + res = devm_kcalloc(&pdev->dev, 2, sizeof(*res), GFP_KERNEL);
> if (!res)
> return -ENOMEM;
>
> @@ -129,6 +154,10 @@ static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> cell->ignore_resource_conflicts = true;
> cell->id = id;
>
> + ret = lpc_sch_get_irq(++res, irq);
> + if (!ret)
> + cell->num_resources++;
Once again, you're masking errors. If it's not an error, don't return
one. If it is, filter it back and fail the bind.
> return 0;
> }
>
> @@ -141,19 +170,19 @@ static int lpc_sch_probe(struct pci_dev *dev,
> int ret;
>
> ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
> - info->io_size_smbus,
> + info->io_size_smbus, -1,
> id->device, &lpc_sch_cells[cells]);
> if (!ret)
> cells++;
>
> ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
> - info->io_size_gpio,
> + info->io_size_gpio, info->irq_gpio,
> id->device, &lpc_sch_cells[cells]);
> if (!ret)
> cells++;
>
> ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> - info->io_size_wdt,
> + info->io_size_wdt, -1,
> id->device, &lpc_sch_cells[cells]);
> if (!ret)
> cells++;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000
2014-09-01 9:22 ` Lee Jones
@ 2014-09-01 10:28 ` Andy Shevchenko
2014-09-01 11:31 ` Lee Jones
0 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-09-01 10:28 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Mon, 2014-09-01 at 10:22 +0100, Lee Jones wrote:
> On Fri, 22 Aug 2014, Andy Shevchenko wrote:
>
> > Intel Quark X1000 SoC supports IRQ based GPIO. This patch will
> > enable MFD support for Quark X1000 and provide IRQ resources
> > to Quark X1000 GPIO device driver.
> >
> > 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>
See my answers below.
> > ---
> > drivers/mfd/lpc_sch.c | 41 +++++++++++++++++++++++++++++++++++------
> > 1 file changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > index c4eb359..6145a4c 100644
> > --- a/drivers/mfd/lpc_sch.c
> > +++ b/drivers/mfd/lpc_sch.c
> > @@ -37,6 +37,9 @@
> > #define GPIO_IO_SIZE 64
> > #define GPIO_IO_SIZE_CENTERTON 128
> >
> > +/* Intel Quark X1000 GPIO IRQ Number */
> > +#define GPIO_IRQ_QUARK_X1000 9
> > +
> > #define WDTBASE 0x84
> > #define WDT_IO_SIZE 64
> >
> > @@ -44,28 +47,37 @@ enum sch_chipsets {
> > LPC_SCH = 0, /* Intel Poulsbo SCH */
> > LPC_ITC, /* Intel Tunnel Creek */
> > LPC_CENTERTON, /* Intel Centerton */
> > + LPC_QUARK_X1000, /* Intel Quark X1000 */
> > };
> >
> > struct lpc_sch_info {
> > unsigned int io_size_smbus;
> > unsigned int io_size_gpio;
> > unsigned int io_size_wdt;
> > + int irq_gpio;
> > };
> >
> > static struct lpc_sch_info sch_chipset_info[] = {
> > [LPC_SCH] = {
> > .io_size_smbus = SMBUS_IO_SIZE,
> > .io_size_gpio = GPIO_IO_SIZE,
> > + .irq_gpio = -1,
> > },
> > [LPC_ITC] = {
> > .io_size_smbus = SMBUS_IO_SIZE,
> > .io_size_gpio = GPIO_IO_SIZE,
> > .io_size_wdt = WDT_IO_SIZE,
> > + .irq_gpio = -1,
> > },
> > [LPC_CENTERTON] = {
> > .io_size_smbus = SMBUS_IO_SIZE,
> > .io_size_gpio = GPIO_IO_SIZE_CENTERTON,
> > .io_size_wdt = WDT_IO_SIZE,
> > + .irq_gpio = -1,
> > + },
> > + [LPC_QUARK_X1000] = {
> > + .io_size_gpio = GPIO_IO_SIZE,
> > + .irq_gpio = GPIO_IRQ_QUARK_X1000,
> > },
> > };
> >
> > @@ -73,6 +85,7 @@ static const struct pci_device_id lpc_sch_ids[] = {
> > { 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 },
> > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB), LPC_QUARK_X1000 },
> > { 0, }
> > };
> > MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> > @@ -106,14 +119,26 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> > return 0;
> > }
> >
> > +static int lpc_sch_get_irq(struct resource *res, int irq)
> > +{
> > + if (irq < 0)
> > + return -EINVAL;
> > +
> > + res->start = irq;
> > + res->end = irq;
> > + res->flags = IORESOURCE_IRQ;
> > +
> > + return 0;
> > +}
>
> Why does this need to be a separate function?
>
> I fear that the code will become unnecessarily fragmented, just for the
> sake of it.
I could do this as a condition branch.
>
> > static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > - const char *name, int size, int id,
> > - struct mfd_cell *cell)
> > + const char *name, int size, int irq,
> > + int id, struct mfd_cell *cell)
> > {
> > struct resource *res;
> > int ret;
> >
> > - res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > + res = devm_kcalloc(&pdev->dev, 2, sizeof(*res), GFP_KERNEL);
> > if (!res)
> > return -ENOMEM;
> >
> > @@ -129,6 +154,10 @@ static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > cell->ignore_resource_conflicts = true;
> > cell->id = id;
> >
> > + ret = lpc_sch_get_irq(++res, irq);
> > + if (!ret)
> > + cell->num_resources++;
>
> Once again, you're masking errors. If it's not an error, don't return
> one. If it is, filter it back and fail the bind.
I have to know if there is such resource or not. Taking into account you
prefer to see lpc_sch_get_irq embedded in here I just can do as a
condition branch and there will be no more question I hope.
>
> > return 0;
> > }
> >
> > @@ -141,19 +170,19 @@ static int lpc_sch_probe(struct pci_dev *dev,
> > int ret;
> >
> > ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
> > - info->io_size_smbus,
> > + info->io_size_smbus, -1,
> > id->device, &lpc_sch_cells[cells]);
> > if (!ret)
> > cells++;
> >
> > ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
> > - info->io_size_gpio,
> > + info->io_size_gpio, info->irq_gpio,
> > id->device, &lpc_sch_cells[cells]);
> > if (!ret)
> > cells++;
> >
> > ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> > - info->io_size_wdt,
> > + info->io_size_wdt, -1,
> > id->device, &lpc_sch_cells[cells]);
> > if (!ret)
> > cells++;
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000
2014-09-01 10:28 ` Andy Shevchenko
@ 2014-09-01 11:31 ` Lee Jones
0 siblings, 0 replies; 26+ messages in thread
From: Lee Jones @ 2014-09-01 11:31 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Mon, 01 Sep 2014, Andy Shevchenko wrote:
> On Mon, 2014-09-01 at 10:22 +0100, Lee Jones wrote:
> > On Fri, 22 Aug 2014, Andy Shevchenko wrote:
> >
> > > Intel Quark X1000 SoC supports IRQ based GPIO. This patch will
> > > enable MFD support for Quark X1000 and provide IRQ resources
> > > to Quark X1000 GPIO device driver.
> > >
> > > 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>
>
> See my answers below.
>
> > > ---
> > > drivers/mfd/lpc_sch.c | 41 +++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 35 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> > > index c4eb359..6145a4c 100644
> > > --- a/drivers/mfd/lpc_sch.c
> > > +++ b/drivers/mfd/lpc_sch.c
> > > @@ -37,6 +37,9 @@
> > > #define GPIO_IO_SIZE 64
> > > #define GPIO_IO_SIZE_CENTERTON 128
> > >
> > > +/* Intel Quark X1000 GPIO IRQ Number */
> > > +#define GPIO_IRQ_QUARK_X1000 9
> > > +
> > > #define WDTBASE 0x84
> > > #define WDT_IO_SIZE 64
> > >
> > > @@ -44,28 +47,37 @@ enum sch_chipsets {
> > > LPC_SCH = 0, /* Intel Poulsbo SCH */
> > > LPC_ITC, /* Intel Tunnel Creek */
> > > LPC_CENTERTON, /* Intel Centerton */
> > > + LPC_QUARK_X1000, /* Intel Quark X1000 */
> > > };
> > >
> > > struct lpc_sch_info {
> > > unsigned int io_size_smbus;
> > > unsigned int io_size_gpio;
> > > unsigned int io_size_wdt;
> > > + int irq_gpio;
> > > };
> > >
> > > static struct lpc_sch_info sch_chipset_info[] = {
> > > [LPC_SCH] = {
> > > .io_size_smbus = SMBUS_IO_SIZE,
> > > .io_size_gpio = GPIO_IO_SIZE,
> > > + .irq_gpio = -1,
> > > },
> > > [LPC_ITC] = {
> > > .io_size_smbus = SMBUS_IO_SIZE,
> > > .io_size_gpio = GPIO_IO_SIZE,
> > > .io_size_wdt = WDT_IO_SIZE,
> > > + .irq_gpio = -1,
> > > },
> > > [LPC_CENTERTON] = {
> > > .io_size_smbus = SMBUS_IO_SIZE,
> > > .io_size_gpio = GPIO_IO_SIZE_CENTERTON,
> > > .io_size_wdt = WDT_IO_SIZE,
> > > + .irq_gpio = -1,
> > > + },
> > > + [LPC_QUARK_X1000] = {
> > > + .io_size_gpio = GPIO_IO_SIZE,
> > > + .irq_gpio = GPIO_IRQ_QUARK_X1000,
> > > },
> > > };
> > >
> > > @@ -73,6 +85,7 @@ static const struct pci_device_id lpc_sch_ids[] = {
> > > { 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 },
> > > + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_QUARK_X1000_ILB), LPC_QUARK_X1000 },
> > > { 0, }
> > > };
> > > MODULE_DEVICE_TABLE(pci, lpc_sch_ids);
> > > @@ -106,14 +119,26 @@ static int lpc_sch_get_io(struct pci_dev *pdev, int where, const char *name,
> > > return 0;
> > > }
> > >
> > > +static int lpc_sch_get_irq(struct resource *res, int irq)
> > > +{
> > > + if (irq < 0)
> > > + return -EINVAL;
> > > +
> > > + res->start = irq;
> > > + res->end = irq;
> > > + res->flags = IORESOURCE_IRQ;
> > > +
> > > + return 0;
> > > +}
> >
> > Why does this need to be a separate function?
> >
> > I fear that the code will become unnecessarily fragmented, just for the
> > sake of it.
>
> I could do this as a condition branch.
You could, but you don't.
> > > static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > > - const char *name, int size, int id,
> > > - struct mfd_cell *cell)
> > > + const char *name, int size, int irq,
> > > + int id, struct mfd_cell *cell)
> > > {
> > > struct resource *res;
> > > int ret;
> > >
> > > - res = devm_kzalloc(&pdev->dev, sizeof(*res), GFP_KERNEL);
> > > + res = devm_kcalloc(&pdev->dev, 2, sizeof(*res), GFP_KERNEL);
> > > if (!res)
> > > return -ENOMEM;
> > >
> > > @@ -129,6 +154,10 @@ static int lpc_sch_populate_cell(struct pci_dev *pdev, int where,
> > > cell->ignore_resource_conflicts = true;
> > > cell->id = id;
> > >
> > > + ret = lpc_sch_get_irq(++res, irq);
> > > + if (!ret)
> > > + cell->num_resources++;
> >
> > Once again, you're masking errors. If it's not an error, don't return
> > one. If it is, filter it back and fail the bind.
>
> I have to know if there is such resource or not. Taking into account you
> prefer to see lpc_sch_get_irq embedded in here I just can do as a
> condition branch and there will be no more question I hope.
This is true.
> > > return 0;
> > > }
> > >
> > > @@ -141,19 +170,19 @@ static int lpc_sch_probe(struct pci_dev *dev,
> > > int ret;
> > >
> > > ret = lpc_sch_populate_cell(dev, SMBASE, "isch_smbus",
> > > - info->io_size_smbus,
> > > + info->io_size_smbus, -1,
> > > id->device, &lpc_sch_cells[cells]);
> > > if (!ret)
> > > cells++;
> > >
> > > ret = lpc_sch_populate_cell(dev, GPIOBASE, "sch_gpio",
> > > - info->io_size_gpio,
> > > + info->io_size_gpio, info->irq_gpio,
> > > id->device, &lpc_sch_cells[cells]);
> > > if (!ret)
> > > cells++;
> > >
> > > ret = lpc_sch_populate_cell(dev, WDTBASE, "ie6xx_wdt",
> > > - info->io_size_wdt,
> > > + info->io_size_wdt, -1,
> > > id->device, &lpc_sch_cells[cells]);
> > > if (!ret)
> > > cells++;
>
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 5/5] mfd: lpc_sch: remove FSF address
2014-08-22 10:58 [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
` (3 preceding siblings ...)
2014-08-22 10:58 ` [PATCH v1 4/5] mfd: lpc_sch: Add support for Intel Quark X1000 Andy Shevchenko
@ 2014-08-22 10:58 ` Andy Shevchenko
2014-08-29 14:26 ` Bjorn Helgaas
2014-08-29 9:57 ` [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
5 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-08-22 10:58 UTC (permalink / raw)
To: Lee Jones, Bjorn Helgaas, linux-kernel, Samuel Ortiz,
Chang Rebecca Swee Fun
Cc: Andy Shevchenko
This patch removes FSF address becase it can be changed. While here, update the
copyright lines by adding Intel Corp. to them.
There is no functional change.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/mfd/lpc_sch.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
index 6145a4c..b14ca7c 100644
--- a/drivers/mfd/lpc_sch.c
+++ b/drivers/mfd/lpc_sch.c
@@ -7,6 +7,7 @@
* Configuration Registers.
*
* Copyright (c) 2010 CompuLab Ltd
+ * Copyright (c) 2014 Intel Corp.
* Author: Denis Turischev <denis@compulab.co.il>
*
* This program is free software; you can redistribute it and/or modify
@@ -17,10 +18,6 @@
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; see the file COPYING. If not, write to
- * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
*/
#include <linux/kernel.h>
--
2.1.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v1 5/5] mfd: lpc_sch: remove FSF address
2014-08-22 10:58 ` [PATCH v1 5/5] mfd: lpc_sch: remove FSF address Andy Shevchenko
@ 2014-08-29 14:26 ` Bjorn Helgaas
0 siblings, 0 replies; 26+ messages in thread
From: Bjorn Helgaas @ 2014-08-29 14:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Fri, Aug 22, 2014 at 4:58 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> This patch removes FSF address becase it can be changed. While here, update the
because
> copyright lines by adding Intel Corp. to them.
>
> There is no functional change.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/mfd/lpc_sch.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/mfd/lpc_sch.c b/drivers/mfd/lpc_sch.c
> index 6145a4c..b14ca7c 100644
> --- a/drivers/mfd/lpc_sch.c
> +++ b/drivers/mfd/lpc_sch.c
> @@ -7,6 +7,7 @@
> * Configuration Registers.
> *
> * Copyright (c) 2010 CompuLab Ltd
> + * Copyright (c) 2014 Intel Corp.
> * Author: Denis Turischev <denis@compulab.co.il>
> *
> * This program is free software; you can redistribute it and/or modify
> @@ -17,10 +18,6 @@
> * but WITHOUT ANY WARRANTY; without even the implied warranty of
> * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> * GNU General Public License for more details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; see the file COPYING. If not, write to
> - * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> #include <linux/kernel.h>
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support
2014-08-22 10:58 [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
` (4 preceding siblings ...)
2014-08-22 10:58 ` [PATCH v1 5/5] mfd: lpc_sch: remove FSF address Andy Shevchenko
@ 2014-08-29 9:57 ` Andy Shevchenko
2014-08-29 11:00 ` Lee Jones
5 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2014-08-29 9:57 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Fri, 2014-08-22 at 13:58 +0300, Andy Shevchenko wrote:
> This brings support of the LPC SCH device found on Intel Quark SoC. Patches
> were compile tested, and driver runs on actual hardware.
>
> Please, note that PCI ID will be used in future in the gpio-sch driver.
Bjorn, could you Ack or comment patch 3/5?
Lee, do you have any comments on the series?
>
> Andy Shevchenko (4):
> mfd: lpc_sch: reduce duplicate code
> mfd: lpc_sch: better code manageability with chipset info struct
> mfd: lpc_sch: Add support for Intel Quark X1000
> mfd: lpc_sch: remove FSF address
>
> Josef Ahmad (1):
> pci_ids: add support for Intel Quark ILB
>
> drivers/mfd/lpc_sch.c | 207 ++++++++++++++++++++++++++++--------------------
> include/linux/pci_ids.h | 1 +
> 2 files changed, 122 insertions(+), 86 deletions(-)
>
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support
2014-08-29 9:57 ` [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support Andy Shevchenko
@ 2014-08-29 11:00 ` Lee Jones
2014-09-01 11:40 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2014-08-29 11:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Fri, 29 Aug 2014, Andy Shevchenko wrote:
> On Fri, 2014-08-22 at 13:58 +0300, Andy Shevchenko wrote:
> > This brings support of the LPC SCH device found on Intel Quark SoC. Patches
> > were compile tested, and driver runs on actual hardware.
> >
> > Please, note that PCI ID will be used in future in the gpio-sch driver.
>
> Bjorn, could you Ack or comment patch 3/5?
>
> Lee, do you have any comments on the series?
I have marked them as 'important' and will get round to them when I
can. This week has been hectic. I will get round to it next week.
> > Andy Shevchenko (4):
> > mfd: lpc_sch: reduce duplicate code
> > mfd: lpc_sch: better code manageability with chipset info struct
> > mfd: lpc_sch: Add support for Intel Quark X1000
> > mfd: lpc_sch: remove FSF address
> >
> > Josef Ahmad (1):
> > pci_ids: add support for Intel Quark ILB
> >
> > drivers/mfd/lpc_sch.c | 207 ++++++++++++++++++++++++++++--------------------
> > include/linux/pci_ids.h | 1 +
> > 2 files changed, 122 insertions(+), 86 deletions(-)
> >
>
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 0/5] mfd: lpc_sch: Intel Quark support
2014-08-29 11:00 ` Lee Jones
@ 2014-09-01 11:40 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2014-09-01 11:40 UTC (permalink / raw)
To: Lee Jones
Cc: Bjorn Helgaas, linux-kernel, Samuel Ortiz, Chang Rebecca Swee Fun
On Fri, 2014-08-29 at 12:00 +0100, Lee Jones wrote:
> On Fri, 29 Aug 2014, Andy Shevchenko wrote:
>
> > On Fri, 2014-08-22 at 13:58 +0300, Andy Shevchenko wrote:
> > > This brings support of the LPC SCH device found on Intel Quark SoC. Patches
> > > were compile tested, and driver runs on actual hardware.
> > >
> > > Please, note that PCI ID will be used in future in the gpio-sch driver.
> >
> > Bjorn, could you Ack or comment patch 3/5?
> >
> > Lee, do you have any comments on the series?
>
> I have marked them as 'important' and will get round to them when I
> can. This week has been hectic. I will get round to it next week.
Thanks for your comments. I will fix everything we agreed about and send
v2. For now I'm waiting for Rebecca's comment regarding to squashing
patch 1/5 and 2/5.
>
> > > Andy Shevchenko (4):
> > > mfd: lpc_sch: reduce duplicate code
> > > mfd: lpc_sch: better code manageability with chipset info struct
> > > mfd: lpc_sch: Add support for Intel Quark X1000
> > > mfd: lpc_sch: remove FSF address
> > >
> > > Josef Ahmad (1):
> > > pci_ids: add support for Intel Quark ILB
> > >
> > > drivers/mfd/lpc_sch.c | 207 ++++++++++++++++++++++++++++--------------------
> > > include/linux/pci_ids.h | 1 +
> > > 2 files changed, 122 insertions(+), 86 deletions(-)
> > >
> >
> >
>
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 26+ messages in thread