Re: [PATCH v3 2/2] trigger: ledtrig-tty: add additional modes

[Jiri Slaby <jirislaby@kernel.org>]

On 20. 02. 23, 10:37, Florian Eckert wrote:
> Add additional modes to trigger the selected LED.
> The following modes are supported:
> 
> Tx/Rx:	Flash LED on data transmission (default)
> CTS:	DCE Ready to accept data from the DTE.
> DSR:	DCE is ready to receive and send data.
> CAR:	DCE is receiving a carrier from a remote DTE.
> RNG:	DCE has detected an incoming ring signal.
> 
> The mode can be changed for example with the following command:
> echo "CTS" > /sys/class/leds/<led>/mode
> 
> This would turn on the LED, when the DTE(modem) signals the DCE that it
> is ready to accept data.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>

> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -7,6 +7,15 @@
>   #include <linux/tty.h>
>   #include <uapi/linux/serial.h>
>   
> +enum tty_led_mode {
> +	TTY_LED_CNT,
> +	TTY_LED_CTS,
> +	TTY_LED_DSR,
> +	TTY_LED_CAR,
> +	TTY_LED_RNG,

I would do:
  __TTY_LED_LAST = TTY_LED_RNG;

instead of:

> +	__TTY_LED_MAX


> +};
> +
>   struct ledtrig_tty_data {
>   	struct led_classdev *led_cdev;
>   	struct delayed_work dwork;
> @@ -14,6 +23,15 @@ struct ledtrig_tty_data {
>   	const char *ttyname;
>   	struct tty_struct *tty;
>   	int rx, tx;
> +	enum tty_led_mode mode;
> +};
> +
> +static const char * const mode[] = {
> +	[TTY_LED_CNT] = "Tx/Rx", // Trasmit Data / Receive Data
> +	[TTY_LED_CTS] = "CTS", // CTS Clear To Send
> +	[TTY_LED_DSR] = "DSR", // DSR Data Set Ready
> +	[TTY_LED_CAR] = "CAR", // CAR Data Carrier Detect (DCD)
> +	[TTY_LED_RNG] = "RNG", // RNG Ring Indicator (RI)
>   };
>   
>   static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
> @@ -21,6 +39,71 @@ static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
>   	schedule_delayed_work(&trigger_data->dwork, 0);
>   }
>   
> +static ssize_t ledtrig_tty_mode_show(char *buf, enum tty_led_mode tty_mode)
> +{
> +	int len = 0;
> +	int i;
> +
> +	for (i = 0; i < __TTY_LED_MAX; i++) {
> +		bool hit;
> +
> +		hit = tty_mode == i;

Both the decl and assignment can be on a single line.

> +		len += sysfs_emit_at(buf, len, "%s%s%s",
> +				  hit ? "[" : "",
> +				  mode[i],
> +				  hit ? "]" : "");
> +
> +		if (i < __TTY_LED_MAX - 1)
> +			len += sysfs_emit_at(buf, len, " ");

And all this could be then:
for (i = 0; i <= __TTY_LED_LAST; i++) {
   bool hit = i == tty_mode;
   bool last = i == __TTY_LED_LAST;

   len += sysfs_emit_at(buf, len, "%s%s%s%s",
				  hit ? "[" : "",
				  mode[i],
				  hit ? "]" : "",
				  last ? "" : " ");
}

> +	}
> +
> +	len += sysfs_emit_at(buf, len, "\n");
> +
> +	return len;
> +}
...
> +static ssize_t tty_led_mode_store(struct device *dev,
> +			  struct device_attribute *attr, const char *buf,
> +			  size_t size)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	ssize_t ret = size;
> +	enum tty_led_mode tty_mode = __TTY_LED_MAX;
> +	int i;
> +
> +	/* Check for new line in string*/
> +	if (size > 0 && buf[size - 1] == '\n')
> +		size -= 1;
> +
> +	for (i = 0; i < __TTY_LED_MAX; i++) {
> +		if (strncmp(buf, mode[i], size) == 0)
> +			tty_mode = i;
> +	}
> +
> +	if (tty_mode == __TTY_LED_MAX)
> +		return -EINVAL;

Ah, you use __TTY_LED_MAX as an extra value. It doesn't look nice. So 
now, what about:

	for (i = 0; i <= __TTY_LED_LAST; i++)
		if (strncmp(buf, mode[i], size) == 0) {
			tty_mode = i;
			break;
		}

	if (i > __TTY_LED_LAST)
		return -EINVAL;

?

thanks,
-- 
js
suse labs