linux-kernel - Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

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, 26 Apr 2018 08:34:05 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Javier Arteaga <javier@...tex.com>
Cc:     Jacek Anaszewski <jacek.anaszewski@...il.com>,
        Pavel Machek <pavel@....cz>, Dan O'Donovan <dan@...tex.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-gpio@...r.kernel.org, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

On Sat, 21 Apr 2018, Javier Arteaga wrote:

> Allow userspace to use the on-board LEDs as "upboard:<color>:".
> 
> Signed-off-by: Javier Arteaga <javier@...tex.com>
> ---
>  drivers/leds/Kconfig        | 10 +++++
>  drivers/leds/Makefile       |  1 +
>  drivers/leds/leds-upboard.c | 87 +++++++++++++++++++++++++++++++++++++

>  drivers/mfd/upboard.c       | 19 ++++++++

This change needs to be placed into a separate patch.

>  include/linux/mfd/upboard.h |  5 +++
>  5 files changed, 122 insertions(+)
>  create mode 100644 drivers/leds/leds-upboard.c

[...]

> diff --git a/drivers/mfd/upboard.c b/drivers/mfd/upboard.c
> index 8bae450cb83d..6e4767e4dc41 100644
> --- a/drivers/mfd/upboard.c
> +++ b/drivers/mfd/upboard.c
> @@ -79,6 +79,14 @@ struct upboard_data {
>  	size_t ncells;
>  };
>  
> +#define UPBOARD_LED_CELL(led_data, n)                   \
> +	{                                               \
> +		.name = "upboard-led",                  \
> +		.id = (n),                              \
> +		.platform_data = &led_data[(n)],        \
> +		.pdata_size = sizeof(*(led_data)),      \
> +	}
> +

There is a subsystem-level MACRO currently being reviewed on the list.

Just use the full format in your structs for now.

>  /* UP Squared */
>  
>  static const struct regmap_range upboard_up2_readable_ranges[] = {
> @@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = {
>  	.wr_table = &upboard_up2_writable_table,
>  };
>  
> +static struct upboard_led_data upboard_up2_led_data[] = {
> +	{ .id = 0, .color = "blue" },
> +	{ .id = 1, .color = "yellow" },
> +	{ .id = 2, .color = "green" },
> +	{ .id = 3, .color = "red" },
> +};

How is this data used?

Does it ever change, from board to board?

>  static const struct mfd_cell upboard_up2_mfd_cells[] = {
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 0),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 1),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 2),
> +	UPBOARD_LED_CELL(upboard_up2_led_data, 3),
>  };
>  
>  static const struct upboard_data upboard_up2_data = {
> diff --git a/include/linux/mfd/upboard.h b/include/linux/mfd/upboard.h
> index d9dd214f4d29..eed68caa23ce 100644
> --- a/include/linux/mfd/upboard.h
> +++ b/include/linux/mfd/upboard.h
> @@ -62,4 +62,9 @@ struct upboard {
>  	struct gpio_desc *dataout_gpio;
>  };
>  
> +struct upboard_led_data {
> +	unsigned int id;
> +	const char *color;
> +};

If this is going to stick around, which I'm not sure it should, you
need to document it (using kerneldoc format), since 'id' is quite
ambiguous.

>  #endif /*  __LINUX_MFD_UPBOARD_H */

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog