Re: [PATCH v2 4/4] led-trigger: rework

[Sascha Hauer <s.hauer@pengutronix.de>]

On Sun, Mar 12, 2017 at 09:19:24AM +0100, Oleksij Rempel wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  commands/trigger.c         |  54 +++++++++++---------
>  drivers/led/led-triggers.c | 125 +++++++++++++++++++++++++++++++++++----------
>  include/led.h              |   6 ++-
>  3 files changed, 131 insertions(+), 54 deletions(-)

Sascha, please write a proper commit message :)

Ok, here we go:

led-trigger: Allow multiple led triggers of the same type

We used to have a static array of trigger types which, allows only one led
per trigger. While this is enough for panic and heartbeat, it falls
short when multiple leds are associated to the default-on trigger. The
default-on trigger is used to turn on a led statically from devicetree
and may be used multiple times.
This patch reworks the led triggers so that a trigger struct is
allocated dynamically when needed and put onto a list.

Sascha

> 
> diff --git a/commands/trigger.c b/commands/trigger.c
> index 2758ce74e..0dd3b346f 100644
> --- a/commands/trigger.c
> +++ b/commands/trigger.c
> @@ -28,59 +28,62 @@
>  #define LED_COMMAND_SHOW_INFO		2
>  #define	LED_COMMAND_DISABLE_TRIGGER	3
>  
> -static char *trigger_names[] = {
> -	[LED_TRIGGER_PANIC] = "panic",
> -	[LED_TRIGGER_HEARTBEAT] = "heartbeat",
> -	[LED_TRIGGER_NET_RX] = "net rx",
> -	[LED_TRIGGER_NET_TX] = "net tx",
> -	[LED_TRIGGER_NET_TXRX] = "net",
> -	[LED_TRIGGER_DEFAULT_ON] = "default on",
> -};
>  
>  static int do_trigger(int argc, char *argv[])
>  {
> -	struct led *led;
> -	int i, opt, ret = 0;
> +	struct led *led = NULL;
> +	int opt, ret = 0;
>  	int cmd = LED_COMMAND_SHOW_INFO;
> -	unsigned long trigger = 0;
> +	enum led_trigger trigger;
> +	const char *led_name = NULL;
> +	const char *trigger_name = NULL;
>  
>  	while((opt = getopt(argc, argv, "t:d:")) > 0) {
>  		switch(opt) {
>  		case 't':
> -			trigger = simple_strtoul(optarg, NULL, 0);
> +			trigger_name = optarg;
>  			cmd = LED_COMMAND_SET_TRIGGER;
>  			break;
>  		case 'd':
> -			trigger = simple_strtoul(optarg, NULL, 0);
> +			led_name = optarg;
>  			cmd = LED_COMMAND_DISABLE_TRIGGER;
>  		}
>  	}
>  
> +	if (optind < argc)
> +		led = led_by_name_or_number(argv[optind]);
> +
>  	switch (cmd) {
>  	case LED_COMMAND_SHOW_INFO:
> -		for (i = 0; i < LED_TRIGGER_MAX; i++) {
> -			int led = led_get_trigger(i);
> -			printf("%d: %s", i, trigger_names[i]);
> -			if (led >= 0)
> -				printf(" (led %d)", led);
> -			printf("\n");
> -		}
> +		led_triggers_show_info();
>  		break;
>  
>  	case LED_COMMAND_DISABLE_TRIGGER:
> -		ret = led_set_trigger(trigger, NULL);
> +		led = led_by_name_or_number(led_name);
> +		if (!led) {
> +			printf("no such led: %s\n", led_name);
> +			return 1;
> +		}
> +
> +		led_trigger_disable(led);
>  		break;
>  
>  	case LED_COMMAND_SET_TRIGGER:
>  		if (argc - optind != 1)
>  			return COMMAND_ERROR_USAGE;
> -		led = led_by_name_or_number(argv[optind]);
>  
> +		led = led_by_name_or_number(argv[optind]);
>  		if (!led) {
>  			printf("no such led: %s\n", argv[optind]);
>  			return 1;
>  		}
>  
> +		trigger = trigger_by_name(trigger_name);
> +		if (trigger == LED_TRIGGER_INVALID) {
> +			printf("no such trigger: %s\n", trigger_name);
> +			return 1;
> +		}
> +
>  		ret = led_set_trigger(trigger, led);
>  		break;
>  	}
> @@ -92,16 +95,17 @@ static int do_trigger(int argc, char *argv[])
>  
>  BAREBOX_CMD_HELP_START(trigger)
>  BAREBOX_CMD_HELP_TEXT("Control a LED trigger. Without options assigned triggers are shown.")
> +BAREBOX_CMD_HELP_TEXT("triggers are given with their name, LEDs are given with their name or number")
>  BAREBOX_CMD_HELP_TEXT("")
>  BAREBOX_CMD_HELP_TEXT("Options:")
> -BAREBOX_CMD_HELP_OPT ("-t",  "set a trigger (needs LED argument)")
> -BAREBOX_CMD_HELP_OPT ("-d",  "disable a trigger")
> +BAREBOX_CMD_HELP_OPT ("-t <trigger> <led>",  "set a trigger")
> +BAREBOX_CMD_HELP_OPT ("-d <led>",  "disable a trigger")
>  BAREBOX_CMD_HELP_END
>  
>  BAREBOX_CMD_START(trigger)
>  	.cmd		= do_trigger,
>  	BAREBOX_CMD_DESC("handle LED triggers")
> -	BAREBOX_CMD_OPTS("[-td] TRIGGER [LED]")
> +	BAREBOX_CMD_OPTS("[-td] [LED]")
>  	BAREBOX_CMD_GROUP(CMD_GRP_HWMANIP)
>  	BAREBOX_CMD_HELP(cmd_trigger_help)
>  BAREBOX_CMD_END
> diff --git a/drivers/led/led-triggers.c b/drivers/led/led-triggers.c
> index 6c05cc5bb..76a1481e1 100644
> --- a/drivers/led/led-triggers.c
> +++ b/drivers/led/led-triggers.c
> @@ -52,7 +52,7 @@ struct led_trigger_struct {
>  	enum led_trigger trigger;
>  };
>  
> -static struct led_trigger_struct triggers[LED_TRIGGER_MAX];
> +static LIST_HEAD(led_triggers);
>  
>  /**
>   * led_trigger - triggers a trigger
> @@ -63,17 +63,53 @@ static struct led_trigger_struct triggers[LED_TRIGGER_MAX];
>   */
>  void led_trigger(enum led_trigger trigger, enum trigger_type type)
>  {
> +	struct led_trigger_struct *led_trigger;
> +
>  	if (trigger >= LED_TRIGGER_MAX)
>  		return;
> -	if (!triggers[trigger].led)
> -		return;
>  
> -	if (type == TRIGGER_FLASH) {
> -		led_flash(triggers[trigger].led, 200);
> -		return;
> +	list_for_each_entry(led_trigger, &led_triggers, list) {
> +		if (led_trigger->trigger != trigger)
> +			continue;
> +
> +		switch (type) {
> +		case TRIGGER_FLASH:
> +			led_flash(led_trigger->led, 200);
> +			break;
> +		case TRIGGER_ENABLE:
> +			led_set(led_trigger->led, led_trigger->led->max_value);
> +			break;
> +		case TRIGGER_DISABLE:
> +			led_set(led_trigger->led, 0);
> +			break;
> +		}
>  	}
> +}
> +
> +static struct led_trigger_struct *led_find_trigger(struct led *led)
> +{
> +	struct led_trigger_struct *led_trigger;
> +
> +	list_for_each_entry(led_trigger, &led_triggers, list)
> +		if (led_trigger->led == led)
> +			return led_trigger;
> +
> +	return NULL;
> +}
>  
> -	led_set(triggers[trigger].led, type == TRIGGER_ENABLE ? triggers[trigger].led->max_value : 0);
> +void led_trigger_disable(struct led *led)
> +{
> +	struct led_trigger_struct *led_trigger;
> +
> +	led_trigger = led_find_trigger(led);
> +	if (!led_trigger)
> +		return;
> +
> +	list_del(&led_trigger->list);
> +
> +	led_set(led, 0);
> +
> +	free(led_trigger);
>  }
>  
>  /**
> @@ -86,40 +122,73 @@ void led_trigger(enum led_trigger trigger, enum trigger_type type)
>   */
>  int led_set_trigger(enum led_trigger trigger, struct led *led)
>  {
> -	int i;
> +	struct led_trigger_struct *led_trigger;
>  
>  	if (trigger >= LED_TRIGGER_MAX)
>  		return -EINVAL;
>  
> -	if (led)
> -		for (i = 0; i < LED_TRIGGER_MAX; i++)
> -			if (triggers[i].led == led)
> -				return -EBUSY;
> +	led_trigger_disable(led);
>  
> -	if (triggers[trigger].led && !led)
> -		led_set(triggers[trigger].led, 0);
> +	led_trigger = xzalloc(sizeof(*led_trigger));
>  
> -	triggers[trigger].led = led;
> +	led_trigger->led = led;
> +	led_trigger->trigger = trigger;
> +	list_add_tail(&led_trigger->list, &led_triggers);
>  
> -	if (led && trigger == LED_TRIGGER_DEFAULT_ON)
> -		led_set(triggers[trigger].led, triggers[trigger].led->max_value);
> -	if (led && trigger == LED_TRIGGER_HEARTBEAT)
> +	if (trigger == LED_TRIGGER_DEFAULT_ON)
> +		led_set(led, led->max_value);
> +	if (trigger == LED_TRIGGER_HEARTBEAT)
>  		led_blink(led, 200, 1000);
>  
>  	return 0;
>  }
>  
> +static char *trigger_names[] = {
> +	[LED_TRIGGER_PANIC] = "panic",
> +	[LED_TRIGGER_HEARTBEAT] = "heartbeat",
> +	[LED_TRIGGER_NET_RX] = "net-rx",
> +	[LED_TRIGGER_NET_TX] = "net-tx",
> +	[LED_TRIGGER_NET_TXRX] = "net",
> +	[LED_TRIGGER_DEFAULT_ON] = "default-on",
> +};
> +
> +const char *trigger_name(enum led_trigger trigger)
> +{
> +	return trigger_names[trigger];
> +}
> +
> +enum led_trigger trigger_by_name(const char *name)
> +{
> +	int i;
> +
> +	for (i = 0; i < LED_TRIGGER_MAX; i++)
> +		if (!strcmp(name, trigger_names[i]))
> +			return i;
> +
> +	return LED_TRIGGER_MAX;
> +}
> +
>  /**
> - * led_get_trigger - get the LED for a trigger
> - * @param trigger	The trigger to set a LED for
> - *
> - * return the LED number of a trigger.
> + * led_triggers_show_info - Show information about all registered
> + * triggers
>   */
> -int led_get_trigger(enum led_trigger trigger)
> +void led_triggers_show_info(void)
>  {
> -	if (trigger >= LED_TRIGGER_MAX)
> -		return -EINVAL;
> -	if (!triggers[trigger].led)
> -		return -ENODEV;
> -	return led_get_number(triggers[trigger].led);
> +	struct led_trigger_struct *led_trigger;
> +	int i;
> +
> +	for (i = 0; i < LED_TRIGGER_MAX; i++) {
> +		printf("%s", trigger_name(i));
> +
> +		list_for_each_entry(led_trigger, &led_triggers, list) {
> +			struct led *led = led_trigger->led;
> +
> +			if (led_trigger->trigger != i)
> +				continue;
> +
> +			printf("\n  LED %d (%s)", led->num, led->name);
> +		}
> +
> +		printf("\n");
> +	}
>  }
> diff --git a/include/led.h b/include/led.h
> index fb5c48b9e..0ce857129 100644
> --- a/include/led.h
> +++ b/include/led.h
> @@ -49,6 +49,7 @@ enum led_trigger {
>  	LED_TRIGGER_NET_TXRX,
>  	LED_TRIGGER_DEFAULT_ON,
>  	LED_TRIGGER_MAX,
> +	LED_TRIGGER_INVALID = LED_TRIGGER_MAX,
>  };
>  
>  enum trigger_type {
> @@ -59,6 +60,7 @@ enum trigger_type {
>  
>  #ifdef CONFIG_LED_TRIGGERS
>  int led_set_trigger(enum led_trigger trigger, struct led *led);
> +void led_trigger_disable(struct led *led);
>  void led_trigger(enum led_trigger trigger, enum trigger_type);
>  #else
>  static inline int led_set_trigger(enum led_trigger trigger, struct led *led)
> @@ -71,7 +73,9 @@ static inline void led_trigger(enum led_trigger trigger, enum trigger_type type)
>  }
>  #endif
>  
> -int led_get_trigger(enum led_trigger trigger);
> +void led_triggers_show_info(void);
> +const char *trigger_name(enum led_trigger trigger);
> +enum led_trigger trigger_by_name(const char *name);
>  
>  void led_of_parse_trigger(struct led *led, struct device_node *np);
>  
> -- 
> 2.11.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox