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 <[email protected]>
> > Tested-by: Chang Rebecca Swee Fun <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
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 <[email protected]>
Intel Finland Oy
--
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/
