[PATCH v2 00/15] auxdisplay: linedisp: Clean up and add new driver

[PATCH v2 00/15] 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 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 (15):
  auxdisplay: img-ascii-lcd: Make container_of() no-op for struct
    linedisp
  auxdisplay: linedisp: Free allocated resources in ->release()
  auxdisplay: linedisp: Use unique number for id
  auxdisplay: linedisp: Unshadow error codes in ->store()
  auxdisplay: linedisp: Add missing header(s)
  auxdisplay: linedisp: Move exported symbols to a namespace
  auxdisplay: linedisp: Group line display drivers together
  auxdisplay: linedisp: Provide struct linedisp_ops for future extension
  auxdisplay: linedisp: Add support for overriding character mapping
  auxdisplay: linedisp: Provide a small buffer in the struct linedisp
  auxdisplay: ht16k33: Move ht16k33_linedisp_ops down
  auxdisplay: ht16k33: Switch to use line display character mapping
  auxdisplay: ht16k33: Use buffer from struct linedisp
  dt-bindings: auxdisplay: Add Maxim MAX6958/6959
  auxdisplay: Add driver for MAX695x 7-segment LED controllers

 .../bindings/auxdisplay/maxim,max6959.yaml    |  35 +++
 drivers/auxdisplay/Kconfig                    |  40 ++--
 drivers/auxdisplay/Makefile                   |  13 +-
 drivers/auxdisplay/ht16k33.c                  | 145 +++++--------
 drivers/auxdisplay/img-ascii-lcd.c            |  24 ++-
 drivers/auxdisplay/line-display.c             | 162 ++++++++++++--
 drivers/auxdisplay/line-display.h             |  57 ++++-
 drivers/auxdisplay/max6959.c                  | 200 ++++++++++++++++++
 8 files changed, 530 insertions(+), 146 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 v2 01/15] auxdisplay: img-ascii-lcd: Make container_of() no-op for struct linedisp

[Andy Shevchenko]

Move embedded struct linedisp member to make container_of() no-op.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/img-ascii-lcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index 56efda0740fb..09014ada38bd 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -32,21 +32,21 @@ struct img_ascii_lcd_config {
 
 /**
  * struct img_ascii_lcd_ctx - Private data structure
+ * @linedisp: line display structure
  * @base: the base address of the LCD registers
  * @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
- * @linedisp: line display structure
  * @curr: the string currently displayed on the LCD
  */
 struct img_ascii_lcd_ctx {
+	struct linedisp linedisp;
 	union {
 		void __iomem *base;
 		struct regmap *regmap;
 	};
 	u32 offset;
 	const struct img_ascii_lcd_config *cfg;
-	struct linedisp linedisp;
 	char curr[] __aligned(8);
 };
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 01/15] auxdisplay: img-ascii-lcd: Make container_of() no-op for struct linedisp

[Geert Uytterhoeven]

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Move embedded struct linedisp member to make container_of() no-op.
>
> 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 v2 02/15] auxdisplay: linedisp: Free allocated resources in ->release()

[Andy Shevchenko]

While there is no issue currently with the resources allocation,
the code may still be made more robust by deallocating message
in the ->release() callback.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/line-display.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 03e7f104aa1a..310e9bfb41ae 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -188,8 +188,16 @@ static struct attribute *linedisp_attrs[] = {
 };
 ATTRIBUTE_GROUPS(linedisp);
 
+static void linedisp_release(struct device *dev)
+{
+	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+
+	kfree(linedisp->message);
+}
+
 static const struct device_type linedisp_type = {
 	.groups	= linedisp_groups,
+	.release = linedisp_release,
 };
 
 /**
@@ -253,7 +261,6 @@ void linedisp_unregister(struct linedisp *linedisp)
 {
 	device_del(&linedisp->dev);
 	del_timer_sync(&linedisp->timer);
-	kfree(linedisp->message);
 	put_device(&linedisp->dev);
 }
 EXPORT_SYMBOL_GPL(linedisp_unregister);
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 02/15] auxdisplay: linedisp: Free allocated resources in ->release()

[Geert Uytterhoeven]

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> While there is no issue currently with the resources allocation,
> the code may still be made more robust by deallocating message
> in the ->release() callback.
>
> 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 v2 03/15] auxdisplay: linedisp: Use unique number for id

[Andy Shevchenko]

The absence of decrementation of linedisp_id is incorrect in two ways,
i.e. it may cause:
- an ID exhaustion
- (and if the above is addressed) a duplicate id number may be allocated
  next time a device is added

Replace above mentioned approach by using IDA framework.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/line-display.c | 13 ++++++++++---
 drivers/auxdisplay/line-display.h |  2 ++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 310e9bfb41ae..c4dbb13293d1 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -11,6 +11,7 @@
 #include <generated/utsrelease.h>
 
 #include <linux/device.h>
+#include <linux/idr.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -188,11 +189,14 @@ static struct attribute *linedisp_attrs[] = {
 };
 ATTRIBUTE_GROUPS(linedisp);
 
+static DEFINE_IDA(linedisp_id);
+
 static void linedisp_release(struct device *dev)
 {
 	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
 
 	kfree(linedisp->message);
+	ida_free(&linedisp_id, linedisp->id);
 }
 
 static const struct device_type linedisp_type = {
@@ -214,7 +218,6 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 		      unsigned int num_chars, char *buf,
 		      void (*update)(struct linedisp *linedisp))
 {
-	static atomic_t linedisp_id = ATOMIC_INIT(-1);
 	int err;
 
 	memset(linedisp, 0, sizeof(*linedisp));
@@ -225,9 +228,13 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	linedisp->num_chars = num_chars;
 	linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
 
+	err = ida_alloc(&linedisp_id, GFP_KERNEL);
+	if (err < 0)
+		return err;
+	linedisp->id = err;
+
 	device_initialize(&linedisp->dev);
-	dev_set_name(&linedisp->dev, "linedisp.%lu",
-		     (unsigned long)atomic_inc_return(&linedisp_id));
+	dev_set_name(&linedisp->dev, "linedisp.%u", linedisp->id);
 
 	/* initialise a timer for scrolling the message */
 	timer_setup(&linedisp->timer, linedisp_scroll, 0);
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 0f5891d34c48..1fbe57fdc4cb 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -14,6 +14,7 @@
 /**
  * struct linedisp - character line display private data structure
  * @dev: the line display device
+ * @id: instance id of this display
  * @timer: timer used to implement scrolling
  * @update: function called to update the display
  * @buf: pointer to the buffer for the string currently displayed
@@ -25,6 +26,7 @@
  */
 struct linedisp {
 	struct device dev;
+	unsigned int id;
 	struct timer_list timer;
 	void (*update)(struct linedisp *linedisp);
 	char *buf;
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 03/15] auxdisplay: linedisp: Use unique number for id

[Geert Uytterhoeven]

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> The absence of decrementation of linedisp_id is incorrect in two ways,
> i.e. it may cause:
> - an ID exhaustion
> - (and if the above is addressed) a duplicate id number may be allocated
>   next time a device is added
>
> Replace above mentioned approach by using IDA framework.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

> --- a/drivers/auxdisplay/line-display.h
> +++ b/drivers/auxdisplay/line-display.h
> @@ -14,6 +14,7 @@
>  /**
>   * struct linedisp - character line display private data structure
>   * @dev: the line display device
> + * @id: instance id of this display
>   * @timer: timer used to implement scrolling
>   * @update: function called to update the display
>   * @buf: pointer to the buffer for the string currently displayed
> @@ -25,6 +26,7 @@
>   */
>  struct linedisp {
>         struct device dev;
> +       unsigned int id;

Note that there is a hole on 64-bit platforms.
Hence I'd move id below, so the hole is at the end of the
structure, and might be filled by future changes.

>         struct timer_list timer;
>         void (*update)(struct linedisp *linedisp);
>         char *buf;

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 v2 03/15] auxdisplay: linedisp: Use unique number for id

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 11:03:27AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> >  struct linedisp {
> >         struct device dev;
> > +       unsigned int id;
> 
> Note that there is a hole on 64-bit platforms.
> Hence I'd move id below, so the hole is at the end of the
> structure, and might be filled by future changes.

I had checked timer_list, but while it has holes, without debug it ends on
4-bytes boundary (without debug enabled), otherwise on 8-bytes.
Nevertheless, relying on the above seems fragile, so I follow your suggestion.
Thank you!

> >         struct timer_list timer;
> >         void (*update)(struct linedisp *linedisp);
> >         char *buf;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 04/15] auxdisplay: linedisp: Unshadow error codes in ->store()

[Andy Shevchenko]

kstrtox() may return different error codes.

Unshadow them in the ->store() callback to give better error report.

While at it, add missing kstrtox.h inclusion.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/line-display.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index c4dbb13293d1..8d91c2099661 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -12,6 +12,7 @@
 
 #include <linux/device.h>
 #include <linux/idr.h>
+#include <linux/kstrtox.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -166,9 +167,11 @@ static ssize_t scroll_step_ms_store(struct device *dev,
 {
 	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
 	unsigned int ms;
+	int err;
 
-	if (kstrtouint(buf, 10, &ms) != 0)
-		return -EINVAL;
+	err = kstrtouint(buf, 10, &ms);
+	if (err)
+		return err;
 
 	linedisp->scroll_rate = msecs_to_jiffies(ms);
 	if (linedisp->message && linedisp->message_len > linedisp->num_chars) {
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 04/15] auxdisplay: linedisp: Unshadow error codes in ->store()

[Geert Uytterhoeven]

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> kstrtox() may return different error codes.
>
> Unshadow them in the ->store() callback to give better error report.
>
> While at it, add missing kstrtox.h inclusion.
>
> 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 v2 05/15] auxdisplay: linedisp: Add missing header(s)

[Andy Shevchenko]

Do not imply that some of the generic headers may be always included.
Instead, include explicitly what we are direct user of.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/line-display.c | 3 +++
 drivers/auxdisplay/line-display.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 8d91c2099661..a0339e4b5939 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -10,8 +10,11 @@
 
 #include <generated/utsrelease.h>
 
+#include <linux/container_of.h>
 #include <linux/device.h>
+#include <linux/export.h>
 #include <linux/idr.h>
+#include <linux/jiffies.h>
 #include <linux/kstrtox.h>
 #include <linux/module.h>
 #include <linux/slab.h>
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 1fbe57fdc4cb..d72c1899dc50 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -11,6 +11,9 @@
 #ifndef _LINEDISP_H
 #define _LINEDISP_H
 
+#include <linux/device.h>
+#include <linux/timer_types.h>
+
 /**
  * struct linedisp - character line display private data structure
  * @dev: the line display device
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 05/15] auxdisplay: linedisp: Add missing header(s)

[Geert Uytterhoeven]

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Do not imply that some of the generic headers may be always included.
> Instead, include explicitly what we are direct user of.
>
> 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 v2 06/15] auxdisplay: linedisp: Move exported symbols to a namespace

[Andy Shevchenko]

Avoid unnecessary pollution of the global symbol namespace by
moving library functions in to a specific namespace and import
that into the drivers that make use of the functions.

For more info: https://lwn.net/Articles/760045/

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

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index a90430b7d07b..c6a42c5c128f 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -831,4 +831,5 @@ module_i2c_driver(ht16k33_driver);
 
 MODULE_DESCRIPTION("Holtek HT16K33 driver");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LINEDISP);
 MODULE_AUTHOR("Robin van der Gracht <robin@protonic.nl>");
diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index 09014ada38bd..c571e54d9eb5 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -298,3 +298,4 @@ module_platform_driver(img_ascii_lcd_driver);
 MODULE_DESCRIPTION("Imagination Technologies ASCII LCD Display");
 MODULE_AUTHOR("Paul Burton <paul.burton@mips.com>");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LINEDISP);
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index a0339e4b5939..8d0ebdf0f10d 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -263,7 +263,7 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	put_device(&linedisp->dev);
 	return err;
 }
-EXPORT_SYMBOL_GPL(linedisp_register);
+EXPORT_SYMBOL_NS_GPL(linedisp_register, LINEDISP);
 
 /**
  * linedisp_unregister - unregister a character line display
@@ -276,6 +276,6 @@ void linedisp_unregister(struct linedisp *linedisp)
 	del_timer_sync(&linedisp->timer);
 	put_device(&linedisp->dev);
 }
-EXPORT_SYMBOL_GPL(linedisp_unregister);
+EXPORT_SYMBOL_NS_GPL(linedisp_unregister, LINEDISP);
 
 MODULE_LICENSE("GPL");
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 06/15] auxdisplay: linedisp: Move exported symbols to a namespace

[Geert Uytterhoeven]

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Avoid unnecessary pollution of the global symbol namespace by
> moving library functions in to a specific namespace and import
> that into the drivers that make use of the functions.
>
> For more info: https://lwn.net/Articles/760045/
>
> 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 v2 07/15] auxdisplay: linedisp: Group line display drivers together

[Andy Shevchenko]

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(-)

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
-- 
2.43.0.rc1.1.gbec44491f096

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

[Geert Uytterhoeven]

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-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 v2 07/15] auxdisplay: linedisp: Group line display drivers together

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[Geert Uytterhoeven]

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

-- 
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 v2 07/15] auxdisplay: linedisp: Group line display drivers together

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 08/15] auxdisplay: linedisp: Provide struct linedisp_ops for future extension

[Andy Shevchenko]

Currently the line display library doesn't scale in case we want to
provide more operations. Prepare the library to take a newly created
struct linedisp_ops that scales.

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

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index c6a42c5c128f..0cdf3fbdf81e 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -448,6 +448,10 @@ static void ht16k33_linedisp_update(struct linedisp *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,
@@ -696,8 +700,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_update);
+	err = linedisp_register(&seg->linedisp, dev, 4, seg->curr, &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 c571e54d9eb5..c0ec7d9a1c62 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -22,12 +22,12 @@ struct img_ascii_lcd_ctx;
  * struct img_ascii_lcd_config - Configuration information about an LCD model
  * @num_chars: the number of characters the LCD can display
  * @external_regmap: true if registers are in a system controller, else false
- * @update: function called to update the LCD
+ * @ops: character line display operations
  */
 struct img_ascii_lcd_config {
 	unsigned int num_chars;
 	bool external_regmap;
-	void (*update)(struct linedisp *linedisp);
+	const struct linedisp_ops ops;
 };
 
 /**
@@ -75,7 +75,9 @@ static void boston_update(struct linedisp *linedisp)
 
 static struct img_ascii_lcd_config boston_config = {
 	.num_chars = 8,
-	.update = boston_update,
+	.ops = {
+		.update = boston_update,
+	},
 };
 
 /*
@@ -103,7 +105,9 @@ static void malta_update(struct linedisp *linedisp)
 static struct img_ascii_lcd_config malta_config = {
 	.num_chars = 8,
 	.external_regmap = true,
-	.update = malta_update,
+	.ops = {
+		.update = malta_update,
+	},
 };
 
 /*
@@ -203,7 +207,9 @@ static void sead3_update(struct linedisp *linedisp)
 static struct img_ascii_lcd_config sead3_config = {
 	.num_chars = 16,
 	.external_regmap = true,
-	.update = sead3_update,
+	.ops = {
+		.update = sead3_update,
+	},
 };
 
 static const struct of_device_id img_ascii_lcd_matches[] = {
@@ -247,8 +253,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->update);
+	err = linedisp_register(&ctx->linedisp, dev, cfg->num_chars, ctx->curr, &cfg->ops);
 	if (err)
 		return err;
 
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 8d0ebdf0f10d..6453a62f653f 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -50,7 +50,7 @@ static void linedisp_scroll(struct timer_list *t)
 	}
 
 	/* update the display */
-	linedisp->update(linedisp);
+	linedisp->ops->update(linedisp);
 
 	/* move on to the next character */
 	linedisp->scroll_pos++;
@@ -94,7 +94,7 @@ static int linedisp_display(struct linedisp *linedisp, const char *msg,
 		linedisp->message = NULL;
 		linedisp->message_len = 0;
 		memset(linedisp->buf, ' ', linedisp->num_chars);
-		linedisp->update(linedisp);
+		linedisp->ops->update(linedisp);
 		return 0;
 	}
 
@@ -216,20 +216,19 @@ static const struct device_type linedisp_type = {
  * @parent: parent device
  * @num_chars: the number of characters that can be displayed
  * @buf: pointer to a buffer that can hold @num_chars characters
- * @update: Function called to update the display.  This must not sleep!
+ * @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,
-		      void (*update)(struct linedisp *linedisp))
+		      unsigned int num_chars, char *buf, const struct linedisp_ops *ops)
 {
 	int err;
 
 	memset(linedisp, 0, sizeof(*linedisp));
 	linedisp->dev.parent = parent;
 	linedisp->dev.type = &linedisp_type;
-	linedisp->update = update;
+	linedisp->ops = ops;
 	linedisp->buf = buf;
 	linedisp->num_chars = num_chars;
 	linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index d72c1899dc50..a4f0d1bf5848 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -14,12 +14,22 @@
 #include <linux/device.h>
 #include <linux/timer_types.h>
 
+struct linedisp;
+
+/**
+ * struct linedisp_ops - character line display operations
+ * @update: Function called to update the display. This must not sleep!
+ */
+struct linedisp_ops {
+	void (*update)(struct linedisp *linedisp);
+};
+
 /**
  * struct linedisp - character line display private data structure
  * @dev: the line display device
  * @id: instance id of this display
  * @timer: timer used to implement scrolling
- * @update: function called to update the display
+ * @ops: character line display operations
  * @buf: pointer to the buffer for the string currently displayed
  * @message: the full message to display or scroll on the display
  * @num_chars: the number of characters that can be displayed
@@ -31,7 +41,7 @@ struct linedisp {
 	struct device dev;
 	unsigned int id;
 	struct timer_list timer;
-	void (*update)(struct linedisp *linedisp);
+	const struct linedisp_ops *ops;
 	char *buf;
 	char *message;
 	unsigned int num_chars;
@@ -41,8 +51,7 @@ struct linedisp {
 };
 
 int linedisp_register(struct linedisp *linedisp, struct device *parent,
-		      unsigned int num_chars, char *buf,
-		      void (*update)(struct linedisp *linedisp));
+		      unsigned int num_chars, char *buf, const struct linedisp_ops *ops);
 void linedisp_unregister(struct linedisp *linedisp);
 
 #endif /* LINEDISP_H */
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 08/15] auxdisplay: linedisp: Provide struct linedisp_ops for future extension

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Currently the line display library doesn't scale in case we want to
> provide more operations. Prepare the library to take a newly created
> struct linedisp_ops that scales.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

For the code changes:
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -696,8 +700,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_update);
> +       err = linedisp_register(&seg->linedisp, dev, 4, seg->curr, &ht16k33_linedisp_ops);

Please wrap this long line (everywhere).
All lines in these drivers fit in 80-columns before.

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 v2 08/15] auxdisplay: linedisp: Provide struct linedisp_ops for future extension

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 11:13:31AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > +       err = linedisp_register(&seg->linedisp, dev, 4, seg->curr, &ht16k33_linedisp_ops);
> 
> Please wrap this long line (everywhere).
> All lines in these drivers fit in 80-columns before.

Done.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 09/15] auxdisplay: linedisp: Add support for overriding character mapping

[Andy Shevchenko]

There is already the driver using character mapping table for
7 or 14 segment display. It is possible to override it. Make
the similar in the line display library to allow other drivers
to utilise the same functionality.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/line-display.c | 111 +++++++++++++++++++++++++++++-
 drivers/auxdisplay/line-display.h |  31 +++++++++
 2 files changed, 140 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 6453a62f653f..75852ce6cc8d 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -22,6 +22,9 @@
 #include <linux/sysfs.h>
 #include <linux/timer.h>
 
