Re: [PATCH] mfd: dm355evm_msp: Convert LEDs to GPIO descriptor table

[Lee Jones]

On Mon, 27 Jul 2020, Linus Walleij wrote:

> This converts the DaVinci DM355EVM LEDs to use GPIO
> descriptor look-ups.
> 
> Cc: Sekhar Nori <nsekhar@xxxxxx>
> Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/mfd/dm355evm_msp.c | 49 ++++++++++++++++++++++++++++----------
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mfd/dm355evm_msp.c b/drivers/mfd/dm355evm_msp.c
> index 151c36ce7343..af24712d605e 100644
> --- a/drivers/mfd/dm355evm_msp.c
> +++ b/drivers/mfd/dm355evm_msp.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/err.h>
>  #include <linux/gpio.h>
> +#include <linux/gpio/machine.h>
>  #include <linux/leds.h>
>  #include <linux/i2c.h>
>  #include <linux/mfd/dm355evm_msp.h>
> @@ -260,31 +261,55 @@ static int add_children(struct i2c_client *client)
>  
>  	/* LED output */
>  	if (msp_has_leds()) {
> -#define GPIO_LED(l)	.name = l, .active_low = true
>  		static struct gpio_led evm_leds[] = {
> -			{ GPIO_LED("dm355evm::ds14"),
> +			{ .name = "dm355evm::ds14",
>  				.default_trigger = "heartbeat", },
> -			{ GPIO_LED("dm355evm::ds15"),
> +			{ .name = "dm355evm::ds15",
>  				.default_trigger = "mmc0", },
> -			{ GPIO_LED("dm355evm::ds16"),
> +			{ .name = "dm355evm::ds16",
>  				/* could also be a CE-ATA drive */
>  				.default_trigger = "mmc1", },
> -			{ GPIO_LED("dm355evm::ds17"),
> +			{ .name = "dm355evm::ds17",
>  				.default_trigger = "nand-disk", },
> -			{ GPIO_LED("dm355evm::ds18"), },
> -			{ GPIO_LED("dm355evm::ds19"), },
> -			{ GPIO_LED("dm355evm::ds20"), },
> -			{ GPIO_LED("dm355evm::ds21"), },
> +			{ .name = "dm355evm::ds18", },
> +			{ .name = "dm355evm::ds19", },
> +			{ .name = "dm355evm::ds20", },
> +			{ .name = "dm355evm::ds21", },
>  		};
> -#undef GPIO_LED
>  
>  		struct gpio_led_platform_data evm_led_data = {
>  			.num_leds	= ARRAY_SIZE(evm_leds),
>  			.leds		= evm_leds,
>  		};
>  
> -		for (i = 0; i < ARRAY_SIZE(evm_leds); i++)
> -			evm_leds[i].gpio = i + dm355evm_msp_gpio.base;
> +		static struct gpiod_lookup_table evm_leds_gpio_table = {
> +			.dev_id = "leds-gpio",
> +			.table = {
> +				/*
> +				 * These GPIOs are on the dm355evm_msp
> +				 * GPIO chip at index 0..7
> +				 */
> +				GPIO_LOOKUP_IDX("dm355evm_msp", 0, NULL,
> +						0, GPIO_ACTIVE_LOW),
> +				GPIO_LOOKUP_IDX("dm355evm_msp", 1, NULL,
> +						1, GPIO_ACTIVE_LOW),
> +				GPIO_LOOKUP_IDX("dm355evm_msp", 2, NULL,
> +						2, GPIO_ACTIVE_LOW),
> +				GPIO_LOOKUP_IDX("dm355evm_msp", 3, NULL,
> +						3, GPIO_ACTIVE_LOW),
> +				GPIO_LOOKUP_IDX("dm355evm_msp", 4, NULL,
> +						4, GPIO_ACTIVE_LOW),
> +				GPIO_LOOKUP_IDX("dm355evm_msp", 5, NULL,
> +						5, GPIO_ACTIVE_LOW),
> +				GPIO_LOOKUP_IDX("dm355evm_msp", 6, NULL,
> +						6, GPIO_ACTIVE_LOW),
> +				GPIO_LOOKUP_IDX("dm355evm_msp", 7, NULL,
> +						7, GPIO_ACTIVE_LOW),
> +				{ },
> +			},
> +		};

Horrible.

Can this (and the table above) be placed somewhere else and simply
referenced from here?  Tables are usually placed globally at the top
of the file to prevent this kind of formatting craziness from
appearing inside functions.

> +		gpiod_add_lookup_table(&evm_leds_gpio_table);
>  
>  		/* NOTE:  these are the only fully programmable LEDs
>  		 * on the board, since GPIO-61/ds22 (and many signals

-- 
Lee Jones [æçæ]
Senior Technical Lead - Developer Services
Linaro.org â Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog