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
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 <[email protected]>
---
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
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)"
+ depends on PARPORT
+ select AUXDISPLAY
+ select PARPORT_PANEL
config CHARLCD
tristate "Character LCD core support" if COMPILE_TEST
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 7ac6776ca3f6..cf54b5efb07e 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -10,4 +10,4 @@ obj-$(CONFIG_CFAG12864B) += cfag12864b.o cfag12864bfb.o
obj-$(CONFIG_IMG_ASCII_LCD) += img-ascii-lcd.o
obj-$(CONFIG_HD44780) += hd44780.o
obj-$(CONFIG_HT16K33) += ht16k33.o
-obj-$(CONFIG_PANEL) += panel.o
+obj-$(CONFIG_PARPORT_PANEL) += panel.o
--
2.20.1
If CONFIG_PANEL_CHANGE_MESSAGE is set, CONFIG_PANEL_BOOT_MESSAGE will
also be defined, so the double ifdef is pointless. Simplify the code
further by using an intermediate macro rather duplicating most of the
line.
Signed-off-by: Mans Rullgard <[email protected]>
---
drivers/auxdisplay/charlcd.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 60e0b772673f..db0356dca2d7 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -763,6 +763,12 @@ static void charlcd_puts(struct charlcd *lcd, const char *s)
}
}
+#ifdef CONFIG_PANEL_BOOT_MESSAGE
+#define LCD_INIT_TEXT CONFIG_PANEL_BOOT_MESSAGE
+#else
+#define LCD_INIT_TEXT "Linux-" UTS_RELEASE "\n"
+#endif
+
/* initialize the LCD driver */
static int charlcd_init(struct charlcd *lcd)
{
@@ -784,13 +790,8 @@ static int charlcd_init(struct charlcd *lcd)
return ret;
/* display a short message */
-#ifdef CONFIG_PANEL_CHANGE_MESSAGE
-#ifdef CONFIG_PANEL_BOOT_MESSAGE
- charlcd_puts(lcd, "\x1b[Lc\x1b[Lb\x1b[L*" CONFIG_PANEL_BOOT_MESSAGE);
-#endif
-#else
- charlcd_puts(lcd, "\x1b[Lc\x1b[Lb\x1b[L*Linux-" UTS_RELEASE "\n");
-#endif
+ charlcd_puts(lcd, "\x1b[Lc\x1b[Lb\x1b[L*" LCD_INIT_TEXT);
+
/* clear the display on the next device opening */
priv->must_clear = true;
charlcd_home(lcd);
--
2.20.1
The charlcd driver currently flashes the backlight once on init.
This may not be desirable. Thus, add options for turning the
backlight off or on as well.
Signed-off-by: Mans Rullgard <[email protected]>
---
drivers/auxdisplay/Kconfig | 21 +++++++++++++++++++++
drivers/auxdisplay/charlcd.c | 10 +++++++++-
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 7d3fe27d6868..c52c738e554a 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -445,6 +445,27 @@ 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.
+choice
+ prompt "Backlight initial state"
+ default CHARLCD_BL_FLASH
+
+ config CHARLCD_BL_OFF
+ bool "Off"
+ help
+ Backlight is initially turned off
+
+ config CHARLCD_BL_ON
+ bool "On"
+ help
+ Backlight is initially turned on
+
+ config CHARLCD_BL_FLASH
+ bool "Flash"
+ help
+ Backlight is flashed briefly on init
+
+endchoice
+
endif # AUXDISPLAY
config PANEL
diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index db0356dca2d7..ff8c53c082ff 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -769,6 +769,14 @@ static void charlcd_puts(struct charlcd *lcd, const char *s)
#define LCD_INIT_TEXT "Linux-" UTS_RELEASE "\n"
#endif
+#ifdef CONFIG_CHARLCD_BL_ON
+#define LCD_INIT_BL "\x1b[L+"
+#elif defined (CONFIG_CHARLCD_BL_FLASH)
+#define LCD_INIT_BL "\x1b[L*"
+#else
+#define LCD_INIT_BL "\x1b[L-"
+#endif
+
/* initialize the LCD driver */
static int charlcd_init(struct charlcd *lcd)
{
@@ -790,7 +798,7 @@ static int charlcd_init(struct charlcd *lcd)
return ret;
/* display a short message */
- charlcd_puts(lcd, "\x1b[Lc\x1b[Lb\x1b[L*" LCD_INIT_TEXT);
+ charlcd_puts(lcd, "\x1b[Lc\x1b[Lb" LCD_INIT_BL LCD_INIT_TEXT);
/* clear the display on the next device opening */
priv->must_clear = true;
--
2.20.1
On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard <[email protected]> wrote:
>
> +#ifdef CONFIG_CHARLCD_BL_ON
> +#define LCD_INIT_BL "\x1b[L+"
> +#elif defined (CONFIG_CHARLCD_BL_FLASH)
Style nitpick: no space after "elif defined". Do you mind if I change
it before sending it to linux-next? Otherwise, looks fine to me.
Thanks!
CC'ing Willy in case he wants to take a look for charlcd.c
Cheers,
Miguel
On Wed, Mar 6, 2019 at 10:56 AM Miguel Ojeda
<[email protected]> wrote:
>
> CC'ing Willy in case he wants to take a look for charlcd.c
(and Geert, sorry!)
Cheers,
Miguel
On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard <[email protected]> wrote:
>
> /* display a short message */
> -#ifdef CONFIG_PANEL_CHANGE_MESSAGE
> - charlcd_puts(lcd, "\x1b[Lc\x1b[Lb\x1b[L*" CONFIG_PANEL_BOOT_MESSAGE);
> -#else
> - charlcd_puts(lcd, "\x1b[Lc\x1b[Lb\x1b[L*Linux-" UTS_RELEASE "\n");
> -#endif
> + charlcd_puts(lcd, "\x1b[Lc\x1b[Lb\x1b[L*" LCD_INIT_TEXT);
> +
Nice simplification, thanks!
CC'ing Willy & Geert in case they want to take a look for charlcd.c
Cheers,
Miguel
On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard <[email protected]> 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 <[email protected]>
> ---
> 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
Miguel Ojeda <[email protected]> writes:
> On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard <[email protected]> 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"
Darn, must have been confused while typing.
>> 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 <[email protected]>
>> ---
>> 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?
I was trying to bring some sanity to it without changing more than
necessary.
> Also, having PANEL_PARPORT and PARPORT_PANEL seems confusing...
It is...
>> 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).
The option is there so 'make oldconfig' on existing configurations
doesn't silently drop that driver since it now depends on AUXDISPLAY.
There have been similar cases when shuffling options. Maybe they used a
different tag. We could of course let the three people using that
driver deal with it themselves.
>> + 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!)
I always run get_maintainer.pl on patches. Sometimes it isn't clever
enough to figure out all the people who might be interested.
--
M?ns Rullg?rd
Miguel Ojeda <[email protected]> writes:
> On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard <[email protected]> wrote:
>>
>> +#ifdef CONFIG_CHARLCD_BL_ON
>> +#define LCD_INIT_BL "\x1b[L+"
>> +#elif defined (CONFIG_CHARLCD_BL_FLASH)
>
> Style nitpick: no space after "elif defined". Do you mind if I change
> it before sending it to linux-next? Otherwise, looks fine to me.
> Thanks!
Of course I don't mind.
--
M?ns Rullg?rd
On Fri, Mar 1, 2019 at 9:14 PM Mans Rullgard <[email protected]> wrote:
>
> The charlcd driver currently flashes the backlight once on init.
> This may not be desirable. Thus, add options for turning the
> backlight off or on as well.
>
> Signed-off-by: Mans Rullgard <[email protected]>
> ---
> drivers/auxdisplay/Kconfig | 21 +++++++++++++++++++++
> drivers/auxdisplay/charlcd.c | 10 +++++++++-
> 2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index 7d3fe27d6868..c52c738e554a 100644
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -445,6 +445,27 @@ 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.
>
> +choice
> + prompt "Backlight initial state"
> + default CHARLCD_BL_FLASH
LGTM, but I don't agree on this default. I would prefer either on or
off, not flashing for sure.
Though it seems the case before the patch...
> +
> + config CHARLCD_BL_OFF
> + bool "Off"
> + help
> + Backlight is initially turned off
> +
> + config CHARLCD_BL_ON
> + bool "On"
> + help
> + Backlight is initially turned on
> +
> + config CHARLCD_BL_FLASH
> + bool "Flash"
> + help
> + Backlight is flashed briefly on init
> +
> +endchoice
> +
> endif # AUXDISPLAY
>
> config PANEL
> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> index db0356dca2d7..ff8c53c082ff 100644
> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -769,6 +769,14 @@ static void charlcd_puts(struct charlcd *lcd, const char *s)
> #define LCD_INIT_TEXT "Linux-" UTS_RELEASE "\n"
> #endif
>
> +#ifdef CONFIG_CHARLCD_BL_ON
> +#define LCD_INIT_BL "\x1b[L+"
> +#elif defined (CONFIG_CHARLCD_BL_FLASH)
> +#define LCD_INIT_BL "\x1b[L*"
> +#else
I would rather put here defined(_OFF)...
> +#define LCD_INIT_BL "\x1b[L-"
...and do
#else
#define LCD_INIT_BL "" // or whatever stands for as-is
> +#endif
> +
> /* initialize the LCD driver */
> static int charlcd_init(struct charlcd *lcd)
> {
> @@ -790,7 +798,7 @@ static int charlcd_init(struct charlcd *lcd)
> return ret;
>
> /* display a short message */
> - charlcd_puts(lcd, "\x1b[Lc\x1b[Lb\x1b[L*" LCD_INIT_TEXT);
> + charlcd_puts(lcd, "\x1b[Lc\x1b[Lb" LCD_INIT_BL LCD_INIT_TEXT);
>
> /* clear the display on the next device opening */
> priv->must_clear = true;
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
Andy Shevchenko <[email protected]> writes:
> On Fri, Mar 1, 2019 at 9:14 PM Mans Rullgard <[email protected]> wrote:
>>
>> The charlcd driver currently flashes the backlight once on init.
>> This may not be desirable. Thus, add options for turning the
>> backlight off or on as well.
>>
>> Signed-off-by: Mans Rullgard <[email protected]>
>> ---
>> drivers/auxdisplay/Kconfig | 21 +++++++++++++++++++++
>> drivers/auxdisplay/charlcd.c | 10 +++++++++-
>> 2 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
>> index 7d3fe27d6868..c52c738e554a 100644
>> --- a/drivers/auxdisplay/Kconfig
>> +++ b/drivers/auxdisplay/Kconfig
>> @@ -445,6 +445,27 @@ 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.
>>
>> +choice
>> + prompt "Backlight initial state"
>> + default CHARLCD_BL_FLASH
>
> LGTM, but I don't agree on this default. I would prefer either on or
> off, not flashing for sure.
> Though it seems the case before the patch...
The current code unconditionally flashes the light once. I though it
best to keep that behaviour as default, even if it's not seen as ideal.
--
M?ns Rullg?rd
On Tue, Mar 12, 2019 at 4:48 PM Måns Rullgård <[email protected]> wrote:
>
> The current code unconditionally flashes the light once. I though it
> best to keep that behaviour as default, even if it's not seen as ideal.
Sent into -next. If no one else says anything after a few days, I will
send the series for -rc2.
By the way, what about the other comment Andy mentioned? i.e.
"defined(_OFF)" (in case you missed to answer it).
Cheers,
Miguel
Miguel Ojeda <[email protected]> writes:
> On Tue, Mar 12, 2019 at 4:48 PM M?ns Rullg?rd <[email protected]> wrote:
>>
>> The current code unconditionally flashes the light once. I though it
>> best to keep that behaviour as default, even if it's not seen as ideal.
>
> Sent into -next. If no one else says anything after a few days, I will
> send the series for -rc2.
>
> By the way, what about the other comment Andy mentioned? i.e.
> "defined(_OFF)" (in case you missed to answer it).
With a correct config, there should be no difference. If someone
managed to create a config without any of the alternatives selected,
a compiler error would result. I don't really have much of an opinion
on which behaviour is preferable in that situation.
--
M?ns Rullg?rd
On Wed, Mar 6, 2019 at 11:14 AM Miguel Ojeda
<[email protected]> wrote:
>
> Since you are touching the panel.c options, CC'ing the maintainers
> (please do run get_maintainer.pl in that case!)
I was preparing the PR for -rc2 and I just noticed we didn't CC Randy
which was the last one changing the Kconfig. CC'ing now in case he
wants to take a look before I send the PR.
Cheers,
Miguel
On 3/23/19 7:28 AM, Miguel Ojeda wrote:
> On Wed, Mar 6, 2019 at 11:14 AM Miguel Ojeda
> <[email protected]> wrote:
>>
>> Since you are touching the panel.c options, CC'ing the maintainers
>> (please do run get_maintainer.pl in that case!)
>
> I was preparing the PR for -rc2 and I just noticed we didn't CC Randy
> which was the last one changing the Kconfig. CC'ing now in case he
> wants to take a look before I send the PR.
>
> Cheers,
> Miguel
Hi,
I haven't seen any issues with this in linux-next, so go ahead, please.
cheers.
--
~Randy
On Sat, Mar 23, 2019 at 3:51 PM Randy Dunlap <[email protected]> wrote:
>
> I haven't seen any issues with this in linux-next, so go ahead, please.
Thanks for the very quick answer, Randy!
Cheers,
Miguel