+#include <linux/map_to_7segment.h>
+#include <linux/map_to_14segment.h>
+
 #include "line-display.h"
 
 #define DEFAULT_SCROLL_RATE	(HZ / 2)
@@ -188,12 +191,71 @@ static ssize_t scroll_step_ms_store(struct device *dev,
 
 static DEVICE_ATTR_RW(scroll_step_ms);
 
+static ssize_t map_seg_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp_map *map = linedisp->map;
+
+	memcpy(buf, &map->map, map->size);
+	return map->size;
+}
+
+static ssize_t map_seg_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp_map *map = linedisp->map;
+
+	if (count != map->size)
+		return -EINVAL;
+
+	memcpy(&map->map, buf, count);
+	return count;
+}
+
+static const SEG7_DEFAULT_MAP(initial_map_seg7);
+static DEVICE_ATTR(map_seg7, 0644, map_seg_show, map_seg_store);
+
+static const SEG14_DEFAULT_MAP(initial_map_seg14);
+static DEVICE_ATTR(map_seg14, 0644, map_seg_show, map_seg_store);
+
 static struct attribute *linedisp_attrs[] = {
 	&dev_attr_message.attr,
 	&dev_attr_scroll_step_ms.attr,
-	NULL,
+	&dev_attr_map_seg7.attr,
+	&dev_attr_map_seg14.attr,
+	NULL
 };
-ATTRIBUTE_GROUPS(linedisp);
+
+static umode_t linedisp_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp_map *map = linedisp->map;
+	umode_t mode = attr->mode;
+
+	if (attr == &dev_attr_map_seg7.attr) {
+		if (!map)
+			return 0;
+		if (map->type != LINEDISP_MAP_SEG7)
+			return 0;
+	}
+
+	if (attr == &dev_attr_map_seg14.attr) {
+		if (!map)
+			return 0;
+		if (map->type != LINEDISP_MAP_SEG14)
+			return 0;
+	}
+
+	return mode;
+};
+
+static const struct attribute_group linedisp_group = {
+	.is_visible	= linedisp_attr_is_visible,
+	.attrs		= linedisp_attrs,
+};
+__ATTRIBUTE_GROUPS(linedisp);
 
 static DEFINE_IDA(linedisp_id);
 
@@ -201,6 +263,7 @@ static void linedisp_release(struct device *dev)
 {
 	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
 
+	kfree(linedisp->map);
 	kfree(linedisp->message);
 	ida_free(&linedisp_id, linedisp->id);
 }
@@ -210,6 +273,44 @@ static const struct device_type linedisp_type = {
 	.release = linedisp_release,
 };
 
+static int linedisp_init_map(struct linedisp *linedisp)
+{
+	struct linedisp_map *map;
+	int err;
+
+	if (!linedisp->ops->get_map_type)
+		return 0;
+
+	err = linedisp->ops->get_map_type(linedisp);
+	if (err < 0)
+		return err;
+
+	map = kmalloc(sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	map->type = err;
+
+	/* assign initial mapping */
+	switch (map->type) {
+	case LINEDISP_MAP_SEG7:
+		map->map.seg7 = initial_map_seg7;
+		map->size = sizeof(map->map.seg7);
+		break;
+	case LINEDISP_MAP_SEG14:
+		map->map.seg14 = initial_map_seg14;
+		map->size = sizeof(map->map.seg14);
+		break;
+	default:
+		kfree(map);
+		return -EINVAL;
+	}
+
+	linedisp->map = map;
+
+	return 0;
+}
+
 /**
  * linedisp_register - register a character line display
  * @linedisp: pointer to character line display structure
@@ -241,6 +342,11 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	device_initialize(&linedisp->dev);
 	dev_set_name(&linedisp->dev, "linedisp.%u", linedisp->id);
 
+	/* initialise a character mapping, if required */
+	err = linedisp_init_map(linedisp);
+	if (err)
+		goto out_put_device;
+
 	/* initialise a timer for scrolling the message */
 	timer_setup(&linedisp->timer, linedisp_scroll, 0);
 
@@ -259,6 +365,7 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	device_del(&linedisp->dev);
 out_del_timer:
 	del_timer_sync(&linedisp->timer);
+out_put_device:
 	put_device(&linedisp->dev);
 	return err;
 }
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index a4f0d1bf5848..65d782111f53 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -14,13 +14,43 @@
 #include <linux/device.h>
 #include <linux/timer_types.h>
 
+#include <linux/map_to_7segment.h>
+#include <linux/map_to_14segment.h>
+
 struct linedisp;
 
+/**
+ * enum linedisp_map_type - type of the character mapping
+ * @LINEDISP_MAP_SEG7: Map characters to 7 segment display
+ * @LINEDISP_MAP_SEG14: Map characters to 14 segment display
+ */
+enum linedisp_map_type {
+	LINEDISP_MAP_SEG7,
+	LINEDISP_MAP_SEG14,
+};
+
+/**
+ * struct linedisp_map - character mapping
+ * @type: type of the character mapping
+ * @map: conversion character mapping
+ * @size: size of the @map
+ */
+struct linedisp_map {
+	enum linedisp_map_type type;
+	union {
+		struct seg7_conversion_map seg7;
+		struct seg14_conversion_map seg14;
+	} map;
+	unsigned int size;
+};
+
 /**
  * struct linedisp_ops - character line display operations
+ * @get_map_type: Function called to get the character mapping, if required
  * @update: Function called to update the display. This must not sleep!
  */
 struct linedisp_ops {
+	int (*get_map_type)(struct linedisp *linedisp);
 	void (*update)(struct linedisp *linedisp);
 };
 
@@ -42,6 +72,7 @@ struct linedisp {
 	unsigned int id;
 	struct timer_list timer;
 	const struct linedisp_ops *ops;
+	struct linedisp_map *map;
 	char *buf;
 	char *message;
 	unsigned int num_chars;
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 09/15] auxdisplay: linedisp: Add support for overriding character mapping

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> There is already the driver using character mapping table for
> 7 or 14 segment display. It is possible to override it. Make
> the similar in the line display library to allow other drivers
> to utilise the same functionality.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

One question below.

> --- a/drivers/auxdisplay/line-display.c
> +++ b/drivers/auxdisplay/line-display.c

> +static const struct attribute_group linedisp_group = {
> +       .is_visible     = linedisp_attr_is_visible,

Shouldn't that be .is_bin_visible?

> +       .attrs          = linedisp_attrs,

Likewise, .bin_attrs?
But that is a pre-existing issue in the ht16k33 driver.

> +};
> +__ATTRIBUTE_GROUPS(linedisp);

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 v2 09/15] auxdisplay: linedisp: Add support for overriding character mapping

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 11:36:47AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > +static const struct attribute_group linedisp_group = {
> > +       .is_visible     = linedisp_attr_is_visible,
> 
> Shouldn't that be .is_bin_visible?
> 
> > +       .attrs          = linedisp_attrs,
> 
> Likewise, .bin_attrs?
> But that is a pre-existing issue in the ht16k33 driver.

I was wondering myself, but we have no infrastructure for that
(there are no DEVICE_BIN_ATTR_*() helpers, nor bin_attr member
 in struct device attribute).

As you pointed out, it's preexisted issue and should be addressed
separately and not only in this driver.

> > +};
> > +__ATTRIBUTE_GROUPS(linedisp);

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp

[Andy Shevchenko]

There is a driver that uses small buffer for the string, when we
add a new one, we may avoid duplication and use one provided by
the line display library. Allow user to skip buffer pointer when
registering a device.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/line-display.c | 4 ++--
 drivers/auxdisplay/line-display.h | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 75852ce6cc8d..d730cd0e1d03 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -330,8 +330,8 @@ 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->buf = buf ? buf : linedisp->curr;
+	linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
 	linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
 
 	err = ida_alloc(&linedisp_id, GFP_KERNEL);
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 65d782111f53..4c354b8f376e 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -54,12 +54,15 @@ struct linedisp_ops {
 	void (*update)(struct linedisp *linedisp);
 };
 
+#define LINEDISP_DEFAULT_BUF_SZ		8u
+
 /**
  * struct linedisp - character line display private data structure
  * @dev: the line display device
  * @id: instance id of this display
  * @timer: timer used to implement scrolling
  * @ops: character line display operations
+ * @curr: fallback buffer for the string
  * @buf: pointer to the buffer for the string currently displayed
  * @message: the full message to display or scroll on the display
  * @num_chars: the number of characters that can be displayed
@@ -73,6 +76,7 @@ struct linedisp {
 	struct timer_list timer;
 	const struct linedisp_ops *ops;
 	struct linedisp_map *map;
+	char curr[LINEDISP_DEFAULT_BUF_SZ];
 	char *buf;
 	char *message;
 	unsigned int num_chars;
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> There is a driver that uses small buffer for the string, when we
> add a new one, we may avoid duplication and use one provided by
> the line display library. Allow user to skip buffer pointer when
> registering a device.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/drivers/auxdisplay/line-display.c
> +++ b/drivers/auxdisplay/line-display.c
> @@ -330,8 +330,8 @@ 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->buf = buf ? buf : linedisp->curr;
> +       linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);

I think it would be safer to return an error if buf == NULL and
num_chars < LINEDISP_DEFAULT_BUF_SZ.
Else a careless driver that doesn't check linedisp->num_chars might
overflow the buffer.

>         linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
>
>         err = ida_alloc(&linedisp_id, GFP_KERNEL);

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 v2 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 11:40:44AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > +       linedisp->buf = buf ? buf : linedisp->curr;
> > +       linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
> 
> I think it would be safer to return an error if buf == NULL and
> num_chars < LINEDISP_DEFAULT_BUF_SZ.

I think you meant >= ?

> Else a careless driver that doesn't check linedisp->num_chars might
> overflow the buffer.

Okay, check has been added.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 02:17:00PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 15, 2024 at 11:40:44AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > +       linedisp->buf = buf ? buf : linedisp->curr;
> > > +       linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
> > 
> > I think it would be safer to return an error if buf == NULL and
> > num_chars < LINEDISP_DEFAULT_BUF_SZ.
> 
> I think you meant >= ?
> 
> > Else a careless driver that doesn't check linedisp->num_chars might
> > overflow the buffer.
> 
> Okay, check has been added.

Hold on, but I have min() being called, isn't it enough?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp

[Geert Uytterhoeven]

Hi Andy,

On Thu, Feb 15, 2024 at 1:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Feb 15, 2024 at 02:17:00PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 15, 2024 at 11:40:44AM +0100, Geert Uytterhoeven wrote:
> > > On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > +       linedisp->buf = buf ? buf : linedisp->curr;
> > > > +       linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
> > >
> > > I think it would be safer to return an error if buf == NULL and
> > > num_chars < LINEDISP_DEFAULT_BUF_SZ.
> >
> > I think you meant >= ?

Oops, yes/

> >
> > > Else a careless driver that doesn't check linedisp->num_chars might
> > > overflow the buffer.
> >
> > Okay, check has been added.
>
> Hold on, but I have min() being called, isn't it enough?

Yes you have.

A careless driver might not use linedisp->num_chars later, but instead
just hardcode e.g. memcpy(linedisp->buf, source, LARGE_BUF_SIZE).

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 v2 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 01:33:29PM +0100, Geert Uytterhoeven wrote:
> On Thu, Feb 15, 2024 at 1:19 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 15, 2024 at 02:17:00PM +0200, Andy Shevchenko wrote:
> > > On Thu, Feb 15, 2024 at 11:40:44AM +0100, Geert Uytterhoeven wrote:
> > > > On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > +       linedisp->buf = buf ? buf : linedisp->curr;
> > > > > +       linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
> > > >
> > > > I think it would be safer to return an error if buf == NULL and
> > > > num_chars < LINEDISP_DEFAULT_BUF_SZ.
> > >
> > > I think you meant >= ?
> 
> Oops, yes/
> 
> > >
> > > > Else a careless driver that doesn't check linedisp->num_chars might
> > > > overflow the buffer.
> > >
> > > Okay, check has been added.
> >
> > Hold on, but I have min() being called, isn't it enough?
> 
> Yes you have.
> 
> A careless driver might not use linedisp->num_chars later, but instead
> just hardcode e.g. memcpy(linedisp->buf, source, LARGE_BUF_SIZE).

I see the point, yes, we need an additional check.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 11/15] 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.

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 0cdf3fbdf81e..75c4a8d31642 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -440,18 +440,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,
@@ -489,6 +477,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

Re: [PATCH v2 11/15] auxdisplay: ht16k33: Move ht16k33_linedisp_ops down

[Robin van der Gracht]

On Mon, 12 Feb 2024 19:01:44 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> We will need the update functions to be defined before
> ht16k33_linedisp_ops. Move the latter down in the code.
> No functional change intended.
> 
> 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 0cdf3fbdf81e..75c4a8d31642 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -440,18 +440,6 @@ static void ht16k33_keypad_stop(struct input_dev *dev)
>  	disable_irq(keypad->client->irq);
>  }

Acked-by: Robin van der Gracht <robin@protonic.nl>

Re: [PATCH v2 11/15] auxdisplay: ht16k33: Move ht16k33_linedisp_ops down

[Geert Uytterhoeven]

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> We will need the update functions to be defined before
> ht16k33_linedisp_ops. Move the latter down in the code.
> No functional change intended.
>
> 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 v2 12/15] 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.

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

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 75c4a8d31642..b104f08252dd 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -87,11 +87,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;
 	char curr[4];
 };
 
@@ -135,33 +130,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;
@@ -445,18 +413,20 @@ 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;
+	struct linedisp *linedisp = &seg->linedisp;
+	struct linedisp_map *map = linedisp->map;
 	char *s = seg->curr;
 	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);
 }
@@ -466,17 +436,39 @@ 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;
+	struct linedisp *linedisp = &seg->linedisp;
+	struct linedisp_map *map = linedisp->map;
 	char *s = seg->curr;
 	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 = container_of(linedisp, struct ht16k33_priv,
+						 seg.linedisp);
+
+	switch (priv->type) {
+	case DISP_MATRIX:
+		/* not handled here */
+		return -EINVAL;
+
+	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;
+	}
+}
+
 static void ht16k33_linedisp_update(struct linedisp *linedisp)
 {
 	struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
@@ -486,6 +478,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,
 };
 
