Re: [PATCH v5 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system

[Lee Jones]

On Tue, 28 Jun 2016, Andy Shevchenko wrote:

> On Tue, Jun 28, 2016 at 10:56 AM, Tan Jui Nee <jui.nee.tan@xxxxxxxxx> wrote:
> > This driver uses the P2SB hide/unhide mechanism cooperatively
> > to pass the PCI BAR address to the gpio platform driver.
> 
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -369,8 +369,9 @@ config MFD_INTEL_QUARK_I2C_GPIO
> >
> >  config LPC_ICH
> >         tristate "Intel ICH LPC"
> > -       depends on PCI
> > +       depends on X86 && PCI
> 
> This might deserve to be a separate change. Lee, what is your opinion?

I have no strong opinion.  Keep it in if you like.

[...]

> > @@ -155,7 +155,7 @@ obj-$(CONFIG_PMIC_ADP5520)  += adp5520.o
> >  obj-$(CONFIG_MFD_KEMPLD)       += kempld-core.o
> >  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
> >  obj-$(CONFIG_LPC_SCH)          += lpc_sch.o
> 
> > -lpc_ich-objs           := lpc_ich-core.o
> > +lpc_ich-objs           := lpc_ich-core.o lpc_ich-apl.o
> >  obj-$(CONFIG_LPC_ICH)          += lpc_ich.o
> 
> I don't know if there is any requirement, but what I see in other Makefiles is
> 
> obj-$(CONFIG_DRV_OPTION) = drv_name.o
> drv_name-objs := main.o aux1.o

Yes, this is nicer.

> Perhaps you may do the same.

> > +++ b/drivers/mfd/lpc_ich-apl.c
> > @@ -0,0 +1,126 @@
> > +/*
> > + * Purpose: Create a platform device to bind with Intel Apollo Lake
> > + * Pinctrl GPIO platform driver
> > + * Copyright (C) 2016 Intel Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/lpc_ich.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <asm/p2sb.h>
> > +
> 
> > +#if defined(CONFIG_X86_INTEL_APL)

I'm not accepting this kind of #ifery in *.c files.

> And why you can't use
> 
> ifeq ($(CONFIG_...,y))
> lpc_ich-objs += ...
> endif
> 
> in the Makefile?

[...]

> > +int lpc_ich_misc(struct pci_dev *dev)
> > +{
> > +       unsigned int apl_p2sb = PCI_DEVFN(PCI_IDSEL_P2SB, 0);
> > +       unsigned int i;
> > +       int ret;
> > +
> > +       /*
> > +        * Apollo lake, has not 1, but 4 gpio controllers,
> > +        * handle it a bit differently.
> > +        */
> > +
> > +       for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res)-1; i++) {
> > +               struct resource *res = &apl_gpio_io_res[i];
> > +
> > +               apl_gpio_devices[i].resources = res;
> > +
> > +               /* Fill MEM resource */
> > +               ret = p2sb_bar(dev, apl_p2sb, res++);
> > +               if (ret)
> > +                       goto warn_continue;
> > +
> > +               apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
> > +                       i + 1);
> > +       }
> > +
> > +       if (apl_pinctrl_pdata.name)
> > +               ret = mfd_add_devices(&dev->dev, apl_gpio_devices->id,
> > +                       apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
> > +                               NULL, 0, NULL);
> > +       else
> > +               ret = -ENOMEM;
> > +
> > +warn_continue:
> > +       if (ret)
> > +               dev_warn(&dev->dev,
> > +                       "Failed to add Apollo Lake GPIO %s: %d\n",
> > +                               apl_pinctrl_pdata.name, ret);
> > +
> > +       kfree(apl_pinctrl_pdata.name);
> > +       return 0;
> > +}
> > +#endif
> 
> Regarding to the above I don't see Lee's comments addressed. Can you
> remind whar is the result of that discussion?

I don't have time to do a full review now, but this does look nasty.

[...]

> > --- a/include/linux/mfd/lpc_ich.h
> > +++ b/include/linux/mfd/lpc_ich.h
> > @@ -43,4 +43,11 @@ struct lpc_ich_info {
> >         u8 use_gpio;
> >  };
> 
> > +#if defined(CONFIG_X86_INTEL_APL)
> > +int lpc_ich_misc(struct pci_dev *dev);
> > +#else
> > +static inline int lpc_ich_misc(struct pci_dev *dev)
> > +{ return -ENODEV; }
> 
> It could be one line, ...

I don't want it in here at all.

> > +#endif
> 
> ...but the name of this function should not be so generic. What if
> tomorrow you will get one more module which needs one more function?
> 
> Moreover this is a part of internal driver interface, I would move
> this part into special header under drivers/mfd.
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog