From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756092AbcGGB6D (ORCPT ); Wed, 6 Jul 2016 21:58:03 -0400 Received: from mail-wm0-f42.google.com ([74.125.82.42]:37604 "EHLO mail-wm0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751635AbcGGB6A (ORCPT ); Wed, 6 Jul 2016 21:58:00 -0400 Message-ID: <1467856676.17253.126.camel@nexus-software.ie> Subject: Re: [RESEND RFC PATCH 5/5] platform: x86: add platform driver for UP Board From: "Bryan O'Donoghue" To: "Dan O'Donovan" , platform-driver-x86@vger.kernel.org, dvhart@infradead.org Cc: lee.jones@linaro.org, andriy.shevchenko@linux.intel.com, mika.westerberg@linux.intel.com, linux-kernel@vger.kernel.org Date: Thu, 07 Jul 2016 02:57:56 +0100 In-Reply-To: <1467648434-29080-6-git-send-email-dan@emutex.com> References: <1465762392-9205-1-git-send-email-dan@emutex.com> <1467648434-29080-1-git-send-email-dan@emutex.com> <1467648434-29080-6-git-send-email-dan@emutex.com> Organization: Nexus Software Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2-0ubuntu1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2016-07-04 at 17:07 +0100, Dan O'Donovan wrote: > This platform driver instantiates a platform device relevant to the > UP board, in particular a device representing the unique I/O pin CPLD > controller on the UP board. > > In addition, this driver registers pin maps to configure > appropriately the underlying SoC GPIO pins for use with the > UP Board I/O pin header. > > Signed-off-by: Dan O'Donovan > --- >  drivers/platform/x86/Kconfig    |  13 ++++ >  drivers/platform/x86/Makefile   |   5 ++ >  drivers/platform/x86/up_board.c | 167 > ++++++++++++++++++++++++++++++++++++++++ >  3 files changed, 185 insertions(+) >  create mode 100644 drivers/platform/x86/up_board.c > > diff --git a/drivers/platform/x86/Kconfig > b/drivers/platform/x86/Kconfig > index 3ec0025..b579adb 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -1011,4 +1011,17 @@ config INTEL_TELEMETRY >     used to get various SoC events and parameters >     directly via debugfs files. Various tools may use >     this interface for SoC state monitoring. > + > +config UP_BOARD > + bool "UP Board Platform I/O Driver" Addressing your question in the cover letter - I'm not sure where up_board.c should go but, I reckon up_board_leds.c should go into drivers/leds, up_board_gpio.c should go into drivers/gpio etc. My highly scientific feeling is that up_board.c and up_board_cpld.c shouldn't be in this directory but, that they would fit well in drivers/mfd. > + depends on ACPI && PINCTRL_CHERRYVIEW > + select GPIOLIB_IRQCHIP > + select LEDS_CLASS > + select NEW_LEDS > + ---help--- > +   This driver provides support for the platform functions on > the UP > +   board.  It includes platform, pinctrl and gpio drivers for > the CPLD > +   that manages the external pin header, as well as a driver > for the > +   built-in LEDs. > + >  endif # X86_PLATFORM_DEVICES > diff --git a/drivers/platform/x86/Makefile > b/drivers/platform/x86/Makefile > index 9b11b40..687c583 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -70,3 +70,8 @@ obj-$(CONFIG_INTEL_TELEMETRY) += > intel_telemetry_core.o \ >      intel_telemetry_pltdrv.o \ >      intel_telemetry_debugfs.o >  obj-$(CONFIG_INTEL_PMC_CORE)    += intel_pmc_core.o > +obj-$(CONFIG_UP_BOARD) += up_board.o \ > +    up_board_cpld.o \ > +    up_board_pinctrl.o \ > +    up_board_gpio.o \ > +    up_board_leds.o > diff --git a/drivers/platform/x86/up_board.c > b/drivers/platform/x86/up_board.c > new file mode 100644 > index 0000000..8635759 > --- /dev/null > +++ b/drivers/platform/x86/up_board.c > @@ -0,0 +1,167 @@ > + Dangling extra line. > + > +/* > + * On the UP board, if the ODEn bit Do you mean Open Drain Enable bit ? I think this comment will parse better if its called out explicitly. > is set on the pad configuration > + * it seems to impair some functions on the I/O header such as UART, > SPI > + * and I2C.  So we disable it for all header pins by default. > + */ Seems to impair is a bit vague... what does impair mean - not working or just wrong ? > +static struct up_board_info *up_board; > + > +static int __init > +up_board_init_devices(void) Why are you breaking up the __init and the function name onto separate lines ? > +{ > + const struct dmi_system_id *system_id; > + int ret; > + > + system_id = dmi_first_match(up_board_id_table); > + if (!system_id) > + return -ENXIO; How about -ENODEV > + > + > + up_board->vreg_pdev = > + regulator_register_always_on(0, "fixed-3.3V", > +      vref3v3_consumers, > +      ARRAY_SIZE(vref3v3_cons > umers), > +      3300000); > + if (!up_board->vreg_pdev) { > + pr_err("Failed to register UP Board ADC vref > regulator"); dev_err(&up_board->cpld_pdev.dev) ? > + platform_device_unregister(up_board->cpld_pdev); > + return -ENODEV; > + } > + > + return 0; > +} > + > +static void __exit > +up_board_exit(void) Same comment on multiple line declarations. --- bod