From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752316AbcF1JOl (ORCPT ); Tue, 28 Jun 2016 05:14:41 -0400 Received: from mail-wm0-f41.google.com ([74.125.82.41]:38333 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752276AbcF1JOh (ORCPT ); Tue, 28 Jun 2016 05:14:37 -0400 Date: Tue, 28 Jun 2016 10:15:27 +0100 From: Lee Jones To: Andy Shevchenko Cc: Tan Jui Nee , Mika Westerberg , "Krogerus, Heikki" , Andy Shevchenko , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , ptyser@xes-inc.com, Linus Walleij , "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" , jonathan.yong@intel.com, ong.hock.yu@intel.com, weifeng.voon@intel.com, wan.ahmad.zainie.wan.mohamad@intel.com Subject: Re: [PATCH v5 3/3] mfd: lpc_ich: Add support for Intel Apollo Lake GPIO pinctrl in non-ACPI system Message-ID: <20160628091527.GO6720@dell> References: <1467100608-11549-1-git-send-email-jui.nee.tan@intel.com> <1467100608-11549-4-git-send-email-jui.nee.tan@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 28 Jun 2016, Andy Shevchenko wrote: > On Tue, Jun 28, 2016 at 10:56 AM, Tan Jui Nee 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. > > +++ 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 > > +#include > > +#include > > +#include > > + > > > +#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? [...] > > +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. [...] > > --- 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. > > +#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? > > Moreover this is a part of internal driver interface, I would move > this part into special header under drivers/mfd. > -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog