[PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

[PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

[Andy Shevchenko]

Add a new initial driver for Maxim MAX6958/6959 chips.
While developing that driver I realised that there is a lot
of duplication between ht16k33 and a new one. Hence set of
cleanups and refactorings.

Note, the new driver has minimum support of the hardware and
I have plans to cover more features in the future.

In v3:
- dropped applied patches
- fixed compilation error (Geert)
- dragging back and force added memory allocation for the character buffer
- dropped 'with keyscan' in new driver (Geert)
- rephrased a bit description in DT bindings for new driver (Geert)
- added tags to the (almost) unchanged patches (Geert, Krzysztof, Robin)

In v2:
- updated DT bindings to follow specifications and requirements (Krzysztof)
- unified return code variable (err everywhere)
- left patches 10 and 13 untouched, we may amend later on (Robin)

Andy Shevchenko (9):
  auxdisplay: linedisp: Group display drivers together
  auxdisplay: linedisp: Allocate buffer for the string
  auxdisplay: ht16k33: Add default to switch-cases
  auxdisplay: ht16k33: Move ht16k33_linedisp_ops down
  auxdisplay: ht16k33: Define a few helper macros
  auxdisplay: ht16k33: Switch to use line display character mapping
  auxdisplay: ht16k33: Drop struct ht16k33_seg
  dt-bindings: auxdisplay: Add Maxim MAX6958/6959
  auxdisplay: Add driver for MAX695x 7-segment LED controllers

 .../bindings/auxdisplay/maxim,max6959.yaml    |  44 +++
 drivers/auxdisplay/Kconfig                    | 306 ++++++++++--------
 drivers/auxdisplay/Makefile                   |  16 +-
 drivers/auxdisplay/ht16k33.c                  | 177 ++++------
 drivers/auxdisplay/img-ascii-lcd.c            |  17 +-
 drivers/auxdisplay/line-display.c             |  11 +-
 drivers/auxdisplay/line-display.h             |   3 +-
 drivers/auxdisplay/max6959.c                  | 194 +++++++++++
 8 files changed, 496 insertions(+), 272 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
 create mode 100644 drivers/auxdisplay/max6959.c

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

[Andy Shevchenko]

For better usability group the display drivers together in Kconfig
and Makefile. With this we will have the following sections:
  - Character LCD
  - Samsung KS0108 LCD controller
  - Single character line display
  - Character LCD with non-conforming interface

While at it, drop redundant 'default n' entries.

Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/Kconfig  | 296 +++++++++++++++++++-----------------
 drivers/auxdisplay/Makefile |  15 +-
 2 files changed, 163 insertions(+), 148 deletions(-)

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index d944d5298eca..109ac253d160 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -16,6 +16,9 @@ menuconfig AUXDISPLAY
 
 if AUXDISPLAY
 
+#
+# Character LCD section
+#
 config CHARLCD
 	tristate "Character LCD core support" if COMPILE_TEST
 	help
@@ -25,12 +28,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,131 +49,6 @@ config HD44780
 	  kernel and started at boot.
 	  If you don't understand what all this is about, say N.
 
-config KS0108
-	tristate "KS0108 LCD Controller"
-	depends on PARPORT_PC
-	default n
-	help
-	  If you have a LCD controlled by one or more KS0108
-	  controllers, say Y. You will need also another more specific
-	  driver for your LCD.
-
-	  Depends on Parallel Port support. If you say Y at
-	  parport, you will be able to compile this as a module (M)
-	  and built-in as well (Y).
-
-	  To compile this as a module, choose M here:
-	  the module will be called ks0108.
-
-	  If unsure, say N.
-
-config KS0108_PORT
-	hex "Parallel port where the LCD is connected"
-	depends on KS0108
-	default 0x378
-	help
-	  The address of the parallel port where the LCD is connected.
-
-	  The first  standard parallel port address is 0x378.
-	  The second standard parallel port address is 0x278.
-	  The third  standard parallel port address is 0x3BC.
-
-	  You can specify a different address if you need.
-
-	  If you don't know what I'm talking about, load the parport module,
-	  and execute "dmesg" or "cat /proc/ioports". You can see there how
-	  many parallel ports are present and which address each one has.
-
-	  Usually you only need to use 0x378.
-
-	  If you compile this as a module, you can still override this
-	  using the module parameters.
-
-config KS0108_DELAY
-	int "Delay between each control writing (microseconds)"
-	depends on KS0108
-	default "2"
-	help
-	  Amount of time the ks0108 should wait between each control write
-	  to the parallel port.
-
-	  If your LCD seems to miss random writings, increment this.
-
-	  If you don't know what I'm talking about, ignore it.
-
-	  If you compile this as a module, you can still override this
-	  value using the module parameters.
-
-config CFAG12864B
-	tristate "CFAG12864B LCD"
-	depends on X86
-	depends on FB
-	depends on KS0108
-	select FB_SYSMEM_HELPERS
-	default n
-	help
-	  If you have a Crystalfontz 128x64 2-color LCD, cfag12864b Series,
-	  say Y. You also need the ks0108 LCD Controller driver.
-
-	  For help about how to wire your LCD to the parallel port,
-	  check Documentation/admin-guide/auxdisplay/cfag12864b.rst
-
-	  Depends on the x86 arch and the framebuffer support.
-
-	  The LCD framebuffer driver can be attached to a console.
-	  It will work fine. However, you can't attach it to the fbdev driver
-	  of the xorg server.
-
-	  To compile this as a module, choose M here:
-	  the modules will be called cfag12864b and cfag12864bfb.
-
-	  If unsure, say N.
-
-config CFAG12864B_RATE
-	int "Refresh rate (hertz)"
-	depends on CFAG12864B
-	default "20"
-	help
-	  Refresh rate of the LCD.
-
-	  As the LCD is not memory mapped, the driver has to make the work by
-	  software. This means you should be careful setting this value higher.
-	  If your CPUs are really slow or you feel the system is slowed down,
-	  decrease the value.
-
-	  Be careful modifying this value to a very high value:
-	  You can freeze the computer, or the LCD maybe can't draw as fast as you
-	  are requesting.
-
-	  If you don't know what I'm talking about, ignore it.
-
-	  If you compile this as a module, you can still override this
-	  value using the module parameters.
-
-config IMG_ASCII_LCD
-	tristate "Imagination Technologies ASCII LCD Display"
-	depends on HAS_IOMEM
-	default y if MIPS_MALTA
-	select MFD_SYSCON
-	select LINEDISP
-	help
-	  Enable this to support the simple ASCII LCD displays found on
-	  development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
-	  from Imagination Technologies.
-
-config HT16K33
-	tristate "Holtek Ht16K33 LED controller with keyscan"
-	depends on FB && I2C && INPUT
-	select FB_SYSMEM_HELPERS
-	select INPUT_MATRIXKMAP
-	select FB_BACKLIGHT
-	select NEW_LEDS
-	select LEDS_CLASS
-	select LINEDISP
-	help
-	  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
@@ -187,16 +59,6 @@ config LCD2S
 	  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
-	help
-	  This is a driver for the character LCD found on the ARM Ltd.
-	  Versatile and RealView Platform Baseboards. It doesn't do
-	  very much more than display the text "ARM Linux" on the first
-	  line and the Linux version on the second line, but that's
-	  still useful.
-
 menuconfig PARPORT_PANEL
 	tristate "Parallel port LCD/Keypad Panel support"
 	depends on PARPORT
@@ -455,7 +317,6 @@ 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
 	  and the driver version with a custom message. This is useful on appliances
@@ -504,8 +365,159 @@ choice
 
 endchoice
 
+#
+# Samsung KS0108 LCD controller section
+#
+config KS0108
+	tristate "KS0108 LCD Controller"
+	depends on PARPORT_PC
+	help
+	  If you have a LCD controlled by one or more KS0108
+	  controllers, say Y. You will need also another more specific
+	  driver for your LCD.
+
+	  Depends on Parallel Port support. If you say Y at
+	  parport, you will be able to compile this as a module (M)
+	  and built-in as well (Y).
+
+	  To compile this as a module, choose M here:
+	  the module will be called ks0108.
+
+	  If unsure, say N.
+
+config KS0108_PORT
+	hex "Parallel port where the LCD is connected"
+	depends on KS0108
+	default 0x378
+	help
+	  The address of the parallel port where the LCD is connected.
+
+	  The first  standard parallel port address is 0x378.
+	  The second standard parallel port address is 0x278.
+	  The third  standard parallel port address is 0x3BC.
+
+	  You can specify a different address if you need.
+
+	  If you don't know what I'm talking about, load the parport module,
+	  and execute "dmesg" or "cat /proc/ioports". You can see there how
+	  many parallel ports are present and which address each one has.
+
+	  Usually you only need to use 0x378.
+
+	  If you compile this as a module, you can still override this
+	  using the module parameters.
+
+config KS0108_DELAY
+	int "Delay between each control writing (microseconds)"
+	depends on KS0108
+	default "2"
+	help
+	  Amount of time the ks0108 should wait between each control write
+	  to the parallel port.
+
+	  If your LCD seems to miss random writings, increment this.
+
+	  If you don't know what I'm talking about, ignore it.
+
+	  If you compile this as a module, you can still override this
+	  value using the module parameters.
+
+config CFAG12864B
+	tristate "CFAG12864B LCD"
+	depends on X86
+	depends on FB
+	depends on KS0108
+	select FB_SYSMEM_HELPERS
+	help
+	  If you have a Crystalfontz 128x64 2-color LCD, cfag12864b Series,
+	  say Y. You also need the ks0108 LCD Controller driver.
+
+	  For help about how to wire your LCD to the parallel port,
+	  check Documentation/admin-guide/auxdisplay/cfag12864b.rst
+
+	  Depends on the x86 arch and the framebuffer support.
+
+	  The LCD framebuffer driver can be attached to a console.
+	  It will work fine. However, you can't attach it to the fbdev driver
+	  of the xorg server.
+
+	  To compile this as a module, choose M here:
+	  the modules will be called cfag12864b and cfag12864bfb.
+
+	  If unsure, say N.
+
+config CFAG12864B_RATE
+	int "Refresh rate (hertz)"
+	depends on CFAG12864B
+	default "20"
+	help
+	  Refresh rate of the LCD.
+
+	  As the LCD is not memory mapped, the driver has to make the work by
+	  software. This means you should be careful setting this value higher.
+	  If your CPUs are really slow or you feel the system is slowed down,
+	  decrease the value.
+
+	  Be careful modifying this value to a very high value:
+	  You can freeze the computer, or the LCD maybe can't draw as fast as you
+	  are requesting.
+
+	  If you don't know what I'm talking about, ignore it.
+
+	  If you compile this as a module, you can still override this
+	  value using the module parameters.
+
+#
+# Single character line display section
+#
+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
+	default y if MIPS_MALTA
+	select MFD_SYSCON
+	select LINEDISP
+	help
+	  Enable this to support the simple ASCII LCD displays found on
+	  development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
+	  from Imagination Technologies.
+
+config HT16K33
+	tristate "Holtek Ht16K33 LED controller with keyscan"
+	depends on FB && I2C && INPUT
+	select FB_SYSMEM_HELPERS
+	select INPUT_MATRIXKMAP
+	select FB_BACKLIGHT
+	select NEW_LEDS
+	select LEDS_CLASS
+	select LINEDISP
+	help
+	  Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
+	  LED controller driver with keyscan.
+
+#
+# Character LCD with non-conforming interface section
+#
+config ARM_CHARLCD
+	bool "ARM Ltd. Character LCD Driver"
+	depends on PLAT_VERSATILE
+	help
+	  This is a driver for the character LCD found on the ARM Ltd.
+	  Versatile and RealView Platform Baseboards. It doesn't do
+	  very much more than display the text "ARM Linux" on the first
+	  line and the Linux version on the second line, but that's
+	  still useful.
+
 endif # AUXDISPLAY
 
+#
+# Deprecated options
+#
 config PANEL
 	tristate "Parallel port LCD/Keypad Panel support (OLD OPTION)"
 	depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 6968ed4d3f0a..9197ea34e2d6 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -5,12 +5,15 @@
 
 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_PARPORT_PANEL)	+= panel.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
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> For better usability group the display drivers together in Kconfig
> and Makefile. With this we will have the following sections:
>   - Character LCD
>   - Samsung KS0108 LCD controller
>   - Single character line display
>   - Character LCD with non-conforming interface
>
> While at it, drop redundant 'default n' entries.
>
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig

> +#
> +# Single character line display section
> +#
> +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
> +       default y if MIPS_MALTA
> +       select MFD_SYSCON
> +       select LINEDISP
> +       help
> +         Enable this to support the simple ASCII LCD displays found on
> +         development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
> +         from Imagination Technologies.
> +
> +config HT16K33
> +       tristate "Holtek Ht16K33 LED controller with keyscan"

HT16K33 also supports dot-matrix displays using fbdev...
Yes, categorizing is difficult.

> --- a/drivers/auxdisplay/Makefile
> +++ b/drivers/auxdisplay/Makefile
> @@ -5,12 +5,15 @@
>
>  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_PARPORT_PANEL)    += panel.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

I still think these should be sorted alphabetically.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 04:28:20PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > For better usability group the display drivers together in Kconfig
> > and Makefile. With this we will have the following sections:
> >   - Character LCD
> >   - Samsung KS0108 LCD controller
> >   - Single character line display
> >   - Character LCD with non-conforming interface

...

> > +config HT16K33
> > +       tristate "Holtek Ht16K33 LED controller with keyscan"
> 
> HT16K33 also supports dot-matrix displays using fbdev...
> Yes, categorizing is difficult.

So, what to do here?

...

> I still think these should be sorted alphabetically.

Okay.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 26, 2024 at 5:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 26, 2024 at 04:28:20PM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > For better usability group the display drivers together in Kconfig
> > > and Makefile. With this we will have the following sections:
> > >   - Character LCD
> > >   - Samsung KS0108 LCD controller
> > >   - Single character line display
> > >   - Character LCD with non-conforming interface
>
> ...
>
> > > +config HT16K33
> > > +       tristate "Holtek Ht16K33 LED controller with keyscan"
> >
> > HT16K33 also supports dot-matrix displays using fbdev...
> > Yes, categorizing is difficult.
>
> So, what to do here?

Create a new section for multi-function displays?

>
> ...
>
> > I still think these should be sorted alphabetically.
>
> Okay.

Thanks!

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 05:03:10PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 26, 2024 at 5:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 26, 2024 at 04:28:20PM +0100, Geert Uytterhoeven wrote:
> > > On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > +config HT16K33
> > > > +       tristate "Holtek Ht16K33 LED controller with keyscan"
> > >
> > > HT16K33 also supports dot-matrix displays using fbdev...
> > > Yes, categorizing is difficult.
> >
> > So, what to do here?
> 
> Create a new section for multi-function displays?

Not sure we need that. Too many sections.

Okay, I'll defer this patch for now. Seems too much bikeshedding around it.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/9] auxdisplay: linedisp: Allocate buffer for the string

[Andy Shevchenko]

Always allocate a buffer for the currently displayed characters.
It makes the line display API simpler.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/ht16k33.c       |  8 +++-----
 drivers/auxdisplay/img-ascii-lcd.c | 17 +++++++----------
 drivers/auxdisplay/line-display.c  | 11 +++++++----
 drivers/auxdisplay/line-display.h  |  3 +--
 4 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 32d3afd29177..19805f39a257 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -92,7 +92,6 @@ struct ht16k33_seg {
 		struct seg14_conversion_map seg14;
 	} map;
 	unsigned int map_size;
-	char curr[4];
 };
 
 struct ht16k33_priv {
@@ -457,7 +456,7 @@ static void ht16k33_seg7_update(struct work_struct *work)
 	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
 						 work.work);
 	struct ht16k33_seg *seg = &priv->seg;
-	char *s = seg->curr;
+	char *s = seg->linedisp.buf;
 	uint8_t buf[9];
 
 	buf[0] = map_to_seg7(&seg->map.seg7, *s++);
@@ -478,7 +477,7 @@ static void ht16k33_seg14_update(struct work_struct *work)
 	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
 						 work.work);
 	struct ht16k33_seg *seg = &priv->seg;
-	char *s = seg->curr;
+	char *s = seg->linedisp.buf;
 	uint8_t buf[8];
 
 	put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf);
@@ -700,8 +699,7 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
 	if (err)
 		return err;
 
-	err = linedisp_register(&seg->linedisp, dev, 4, seg->curr,
-				&ht16k33_linedisp_ops);
+	err = linedisp_register(&seg->linedisp, dev, 4, &ht16k33_linedisp_ops);
 	if (err)
 		goto err_remove_map_file;
 
diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index ecfb1c05bf55..925c4cd101e9 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -37,7 +37,6 @@ struct img_ascii_lcd_config {
  * @regmap: the regmap through which LCD registers are accessed
  * @offset: the offset within regmap to the start of the LCD registers
  * @cfg: pointer to the LCD model configuration
- * @curr: the string currently displayed on the LCD
  */
 struct img_ascii_lcd_ctx {
 	struct linedisp linedisp;
@@ -47,7 +46,6 @@ struct img_ascii_lcd_ctx {
 	};
 	u32 offset;
 	const struct img_ascii_lcd_config *cfg;
-	char curr[] __aligned(8);
 };
 
 /*
@@ -61,12 +59,12 @@ static void boston_update(struct linedisp *linedisp)
 	ulong val;
 
 #if BITS_PER_LONG == 64
-	val = *((u64 *)&ctx->curr[0]);
+	val = *((u64 *)&linedisp->buf[0]);
 	__raw_writeq(val, ctx->base);
 #elif BITS_PER_LONG == 32
-	val = *((u32 *)&ctx->curr[0]);
+	val = *((u32 *)&linedisp->buf[0]);
 	__raw_writel(val, ctx->base);
-	val = *((u32 *)&ctx->curr[4]);
+	val = *((u32 *)&linedisp->buf[4]);
 	__raw_writel(val, ctx->base + 4);
 #else
 # error Not 32 or 64 bit
@@ -93,7 +91,7 @@ static void malta_update(struct linedisp *linedisp)
 
 	for (i = 0; i < linedisp->num_chars; i++) {
 		err = regmap_write(ctx->regmap,
-				   ctx->offset + (i * 8), ctx->curr[i]);
+				   ctx->offset + (i * 8), linedisp->buf[i]);
 		if (err)
 			break;
 	}
@@ -195,7 +193,7 @@ static void sead3_update(struct linedisp *linedisp)
 
 		err = regmap_write(ctx->regmap,
 				   ctx->offset + SEAD3_REG_LCD_DATA,
-				   ctx->curr[i]);
+				   linedisp->buf[i]);
 		if (err)
 			break;
 	}
@@ -236,7 +234,7 @@ static int img_ascii_lcd_probe(struct platform_device *pdev)
 	struct img_ascii_lcd_ctx *ctx;
 	int err;
 
-	ctx = devm_kzalloc(dev, sizeof(*ctx) + cfg->num_chars, GFP_KERNEL);
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
@@ -253,8 +251,7 @@ static int img_ascii_lcd_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->base);
 	}
 
-	err = linedisp_register(&ctx->linedisp, dev, cfg->num_chars, ctx->curr,
-				&cfg->ops);
+	err = linedisp_register(&ctx->linedisp, dev, cfg->num_chars, &cfg->ops);
 	if (err)
 		return err;
 
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 13be7c2f6bc3..e2b546210f8d 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -265,6 +265,7 @@ static void linedisp_release(struct device *dev)
 
 	kfree(linedisp->map);
 	kfree(linedisp->message);
+	kfree(linedisp->buf);
 	ida_free(&linedisp_id, linedisp->id);
 }
 
@@ -316,14 +317,12 @@ static int linedisp_init_map(struct linedisp *linedisp)
  * @linedisp: pointer to character line display structure
  * @parent: parent device
  * @num_chars: the number of characters that can be displayed
- * @buf: pointer to a buffer that can hold @num_chars characters
  * @ops: character line display operations
  *
  * Return: zero on success, else a negative error code.
  */
 int linedisp_register(struct linedisp *linedisp, struct device *parent,
-		      unsigned int num_chars, char *buf,
-		      const struct linedisp_ops *ops)
+		      unsigned int num_chars, const struct linedisp_ops *ops)
 {
 	int err;
 
@@ -331,7 +330,6 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	linedisp->dev.parent = parent;
 	linedisp->dev.type = &linedisp_type;
 	linedisp->ops = ops;
-	linedisp->buf = buf;
 	linedisp->num_chars = num_chars;
 	linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
 
@@ -343,6 +341,11 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	device_initialize(&linedisp->dev);
 	dev_set_name(&linedisp->dev, "linedisp.%u", linedisp->id);
 
+	err = -ENOMEM;
+	linedisp->buf = kzalloc(linedisp->num_chars, GFP_KERNEL);
+	if (!linedisp->buf)
+		goto out_put_device;
+
 	/* initialise a character mapping, if required */
 	err = linedisp_init_map(linedisp);
 	if (err)
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 4e310b0e611e..4348d7a2f69a 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -82,8 +82,7 @@ struct linedisp {
 };
 
 int linedisp_register(struct linedisp *linedisp, struct device *parent,
-		      unsigned int num_chars, char *buf,
-		      const struct linedisp_ops *ops);
+		      unsigned int num_chars, const struct linedisp_ops *ops);
 void linedisp_unregister(struct linedisp *linedisp);
 
 #endif /* LINEDISP_H */
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v3 2/9] auxdisplay: linedisp: Allocate buffer for the string

[Geert Uytterhoeven]

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Always allocate a buffer for the currently displayed characters.
> It makes the line display API simpler.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

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

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

[PATCH v3 3/9] auxdisplay: ht16k33: Add default to switch-cases

[Andy Shevchenko]

Currently the compiler (GCC) is able to figure out that there is no
other choices possible than those that are already listed in the
switch-cases. However, if we want to move some code to the callback,
compiler will start complaining that no default is defined. Make
sure we have all switch-cases equiped with default.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/ht16k33.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 19805f39a257..c0067a3b2b61 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -677,11 +677,6 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
 		return err;
 
 	switch (priv->type) {
-	case DISP_MATRIX:
-		/* not handled here */
-		err = -EINVAL;
-		break;
-
 	case DISP_QUAD_7SEG:
 		INIT_DELAYED_WORK(&priv->work, ht16k33_seg7_update);
 		seg->map.seg7 = initial_map_seg7;
@@ -695,6 +690,9 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
 		seg->map_size = sizeof(seg->map.seg14);
 		err = device_create_file(dev, &dev_attr_map_seg14);
 		break;
+
+	default:
+		return -EINVAL;
 	}
 	if (err)
 		return err;
@@ -772,6 +770,9 @@ static int ht16k33_probe(struct i2c_client *client)
 		/* Segment Display */
 		err = ht16k33_seg_probe(dev, priv, dft_brightness);
 		break;
+
+	default:
+		return -EINVAL;
 	}
 	return err;
 }
@@ -796,6 +797,9 @@ static void ht16k33_remove(struct i2c_client *client)
 		device_remove_file(&client->dev, &dev_attr_map_seg7);
 		device_remove_file(&client->dev, &dev_attr_map_seg14);
 		break;
+
+	default:
+		break;
 	}
 }
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v3 3/9] auxdisplay: ht16k33: Add default to switch-cases

[Geert Uytterhoeven]

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Currently the compiler (GCC) is able to figure out that there is no
> other choices possible than those that are already listed in the
> switch-cases. However, if we want to move some code to the callback,
> compiler will start complaining that no default is defined. Make
> sure we have all switch-cases equiped with default.
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

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

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

[PATCH v3 4/9] auxdisplay: ht16k33: Move ht16k33_linedisp_ops down

[Andy Shevchenko]

We will need the update functions to be defined before
ht16k33_linedisp_ops. Move the latter down in the code.
No functional change intended.

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Robin van der Gracht <robin@protonic.nl>
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/ht16k33.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index c0067a3b2b61..f016130835b1 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -439,18 +439,6 @@ static void ht16k33_keypad_stop(struct input_dev *dev)
 	disable_irq(keypad->client->irq);
 }
 
-static void ht16k33_linedisp_update(struct linedisp *linedisp)
-{
-	struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
-						 seg.linedisp);
-
-	schedule_delayed_work(&priv->work, 0);
-}
-
-static const struct linedisp_ops ht16k33_linedisp_ops = {
-	.update = ht16k33_linedisp_update,
-};
-
 static void ht16k33_seg7_update(struct work_struct *work)
 {
 	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
@@ -488,6 +476,18 @@ static void ht16k33_seg14_update(struct work_struct *work)
 	i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
 }
 
+static void ht16k33_linedisp_update(struct linedisp *linedisp)
+{
+	struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
+						 seg.linedisp);
+
+	schedule_delayed_work(&priv->work, 0);
+}
+
+static const struct linedisp_ops ht16k33_linedisp_ops = {
+	.update = ht16k33_linedisp_update,
+};
+
 static int ht16k33_led_probe(struct device *dev, struct led_classdev *led,
 			     unsigned int brightness)
 {
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v3 5/9] auxdisplay: ht16k33: Define a few helper macros

[Andy Shevchenko]

Define a few helper macros — wrappers on contaoner_of)() — for easier
maintenance in the future. While at it, include missing container_of.h.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/ht16k33.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index f016130835b1..b76c4d83528f 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -15,6 +15,7 @@
 #include <linux/property.h>
 #include <linux/fb.h>
 #include <linux/backlight.h>
+#include <linux/container_of.h>
 #include <linux/input.h>
 #include <linux/input/matrix_keypad.h>
 #include <linux/leds.h>
@@ -107,6 +108,15 @@ struct ht16k33_priv {
 	uint8_t blink;
 };
 
+#define ht16k33_work_to_priv(p)				\
+	container_of(p, struct ht16k33_priv, work.work)
+
+#define ht16k33_led_to_priv(p)				\
+	container_of(p, struct ht16k33_priv, led)
+
+#define ht16k33_linedisp_to_priv(p)			\
+	container_of(p, struct ht16k33_priv, seg.linedisp)
+
 static const struct fb_fix_screeninfo ht16k33_fb_fix = {
 	.id		= DRIVER_NAME,
 	.type		= FB_TYPE_PACKED_PIXELS,
@@ -194,8 +204,7 @@ static int ht16k33_brightness_set(struct ht16k33_priv *priv,
 static int ht16k33_brightness_set_blocking(struct led_classdev *led_cdev,
 					   enum led_brightness brightness)
 {
-	struct ht16k33_priv *priv = container_of(led_cdev, struct ht16k33_priv,
-						 led);
+	struct ht16k33_priv *priv = ht16k33_led_to_priv(led_cdev);
 
 	return ht16k33_brightness_set(priv, brightness);
 }
@@ -203,8 +212,7 @@ static int ht16k33_brightness_set_blocking(struct led_classdev *led_cdev,
 static int ht16k33_blink_set(struct led_classdev *led_cdev,
 			     unsigned long *delay_on, unsigned long *delay_off)
 {
-	struct ht16k33_priv *priv = container_of(led_cdev, struct ht16k33_priv,
-						 led);
+	struct ht16k33_priv *priv = ht16k33_led_to_priv(led_cdev);
 	unsigned int delay;
 	uint8_t blink;
 	int err;
@@ -246,8 +254,7 @@ static void ht16k33_fb_queue(struct ht16k33_priv *priv)
  */
 static void ht16k33_fb_update(struct work_struct *work)
 {
-	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
-						 work.work);
+	struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
 	struct ht16k33_fbdev *fbdev = &priv->fbdev;
 
 	uint8_t *p1, *p2;
@@ -441,8 +448,7 @@ static void ht16k33_keypad_stop(struct input_dev *dev)
 
 static void ht16k33_seg7_update(struct work_struct *work)
 {
-	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
-						 work.work);
+	struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
 	struct ht16k33_seg *seg = &priv->seg;
 	char *s = seg->linedisp.buf;
 	uint8_t buf[9];
@@ -462,8 +468,7 @@ static void ht16k33_seg7_update(struct work_struct *work)
 
 static void ht16k33_seg14_update(struct work_struct *work)
 {
-	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
-						 work.work);
+	struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
 	struct ht16k33_seg *seg = &priv->seg;
 	char *s = seg->linedisp.buf;
 	uint8_t buf[8];
@@ -478,8 +483,7 @@ static void ht16k33_seg14_update(struct work_struct *work)
 
 static void ht16k33_linedisp_update(struct linedisp *linedisp)
 {
-	struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
-						 seg.linedisp);
+	struct ht16k33_priv *priv = ht16k33_linedisp_to_priv(linedisp);
 
 	schedule_delayed_work(&priv->work, 0);
 }
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v3 5/9] auxdisplay: ht16k33: Define a few helper macros

[Andy Shevchenko]

On Mon, Feb 19, 2024 at 06:58:04PM +0200, Andy Shevchenko wrote:
> Define a few helper macros — wrappers on contaoner_of)() — for easier

I have fixed 'container_of()' above locally.

> maintenance in the future. While at it, include missing container_of.h.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/9] auxdisplay: ht16k33: Define a few helper macros

[Geert Uytterhoeven]

On Tue, Feb 20, 2024 at 7:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 19, 2024 at 06:58:04PM +0200, Andy Shevchenko wrote:
> > Define a few helper macros — wrappers on contaoner_of)() — for easier
>
> I have fixed 'container_of()' above locally.

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

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

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

[PATCH v3 6/9] auxdisplay: ht16k33: Switch to use line display character mapping

[Andy Shevchenko]

Since line display library supports necessary bits to map the characters
(if required), switch this driver to use that.

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Robin van der Gracht <robin@protonic.nl>
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/ht16k33.c | 103 ++++++++++-------------------------
 1 file changed, 30 insertions(+), 73 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index b76c4d83528f..41a961342dc3 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -88,11 +88,6 @@ struct ht16k33_fbdev {
 
 struct ht16k33_seg {
 	struct linedisp linedisp;
-	union {
-		struct seg7_conversion_map seg7;
-		struct seg14_conversion_map seg14;
-	} map;
-	unsigned int map_size;
 };
 
 struct ht16k33_priv {
@@ -144,33 +139,6 @@ static const struct fb_var_screeninfo ht16k33_fb_var = {
 	.vmode = FB_VMODE_NONINTERLACED,
 };
 
-static const SEG7_DEFAULT_MAP(initial_map_seg7);
-static const SEG14_DEFAULT_MAP(initial_map_seg14);
-
-static ssize_t map_seg_show(struct device *dev, struct device_attribute *attr,
-			    char *buf)
-{
-	struct ht16k33_priv *priv = dev_get_drvdata(dev);
-
-	memcpy(buf, &priv->seg.map, priv->seg.map_size);
-	return priv->seg.map_size;
-}
-
-static ssize_t map_seg_store(struct device *dev, struct device_attribute *attr,
-			     const char *buf, size_t cnt)
-{
-	struct ht16k33_priv *priv = dev_get_drvdata(dev);
-
-	if (cnt != priv->seg.map_size)
-		return -EINVAL;
-
-	memcpy(&priv->seg.map, buf, cnt);
-	return cnt;
-}
-
-static DEVICE_ATTR(map_seg7, 0644, map_seg_show, map_seg_store);
-static DEVICE_ATTR(map_seg14, 0644, map_seg_show, map_seg_store);
-
 static int ht16k33_display_on(struct ht16k33_priv *priv)
 {
 	uint8_t data = REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON | priv->blink;
@@ -450,18 +418,19 @@ static void ht16k33_seg7_update(struct work_struct *work)
 {
 	struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
 	struct ht16k33_seg *seg = &priv->seg;
+	struct linedisp_map *map = seg->linedisp.map;
 	char *s = seg->linedisp.buf;
 	uint8_t buf[9];
 
-	buf[0] = map_to_seg7(&seg->map.seg7, *s++);
+	buf[0] = map_to_seg7(&map->map.seg7, *s++);
 	buf[1] = 0;
-	buf[2] = map_to_seg7(&seg->map.seg7, *s++);
+	buf[2] = map_to_seg7(&map->map.seg7, *s++);
 	buf[3] = 0;
 	buf[4] = 0;
 	buf[5] = 0;
-	buf[6] = map_to_seg7(&seg->map.seg7, *s++);
+	buf[6] = map_to_seg7(&map->map.seg7, *s++);
 	buf[7] = 0;
-	buf[8] = map_to_seg7(&seg->map.seg7, *s++);
+	buf[8] = map_to_seg7(&map->map.seg7, *s++);
 
 	i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
 }
@@ -470,17 +439,36 @@ static void ht16k33_seg14_update(struct work_struct *work)
 {
 	struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
 	struct ht16k33_seg *seg = &priv->seg;
+	struct linedisp_map *map = seg->linedisp.map;
 	char *s = seg->linedisp.buf;
 	uint8_t buf[8];
 
-	put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf);
-	put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf + 2);
-	put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf + 4);
-	put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf + 6);
+	put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 0);
+	put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 2);
+	put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 4);
+	put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 6);
 
 	i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
 }
 
+static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
+{
+	struct ht16k33_priv *priv = ht16k33_linedisp_to_priv(linedisp);
+
+	switch (priv->type) {
+	case DISP_QUAD_7SEG:
+		INIT_DELAYED_WORK(&priv->work, ht16k33_seg7_update);
+		return LINEDISP_MAP_SEG7;
+
+	case DISP_QUAD_14SEG:
+		INIT_DELAYED_WORK(&priv->work, ht16k33_seg14_update);
+		return LINEDISP_MAP_SEG14;
+
+	default:
+		return -EINVAL;
+	}
+}
+
 static void ht16k33_linedisp_update(struct linedisp *linedisp)
 {
 	struct ht16k33_priv *priv = ht16k33_linedisp_to_priv(linedisp);
@@ -489,6 +477,7 @@ static void ht16k33_linedisp_update(struct linedisp *linedisp)
 }
 
 static const struct linedisp_ops ht16k33_linedisp_ops = {
+	.get_map_type = ht16k33_linedisp_get_map_type,
 	.update = ht16k33_linedisp_update,
 };
 
@@ -680,37 +669,7 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
 	if (err)
 		return err;
 
-	switch (priv->type) {
-	case DISP_QUAD_7SEG:
-		INIT_DELAYED_WORK(&priv->work, ht16k33_seg7_update);
-		seg->map.seg7 = initial_map_seg7;
-		seg->map_size = sizeof(seg->map.seg7);
-		err = device_create_file(dev, &dev_attr_map_seg7);
-		break;
-
-	case DISP_QUAD_14SEG:
-		INIT_DELAYED_WORK(&priv->work, ht16k33_seg14_update);
-		seg->map.seg14 = initial_map_seg14;
-		seg->map_size = sizeof(seg->map.seg14);
-		err = device_create_file(dev, &dev_attr_map_seg14);
-		break;
-
-	default:
-		return -EINVAL;
-	}
-	if (err)
-		return err;
-
-	err = linedisp_register(&seg->linedisp, dev, 4, &ht16k33_linedisp_ops);
-	if (err)
-		goto err_remove_map_file;
-
-	return 0;
-
-err_remove_map_file:
-	device_remove_file(dev, &dev_attr_map_seg7);
-	device_remove_file(dev, &dev_attr_map_seg14);
-	return err;
+	return linedisp_register(&seg->linedisp, dev, 4, &ht16k33_linedisp_ops);
 }
 
 static int ht16k33_probe(struct i2c_client *client)
@@ -798,8 +757,6 @@ static void ht16k33_remove(struct i2c_client *client)
 	case DISP_QUAD_7SEG:
 	case DISP_QUAD_14SEG:
 		linedisp_unregister(&priv->seg.linedisp);
-		device_remove_file(&client->dev, &dev_attr_map_seg7);
-		device_remove_file(&client->dev, &dev_attr_map_seg14);
 		break;
 
 	default:
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v3 7/9] auxdisplay: ht16k33: Drop struct ht16k33_seg

[Andy Shevchenko]

The struct ht16k33_seg is repeating struct linedisp. Use the latter
directly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/ht16k33.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 41a961342dc3..96acfb2b58cd 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -86,10 +86,6 @@ struct ht16k33_fbdev {
 	uint8_t *cache;
 };
 
-struct ht16k33_seg {
-	struct linedisp linedisp;
-};
-
 struct ht16k33_priv {
 	struct i2c_client *client;
 	struct delayed_work work;
@@ -97,7 +93,7 @@ struct ht16k33_priv {
 	struct ht16k33_keypad keypad;
 	union {
 		struct ht16k33_fbdev fbdev;
-		struct ht16k33_seg seg;
+		struct linedisp linedisp;
 	};
 	enum display_type type;
 	uint8_t blink;
@@ -110,7 +106,7 @@ struct ht16k33_priv {
 	container_of(p, struct ht16k33_priv, led)
 
 #define ht16k33_linedisp_to_priv(p)			\
-	container_of(p, struct ht16k33_priv, seg.linedisp)
+	container_of(p, struct ht16k33_priv, linedisp)
 
 static const struct fb_fix_screeninfo ht16k33_fb_fix = {
 	.id		= DRIVER_NAME,
@@ -417,9 +413,8 @@ static void ht16k33_keypad_stop(struct input_dev *dev)
 static void ht16k33_seg7_update(struct work_struct *work)
 {
 	struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
-	struct ht16k33_seg *seg = &priv->seg;
-	struct linedisp_map *map = seg->linedisp.map;
-	char *s = seg->linedisp.buf;
+	struct linedisp_map *map = priv->linedisp.map;
+	char *s = priv->linedisp.buf;
 	uint8_t buf[9];
 
 	buf[0] = map_to_seg7(&map->map.seg7, *s++);
@@ -438,9 +433,8 @@ static void ht16k33_seg7_update(struct work_struct *work)
 static void ht16k33_seg14_update(struct work_struct *work)
 {
 	struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
-	struct ht16k33_seg *seg = &priv->seg;
-	struct linedisp_map *map = seg->linedisp.map;
-	char *s = seg->linedisp.buf;
+	struct linedisp_map *map = priv->linedisp.map;
+	char *s = priv->linedisp.buf;
 	uint8_t buf[8];
 
 	put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 0);
@@ -662,14 +656,14 @@ static int ht16k33_fbdev_probe(struct device *dev, struct ht16k33_priv *priv,
 static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
 			     uint32_t brightness)
 {
-	struct ht16k33_seg *seg = &priv->seg;
+	struct linedisp *linedisp = &priv->linedisp;
 	int err;
 
 	err = ht16k33_brightness_set(priv, brightness);
 	if (err)
 		return err;
 
-	return linedisp_register(&seg->linedisp, dev, 4, &ht16k33_linedisp_ops);
+	return linedisp_register(linedisp, dev, 4, &ht16k33_linedisp_ops);
 }
 
 static int ht16k33_probe(struct i2c_client *client)
@@ -756,7 +750,7 @@ static void ht16k33_remove(struct i2c_client *client)
 
 	case DISP_QUAD_7SEG:
 	case DISP_QUAD_14SEG:
-		linedisp_unregister(&priv->seg.linedisp);
+		linedisp_unregister(&priv->linedisp);
 		break;
 
 	default:
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v3 7/9] auxdisplay: ht16k33: Drop struct ht16k33_seg

[Geert Uytterhoeven]

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> The struct ht16k33_seg is repeating struct linedisp. Use the latter
> directly.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

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

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

[PATCH v3 8/9] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Andy Shevchenko]

Add initial device tree documentation for Maxim MAX6958/6959.
As per reviewer's request mention the fact of absence reset and
power enable pins, since the hardware is quite simple.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/auxdisplay/maxim,max6959.yaml    | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
new file mode 100644
index 000000000000..6c78bb185348
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/maxim,max6959.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MAX6958/6959 7-segment LED display controller
+
+maintainers:
+  - Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+
+description:
+  The Maxim MAX6958/6959 7-segment LED display controller provides
+  an I2C interface to up to four 7-segment LED digits. The MAX6959,
+  in comparison to MAX6958, adds input support. Type of the chip can
+  be autodetected via specific register read, and hence the features
+  may be enabled in the driver at run-time, in case they are requested
+  via Device Tree. A given hardware is simple and does not provide
+  any additional pins, such as reset or enable.
+
+properties:
+  compatible:
+    const: maxim,max6959
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        display-controller@38 {
+            compatible = "maxim,max6959";
+            reg = <0x38>;
+        };
+    };
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v3 8/9] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Add initial device tree documentation for Maxim MAX6958/6959.
> As per reviewer's request mention the fact of absence reset and
> power enable pins, since the hardware is quite simple.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/maxim,max6959.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MAX6958/6959 7-segment LED display controller
> +
> +maintainers:
> +  - Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> +
> +description:
> +  The Maxim MAX6958/6959 7-segment LED display controller provides
> +  an I2C interface to up to four 7-segment LED digits. The MAX6959,
> +  in comparison to MAX6958, adds input support. Type of the chip can
> +  be autodetected via specific register read, and hence the features
> +  may be enabled in the driver at run-time, in case they are requested
> +  via Device Tree. A given hardware is simple and does not provide
> +  any additional pins, such as reset or enable.
> +
> +properties:
> +  compatible:
> +    const: maxim,max6959
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg

To handle cases where less than 4 characters are wired
(based on hit,hd44780.yaml):

    display-width-chars:
      description: Width of the display, in character cells.
      minimum: 1
      maximum: 4
      default: 4

The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 8/9] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 04:52:34PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> To handle cases where less than 4 characters are wired
> (based on hit,hd44780.yaml):
> 
>     display-width-chars:
>       description: Width of the display, in character cells.
>       minimum: 1
>       maximum: 4
>       default: 4

As discussed in the patch 9 this seems to be a material for other update.

> The rest LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

I'll take this tag when applying the patch. Tell me if it's not the case.
Thank you!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

[Andy Shevchenko]

Add initial driver for the MAX6958 and MAX6959 7-segment LED
controllers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/Kconfig   |  14 +++
 drivers/auxdisplay/Makefile  |   1 +
 drivers/auxdisplay/max6959.c | 194 +++++++++++++++++++++++++++++++++++
 3 files changed, 209 insertions(+)
 create mode 100644 drivers/auxdisplay/max6959.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 109ac253d160..bc295ede3c2c 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -59,6 +59,20 @@ config LCD2S
 	  is a simple single color character display. You have to connect it
 	  to an I2C bus.
 
+config MAX6959
+	tristate "Maxim MAX6958/6959 7-segment LED controller"
+	depends on I2C
+	select REGMAP_I2C
+	select LINEDISP
+	help
+	  If you say yes here you get support for the following Maxim chips
+	  (I2C 7-segment LED display controller):
+	  - MAX6958
+	  - MAX6959 (input support)
+
+	  This driver can also be built as a module. If so, the module
+	  will be called max6959.
+
 menuconfig PARPORT_PANEL
 	tristate "Parallel port LCD/Keypad Panel support"
 	depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 9197ea34e2d6..9718aedf6ee2 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -15,5 +15,6 @@ obj-$(CONFIG_CFAG12864B)	+= cfag12864b.o cfag12864bfb.o
 obj-$(CONFIG_LINEDISP)		+= line-display.o
 obj-$(CONFIG_IMG_ASCII_LCD)	+= img-ascii-lcd.o
 obj-$(CONFIG_HT16K33)		+= ht16k33.o
+obj-$(CONFIG_MAX6959)		+= max6959.o
 
 obj-$(CONFIG_ARM_CHARLCD)	+= arm-charlcd.o
diff --git a/drivers/auxdisplay/max6959.c b/drivers/auxdisplay/max6959.c
new file mode 100644
index 000000000000..5519c014bd29
--- /dev/null
+++ b/drivers/auxdisplay/max6959.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MAX6958/6959 7-segment LED display controller
+ * Datasheet:
+ * https://www.analog.com/media/en/technical-documentation/data-sheets/MAX6958-MAX6959.pdf
+ *
+ * Copyright (c) 2024, Intel Corporation.
+ * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ */
+#include <linux/array_size.h>
+#include <linux/bitrev.h>
+#include <linux/bits.h>
+#include <linux/container_of.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+#include <linux/map_to_7segment.h>
+
+#include "line-display.h"
+
+/* Registers */
+#define REG_DECODE_MODE			0x01
+#define REG_INTENSITY			0x02
+#define REG_SCAN_LIMIT			0x03
+#define REG_CONFIGURATION		0x04
+#define REG_CONFIGURATION_S_BIT		BIT(0)
+
+#define REG_DIGIT(x)			(0x20 + (x))
+#define REG_DIGIT0			0x20
+#define REG_DIGIT1			0x21
+#define REG_DIGIT2			0x22
+#define REG_DIGIT3			0x23
+
+#define REG_SEGMENTS			0x24
+#define REG_MAX				REG_SEGMENTS
+
+struct max6959_priv {
+	struct linedisp linedisp;
+	struct delayed_work work;
+	struct regmap *regmap;
+};
+
+static void max6959_disp_update(struct work_struct *work)
+{
+	struct max6959_priv *priv = container_of(work, struct max6959_priv, work.work);
+	struct linedisp *linedisp = &priv->linedisp;
+	struct linedisp_map *map = linedisp->map;
+	char *s = linedisp->buf;
+	u8 buf[4];
+
+	/* Map segments according to datasheet */
+	buf[0] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
+	buf[1] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
+	buf[2] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
+	buf[3] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
+
+	regmap_bulk_write(priv->regmap, REG_DIGIT(0), buf, ARRAY_SIZE(buf));
+}
+
+static int max6959_linedisp_get_map_type(struct linedisp *linedisp)
+{
+	struct max6959_priv *priv = container_of(linedisp, struct max6959_priv, linedisp);
+
+	INIT_DELAYED_WORK(&priv->work, max6959_disp_update);
+	return LINEDISP_MAP_SEG7;
+}
+
+static void max6959_linedisp_update(struct linedisp *linedisp)
+{
+	struct max6959_priv *priv = container_of(linedisp, struct max6959_priv, linedisp);
+
+	schedule_delayed_work(&priv->work, 0);
+}
+
+static const struct linedisp_ops max6959_linedisp_ops = {
+	.get_map_type = max6959_linedisp_get_map_type,
+	.update = max6959_linedisp_update,
+};
+
+static int max6959_enable(struct max6959_priv *priv, bool enable)
+{
+	u8 mask = REG_CONFIGURATION_S_BIT;
+	u8 value = enable ? mask : 0;
+
+	return regmap_update_bits(priv->regmap, REG_CONFIGURATION, mask, value);
+}
+
+static void max6959_power_off(void *priv)
+{
+	max6959_enable(priv, false);
+}
+
+static int max6959_power_on(struct max6959_priv *priv)
+{
+	struct device *dev = regmap_get_device(priv->regmap);
+	int ret;
+
+	ret = max6959_enable(priv, true);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, max6959_power_off, priv);
+}
+
+static const struct regmap_config max6959_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = REG_MAX,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int max6959_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct max6959_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = devm_regmap_init_i2c(client, &max6959_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	ret = max6959_power_on(priv);
+	if (ret)
+		return ret;
+
+	ret = linedisp_register(&priv->linedisp, dev, 4, &max6959_linedisp_ops);
+	if (ret)
+		return ret;
+
+	i2c_set_clientdata(client, priv);
+
+	return 0;
+}
+
+static void max6959_i2c_remove(struct i2c_client *client)
+{
+	struct max6959_priv *priv = i2c_get_clientdata(client);
+
+	cancel_delayed_work_sync(&priv->work);
+	linedisp_unregister(&priv->linedisp);
+}
+
+static int max6959_suspend(struct device *dev)
+{
+	return max6959_enable(dev_get_drvdata(dev), false);
+}
+
+static int max6959_resume(struct device *dev)
+{
+	return max6959_enable(dev_get_drvdata(dev), true);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(max6959_pm_ops, max6959_suspend, max6959_resume);
+
+static const struct i2c_device_id max6959_i2c_id[] = {
+	{ "max6959" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max6959_i2c_id);
+
+static const struct of_device_id max6959_of_table[] = {
+	{ .compatible = "maxim,max6959" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max6959_of_table);
+
+static struct i2c_driver max6959_i2c_driver = {
+	.driver = {
+		.name = "max6959",
+		.pm = pm_sleep_ptr(&max6959_pm_ops),
+		.of_match_table = max6959_of_table,
+	},
+	.probe = max6959_i2c_probe,
+	.remove = max6959_i2c_remove,
+	.id_table = max6959_i2c_id,
+};
+module_i2c_driver(max6959_i2c_driver);
+
+MODULE_DESCRIPTION("MAX6958/6959 7-segment LED controller");
+MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LINEDISP);
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Add initial driver for the MAX6958 and MAX6959 7-segment LED
> controllers.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

LGTM, so
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

> --- /dev/null
> +++ b/drivers/auxdisplay/max6959.c

> +static void max6959_disp_update(struct work_struct *work)
> +{
> +       struct max6959_priv *priv = container_of(work, struct max6959_priv, work.work);
> +       struct linedisp *linedisp = &priv->linedisp;
> +       struct linedisp_map *map = linedisp->map;
> +       char *s = linedisp->buf;
> +       u8 buf[4];
> +
> +       /* Map segments according to datasheet */
> +       buf[0] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> +       buf[1] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> +       buf[2] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> +       buf[3] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;

for (unsigned int i = 0; i < linedisp->num_chars; i++) { ... }

> +
> +       regmap_bulk_write(priv->regmap, REG_DIGIT(0), buf, ARRAY_SIZE(buf));

linedisp->num_chars

> +}

> +static int max6959_i2c_probe(struct i2c_client *client)
> +{
> +       struct device *dev = &client->dev;
> +       struct max6959_priv *priv;
> +       int ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->regmap = devm_regmap_init_i2c(client, &max6959_regmap_config);
> +       if (IS_ERR(priv->regmap))
> +               return PTR_ERR(priv->regmap);
> +
> +       ret = max6959_power_on(priv);
> +       if (ret)
> +               return ret;
> +
> +       ret = linedisp_register(&priv->linedisp, dev, 4, &max6959_linedisp_ops);

+ device_property_read_u32(dev, "display-width-chars", ...) handling.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 05:01:46PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Add initial driver for the MAX6958 and MAX6959 7-segment LED
> > controllers.

> LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks, but see below.

...

> > +       u8 buf[4];
> > +
> > +       /* Map segments according to datasheet */
> > +       buf[0] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > +       buf[1] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > +       buf[2] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > +       buf[3] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> 
> for (unsigned int i = 0; i < linedisp->num_chars; i++) { ... }
> 
> > +
> > +       regmap_bulk_write(priv->regmap, REG_DIGIT(0), buf, ARRAY_SIZE(buf));
> 
> linedisp->num_chars

Maybe, but then we probably want to synchronize the 4 there and here as we
can't have VLA on stack.

> > +}

...

> > +       ret = linedisp_register(&priv->linedisp, dev, 4, &max6959_linedisp_ops);
> 
> + device_property_read_u32(dev, "display-width-chars", ...) handling.

Not sure it should be part of this series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 26, 2024 at 5:17 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 26, 2024 at 05:01:46PM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > Add initial driver for the MAX6958 and MAX6959 7-segment LED
> > > controllers.
>
> > LGTM, so
> > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Thanks, but see below.
>
> ...
>
> > > +       u8 buf[4];
> > > +
> > > +       /* Map segments according to datasheet */
> > > +       buf[0] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > > +       buf[1] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > > +       buf[2] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > > +       buf[3] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> >
> > for (unsigned int i = 0; i < linedisp->num_chars; i++) { ... }
> >
> > > +
> > > +       regmap_bulk_write(priv->regmap, REG_DIGIT(0), buf, ARRAY_SIZE(buf));
> >
> > linedisp->num_chars
>
> Maybe, but then we probably want to synchronize the 4 there and here as we
> can't have VLA on stack.

You can still keep the maximum buf[4], so no VLA needed?

>
> > > +}
>
> ...
>
> > > +       ret = linedisp_register(&priv->linedisp, dev, 4, &max6959_linedisp_ops);
> >
> > + device_property_read_u32(dev, "display-width-chars", ...) handling.
>
> Not sure it should be part of this series.

Fair enough, so please go ahead without.

Even "display-width-chars" might not cover all cases, as a board
may use any subset of the 4 DIGn signals, even in any order...

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 05:01:46PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Add initial driver for the MAX6958 and MAX6959 7-segment LED
> > controllers.

...

> LGTM, so
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

As per discussion, let consider maximum digits as a next feature coming later
on. I'll take this tag, tell me if I shouldn't.
Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

[Andy Shevchenko]

On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> Add a new initial driver for Maxim MAX6958/6959 chips.
> While developing that driver I realised that there is a lot
> of duplication between ht16k33 and a new one. Hence set of
> cleanups and refactorings.
> 
> Note, the new driver has minimum support of the hardware and
> I have plans to cover more features in the future.

Geert, would it be possible to give one more round of reviewing/testing
this week? I want to close auxdisplay for next merge window next week.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

[Geert Uytterhoeven]

Hi Andy,

On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > Add a new initial driver for Maxim MAX6958/6959 chips.
> > While developing that driver I realised that there is a lot
> > of duplication between ht16k33 and a new one. Hence set of
> > cleanups and refactorings.
> >
> > Note, the new driver has minimum support of the hardware and
> > I have plans to cover more features in the future.
>
> Geert, would it be possible to give one more round of reviewing/testing
> this week? I want to close auxdisplay for next merge window next week.

For 1-7 (linedisp and ht16k33):
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

I hope to get to the actual review later...

Gr{oetje,eeting}s,

                        Geert


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

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

Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 02:56:35PM +0100, Geert Uytterhoeven wrote:
> On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > > Add a new initial driver for Maxim MAX6958/6959 chips.
> > > While developing that driver I realised that there is a lot
> > > of duplication between ht16k33 and a new one. Hence set of
> > > cleanups and refactorings.
> > >
> > > Note, the new driver has minimum support of the hardware and
> > > I have plans to cover more features in the future.
> >
> > Geert, would it be possible to give one more round of reviewing/testing
> > this week? I want to close auxdisplay for next merge window next week.
> 
> For 1-7 (linedisp and ht16k33):
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thank you!

> I hope to get to the actual review later...

Hope you will find time soon...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

[Andy Shevchenko]

On Thu, Feb 22, 2024 at 02:56:35PM +0100, Geert Uytterhoeven wrote:
> On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > > Add a new initial driver for Maxim MAX6958/6959 chips.
> > > While developing that driver I realised that there is a lot
> > > of duplication between ht16k33 and a new one. Hence set of
> > > cleanups and refactorings.
> > >
> > > Note, the new driver has minimum support of the hardware and
> > > I have plans to cover more features in the future.
> >
> > Geert, would it be possible to give one more round of reviewing/testing
> > this week? I want to close auxdisplay for next merge window next week.
> 
> For 1-7 (linedisp and ht16k33):
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thank you for the testing and review, I have pushed patches 2-7, postponed
patch 1 and will see what I can do with patches 8-9.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 06:24:29PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 02:56:35PM +0100, Geert Uytterhoeven wrote:
> > On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > > > Add a new initial driver for Maxim MAX6958/6959 chips.
> > > > While developing that driver I realised that there is a lot
> > > > of duplication between ht16k33 and a new one. Hence set of
> > > > cleanups and refactorings.
> > > >
> > > > Note, the new driver has minimum support of the hardware and
> > > > I have plans to cover more features in the future.
> > >
> > > Geert, would it be possible to give one more round of reviewing/testing
> > > this week? I want to close auxdisplay for next merge window next week.
> > 
> > For 1-7 (linedisp and ht16k33):
> > Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> Thank you for the testing and review, I have pushed patches 2-7, postponed
> patch 1 and will see what I can do with patches 8-9.

After discussion I have applied the patches 8-9 as is in this series with
Geert's tags. The other things are considered as new features that can be
implemented later on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 26, 2024 at 7:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 26, 2024 at 06:24:29PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 22, 2024 at 02:56:35PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > > > > Add a new initial driver for Maxim MAX6958/6959 chips.
> > > > > While developing that driver I realised that there is a lot
> > > > > of duplication between ht16k33 and a new one. Hence set of
> > > > > cleanups and refactorings.
> > > > >
> > > > > Note, the new driver has minimum support of the hardware and
> > > > > I have plans to cover more features in the future.
> > > >
> > > > Geert, would it be possible to give one more round of reviewing/testing
> > > > this week? I want to close auxdisplay for next merge window next week.
> > >
> > > For 1-7 (linedisp and ht16k33):
> > > Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> > Thank you for the testing and review, I have pushed patches 2-7, postponed
> > patch 1 and will see what I can do with patches 8-9.
>
> After discussion I have applied the patches 8-9 as is in this series with
> Geert's tags. The other things are considered as new features that can be
> implemented later on.

Thanks!

Gr{oetje,eeting}s,

                        Geert

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

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