Received-SPF: pass (google.com: domain of linux-kernel+bounces-196424-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240530232054.3559043-1-chris.packham@alliedtelesis.co.nz>
In-Reply-To: <20240530232054.3559043-1-chris.packham@alliedtelesis.co.nz>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Fri, 31 May 2024 09:45:10 +0200
Message-ID: <CAMuHMdXGJxwRDRjKQT5aa6off9nQ5WqreK9K-QhJwhUXngYA=Q@mail.gmail.com>
Subject: Re: [PATCH] auxdisplay: linedisp: Support configuring the boot message
To: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: andy@kernel.org, tzimmermann@suse.de, ojeda@kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Chris,

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

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 *linedi=
sp)
>         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@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--=20
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
org

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