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

[Tan, Jui Nee]

> -----Original Message-----
> From: Lee Jones [mailto:lee.jones@xxxxxxxxxx]
> Sent: Tuesday, June 28, 2016 5:15 PM
> To: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Cc: Tan, Jui Nee <jui.nee.tan@xxxxxxxxx>; Mika Westerberg
> <mika.westerberg@xxxxxxxxxxxxxxx>; Krogerus, Heikki
> <heikki.krogerus@xxxxxxxxxxxxxxx>; Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx>; Thomas Gleixner
> <tglx@xxxxxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>; H. Peter Anvin
> <hpa@xxxxxxxxx>; x86@xxxxxxxxxx; ptyser@xxxxxxxxxxx; Linus Walleij
> <linus.walleij@xxxxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Yong, Jonathan <jonathan.yong@xxxxxxxxx>; Yu,
> Ong Hock <ong.hock.yu@xxxxxxxxx>; Voon, Weifeng
> <weifeng.voon@xxxxxxxxx>; Wan Mohamad, Wan Ahmad Zainie
> <wan.ahmad.zainie.wan.mohamad@xxxxxxxxx>
> 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.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.
> 
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