diff mbox series

[v2,07/15] auxdisplay: linedisp: Group line display drivers together

Message ID 20240212170423.2860895-8-andriy.shevchenko@linux.intel.com
State New
Headers
Series auxdisplay: linedisp: Clean up and add new driver |

Commit Message

Andy Shevchenko Feb. 12, 2024, 5:01 p.m. UTC 
  For better usability group the line display drivers together in Kconfig
and Makefile.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/Kconfig  | 32 ++++++++++++++++----------------
 drivers/auxdisplay/Makefile | 12 ++++++------
 2 files changed, 22 insertions(+), 22 deletions(-)

Comments

Geert Uytterhoeven Feb. 15, 2024, 10:05 a.m. UTC | #1 
Hi Andy,

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> For better usability group the line display drivers together in Kconfig
> and Makefile.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -25,12 +25,6 @@ config CHARLCD
>           This is some character LCD core interface that multiple drivers can
>           use.
>
> -config LINEDISP
> -       tristate "Character line display core support" if COMPILE_TEST
> -       help
> -         This is the core support for single-line character displays, to be
> -         selected by drivers that use it.
> -
>  config HD44780_COMMON
>         tristate "Common functions for HD44780 (and compatibles) LCD displays" if COMPILE_TEST
>         select CHARLCD
> @@ -52,6 +46,16 @@ config HD44780
>           kernel and started at boot.
>           If you don't understand what all this is about, say N.
>
> +config LCD2S
> +       tristate "lcd2s 20x4 character display over I2C console"
> +       depends on I2C
> +       select CHARLCD
> +       help
> +         This is a driver that lets you use the lcd2s 20x4 character display
> +         from Modtronix engineering as a console output device. The display
> +         is a simple single color character display. You have to connect it
> +         to an I2C bus.
> +

I think it would be good to have "# <display type> section" comments,
to make the grouping clear.
Else I wonder why "L" is sorted before "K" ;-)

>  config KS0108
>         tristate "KS0108 LCD Controller"
>         depends on PARPORT_PC
> @@ -153,6 +157,12 @@ config CFAG12864B_RATE
>           If you compile this as a module, you can still override this
>           value using the module parameters.
>
> +config LINEDISP
> +       tristate "Character line display core support" if COMPILE_TEST
> +       help
> +         This is the core support for single-line character displays, to be
> +         selected by drivers that use it.
> +
>  config IMG_ASCII_LCD
>         tristate "Imagination Technologies ASCII LCD Display"
>         depends on HAS_IOMEM
> @@ -177,16 +187,6 @@ config HT16K33
>           Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
>           LED controller driver with keyscan.
>
> -config LCD2S
> -       tristate "lcd2s 20x4 character display over I2C console"
> -       depends on I2C
> -       select CHARLCD
> -       help
> -         This is a driver that lets you use the lcd2s 20x4 character display
> -         from Modtronix engineering as a console output device. The display
> -         is a simple single color character display. You have to connect it
> -         to an I2C bus.
> -
>  config ARM_CHARLCD
>         bool "ARM Ltd. Character LCD Driver"
>         depends on PLAT_VERSATILE

Shouldn't this (and PARPORT_PANEL and friends) be moved up, to the
character LCD section?

> diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
> index 6968ed4d3f0a..43bad850481c 100644
> --- a/drivers/auxdisplay/Makefile
> +++ b/drivers/auxdisplay/Makefile
> @@ -5,12 +5,12 @@
>
>  obj-$(CONFIG_CHARLCD)          += charlcd.o
>  obj-$(CONFIG_HD44780_COMMON)   += hd44780_common.o
> -obj-$(CONFIG_ARM_CHARLCD)      += arm-charlcd.o
> +obj-$(CONFIG_HD44780)          += hd44780.o
> +obj-$(CONFIG_LCD2S)            += lcd2s.o
>  obj-$(CONFIG_KS0108)           += ks0108.o
>  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_PARPORT_PANEL)    += panel.o
> -obj-$(CONFIG_LCD2S)            += lcd2s.o
>  obj-$(CONFIG_LINEDISP)         += line-display.o
> +obj-$(CONFIG_IMG_ASCII_LCD)    += img-ascii-lcd.o
> +obj-$(CONFIG_HT16K33)          += ht16k33.o
> +obj-$(CONFIG_ARM_CHARLCD)      += arm-charlcd.o
> +obj-$(CONFIG_PARPORT_PANEL)    += panel.o

