lkml.org 
[lkml]   [2016]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RESEND RFC PATCH 5/5] platform: x86: add platform driver for UP Board
From
Date
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 <dan@emutex.com>
> ---
>  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

\
 
 \ /
  Last update: 2016-07-07 04:41    [W:0.074 / U:1.812 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site