@@ -677,39 +670,7 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
 	if (err)
 		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;
-		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;
-	}
-	if (err)
-		return err;
-
-	err = linedisp_register(&seg->linedisp, dev, 4, seg->curr, &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, seg->curr, &ht16k33_linedisp_ops);
 }
 
 static int ht16k33_probe(struct i2c_client *client)
@@ -794,8 +755,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;
 	}
 }
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 12/15] auxdisplay: ht16k33: Switch to use line display character mapping

[Robin van der Gracht]

On Mon, 12 Feb 2024 19:01:45 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Since line display library supports necessary bits to map the characters
> (if required), switch this driver to use that.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/auxdisplay/ht16k33.c | 109 +++++++++++------------------------
>  1 file changed, 34 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index 75c4a8d31642..b104f08252dd 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c

Acked-by: Robin van der Gracht <robin@protonic.nl>

Re: [PATCH v2 12/15] auxdisplay: ht16k33: Switch to use line display character mapping

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Since line display library supports necessary bits to map the characters
> (if required), switch this driver to use that.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c

> +static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
> +{
> +       struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
> +                                                seg.linedisp);
> +
> +       switch (priv->type) {
> +       case DISP_MATRIX:
> +               /* not handled here */
> +               return -EINVAL;
> +
> +       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;
> +       }

error: control reaches end of non-void function [-Werror=return-type]

Missing "return -EINVAL";

This case cannot happen, so it wasn't handled in the old code.
But with the new code, it fails at compile-time.

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 v2 12/15] auxdisplay: ht16k33: Switch to use line display character mapping

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 09:16:05AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> > +static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
> > +{
> > +       struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
> > +                                                seg.linedisp);
> > +
> > +       switch (priv->type) {
> > +       case DISP_MATRIX:
> > +               /* not handled here */
> > +               return -EINVAL;
> > +
> > +       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;
> > +       }
> 
> error: control reaches end of non-void function [-Werror=return-type]
> 
> Missing "return -EINVAL";
> 
> This case cannot happen, so it wasn't handled in the old code.
> But with the new code, it fails at compile-time.

What is the command line and compiler you are using?
I have compiled this code without issues.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 12/15] auxdisplay: ht16k33: Switch to use line display character mapping

[Geert Uytterhoeven]

On Thu, Feb 15, 2024 at 1:20 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 15, 2024 at 09:16:05AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > > +static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
> > > +{
> > > +       struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
> > > +                                                seg.linedisp);
> > > +
> > > +       switch (priv->type) {
> > > +       case DISP_MATRIX:
> > > +               /* not handled here */
> > > +               return -EINVAL;
> > > +
> > > +       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;
> > > +       }
> >
> > error: control reaches end of non-void function [-Werror=return-type]
> >
> > Missing "return -EINVAL";
> >
> > This case cannot happen, so it wasn't handled in the old code.
> > But with the new code, it fails at compile-time.
>
> What is the command line and compiler you are using?
> I have compiled this code without issues.

riscv64-linux-gnu-gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04)
aarch64-linux-gnu-gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04)

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 v2 12/15] auxdisplay: ht16k33: Switch to use line display character mapping

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Since line display library supports necessary bits to map the characters
> (if required), switch this driver to use that.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

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

Minor nits below.

> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -445,18 +413,20 @@ 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;
> +       struct linedisp *linedisp = &seg->linedisp;
> +       struct linedisp_map *map = linedisp->map;

struct linedisp_map *map = seg.linedisp->map;

as linedisp is not used below.

>         char *s = seg->curr;
>         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);
>  }
> @@ -466,17 +436,39 @@ 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;
> +       struct linedisp *linedisp = &seg->linedisp;
> +       struct linedisp_map *map = linedisp->map;

Likewise.

>         char *s = seg->curr;
>         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);
>  }

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 v2 12/15] auxdisplay: ht16k33: Switch to use line display character mapping

[Geert Uytterhoeven]

On Thu, Feb 15, 2024 at 11:44 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Since line display library supports necessary bits to map the characters
> > (if required), switch this driver to use that.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

With the missing return-statement reported before added, of course ;-)

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 v2 13/15] auxdisplay: ht16k33: Use buffer from struct linedisp

[Andy Shevchenko]

struct linedips embedds a small buffer for the string that we may reuse.
Update the driver accordingly.

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

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index b104f08252dd..08cc05b9d216 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -85,11 +85,6 @@ struct ht16k33_fbdev {
 	uint8_t *cache;
 };
 
-struct ht16k33_seg {
-	struct linedisp linedisp;
-	char curr[4];
-};
-
 struct ht16k33_priv {
 	struct i2c_client *client;
 	struct delayed_work work;
@@ -97,7 +92,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;
@@ -412,10 +407,9 @@ 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;
-	struct linedisp *linedisp = &seg->linedisp;
+	struct linedisp *linedisp = &priv->linedisp;
 	struct linedisp_map *map = linedisp->map;
-	char *s = seg->curr;
+	char *s = linedisp->curr;
 	uint8_t buf[9];
 
 	buf[0] = map_to_seg7(&map->map.seg7, *s++);
@@ -435,10 +429,9 @@ 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;
-	struct linedisp *linedisp = &seg->linedisp;
+	struct linedisp *linedisp = &priv->linedisp;
 	struct linedisp_map *map = linedisp->map;
-	char *s = seg->curr;
+	char *s = linedisp->curr;
 	uint8_t buf[8];
 
 	put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 0);
@@ -451,8 +444,7 @@ static void ht16k33_seg14_update(struct work_struct *work)
 
 static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
 {
-	struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
-						 seg.linedisp);
+	struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv, linedisp);
 
 	switch (priv->type) {
 	case DISP_MATRIX:
@@ -471,8 +463,7 @@ static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
 
 static void ht16k33_linedisp_update(struct linedisp *linedisp)
 {
-	struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
-						 seg.linedisp);
+	struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv, linedisp);
 
 	schedule_delayed_work(&priv->work, 0);
 }
