Re: [PATCH] leds: ledtrig-morse: send out morse code

[Geert Uytterhoeven]

Hi Andreas,

On Thu, Jun 28, 2018 at 3:42 PM Andreas Klinger <ak@xxxxxxxxxxxxx> wrote:
> Send out a morse code by using LEDs.
>
> This is useful especially on embedded systems without displays to tell the
> user about error conditions and status information.
>
> The trigger will be called "morse"
>
> The string to be send is written into the file morse_string and sent out
> with a workqueue. Supported are letters and digits.
>
> With the file dot_unit the minimal time unit can be adjusted in
> milliseconds.
>
> Signed-off-by: Andreas Klinger <ak@xxxxxxxxxxxxx>\

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-morse.c
> @@ -0,0 +1,298 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ledtrig-morse: LED Morse Trigger
> + *
> + * send a string as morse code out through LEDs
> + *
> + * can be used to send error codes or messages
> + *
> + * string to be send is written into morse_string
> + * supported are letters and digits
> + *
> + * Author: Andreas Klinger <ak@xxxxxxxxxxxxx>
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/ctype.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/workqueue.h>
> +#include <linux/leds.h>
> +
> +
> +#define MORSE_DOT_UNIT_DEFAULT 500
> +#define MORSE_TELEGRAM_SIZE    100
> +
> +struct morse_data {
> +       unsigned int            dot_unit;
> +       struct led_classdev     *led_cdev;
> +       struct work_struct      work;
> +       char                    telegram[MORSE_TELEGRAM_SIZE];
> +       unsigned int            telegram_size;
> +       struct mutex            lock;
> +};
> +
> +struct morse_char {
> +       char    c;
> +       char    *z;
> +};
> +
> +static struct morse_char morse_table[] = {
> +       {'a', ".-S"},

What's the added value of the trailing "S", which is present in each string,
over the standard NUL terminator, which is also present?

Given no character uses more than 5 symbols, a more compact encoding
(e.g. 3 bits for length, 5 bits for symbols) could be used.
But it may not be worth doing that optimization. And you may want to add
support for more characters later.

> +static void morse_send_char(struct led_classdev *led_cdev, char ch)
> +{
> +       int i = 0;

unsigned int

> +
> +       while ((morse_table[i].c) && (morse_table[i].c != tolower(ch)))
> +               i++;
> +
> +       if (morse_table[i].c) {
> +               int j = 0;

unsigned int

> +
> +               while (morse_table[i].z[j] != 'S') {

Without the trailing "S"es, you could just check for "morse_table[i].z[j]".

> +                       switch (morse_table[i].z[j]) {
> +                       case '.':
> +                               morse_short(led_cdev);
> +                               break;
> +                       case '-':
> +                               morse_long(led_cdev);
> +                               break;
> +                       }
> +                       j++;
> +               }
> +               morse_letter_space(led_cdev);
> +       } else {
> +               /*
> +                * keep it simple:
> +                * whenever there is an unrecognized character make a word
> +                * space
> +                */
> +               morse_word_space(led_cdev);
> +       }
> +}
> +
> +static void morse_work(struct work_struct *work)
> +{
> +       struct morse_data *data = container_of(work, struct morse_data, work);
> +       int i;

unsigned int i;

> +
> +       mutex_lock(&data->lock);
> +
> +       for (i = 0; i < data->telegram_size; i++)
> +               morse_send_char(data->led_cdev, data->telegram[i]);
> +
> +       mutex_unlock(&data->lock);
> +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds