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

From: Lee Jones
Date: Thu Apr 26 2018 - 03:34:31 EST


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@xxxxxxxxxx>
> ---
> 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