@@ -663,14 +654,13 @@ 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;
 	int err;
 
 	err = ht16k33_brightness_set(priv, brightness);
 	if (err)
 		return err;
 
-	return linedisp_register(&seg->linedisp, dev, 4, seg->curr, &ht16k33_linedisp_ops);
+	return linedisp_register(&priv->linedisp, dev, 4, NULL, &ht16k33_linedisp_ops);
 }
 
 static int ht16k33_probe(struct i2c_client *client)
@@ -754,7 +744,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;
 	}
 }
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 13/15] auxdisplay: ht16k33: Use buffer from struct linedisp

[Robin van der Gracht]

On Mon, 12 Feb 2024 19:01:46 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> struct linedips embedds a small buffer for the string that we may reuse.
> Update the driver accordingly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/auxdisplay/ht16k33.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index b104f08252dd..08cc05b9d216 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c

Acked-by: Robin van der Gracht <robin@protonic.nl>

Re: [PATCH v2 13/15] auxdisplay: ht16k33: Use buffer from struct linedisp

[Geert Uytterhoeven]

On Mon, Feb 12, 2024 at 6:04 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> struct linedips embedds a small buffer for the string that we may reuse.
> Update the driver accordingly.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -451,8 +444,7 @@ static void ht16k33_seg14_update(struct work_struct *work)
>
>  static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
>  {
> -       struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
> -                                                seg.linedisp);
> +       struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv, linedisp);

Please wrap long lines.

>
>         switch (priv->type) {
>         case DISP_MATRIX:
> @@ -471,8 +463,7 @@ static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
>
>  static void ht16k33_linedisp_update(struct linedisp *linedisp)
>  {
> -       struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
> -                                                seg.linedisp);
> +       struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv, linedisp);

Likewise.

>
>         schedule_delayed_work(&priv->work, 0);
>  }

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 v2 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Andy Shevchenko]

Add initial device tree documentation for Maxim MAX6958/6959.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../bindings/auxdisplay/maxim,max6959.yaml    | 35 +++++++++++++++++++
 1 file changed, 35 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..49ce26176797
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
@@ -0,0 +1,35 @@
+# 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 with keyscan
+
+maintainers:
+  - Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+
+properties:
+  compatible:
+    const: maxim,max6959
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            max6959: max6959@38 {
+                    compatible = "maxim,max6959";
+                    reg = <0x38>;
+            };
+      };
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v2 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 07:01:47PM +0200, Andy Shevchenko wrote:
> Add initial device tree documentation for Maxim MAX6958/6959.

Oh, this is an old version :-(


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 07:01:47PM +0200, Andy Shevchenko wrote:
> > Add initial device tree documentation for Maxim MAX6958/6959.
> 
> Oh, this is an old version :-(

Here is a new one:

From d8c826e06cf9237cd5fc6b2bb0b1cac5aff4fd8a Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Thu, 8 Feb 2024 17:23:38 +0200
Subject: [PATCH 1/1] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

Add initial device tree documentation for Maxim MAX6958/6959.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../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..e7d602d02df1
--- /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 with keyscan
+
+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 has the debounce and interrupt 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.
+  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



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Krzysztof Kozlowski]

On 12/02/2024 18:16, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:
>> On Mon, Feb 12, 2024 at 07:01:47PM +0200, Andy Shevchenko wrote:
>>> Add initial device tree documentation for Maxim MAX6958/6959.
>>
>> Oh, this is an old version :-(
> 
> Here is a new one:
> 
> From d8c826e06cf9237cd5fc6b2bb0b1cac5aff4fd8a Mon Sep 17 00:00:00 2001
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date: Thu, 8 Feb 2024 17:23:38 +0200
> Subject: [PATCH 1/1] dt-bindings: auxdisplay: Add Maxim MAX6958/6959
> 
> Add initial device tree documentation for Maxim MAX6958/6959.
> 

Anyway you need to send the new version to the list, so the bot will
test it.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 09:21:10AM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 18:16, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:

...

> > Add initial device tree documentation for Maxim MAX6958/6959.
> 
> Anyway you need to send the new version to the list, so the bot will
> test it.

Sure, that's the plan after applying first ones, so the only two can be left
in v4. I will wait for Geert's comments as well.

> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 12, 2024 at 6:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 07:01:47PM +0200, Andy Shevchenko wrote:
> > > Add initial device tree documentation for Maxim MAX6958/6959.
> >
> > Oh, this is an old version :-(
>
> Here is a new one:
>
> From d8c826e06cf9237cd5fc6b2bb0b1cac5aff4fd8a Mon Sep 17 00:00:00 2001
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date: Thu, 8 Feb 2024 17:23:38 +0200
> Subject: [PATCH 1/1] dt-bindings: auxdisplay: Add Maxim MAX6958/6959
>
> Add initial device tree documentation for Maxim MAX6958/6959.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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 with keyscan
> +
> +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 has the debounce and interrupt support.

IUIC, the primary differentiating factor is that the MAX6959 adds input
and GPIO support?  Debounce and interrupt support are merely features
of 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.

I don't think you need to read that register, as the users of the
features (keypad mapping, interrupts property, ...) also need to be
described in DTS (once supported).

> +  Given hardware is simple and does not provide any additional pins,
> +  such as reset or enable.

Does this matter? I.e. is it important to say this in the bindings?

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 v2 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 11:57:53AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 12, 2024 at 6:16 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:

...

> > +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 has the debounce and interrupt support.
> 
> IUIC, the primary differentiating factor is that the MAX6959 adds input
> and GPIO support?  Debounce and interrupt support are merely features
> of input support.

What should I do here? Rephrase?

> > +  Type of the chip can be autodetected via specific register read,
> > +  and hence the features may be enabled in the driver at run-time.
> 
> I don't think you need to read that register, as the users of the
> features (keypad mapping, interrupts property, ...) also need to be
> described in DTS (once supported).

So, the idea that if DT describes those we will check the chip ID and
instantiate what is asked?

> > +  Given hardware is simple and does not provide any additional pins,
> > +  such as reset or enable.
> 
> Does this matter? I.e. is it important to say this in the bindings?

From Krzysztof's review of v1 I understood that it is important to say so
people wouldn't wonder if HW has support of that which is not implemented
(yet) or simply has no such pins.

Personally I would lean towards leaving this in the description.

...

Can you propose the full description text how you see it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

[Geert Uytterhoeven]

Hi Andy,

On Thu, Feb 15, 2024 at 12:17 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Feb 15, 2024 at 11:57:53AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 12, 2024 at 6:16 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:
> > > +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 has the debounce and interrupt support.
> >
> > IUIC, the primary differentiating factor is that the MAX6959 adds input
> > and GPIO support?  Debounce and interrupt support are merely features
> > of input support.
>
> What should I do here? Rephrase?

    The Maxim MAX6958/6959 7-segment LED display controller provides
    an I2C interface to up to four 7-segment LED digits. The MAX6959
    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.
> >
> > I don't think you need to read that register, as the users of the
> > features (keypad mapping, interrupts property, ...) also need to be
> > described in DTS (once supported).
>
> So, the idea that if DT describes those we will check the chip ID and
> instantiate what is asked?

Even the check for the chip ID is probably not needed. E.g.
  - If the DTS has an interrupt property, (the future version of)
    the driver sets up the interrupt.
  - If the DTS has a linux,keymap, (the future version of) enables the
    keypad.

> > > +  Given hardware is simple and does not provide any additional pins,
> > > +  such as reset or enable.
> >
> > Does this matter? I.e. is it important to say this in the bindings?
>
> From Krzysztof's review of v1 I understood that it is important to say so
> people wouldn't wonder if HW has support of that which is not implemented
> (yet) or simply has no such pins.

It might be good to mention that in the commit description.
IMHO a list of all possible things that do not exist does not belong in the
bindings.

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 v2 15/15] 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 | 200 +++++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/auxdisplay/max6959.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index a34a9a52158f..079d58bb0293 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -187,6 +187,20 @@ config HT16K33
 	  Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
 	  LED controller driver with keyscan.
 
+config MAX6959
+	tristate "Maxim MAX6958/6959 7-segment LED controller with keyscan"
+	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 with keyscan):
+	  - MAX6958
+	  - MAX6959 (debounce support)
+
+	  This driver can also be built as a module. If so, the module
+	  will be called max6959.
+
 config ARM_CHARLCD
 	bool "ARM Ltd. Character LCD Driver"
 	depends on PLAT_VERSATILE
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 43bad850481c..f62a258809ef 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -12,5 +12,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
 obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
diff --git a/drivers/auxdisplay/max6959.c b/drivers/auxdisplay/max6959.c
new file mode 100644
index 000000000000..0c5cbd16c3fe
--- /dev/null
+++ b/drivers/auxdisplay/max6959.c
@@ -0,0 +1,200 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MAX6958/6959 7-segment LED display controller with keyscan
+ * 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
+
+/* Defines */
+#define MIN_BRIGHTNESS			0x01
+#define MAX_BRIGHTNESS			0x40
+
+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->curr;
+	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, NULL, &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 with keyscan");
+MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LINEDISP);
-- 
2.43.0.rc1.1.gbec44491f096

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

[Geert Uytterhoeven]

Hi Andy,

On Mon, Feb 12, 2024 at 6:04 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!

> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -187,6 +187,20 @@ config HT16K33
>           Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
>           LED controller driver with keyscan.
>
> +config MAX6959
> +       tristate "Maxim MAX6958/6959 7-segment LED controller with keyscan"

I'd drop the "with keyscan" for now...

> +       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 with keyscan):
> +         - MAX6958
> +         - MAX6959 (debounce support)

s/debounce/input/

> +
> +         This driver can also be built as a module. If so, the module
> +         will be called max6959.

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

> +/* Defines */
> +#define MIN_BRIGHTNESS                 0x01
> +#define MAX_BRIGHTNESS                 0x40

Unused? (for now, until you add LED brightness support)

> +
> +struct max6959_priv {
> +       struct linedisp linedisp;
> +
> +       struct delayed_work work;
> +

IMHO these blank lines don't add any value.

> +       struct regmap *regmap;
> +};

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 v2 00/15] auxdisplay: linedisp: Clean up and add new driver

[Andy Shevchenko]

On Mon, Feb 12, 2024 at 07:01:33PM +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.
> 
> 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)

Geert, I would like to apply at least the first 13 patches.
Do you have any comments or even possibility to perform a regression test?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/15] auxdisplay: linedisp: Clean up and add new driver

[Geert Uytterhoeven]

Hi Andy,

On Wed, Feb 14, 2024 at 6:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 12, 2024 at 07:01:33PM +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.
> >
> > 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)
>
> Geert, I would like to apply at least the first 13 patches.
> Do you have any comments or even possibility to perform a regression test?

I'll try to give it a try on my Adafruit Quad 14-segment display tomorrow...

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 v2 00/15] auxdisplay: linedisp: Clean up and add new driver

[Andy Shevchenko]

On Wed, Feb 14, 2024 at 07:45:01PM +0100, Geert Uytterhoeven wrote:
> On Wed, Feb 14, 2024 at 6:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 12, 2024 at 07:01:33PM +0200, Andy Shevchenko wrote:

...

> > Geert, I would like to apply at least the first 13 patches.
> > Do you have any comments or even possibility to perform a regression test?
> 
> I'll try to give it a try on my Adafruit Quad 14-segment display tomorrow...

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 00/15] auxdisplay: linedisp: Clean up and add new driver

[Geert Uytterhoeven]

Hi Andy,

On Wed, Feb 14, 2024 at 7:45 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Feb 14, 2024 at 6:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 12, 2024 at 07:01:33PM +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.
> > >
> > > 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)
> >
> > Geert, I would like to apply at least the first 13 patches.
> > Do you have any comments or even possibility to perform a regression test?
>
> I'll try to give it a try on my Adafruit Quad 14-segment display tomorrow...

With the missing return-statement added, the ht16k33 driver builds
and works fine: the kernel version is happily scrolling by.
I didn't test userspace line display control, as there is an issue
with the uSD interface on my OrangeCrab, preventing it from booting
into userspace.

Tested-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 v2 00/15] auxdisplay: linedisp: Clean up and add new driver

[Andy Shevchenko]

On Thu, Feb 15, 2024 at 12:05:37PM +0100, Geert Uytterhoeven wrote:
> On Wed, Feb 14, 2024 at 7:45 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Feb 14, 2024 at 6:57 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 12, 2024 at 07:01:33PM +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.
> > > >
> > > > 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)
> > >
> > > Geert, I would like to apply at least the first 13 patches.
> > > Do you have any comments or even possibility to perform a regression test?
> >
> > I'll try to give it a try on my Adafruit Quad 14-segment display tomorrow...
> 
> With the missing return-statement added, the ht16k33 driver builds
> and works fine: the kernel version is happily scrolling by.
> I didn't test userspace line display control, as there is an issue
> with the uSD interface on my OrangeCrab, preventing it from booting
> into userspace.
> 
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thank you!

So far I have applied patches 1-6,8-9 with the respective suggestions
implemented.

-- 
With Best Regards,
Andy Shevchenko