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@xxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
