From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752578AbcI1Jvf (ORCPT ); Wed, 28 Sep 2016 05:51:35 -0400 Received: from mga04.intel.com ([192.55.52.120]:46752 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752389AbcI1JvZ (ORCPT ); Wed, 28 Sep 2016 05:51:25 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,409,1470726000"; d="scan'208";a="174060031" From: "Tan, Jui Nee" To: "'Lee Jones'" CC: "mika.westerberg@linux.intel.com" , "heikki.krogerus@linux.intel.com" , "andriy.shevchenko@linux.intel.com" , "tglx@linutronix.de" , "mingo@redhat.com" , "hpa@zytor.com" , "x86@kernel.org" , "ptyser@xes-inc.com" , "linus.walleij@linaro.org" , "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Yong, Jonathan" , "Yu, Ong Hock" , "Wan Mohamad, Wan Ahmad Zainie" , "Luck, Tony" Subject: RE: [PATCH v6 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system Thread-Topic: [PATCH v6 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system Thread-Index: AQHR3aeANiRLQsTTME+LC5/IYUQgDKA/2liAgAIpwjA= Date: Wed, 28 Sep 2016 09:51:19 +0000 Message-ID: <0158A29DB680F54A88142ED28D55B1D0082F5E69@PGSMSX107.gar.corp.intel.com> References: <1468483919-31258-1-git-send-email-jui.nee.tan@intel.com> <1468483919-31258-4-git-send-email-jui.nee.tan@intel.com> <20160809071601.GT5243@dell> In-Reply-To: <20160809071601.GT5243@dell> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2MzYjgxOTItZTFhYy00NzBlLWFmZTQtNDJiMGEwYzk0MWI1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlczVjZnQ2ZlSGFiTVBSMnJTVHBYNDJPQXRWNUlland0d0ROYW9IUFFNNHM9In0= x-originating-ip: [172.30.20.206] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u8S9pdSh014236 > -----Original Message----- > From: Lee Jones [mailto:lee.jones@linaro.org] > Sent: Tuesday, August 9, 2016 3:16 PM > To: Tan, Jui Nee > Cc: mika.westerberg@linux.intel.com; heikki.krogerus@linux.intel.com; > andriy.shevchenko@linux.intel.com; tglx@linutronix.de; > mingo@redhat.com; hpa@zytor.com; x86@kernel.org; ptyser@xes-inc.com; > linus.walleij@linaro.org; linux-gpio@vger.kernel.org; linux- > kernel@vger.kernel.org; Yong, Jonathan ; Yu, > Ong Hock ; Voon, Weifeng > ; Wan Mohamad, Wan Ahmad Zainie > > Subject: Re: [PATCH v6 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake > GPIO pinctrl in non-ACPI system > > 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 > > --- > > 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. > Thanks for your comment. The changes will be applied in next patch-set. > > 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. > Okay, the changes will be applied in next patch-set. > > + * Copyright (C) 2016 Intel Corporation > > Author? > I will add the Author information in next patch-set. > > + * 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 > > +#include > > +#include > > +#include > > +#include > > Alphabetical. > Thanks for pointing that out. > > +#include "lpc_ich-apl.h" > > Don't' mix '_'s with '-'s. > The header file will be renamed as lpc_ich_apl.h. > > +/* 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? > Since we do not need to access platform data at here, I have removed the struct, .pdata_size and .platform_data (from below mfd_cell). > > +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. > Noted. The changes will be applied in next patch-set. > > + 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. > Okay. > > + 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? > There are total 4 GPIO controllers / communities and each of them has different MMIO offset addresses. I have removed following line apl_gpio_devices[i].resources = res; in my next patch-set and make modification at device cell, something like below: 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[0], .ignore_resource_conflicts = true, }, ... > > + /* Fill MEM resource */ > > + ret = p2sb_bar(dev, apl_p2sb, res++); > > Why res++? > I have simplified it so that no need to call p2sb_bar() function four times inside the for loop. And make p2sb_bar() function just to fill in the base address into a scratch "struct resource" and have the loop do the additions to base/end. Something like: struct resource base; ret = p2sb_bar(dev, PCI_DEVFN(PCI_IDSEL_P2SB, 0), &base); for (i = 0; i < ARRAY_SIZE(GPIO_CONTROLLER_COUNT); i++) { struct resource *res = apl_gpio_io_res[i]; apl_gpio_devices[i].resources = res; /* Fill MEM resource */ res->start += base.start; res->end += base.start; res->flags = base.flags; 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. > The entire apl_pinctrl_pdata.name memory allocation is no longer needed here and tested working fine after remove. > > + } > > + } > > + > > + 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? > It should be return ret. > > +} > > 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. > Noted. The changes will be applied in next patch-set. > > + * Copyright (C) 2016, Intel Corporation > > + * > > + * Authors: Tan, Jui Nee > > "Author" > Thanks for pointing that out. The changes will be applied in next patch-set. > > + * 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 > > + > > +#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 > > #include > > > > +#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. > Noted. I will place that in separate patch. > > 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. > Okay. I will place that in 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