linux-kernel - Re: [RESEND RFC PATCH 5/5] platform: x86: add platform driver for UP Board

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 07 Jul 2016 02:57:56 +0100
From:	Bryan O'Donoghue <pure.logic@...us-software.ie>
To:	Dan O'Donovan <dan@...tex.com>,
	platform-driver-x86@...r.kernel.org, dvhart@...radead.org
Cc:	lee.jones@...aro.org, andriy.shevchenko@...ux.intel.com,
	mika.westerberg@...ux.intel.com, linux-kernel@...r.kernel.org
Subject: Re: [RESEND RFC PATCH 5/5] platform: x86: add platform driver for
 UP Board

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@...tex.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