IMHO it hurts to not sort Makefile entries alphabetically.

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
Andy Shevchenko Feb. 15, 2024, 11:30 a.m. UTC | #2 
On Thu, Feb 15, 2024 at 11:05:00AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

> I think it would be good to have "# <display type> section" comments,
> to make the grouping clear.
> Else I wonder why "L" is sorted before "K" ;-)

Makes sense, I added locally.

..


> Shouldn't this (and PARPORT_PANEL and friends) be moved up, to the
> character LCD section?

I tried to be less invasive.

..

> >  obj-$(CONFIG_CHARLCD)          += charlcd.o
> >  obj-$(CONFIG_HD44780_COMMON)   += hd44780_common.o
> > -obj-$(CONFIG_ARM_CHARLCD)      += arm-charlcd.o
> > +obj-$(CONFIG_HD44780)          += hd44780.o
> > +obj-$(CONFIG_LCD2S)            += lcd2s.o
> >  obj-$(CONFIG_KS0108)           += ks0108.o
> >  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_PARPORT_PANEL)    += panel.o
> > -obj-$(CONFIG_LCD2S)            += lcd2s.o
> >  obj-$(CONFIG_LINEDISP)         += line-display.o
> > +obj-$(CONFIG_IMG_ASCII_LCD)    += img-ascii-lcd.o
> > +obj-$(CONFIG_HT16K33)          += ht16k33.o
> > +obj-$(CONFIG_ARM_CHARLCD)      += arm-charlcd.o
> > +obj-$(CONFIG_PARPORT_PANEL)    += panel.o
> 
> IMHO it hurts to not sort Makefile entries alphabetically.

I can add blank lines to follow the same grouping as in Kconfig. Would it work
for you?
Geert Uytterhoeven Feb. 15, 2024, 12:35 p.m. UTC | #3 
Hi Andy,

On Thu, Feb 15, 2024 at 12:30 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Feb 15, 2024 at 11:05:00AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >  obj-$(CONFIG_CHARLCD)          += charlcd.o
> > >  obj-$(CONFIG_HD44780_COMMON)   += hd44780_common.o
> > > -obj-$(CONFIG_ARM_CHARLCD)      += arm-charlcd.o
> > > +obj-$(CONFIG_HD44780)          += hd44780.o
> > > +obj-$(CONFIG_LCD2S)            += lcd2s.o
> > >  obj-$(CONFIG_KS0108)           += ks0108.o
> > >  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_PARPORT_PANEL)    += panel.o
> > > -obj-$(CONFIG_LCD2S)            += lcd2s.o
> > >  obj-$(CONFIG_LINEDISP)         += line-display.o
> > > +obj-$(CONFIG_IMG_ASCII_LCD)    += img-ascii-lcd.o
> > > +obj-$(CONFIG_HT16K33)          += ht16k33.o
> > > +obj-$(CONFIG_ARM_CHARLCD)      += arm-charlcd.o
> > > +obj-$(CONFIG_PARPORT_PANEL)    += panel.o
> >
> > IMHO it hurts to not sort Makefile entries alphabetically.
>
> I can add blank lines to follow the same grouping as in Kconfig. Would it work
> for you?

Unlike Kconfig, this is not user-visible.
Most Makefiles use sorted entries.  If they look unsorted, people will
just add new entries at the end.

Gr{oetje,eeting}s,

                        Geert
Andy Shevchenko Feb. 15, 2024, 1:57 p.m. UTC | #4 
On Thu, Feb 15, 2024 at 01:35:32PM +0100, Geert Uytterhoeven wrote:
> On Thu, Feb 15, 2024 at 12:30 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 15, 2024 at 11:05:00AM +0100, Geert Uytterhoeven wrote:

..

> > I can add blank lines to follow the same grouping as in Kconfig. Would it
> > work for you?
> 
> Unlike Kconfig, this is not user-visible.
> Most Makefiles use sorted entries.  If they look unsorted, people will
> just add new entries at the end.

Wouldn't be easy to catch at review phase?
diff mbox series

Patch

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index d944d5298eca..a34a9a52158f 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -25,12 +25,6 @@  config CHARLCD
 	  This is some character LCD core interface that multiple drivers can
 	  use.
 
-config LINEDISP
-	tristate "Character line display core support" if COMPILE_TEST
-	help
-	  This is the core support for single-line character displays, to be
-	  selected by drivers that use it.
-
 config HD44780_COMMON
 	tristate "Common functions for HD44780 (and compatibles) LCD displays" if COMPILE_TEST
 	select CHARLCD
@@ -52,6 +46,16 @@  config HD44780
 	  kernel and started at boot.
 	  If you don't understand what all this is about, say N.
 
+config LCD2S
+	tristate "lcd2s 20x4 character display over I2C console"
+	depends on I2C
+	select CHARLCD
+	help
+	  This is a driver that lets you use the lcd2s 20x4 character display
+	  from Modtronix engineering as a console output device. The display
+	  is a simple single color character display. You have to connect it
+	  to an I2C bus.
+
 config KS0108
 	tristate "KS0108 LCD Controller"
 	depends on PARPORT_PC
@@ -153,6 +157,12 @@  config CFAG12864B_RATE
 	  If you compile this as a module, you can still override this
 	  value using the module parameters.
 
+config LINEDISP
+	tristate "Character line display core support" if COMPILE_TEST
+	help
+	  This is the core support for single-line character displays, to be
+	  selected by drivers that use it.
+
 config IMG_ASCII_LCD
 	tristate "Imagination Technologies ASCII LCD Display"
 	depends on HAS_IOMEM
@@ -177,16 +187,6 @@  config HT16K33
 	  Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
 	  LED controller driver with keyscan.
 
-config LCD2S
-	tristate "lcd2s 20x4 character display over I2C console"
-	depends on I2C
-	select CHARLCD
-	help
-	  This is a driver that lets you use the lcd2s 20x4 character display
-	  from Modtronix engineering as a console output device. The display
-	  is a simple single color character display. You have to connect it
-	  to an I2C bus.
-
 config ARM_CHARLCD
 	bool "ARM Ltd. Character LCD Driver"
 	depends on PLAT_VERSATILE
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 6968ed4d3f0a..43bad850481c 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -5,12 +5,12 @@ 
 
 obj-$(CONFIG_CHARLCD)		+= charlcd.o
 obj-$(CONFIG_HD44780_COMMON)	+= hd44780_common.o
-obj-$(CONFIG_ARM_CHARLCD)	+= arm-charlcd.o
+obj-$(CONFIG_HD44780)		+= hd44780.o
+obj-$(CONFIG_LCD2S)		+= lcd2s.o
 obj-$(CONFIG_KS0108)		+= ks0108.o
 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_PARPORT_PANEL)	+= panel.o
-obj-$(CONFIG_LCD2S)		+= lcd2s.o
 obj-$(CONFIG_LINEDISP)		+= line-display.o
+obj-$(CONFIG_IMG_ASCII_LCD)	+= img-ascii-lcd.o
+obj-$(CONFIG_HT16K33)		+= ht16k33.o
+obj-$(CONFIG_ARM_CHARLCD)	+= arm-charlcd.o
+obj-$(CONFIG_PARPORT_PANEL)	+= panel.o