Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190301184816.784-1-mans@mansr.com>
In-Reply-To: <20190301184816.784-1-mans@mansr.com>
From:   Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date:   Wed, 6 Mar 2019 11:14:45 +0100
Message-ID: <CANiq72=H1Gubbr+eoEKUR+m49fkcC7Sr_hsyP3azzpk=i-au+Q@mail.gmail.com>
Subject: Re: [PATCH 1/3] auxdisplay: deconfuse configuration
To:     Mans Rullgard <mans@mansr.com>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Ksenija Stanojevic <ksenija.stanojevic@gmail.com>,
        Willy Tarreau <w@1wt.eu>,
        Geert Uytterhoeven <geert.uytterhoeven@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard <mans@mansr.com> wrote:
>
> The auxdisplay Kconfig is confusing.  It creates two separate menus
> even though the settings are closely related.  Moreover, the options
> for setting the boot message depend on CONFIG_PARPORT even though they
> are used by drivers that do not.
>
> Clear up the confustion by moving the "Parallel port LCD/Keypad" menu

"confustion" -> "confusion"

> under auxdisplay where it logically belongs.  Change the boot message
> options to depend only on CONFIG_CHARLCD, making them accessible also
> when only the HD44780 is selected.
>
> Since the "Parallel port LCD/Keypad" driver now has a new dependency
> on CONFIG_AUXDISPLAY, rename its Kconfig symbol and keep the old one
> such that make oldconfig will not disable the driver.
>
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  drivers/auxdisplay/Kconfig  | 17 ++++++++++++-----
>  drivers/auxdisplay/Makefile |  2 +-
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index 57410f9c5d44..7d3fe27d6868 100644
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -164,9 +164,7 @@ config ARM_CHARLCD
>           line and the Linux version on the second line, but that's
>           still useful.
>
> -endif # AUXDISPLAY
> -
> -menuconfig PANEL
> +menuconfig PARPORT_PANEL

Do we want the PARPORT_ prefix here but not on the suboptions?

Also, having PANEL_PARPORT and PARPORT_PANEL seems confusing...

>         tristate "Parallel port LCD/Keypad Panel support"
>         depends on PARPORT
>         select CHARLCD
> @@ -178,7 +176,7 @@ menuconfig PANEL
>           compiled as a module, or linked into the kernel and started at boot.
>           If you don't understand what all this is about, say N.
>
> -if PANEL
> +if PARPORT_PANEL
>
>  config PANEL_PARPORT
>         int "Default parallel port number (0=LPT1)"
> @@ -419,8 +417,11 @@ config PANEL_LCD_PIN_BL
>
>           Default for the 'BL' pin in custom profile is '0' (uncontrolled).
>
> +endif # PARPORT_PANEL
> +
>  config PANEL_CHANGE_MESSAGE
>         bool "Change LCD initialization message ?"
> +       depends on CHARLCD
>         default "n"
>         ---help---
>           This allows you to replace the boot message indicating the kernel version
> @@ -444,7 +445,13 @@ config PANEL_BOOT_MESSAGE
>           An empty message will only clear the display at driver init time. Any other
>           printf()-formatted message is valid with newline and escape codes.
>
> -endif # PANEL
> +endif # AUXDISPLAY
> +
> +config PANEL
> +       tristate "Parallel port LCD/Keypad Panel support (OLD OPTION)"

Hm... what do you mean by "OLD OPTION"? Should we keep it? (I don't
see any other place using this marking).

> +       depends on PARPORT
> +       select AUXDISPLAY
> +       select PARPORT_PANEL

I agree the menu was a bit convoluted and we didn't get to clean it.

Since you are touching the panel.c options, CC'ing the maintainers
(please do run get_maintainer.pl in that case!)

Cheers,
Miguel