Re: [PATCH] auxdisplay: linedisp: Support configuring the boot message

[Geert Uytterhoeven]

Hi Chris,

On Fri, May 31, 2024 at 7:28 AM Chris Packham
<chris.packham@xxxxxxxxxxxxxxxxxxx> wrote:
> Like we do for charlcd, allow the configuration of the initial message
> on line-display devices.
>
> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/auxdisplay/line-display.c
> +++ b/drivers/auxdisplay/line-display.c
> @@ -8,7 +8,9 @@
>   * Copyright (C) 2021 Glider bv
>   */
>
> +#ifndef CONFIG_PANEL_BOOT_MESSAGE
>  #include <generated/utsrelease.h>
> +#endif

The #ifndef/#endif is not really needed.

>  #include <linux/container_of.h>
>  #include <linux/device.h>
> @@ -312,6 +314,12 @@ static int linedisp_init_map(struct linedisp *linedisp)
>         return 0;
>  }
>
> +#ifdef CONFIG_PANEL_BOOT_MESSAGE
> +#define LINE_DISP_INIT_TEXT CONFIG_PANEL_BOOT_MESSAGE

So the user has to add extra spaces at the end when needed.
This makes sense, as they are not always needed (e.g. when the length of
the message matches the display size, no scrolling is needed/wanted).
I have verified that Kconfig actually preserves such spaces.

Nit: this is the only definition having an underscore between "line"
and "disp".

> +#else
> +#define LINE_DISP_INIT_TEXT "Linux " UTS_RELEASE "       "
> +#endif

I'd rather move this up, next to the other definitions at the top of
the file.

> +
>  /**
>   * linedisp_register - register a character line display
>   * @linedisp: pointer to character line display structure

As I see no real deficiencies:
Reviewed-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>

Gr{oetje,eeting}s,

                        Geert

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

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