On Thu, 14 Jul 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@xxxxxxxxx> > --- > Changes in V6: > - Rename CONFIG_X86_INTEL_APL to CONFIG_X86_INTEL_IVI so that it > relates to the actual product, as suggested by Mika. > - Rework Makefile according Andy's comments. > - Rename lpc_ich_misc() to lpc_ich_add_gpio() so that the name should not > be so generic, as suggested by Andy. > - Call lpc_ich_add_gpio() via priv->chipset. > - lpc_ich_add_gpio() function will be moved from > .../include/linux/mfd/lpc_ich.h to > .../drivers/mfd/lpc_ich-apl.h > as this is a part of internal driver interface as suggested by Andy. > - Move enum lpc_chipsets from > .../drivers/mfd/lpc_ich-core.c to > .../include/linux/mfd/lpc_ich.h > as lpc_chipsets is also accessed by lpc_ich_add_gpio(). > - Check if kasprintf return value for all 4 gpio controllers before > proceed to add platform device by using mfd_add_devices(). > > Changes in V5: > - Split lpc-ich driver into two parts (lpc_ich-core and lpc_ich-apl). > The file lpc_ich-apl.c introduces gpio platform driver in MFD. > - Rename Kconfig option CONFIG_X86_INTEL_NON_ACPI to CONFIG_X86_INTEL_APL > so that it reflects actual product as suggested by Mika. > > Changes in V4: > - Move Kconfig option CONFIG_X86_INTEL_NON_ACPI from > [PATCH 2/3] x86/platform/p2sb: New Primary to Sideband bridge support driver for Intel SOC's > to > [PATCH 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system > since the config is used in latter patch. > - Select CONFIG_P2SB when CONFIG_LPC_ICH is enabled. > - Remove #ifdef CONFIG_X86_INTEL_NON_ACPI and use > #if defined(CONFIG_X86_INTEL_NON_ACPI) when lpc_ich_misc is called > as suggested by Lee Jones. > - Use single dimensional array instead of 2D array for apl_gpio_io_res > structure and use DEFINE_RES_IRQ for its IRQ resource. > > Changes in V3: > - Simplify register addresses calculation and use DEFINE_RES_MEM_NAMED > defines for apl_gpio_io_res structure > - Define magic number for P2SB PCI ID > - Replace switch-case with if-else since currently we have only one > use case > - Only call mfd_add_devices() once for all gpio communities > > Changes in V2: > - Add new config option CONFIG_X86_INTEL_NON_ACPI and "select PINCTRL" > to fix kbuildbot error > > arch/x86/Kconfig | 9 +++ > drivers/mfd/Kconfig | 3 +- > drivers/mfd/Makefile | 5 +- > drivers/mfd/lpc_ich-apl.c | 130 ++++++++++++++++++++++++++++++++++++++++++++ > drivers/mfd/lpc_ich-apl.h | 28 ++++++++++ > drivers/mfd/lpc_ich-core.c | 81 ++++----------------------- > include/linux/mfd/lpc_ich.h | 73 +++++++++++++++++++++++++ > 7 files changed, 256 insertions(+), 73 deletions(-) > create mode 100644 drivers/mfd/lpc_ich-apl.c > create mode 100644 drivers/mfd/lpc_ich-apl.h > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index d305d81..c0b427b 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -513,6 +513,15 @@ config X86_INTEL_CE > This option compiles in support for the CE4100 SOC for settop > boxes and media devices. > > +config X86_INTEL_IVI > + bool "Intel in-vehicle infotainment (IVI) systems used in cars" > + select PINCTRL > + ---help--- > + Select this option to enable MMIO BAR access over the P2SB for > + non-ACPI Intel Apollo Lake SoC platforms. This driver uses the P2SB > + hide/unhide mechanism cooperatively to pass the PCI BAR address to > + the platform driver, currently GPIO. > + This should be a separate patch. > config X86_INTEL_MID > bool "Intel MID platform support" > depends on X86_EXTENDED_PLATFORM > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 1bcf601..dc4e543 100644 > --- 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 > select MFD_CORE > + select P2SB > 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/Makefile b/drivers/mfd/Makefile > index 1dfe5fb..0aa3e1f 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -155,8 +155,11 @@ 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 > obj-$(CONFIG_LPC_ICH) += lpc_ich.o > +lpc_ich-objs := lpc_ich-core.o > +ifeq ($(CONFIG_X86_INTEL_IVI),y) > +lpc_ich-objs += lpc_ich-apl.o > +endif > obj-$(CONFIG_MFD_RDC321X) += rdc321x-southbridge.o > obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o > obj-$(CONFIG_MFD_JZ4740_ADC) += jz4740-adc.o > diff --git a/drivers/mfd/lpc_ich-apl.c b/drivers/mfd/lpc_ich-apl.c > new file mode 100644 > index 0000000..e4b9688 > --- /dev/null > +++ b/drivers/mfd/lpc_ich-apl.c > @@ -0,0 +1,130 @@ > +/* > + * Purpose: Create a platform device to bind with Intel Apollo Lake Never seen this before. Looks like more of a commit log entry than something that should live in a source file header. > + * Pinctrl GPIO platform driver No need to mention "platform driver" here. Most drivers are. > + * Copyright (C) 2016 Intel Corporation Author? > + * 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/pci.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/lpc_ich.h> > +#include <linux/pinctrl/pinctrl.h> > +#include <asm/p2sb.h> Alphabetical. > +#include "lpc_ich-apl.h" Don't' mix '_'s with '-'s. > +/* Offset data for Apollo Lake GPIO communities */ > +#define APL_GPIO_SOUTHWEST_OFFSET 0xc00000 > +#define APL_GPIO_NORTHWEST_OFFSET 0xc40000 > +#define APL_GPIO_NORTH_OFFSET 0xc50000 > +#define APL_GPIO_WEST_OFFSET 0xc70000 > + > +#define APL_GPIO_SOUTHWEST_NPIN 43 > +#define APL_GPIO_NORTHWEST_NPIN 77 > +#define APL_GPIO_NORTH_NPIN 78 > +#define APL_GPIO_WEST_NPIN 47 > + > +#define APL_GPIO_IRQ 14 > + > +#define PCI_IDSEL_P2SB 0x0d > + > +static struct resource apl_gpio_io_res[] = { > + DEFINE_RES_MEM_NAMED(APL_GPIO_NORTH_OFFSET, > + APL_GPIO_NORTH_NPIN * SZ_8, "apl_pinctrl_n"), > + DEFINE_RES_MEM_NAMED(APL_GPIO_NORTHWEST_OFFSET, > + APL_GPIO_NORTHWEST_NPIN * SZ_8, "apl_pinctrl_nw"), > + DEFINE_RES_MEM_NAMED(APL_GPIO_WEST_OFFSET, > + APL_GPIO_WEST_NPIN * SZ_8, "apl_pinctrl_w"), > + DEFINE_RES_MEM_NAMED(APL_GPIO_SOUTHWEST_OFFSET, > + APL_GPIO_SOUTHWEST_NPIN * SZ_8, "apl_pinctrl_sw"), > + DEFINE_RES_IRQ(APL_GPIO_IRQ), > +}; > + > +static struct pinctrl_pin_desc apl_pinctrl_pdata; This seems pretty pointless. What's it for? > +static struct mfd_cell apl_gpio_devices[] = { > + { > + .name = "apl-pinctrl", > + .id = 0, > + .num_resources = ARRAY_SIZE(apl_gpio_io_res), > + .resources = apl_gpio_io_res, > + .pdata_size = sizeof(apl_pinctrl_pdata), > + .platform_data = &apl_pinctrl_pdata, > + .ignore_resource_conflicts = true, > + }, > + { > + .name = "apl-pinctrl", > + .id = 1, > + .num_resources = ARRAY_SIZE(apl_gpio_io_res), > + .resources = apl_gpio_io_res, > + .pdata_size = sizeof(apl_pinctrl_pdata), > + .platform_data = &apl_pinctrl_pdata, > + .ignore_resource_conflicts = true, > + }, > + { > + .name = "apl-pinctrl", > + .id = 2, > + .num_resources = ARRAY_SIZE(apl_gpio_io_res), > + .resources = apl_gpio_io_res, > + .pdata_size = sizeof(apl_pinctrl_pdata), > + .platform_data = &apl_pinctrl_pdata, > + .ignore_resource_conflicts = true, > + }, > + { > + .name = "apl-pinctrl", > + .id = 3, > + .num_resources = ARRAY_SIZE(apl_gpio_io_res), > + .resources = apl_gpio_io_res, > + .pdata_size = sizeof(apl_pinctrl_pdata), > + .platform_data = &apl_pinctrl_pdata, > + .ignore_resource_conflicts = true, > + }, > +}; > + > +int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset) > +{ > + unsigned int apl_p2sb = PCI_DEVFN(PCI_IDSEL_P2SB, 0); You only use this variable one. Just switch it out for the macro. > + unsigned int i; > + int ret; > + > + if (chipset != LPC_APL) > + return -ENODEV; > + /* > + * 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++) { This is fragile. Just define a GPIO_CONTROLLER_COUNT or similar. > + struct resource *res = &apl_gpio_io_res[i]; > + > + apl_gpio_devices[i].resources = res; What's happening here? Are you manually pulling resources out of the resource structure and linking them to the device cells? Why? > + /* Fill MEM resource */ > + ret = p2sb_bar(dev, apl_p2sb, res++); Why res++? > + if (ret) > + goto warn_continue; > + > + apl_pinctrl_pdata.name = kasprintf(GFP_KERNEL, "%u", > + i + 1); Are you sure this does what you think it does? I think you will keep changing *.name, and the only one you will see/use is the last one. Have you tested this? > + if (apl_pinctrl_pdata.name == NULL) { > + ret = -ENOMEM; > + goto warn_continue; No, this is a failure. You need to 'error' out and return ret. > + } > + } > + > + ret = mfd_add_devices(&dev->dev, 0, > + apl_gpio_devices, ARRAY_SIZE(apl_gpio_devices), > + NULL, 0, NULL); > + > +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); You've just passed the child device a NULL pointer. > + return 0; Why are you returning 0 even during failure? > +} > diff --git a/drivers/mfd/lpc_ich-apl.h b/drivers/mfd/lpc_ich-apl.h > new file mode 100644 > index 0000000..db8325d > --- /dev/null > +++ b/drivers/mfd/lpc_ich-apl.h > @@ -0,0 +1,28 @@ > +/* > + * lpc_ich-apl.h - Intel in-vehicle infotainment (IVI) systems used in cars > + * support The "In", "Vehicle" and "Infotainment" should be capitalised. > + * Copyright (C) 2016, Intel Corporation > + * > + * Authors: Tan, Jui Nee <jui.nee.tan@xxxxxxxxx> "Author" > + * 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. > + */ > + > +#ifndef __LPC_ICH_APL_H__ > +#define __LPC_ICH_APL_H__ > + > +#include <linux/pci.h> > + > +#if IS_ENABLED(CONFIG_X86_INTEL_IVI) > +int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset); > +#else /* CONFIG_X86_INTEL_IVI is not set */ > +static inline int lpc_ich_add_gpio(struct pci_dev *dev, enum lpc_chipsets chipset) > +{ > + return -ENODEV; > +} > +#endif > + > +#endif > diff --git a/drivers/mfd/lpc_ich-core.c b/drivers/mfd/lpc_ich-core.c > index bd3aa45..e09d1e2 100644 > --- a/drivers/mfd/lpc_ich-core.c > +++ b/drivers/mfd/lpc_ich-core.c > @@ -69,6 +69,8 @@ > #include <linux/mfd/lpc_ich.h> > #include <linux/platform_data/itco_wdt.h> > > +#include "lpc_ich-apl.h" > + > #define ACPIBASE 0x40 > #define ACPIBASE_GPE_OFF 0x28 > #define ACPIBASE_GPE_END 0x2f > @@ -147,77 +149,6 @@ static struct mfd_cell lpc_ich_gpio_cell = { > .ignore_resource_conflicts = true, > }; > > -/* chipset related info */ > -enum lpc_chipsets { > - LPC_ICH = 0, /* ICH */ > - LPC_ICH0, /* ICH0 */ > - LPC_ICH2, /* ICH2 */ > - LPC_ICH2M, /* ICH2-M */ > - LPC_ICH3, /* ICH3-S */ > - LPC_ICH3M, /* ICH3-M */ > - LPC_ICH4, /* ICH4 */ > - LPC_ICH4M, /* ICH4-M */ > - LPC_CICH, /* C-ICH */ > - LPC_ICH5, /* ICH5 & ICH5R */ > - LPC_6300ESB, /* 6300ESB */ > - LPC_ICH6, /* ICH6 & ICH6R */ > - LPC_ICH6M, /* ICH6-M */ > - LPC_ICH6W, /* ICH6W & ICH6RW */ > - LPC_631XESB, /* 631xESB/632xESB */ > - LPC_ICH7, /* ICH7 & ICH7R */ > - LPC_ICH7DH, /* ICH7DH */ > - LPC_ICH7M, /* ICH7-M & ICH7-U */ > - LPC_ICH7MDH, /* ICH7-M DH */ > - LPC_NM10, /* NM10 */ > - LPC_ICH8, /* ICH8 & ICH8R */ > - LPC_ICH8DH, /* ICH8DH */ > - LPC_ICH8DO, /* ICH8DO */ > - LPC_ICH8M, /* ICH8M */ > - LPC_ICH8ME, /* ICH8M-E */ > - LPC_ICH9, /* ICH9 */ > - LPC_ICH9R, /* ICH9R */ > - LPC_ICH9DH, /* ICH9DH */ > - LPC_ICH9DO, /* ICH9DO */ > - LPC_ICH9M, /* ICH9M */ > - LPC_ICH9ME, /* ICH9M-E */ > - LPC_ICH10, /* ICH10 */ > - LPC_ICH10R, /* ICH10R */ > - LPC_ICH10D, /* ICH10D */ > - LPC_ICH10DO, /* ICH10DO */ > - LPC_PCH, /* PCH Desktop Full Featured */ > - LPC_PCHM, /* PCH Mobile Full Featured */ > - LPC_P55, /* P55 */ > - LPC_PM55, /* PM55 */ > - LPC_H55, /* H55 */ > - LPC_QM57, /* QM57 */ > - LPC_H57, /* H57 */ > - LPC_HM55, /* HM55 */ > - LPC_Q57, /* Q57 */ > - LPC_HM57, /* HM57 */ > - LPC_PCHMSFF, /* PCH Mobile SFF Full Featured */ > - LPC_QS57, /* QS57 */ > - LPC_3400, /* 3400 */ > - LPC_3420, /* 3420 */ > - LPC_3450, /* 3450 */ > - LPC_EP80579, /* EP80579 */ > - LPC_CPT, /* Cougar Point */ > - LPC_CPTD, /* Cougar Point Desktop */ > - LPC_CPTM, /* Cougar Point Mobile */ > - LPC_PBG, /* Patsburg */ > - LPC_DH89XXCC, /* DH89xxCC */ > - LPC_PPT, /* Panther Point */ > - LPC_LPT, /* Lynx Point */ > - LPC_LPT_LP, /* Lynx Point-LP */ > - LPC_WBG, /* Wellsburg */ > - LPC_AVN, /* Avoton SoC */ > - LPC_BAYTRAIL, /* Bay Trail SoC */ > - LPC_COLETO, /* Coleto Creek */ > - LPC_WPT_LP, /* Wildcat Point-LP */ > - LPC_BRASWELL, /* Braswell SoC */ > - LPC_LEWISBURG, /* Lewisburg */ > - LPC_9S, /* 9 Series */ > -}; What does this move have to do with this patch? I think it should be separate and be paired with a reason for the change. > static struct lpc_ich_info lpc_chipset_info[] = { > [LPC_ICH] = { > .name = "ICH", > @@ -531,6 +462,10 @@ static struct lpc_ich_info lpc_chipset_info[] = { > .name = "9 Series", > .iTCO_version = 2, > }, > + [LPC_APL] = { > + .name = "Apollo Lake SoC", > + .iTCO_version = 5, > + }, Enabling a new platform should be a separate patch. > }; > > /* > @@ -679,6 +614,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}, > @@ -1093,6 +1029,9 @@ static int lpc_ich_probe(struct pci_dev *dev, > cell_added = true; > } > > + if (!lpc_ich_add_gpio(dev, priv->chipset)) > + cell_added = true; > + > /* > * We only care if at least one or none of the cells registered > * successfully. > diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h > index 2b300b4..03c2ca3 100644 > --- a/include/linux/mfd/lpc_ich.h > +++ b/include/linux/mfd/lpc_ich.h > @@ -34,6 +34,7 @@ enum { > ICH_V10CORP_GPIO, > ICH_V10CONS_GPIO, > AVOTON_GPIO, > + APL_GPIO, > }; > > struct lpc_ich_info { > @@ -43,4 +44,76 @@ struct lpc_ich_info { > u8 use_gpio; > }; > > +/* chipset related info */ > +enum lpc_chipsets { > + LPC_ICH = 0, /* ICH */ > + LPC_ICH0, /* ICH0 */ > + LPC_ICH2, /* ICH2 */ > + LPC_ICH2M, /* ICH2-M */ > + LPC_ICH3, /* ICH3-S */ > + LPC_ICH3M, /* ICH3-M */ > + LPC_ICH4, /* ICH4 */ > + LPC_ICH4M, /* ICH4-M */ > + LPC_CICH, /* C-ICH */ > + LPC_ICH5, /* ICH5 & ICH5R */ > + LPC_6300ESB, /* 6300ESB */ > + LPC_ICH6, /* ICH6 & ICH6R */ > + LPC_ICH6M, /* ICH6-M */ > + LPC_ICH6W, /* ICH6W & ICH6RW */ > + LPC_631XESB, /* 631xESB/632xESB */ > + LPC_ICH7, /* ICH7 & ICH7R */ > + LPC_ICH7DH, /* ICH7DH */ > + LPC_ICH7M, /* ICH7-M & ICH7-U */ > + LPC_ICH7MDH, /* ICH7-M DH */ > + LPC_NM10, /* NM10 */ > + LPC_ICH8, /* ICH8 & ICH8R */ > + LPC_ICH8DH, /* ICH8DH */ > + LPC_ICH8DO, /* ICH8DO */ > + LPC_ICH8M, /* ICH8M */ > + LPC_ICH8ME, /* ICH8M-E */ > + LPC_ICH9, /* ICH9 */ > + LPC_ICH9R, /* ICH9R */ > + LPC_ICH9DH, /* ICH9DH */ > + LPC_ICH9DO, /* ICH9DO */ > + LPC_ICH9M, /* ICH9M */ > + LPC_ICH9ME, /* ICH9M-E */ > + LPC_ICH10, /* ICH10 */ > + LPC_ICH10R, /* ICH10R */ > + LPC_ICH10D, /* ICH10D */ > + LPC_ICH10DO, /* ICH10DO */ > + LPC_PCH, /* PCH Desktop Full Featured */ > + LPC_PCHM, /* PCH Mobile Full Featured */ > + LPC_P55, /* P55 */ > + LPC_PM55, /* PM55 */ > + LPC_H55, /* H55 */ > + LPC_QM57, /* QM57 */ > + LPC_H57, /* H57 */ > + LPC_HM55, /* HM55 */ > + LPC_Q57, /* Q57 */ > + LPC_HM57, /* HM57 */ > + LPC_PCHMSFF, /* PCH Mobile SFF Full Featured */ > + LPC_QS57, /* QS57 */ > + LPC_3400, /* 3400 */ > + LPC_3420, /* 3420 */ > + LPC_3450, /* 3450 */ > + LPC_EP80579, /* EP80579 */ > + LPC_CPT, /* Cougar Point */ > + LPC_CPTD, /* Cougar Point Desktop */ > + LPC_CPTM, /* Cougar Point Mobile */ > + LPC_PBG, /* Patsburg */ > + LPC_DH89XXCC, /* DH89xxCC */ > + LPC_PPT, /* Panther Point */ > + LPC_LPT, /* Lynx Point */ > + LPC_LPT_LP, /* Lynx Point-LP */ > + LPC_WBG, /* Wellsburg */ > + LPC_AVN, /* Avoton SoC */ > + LPC_BAYTRAIL, /* Bay Trail SoC */ > + LPC_COLETO, /* Coleto Creek */ > + LPC_WPT_LP, /* Wildcat Point-LP */ > + LPC_BRASWELL, /* Braswell SoC */ > + LPC_LEWISBURG, /* Lewisburg */ > + LPC_9S, /* 9 Series */ > + LPC_APL, /* Apollo Lake SoC */ > +}; > + > #endif -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html