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

[Lee Jones <lee.jones@linaro.org>]

On Tue, 26 Apr 2016, Tan Jui Nee wrote:

> This driver uses the P2SB hide/unhide mechanism cooperatively
> to pass the PCI BAR address to the gpio platform driver.
> 
> Signed-off-by: Tan Jui Nee <jui.nee.tan@intel.com>
> ---
>  drivers/mfd/Kconfig   |   3 +-
>  drivers/mfd/lpc_ich.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 130 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index eea61e3..54e595c 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -359,8 +359,9 @@ config MFD_INTEL_QUARK_I2C_GPIO
>  
>  config LPC_ICH
>  	tristate "Intel ICH LPC"
> -	depends on PCI
> +	depends on X86 && PCI
>  	select MFD_CORE
> +	select P2SB if X86_INTEL_NON_ACPI
>  	help
>  	  The LPC bridge function of the Intel ICH provides support for
>  	  many functional units. This driver provides needed support for
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index bd3aa45..5d0cc9b 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -68,6 +68,10 @@
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/lpc_ich.h>
>  #include <linux/platform_data/itco_wdt.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/types.h>
> +
> +#include <asm/p2sb.h>
>  
>  #define ACPIBASE		0x40
>  #define ACPIBASE_GPE_OFF	0x28
> @@ -94,6 +98,19 @@
>  #define wdt_mem_res(i) wdt_res(ICH_RES_MEM_OFF, i)
>  #define wdt_res(b, i) (&wdt_ich_res[(b) + (i)])
>  
> +/* Offset data for Apollo Lake GPIO communities */
> +#define APL_GPIO_SOUTHWEST_OFFSET	0xc0
> +#define APL_GPIO_NORTHWEST_OFFSET	0xc4
> +#define APL_GPIO_NORTH_OFFSET		0xc5
> +#define APL_GPIO_WEST_OFFSET		0xc7
> +
> +#define APL_GPIO_SOUTHWEST_END		(43 * 0x8)
> +#define APL_GPIO_NORTHWEST_END		(77 * 0x8)
> +#define APL_GPIO_NORTH_END		(90 * 0x8)
> +#define APL_GPIO_WEST_END		(47 * 0x8)
> +
> +#define APL_GPIO_IRQ 14
> +
>  struct lpc_ich_priv {
>  	int chipset;
>  
> @@ -133,6 +150,50 @@ static struct resource gpio_ich_res[] = {
>  	},
>  };
>  
> +#ifdef CONFIG_X86_INTEL_NON_ACPI

No, thank you.  No unnecessary #ifery.

> +static struct resource apl_gpio_io_res[][2] = {

The "[][2]" is a warning sign to me.

> +	{
> +		{
> +			.start = APL_GPIO_NORTH_OFFSET << 16,
> +			.end = (APL_GPIO_NORTH_OFFSET << 16)
> +				+ APL_GPIO_NORTH_END,

This is a strange (and complicated) way of calculating register
addresses.  Please simplify.  If in doubt, check out how other
drivers/platforms handle it.

> +		},
> +	},
> +	{
> +		{
> +			.start = APL_GPIO_NORTHWEST_OFFSET << 16,
> +			.end = (APL_GPIO_NORTHWEST_OFFSET << 16)
> +				+ APL_GPIO_NORTHWEST_END,
> +		},
> +	},
> +	{
> +		{
> +			.start = APL_GPIO_WEST_OFFSET << 16,
> +			.end = (APL_GPIO_WEST_OFFSET << 16)
> +				+ APL_GPIO_WEST_END,
> +		},
> +	},
> +	{
> +		{
> +			.start = APL_GPIO_SOUTHWEST_OFFSET << 16,
> +			.end = (APL_GPIO_SOUTHWEST_OFFSET << 16)
> +				+ APL_GPIO_SOUTHWEST_END,
> +		},
> +	},
> +};

Use the DEFINE_RES_* defines from include/linux/ioport.h.

> +static struct pinctrl_pin_desc apl_pinctrl_pdata;

No externs.  Have you ran this through checkpatch.pl?

I don't even see this struct.  Where is it?

> +static struct mfd_cell apl_gpio_devices = {
> +	.name = "apl-pinctrl",
> +	.num_resources = ARRAY_SIZE(apl_gpio_io_res),
> +	.resources = apl_gpio_io_res[1],
> +	.pdata_size = sizeof(apl_pinctrl_pdata),
> +	.platform_data = &apl_pinctrl_pdata,
> +	.ignore_resource_conflicts = true,
> +};
> +#endif /* CONFIG_X86_INTEL_NON_ACPI */
> +
>  static struct mfd_cell lpc_ich_wdt_cell = {
>  	.name = "iTCO_wdt",
>  	.num_resources = ARRAY_SIZE(wdt_ich_res),
> @@ -216,6 +277,7 @@ enum lpc_chipsets {
>  	LPC_BRASWELL,	/* Braswell SoC */
>  	LPC_LEWISBURG,	/* Lewisburg */
>  	LPC_9S,		/* 9 Series */
> +	LPC_APL,	/* Apollo Lake SoC */
>  };
>  
>  static struct lpc_ich_info lpc_chipset_info[] = {
> @@ -531,6 +593,10 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  		.name = "9 Series",
>  		.iTCO_version = 2,
>  	},
> +	[LPC_APL]  = {
> +		.name = "Apollo Lake SoC",
> +		.iTCO_version = 5,
> +	},
>  };
>  
>  /*
> @@ -679,6 +745,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
>  	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
>  	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
>  	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
> +	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
>  	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
>  	{ PCI_VDEVICE(INTEL, 0x8c41), LPC_LPT},
>  	{ PCI_VDEVICE(INTEL, 0x8c42), LPC_LPT},
> @@ -1050,6 +1117,64 @@ wdt_done:
>  	return ret;
>  }
>  
> +#ifdef CONFIG_X86_INTEL_NON_ACPI
> +static int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
> +{
> +	unsigned int apl_p2sb = PCI_DEVFN(0x0d, 0);

No magic numbers?  Please define them.

> +	unsigned int i;
> +	int ret;
> +
> +	switch (chipset) {

Why a switch?  This would look better if you:

if (chipset != LPC_APL)
   return -ENODEV;

... until you actually *need* a switch.

> +	case LPC_APL:
> +		/*
> +		 * Apollo lake, has not 1, but 4 gpio controllers,
> +		 * handle it a bit differently.
> +		 */
> +
> +		for (i = 0; i < ARRAY_SIZE(apl_gpio_io_res); i++) {
> +			struct resource *res = apl_gpio_io_res[i];
> +
> +			apl_gpio_devices.resources = res;
> +
> +			/* Fill MEM resource */
> +			ret = p2sb_bar(dev, apl_p2sb, res++);
> +			if (ret)
> +				goto warn_continue;

What does this do?

> +			/* Fill IRQ resource */
> +			res->start = APL_GPIO_IRQ;
> +			res->end = res->start;
> +			res->flags = IORESOURCE_IRQ;
> +
> +			apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u",
> +				i + 1);

All this to call a device "1", "2", etc?

> +			if (apl_pinctrl_pdata.name)
> +				ret = mfd_add_devices(&dev->dev, i,
> +					&apl_gpio_devices, 1, NULL, 0, NULL);

mfd_add_devices() is designed to take a group of devices and register
them all for you.  Calling it once for each separate device you have
is not correct.

> +			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);
> +		}

This code just looks like one big hack to me.

Why don't you just declare the correct amount of resources in
apl_gpio_devices instead of this hodge-podge?

> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +#else
> +static inline int lpc_ich_misc(struct pci_dev *dev, enum lpc_chipsets chipset)
> +{
> +	return -ENODEV;
> +}
> +#endif /* CONFIG_X86_INTEL_NON_ACPI */

Don't do this in drivers.

>  static int lpc_ich_probe(struct pci_dev *dev,
>  				const struct pci_device_id *id)
>  {
> @@ -1093,6 +1218,9 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  			cell_added = true;
>  	}
>  
> +	if (!lpc_ich_misc(dev, priv->chipset))
> +		cell_added = true;
> +
>  	/*
>  	 * We only care if at least one or none of the cells registered
>  	 * successfully.

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