> -----Original Message-----
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: Tuesday, June 28, 2016 5:15 PM
> To: Andy Shevchenko <andy.shevche...@gmail.com>
> Cc: Tan, Jui Nee <jui.nee....@intel.com>; Mika Westerberg
> <mika.westerb...@linux.intel.com>; Krogerus, Heikki
> <heikki.kroge...@linux.intel.com>; Andy Shevchenko
> <andriy.shevche...@linux.intel.com>; Thomas Gleixner
> <t...@linutronix.de>; Ingo Molnar <mi...@redhat.com>; H. Peter Anvin
> <h...@zytor.com>; x...@kernel.org; pty...@xes-inc.com; Linus Walleij
> <linus.wall...@linaro.org>; linux-g...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Yong, Jonathan <jonathan.y...@intel.com>; Yu,
> Ong Hock <ong.hock...@intel.com>; Voon, Weifeng
> <weifeng.v...@intel.com>; Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.moha...@intel.com>
> Subject: Re: [PATCH v5 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake
> GPIO pinctrl in non-ACPI system
>
> On Tue, 28 Jun 2016, Andy Shevchenko wrote:
>
> > On Tue, Jun 28, 2016 at 10:56 AM, Tan Jui Nee <jui.nee....@intel.com>
> 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.
>
Thanks for your comment. The changes will be applied in next patch-set.
> > > +++ 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?
>
> [...]
>
Applied with Andy's suggestion and #ifery in *.c files will be removed in
next patch-set.
> > > +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.
>
> [...]
>
I would greatly appreciate to have your feedback on that.
I have make minor changes, something like below:
...
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 == NULL) {
ret = -ENOMEM;
goto warn_continue;
}
}
ret = mfd_add_devices(&dev->dev, 0,
apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices),
NULL, 0, NULL);
warn_continue:
...
I have tested this on Intel Apollo Lake platform and it works fine.
> > > --- 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.
>
lpc_ich_misc() function will be moved from .../include/linux/mfd/lpc_ich.h to
.../drivers/mfd/lpc_ich-apl.h.
lpc_ich-apl.h is a newly created file, which is something like this:
#if IS_ENABLED(CONFIG_X86_INTEL_IVI)
int lpc_ich_apl_gpio(struct pci_dev *dev, enum lpc_chipsets chipset);
#else /* CONFIG_X86_INTEL_IVI is not set */
static inline int lpc_ich_apl_gpio(struct pci_dev *dev, enum
lpc_chipsets chipset)
{
return -ENODEV;
}
#endif
Is this acceptable?
> > > +#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?
> >
Rename lpc_ich_misc() to lpc_ich_apl_gpio().
> > Moreover this is a part of internal driver interface, I would move
> > this part into special header under drivers/mfd.
> >
>
Thanks for pointing that out.
The function lpc_ich_misc() will be renamed to lpc_ich_apl_gpio() in next
patch-set.
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
