linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/17] auxdisplay: ht16k33: Add character display support
@ 2021-03-22 14:48 Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 01/17] uapi: Add <linux/map_to_14segment.h> Geert Uytterhoeven
                   ` (17 more replies)
  0 siblings, 18 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

	Hi all,

The Holtek HT16K33 LED controller is not only used for driving
dot-matrix displays, but also for driving segment displays.
The current auxdisplay driver is limited to dot-matrix displays, which
are exposed as a frame buffer device.

This patch series extends the driver to 4-digit 7-segment and quad
14-segment alphanumeric displays, allowing the user to display and
scroll text messages.

List of patches:
  - Patch 1 provides font data for displaying ASCII characters on
    14-segment displays,
  - Patch 2 updates the HT16K33 DT bindings for segment displays,
  - Patches 3-5 contain a bug fix and small improvements for the
    Imagination Technologies ASCII LCD Display driver,
  - Patch 6 extracts the character line display core support from the
    Imagination Technologies ASCII LCD Display driver, for reuse,
  - Patches 7-8 contain cleanups and improvements for the character line
    display core driver,
  - Patches 9-15 contain cleanups and improvements for the HT16K33
    driver, to prepare for segment display support,
  - Patch 16 adds support for 7/14-segment displays to the HT16K33
    driver,
  - Patch 17 adds segment display LED support to the HT16K33 driver,
    to make use of hardware blinking, and to expose display color.

This series has been tested using an Adafruit 0.54" Quad Alphanumeric
Red FeatherWing Display, plugged into an OrangeCrab ECP5-based FPGA
board running linux-on-litex-vexriscv.
7-segment display support is based purely on schematics, and has not
been tested on actual hardware.  The changes to img-ascii-lcd.c are also
untested, due to lack of hardware.

Thanks for your comments!

Geert Uytterhoeven (17):
  uapi: Add <linux/map_to_14segment.h>
  dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays
  auxdisplay: img-ascii-lcd: Fix lock-up when displaying empty string
  auxdisplay: img-ascii-lcd: Add helper variable dev
  auxdisplay: img-ascii-lcd: Convert device attribute to sysfs_emit()
  auxdisplay: Extract character line display core support
  auxdisplay: linedisp: Use kmemdup_nul() helper
  auxdisplay: linedisp: Add support for changing scroll rate
  auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize()
  auxdisplay: ht16k33: Remove unneeded error check in keypad probe()
  auxdisplay: ht16k33: Convert to simple i2c probe function
  auxdisplay: ht16k33: Add helper variable dev
  auxdisplay: ht16k33: Move delayed work
  auxdisplay: ht16k33: Extract ht16k33_brightness_set()
  auxdisplay: ht16k33: Extract frame buffer probing
  auxdisplay: ht16k33: Add support for segment displays
  auxdisplay: ht16k33: Add segment display LED support

 .../bindings/auxdisplay/holtek,ht16k33.yaml   |  22 +-
 drivers/auxdisplay/Kconfig                    |   8 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/ht16k33.c                  | 451 ++++++++++++++----
 drivers/auxdisplay/img-ascii-lcd.c            | 199 +-------
 drivers/auxdisplay/line-display.c             | 261 ++++++++++
 drivers/auxdisplay/line-display.h             |  43 ++
 include/uapi/linux/map_to_14segment.h         | 240 ++++++++++
 8 files changed, 961 insertions(+), 264 deletions(-)
 create mode 100644 drivers/auxdisplay/line-display.c
 create mode 100644 drivers/auxdisplay/line-display.h
 create mode 100644 include/uapi/linux/map_to_14segment.h

-- 
2.25.1

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH 01/17] uapi: Add <linux/map_to_14segment.h>
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 17:05   ` Miguel Ojeda
  2021-03-22 14:48 ` [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays Geert Uytterhoeven
                   ` (16 subsequent siblings)
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Add a header file providing translation primitives and tables for the
conversion of (ASCII) characters to a 14-segments notation, as used by
14-segment alphanumeric displays.

This follows the spirit of include/uapi/linux/map_to_7segment.h.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
You can find an image of the full character set at
https://drive.google.com/file/d/1h3EYFBWHIjh8B_cwPA5ocAD-lFYipRie/view

Note that all non-empty characters are unique, except for "[" and "C",
and "|" and ":".
---
 include/uapi/linux/map_to_14segment.h | 240 ++++++++++++++++++++++++++
 1 file changed, 240 insertions(+)
 create mode 100644 include/uapi/linux/map_to_14segment.h

diff --git a/include/uapi/linux/map_to_14segment.h b/include/uapi/linux/map_to_14segment.h
new file mode 100644
index 0000000000000000..3531bcd0360564c4
--- /dev/null
+++ b/include/uapi/linux/map_to_14segment.h
@@ -0,0 +1,240 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * Copyright (C) 2021 Glider bv
+ *
+ * Based on include/uapi/linux/map_to_7segment.h:
+
+ * Copyright (c) 2005 Henk Vergonet <Henk.Vergonet@gmail.com>
+ */
+
+#ifndef MAP_TO_14SEGMENT_H
+#define MAP_TO_14SEGMENT_H
+
+/* This file provides translation primitives and tables for the conversion
+ * of (ASCII) characters to a 14-segments notation.
+ *
+ * The 14 segment's wikipedia notation below is used as standard.
+ * See: https://en.wikipedia.org/wiki/Fourteen-segment_display
+ *
+ * Notation:	+---a---+
+ *		|\  |  /|
+ *		f h i j b
+ *		|  \|/  |
+ *		+-g1+-g2+
+ *		|  /|\  |
+ *		e k l m c
+ *		|/  |  \|
+ *		+---d---+
+ *
+ * Usage:
+ *
+ *   Register a map variable, and fill it with a character set:
+ *	static SEG14_DEFAULT_MAP(map_seg14);
+ *
+ *
+ *   Then use for conversion:
+ *	seg14 = map_to_seg14(&map_seg14, some_char);
+ *	...
+ *
+ * In device drivers it is recommended, if required, to make the char map
+ * accessible via the sysfs interface using the following scheme:
+ *
+ * static ssize_t map_seg14_show(struct device *dev,
+ *				 struct device_attribute *attr, char *buf)
+ * {
+ *	memcpy(buf, &map_seg14, sizeof(map_seg14));
+ *	return sizeof(map_seg14);
+ * }
+ * static ssize_t map_seg14_store(struct device *dev,
+ *				  struct device_attribute *attr,
+ *				  const char *buf, size_t cnt)
+ * {
+ *	if (cnt != sizeof(map_seg14))
+ *		return -EINVAL;
+ *	memcpy(&map_seg14, buf, cnt);
+ *	return cnt;
+ * }
+ * static DEVICE_ATTR_RW(map_seg14);
+ */
+#include <linux/errno.h>
+#include <linux/types.h>
+
+#include <asm/byteorder.h>
+
+#define BIT_SEG14_A		0
+#define BIT_SEG14_B		1
+#define BIT_SEG14_C		2
+#define BIT_SEG14_D		3
+#define BIT_SEG14_E		4
+#define BIT_SEG14_F		5
+#define BIT_SEG14_G1		6
+#define BIT_SEG14_G2		7
+#define BIT_SEG14_H		8
+#define BIT_SEG14_I		9
+#define BIT_SEG14_J		10
+#define BIT_SEG14_K		11
+#define BIT_SEG14_L		12
+#define BIT_SEG14_M		13
+#define BIT_SEG14_RESERVED1	14
+#define BIT_SEG14_RESERVED2	15
+
+struct seg14_conversion_map {
+	__be16 table[128];
+};
+
+static __inline__ int map_to_seg14(struct seg14_conversion_map *map, int c)
+{
+	return c >= 0 && c < sizeof(map->table) ? __be16_to_cpu(map->table[c])
+						: -EINVAL;
+}
+
+#define SEG14_CONVERSION_MAP(_name, _map)	\
+	struct seg14_conversion_map _name = { .table = { _map } }
+
+/*
+ * It is recommended to use a facility that allows user space to redefine
+ * custom character sets for LCD devices. Please use a sysfs interface
+ * as described above.
+ */
+#define MAP_TO_SEG14_SYSFS_FILE	"map_seg14"
+
+/*******************************************************************************
+ * ASCII conversion table
+ ******************************************************************************/
+
+#define _SEG14(sym, a, b, c, d, e, f, g1, g2, h, j, k, l, m, n)	\
+	__cpu_to_be16( a << BIT_SEG14_A  |  b << BIT_SEG14_B  |	\
+		       c << BIT_SEG14_C  |  d << BIT_SEG14_D  |	\
+		       e << BIT_SEG14_E  |  f << BIT_SEG14_F  |	\
+		      g1 << BIT_SEG14_G1 | g2 << BIT_SEG14_G2 |	\
+		       h << BIT_SEG14_H  |  j << BIT_SEG14_I  |	\
+		       k << BIT_SEG14_J  |  l << BIT_SEG14_K  |	\
+		       m << BIT_SEG14_L  |  n << BIT_SEG14_M )
+
+#define _MAP_0_32_ASCII_SEG14_NON_PRINTABLE				\
+	0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
+
+#define _MAP_33_47_ASCII_SEG14_SYMBOL				\
+	_SEG14('!', 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('"', 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0),	\
+	_SEG14('#', 0, 1, 1, 1, 0, 0, 1, 1, 0, 1, 0, 0, 1, 0),	\
+	_SEG14('$', 1, 0, 1, 1, 0, 1, 1, 1, 0, 1, 0, 0, 1, 0),	\
+	_SEG14('%', 0, 0, 1, 0, 0, 1, 0, 0, 0, 0, 1, 1, 0, 0),	\
+	_SEG14('&', 1, 0, 0, 1, 1, 0, 1, 0, 1, 0, 1, 0, 0, 1),	\
+	_SEG14('\'',0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0),	\
+	_SEG14('(', 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1),	\
+	_SEG14(')', 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0, 0),	\
+	_SEG14('*', 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1),	\
+	_SEG14('+', 0, 0, 0, 0, 0, 0, 1, 1, 0, 1, 0, 0, 1, 0),	\
+	_SEG14(',', 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0),	\
+	_SEG14('-', 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('.', 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1),	\
+	_SEG14('/', 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0),
+
+#define _MAP_48_57_ASCII_SEG14_NUMERIC				\
+	_SEG14('0', 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 1, 1, 0, 0),	\
+	_SEG14('1', 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0),	\
+	_SEG14('2', 1, 1, 0, 1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('3', 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('4', 0, 1, 1, 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('5', 1, 0, 0, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0, 1),	\
+	_SEG14('6', 1, 0, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('7', 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 0),	\
+	_SEG14('8', 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('9', 1, 1, 1, 1, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0),
+
+#define _MAP_58_64_ASCII_SEG14_SYMBOL				\
+	_SEG14(':', 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0),	\
+	_SEG14(';', 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 0, 0),	\
+	_SEG14('<', 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 1),	\
+	_SEG14('=', 0, 0, 0, 1, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('>', 0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 1, 0, 0),	\
+	_SEG14('?', 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 1, 0),	\
+	_SEG14('@', 1, 1, 0, 1, 1, 1, 0, 1, 0, 1, 0, 0, 0, 0),
+
+#define _MAP_65_90_ASCII_SEG14_ALPHA_UPPER			\
+	_SEG14('A', 1, 1, 1, 0, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('B', 1, 1, 1, 1, 0, 0, 0, 1, 0, 1, 0, 0, 1, 0),	\
+	_SEG14('C', 1, 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('D', 1, 1, 1, 1, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0),	\
+	_SEG14('E', 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('F', 1, 0, 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('G', 1, 0, 1, 1, 1, 1, 0, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('H', 0, 1, 1, 0, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('I', 1, 0, 0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0),	\
+	_SEG14('J', 0, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('K', 0, 0, 0, 0, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1),	\
+	_SEG14('L', 0, 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('M', 0, 1, 1, 0, 1, 1, 0, 0, 1, 0, 1, 0, 0, 0),	\
+	_SEG14('N', 0, 1, 1, 0, 1, 1, 0, 0, 1, 0, 0, 0, 0, 1),	\
+	_SEG14('O', 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('P', 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('Q', 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 1),	\
+	_SEG14('R', 1, 1, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 0, 1),	\
+	_SEG14('S', 1, 0, 1, 1, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('T', 1, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0),	\
+	_SEG14('U', 0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('V', 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 1, 1, 0, 0),	\
+	_SEG14('W', 0, 1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 1, 0, 1),	\
+	_SEG14('X', 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 1, 0, 1),	\
+	_SEG14('Y', 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 0, 1, 0),	\
+	_SEG14('Z', 1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0),
+
+#define _MAP_91_96_ASCII_SEG14_SYMBOL				\
+	_SEG14('[', 1, 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('\\',0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1),	\
+	_SEG14(']', 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('^', 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1),	\
+	_SEG14('_', 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('`', 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0),
+
+#define _MAP_97_122_ASCII_SEG14_ALPHA_LOWER			\
+	_SEG14('a', 0, 0, 0, 1, 1, 0, 1, 0, 0, 0, 0, 0, 1, 0),	\
+	_SEG14('b', 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 1),	\
+	_SEG14('c', 0, 0, 0, 1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('d', 0, 1, 1, 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0),	\
+	_SEG14('e', 0, 0, 0, 1, 1, 0, 1, 0, 0, 0, 0, 1, 0, 0),	\
+	_SEG14('f', 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 1, 0, 1, 0),	\
+	_SEG14('g', 0, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1, 0, 0, 0),	\
+	_SEG14('h', 0, 0, 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 1, 0),	\
+	_SEG14('i', 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0),	\
+	_SEG14('j', 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 1, 0, 0),	\
+	_SEG14('k', 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 0, 1, 1),	\
+	_SEG14('l', 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('m', 0, 0, 1, 0, 1, 0, 1, 1, 0, 0, 0, 0, 1, 0),	\
+	_SEG14('n', 0, 0, 0, 0, 1, 0, 1, 0, 0, 0, 0, 0, 1, 0),	\
+	_SEG14('o', 0, 0, 1, 1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('p', 0, 0, 0, 0, 1, 1, 1, 0, 1, 0, 0, 0, 0, 0),	\
+	_SEG14('q', 0, 1, 1, 0, 0, 0, 0, 1, 0, 0, 1, 0, 0, 0),	\
+	_SEG14('r', 0, 0, 0, 0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('s', 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 1),	\
+	_SEG14('t', 0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('u', 0, 0, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0),	\
+	_SEG14('v', 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 0),	\
+	_SEG14('w', 0, 0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 1, 0, 1),	\
+	_SEG14('x', 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 1, 0, 1),	\
+	_SEG14('y', 0, 1, 1, 1, 0, 0, 0, 1, 0, 1, 0, 0, 0, 0),	\
+	_SEG14('z', 0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0),
+
+#define _MAP_123_126_ASCII_SEG14_SYMBOL				\
+	_SEG14('{', 1, 0, 0, 1, 0, 0, 1, 0, 1, 0, 0, 1, 0, 0),	\
+	_SEG14('|', 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0),	\
+	_SEG14('}', 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 1, 0, 0, 1),	\
+	_SEG14('~', 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 1, 1, 0, 0),
+
+/* Maps */
+#define MAP_ASCII14SEG_ALPHANUM			\
+	_MAP_0_32_ASCII_SEG14_NON_PRINTABLE	\
+	_MAP_33_47_ASCII_SEG14_SYMBOL		\
+	_MAP_48_57_ASCII_SEG14_NUMERIC		\
+	_MAP_58_64_ASCII_SEG14_SYMBOL		\
+	_MAP_65_90_ASCII_SEG14_ALPHA_UPPER	\
+	_MAP_91_96_ASCII_SEG14_SYMBOL		\
+	_MAP_97_122_ASCII_SEG14_ALPHA_LOWER	\
+	_MAP_123_126_ASCII_SEG14_SYMBOL
+
+#define SEG14_DEFAULT_MAP(_name)		\
+	SEG14_CONVERSION_MAP(_name, MAP_ASCII14SEG_ALPHANUM)
+
+#endif	/* MAP_TO_14SEGMENT_H */
+
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 01/17] uapi: Add <linux/map_to_14segment.h> Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 17:38   ` Rob Herring
  2021-03-23  9:12   ` robin
  2021-03-22 14:48 ` [PATCH 03/17] auxdisplay: img-ascii-lcd: Fix lock-up when displaying empty string Geert Uytterhoeven
                   ` (15 subsequent siblings)
  17 siblings, 2 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

The Holtek HT16K33 LED controller is not only used for driving
dot-matrix displays, but also for driving segment displays.

Document compatible values for the Adafruit 7-segment[1] and
14-segment[2] FeatherWing expansion boards with red displays.  According
to the schematics, all other Adafruit 7-segment and 14-segment display
backpack and FeatherWing expansion boards (including bare boards and
boards fitted with displays) are compatible with these two boards.
Add a "color" property to support the different color variants.

[1] https://www.adafruit.com/product/3108
[2] https://www.adafruit.com/product/3130

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Alternatives I considered:
  1. Document the attached display type in a child node.
     I.e. specify segment type, number of characters, and wiring.
     Especially the latter would become really complex, due to the sheer
     amount of possible wiring combinations.
     Using this method, you also loose the ability to just connect a
     display to an i2c bus, and instantiate the device from sysfs,
     without using DT:

	echo adafruit,3130 0x70 > /sys/class/i2c/i2c-adapter/.../new_device

  2. Do not use the "color" property, but document all Adafruit
     7-segment and 14-segment display backpack and FeatherWing expansion
     boards.
     This would lead to a myriad of compatible values:

      - items:
	  - enum:
	      - adafruit,878      # 0.56" 4-Digit 7-Segment Display Backpack (Red)
	      - adafruit,879      # 0.56" 4-Digit 7-Segment Display Backpack (Yellow)
	      - adafruit,880      # 0.56" 4-Digit 7-Segment Display Backpack (Green)
	      - adafruit,881      # 0.56" 4-Digit 7-Segment Display Backpack (Blue)
	      - adafruit,1002     # 0.56" 4-Digit 7-Segment Display Backpack (White)
	  - const: adafruit,877   # 0.56" 4-Digit 7-Segment Backpack
	  - const: holtek,ht16k33

      - items:
	  - enum:
	      - adafruit,1268     # 1.2" 4-Digit 7-Segment Display Backpack (Green)
	      - adafruit,1269     # 1.2" 4-Digit 7-Segment Display Backpack (Yellow)
	      - adafruit,1270     # 1.2" 4-Digit 7-Segment Display Backpack (Red)
	  - const: adafruit,1271  # 1.2" 4-Digit 7-Segment Backpack
	  - const: holtek,ht16k33

      - items:
	  - enum:
	      - adafruit,1911     # 0.54" Quad Alphanumeric Display Backpack (Red)
	      - adafruit,1912     # 0.54" Quad Alphanumeric Display Backpack (Blue)
	      - adafruit,2157     # 0.54" Quad Alphanumeric Display Backpack (White)
	      - adafruit,2158     # 0.54" Quad Alphanumeric Display Backpack (Yellow)
	      - adafruit,2159     # 0.54" Quad Alphanumeric Display Backpack (Yellow-Green)
	      - adafruit,2160     # 0.54" Quad Alphanumeric Display Backpack (Green)
	  - const: adafruit,1910  # 0.54" Quad 14-segment Alphanumeric Backpack
	  - const: holtek,ht16k33

      - items:
	  - enum:
	      - adafruit,3106     # 0.56" 4-Digit 7-Segment FeatherWing Display (Blue)
	      - adafruit,3107     # 0.56" 4-Digit 7-Segment FeatherWing Display (Green)
	      - adafruit,3108     # 0.56" 4-Digit 7-Segment FeatherWing Display (Red)
	      - adafruit,3109     # 0.56" 4-Digit 7-Segment FeatherWing Display (White)
	      - adafruit,3110     # 0.56" 4-Digit 7-Segment FeatherWing Display (Yellow)
	  - const: adafruit,3088  # 0.56" 4-Digit 7-Segment FeatherWing
	  - const: holtek,ht16k33

      - items:
	  - enum:
	      - adafruit,3127     # 0.54" Quad Alphanumeric FeatherWing Display (White)
	      - adafruit,3128     # 0.54" Quad Alphanumeric FeatherWing Display (Blue)
	      - adafruit,3129     # 0.54" Quad Alphanumeric FeatherWing Display (Green)
	      - adafruit,3130     # 0.54" Quad Alphanumeric FeatherWing Display (Red)
	      - adafruit,3131     # 0.54" Quad Alphanumeric FeatherWing Display (Yellow)
	      - adafruit,3132     # 0.54" Quad Alphanumeric FeatherWing Display (Yellow-Green)
	  - const: adafruit,3089  # 0.54" Quad 14-segment Alphanumeric FeatherWing
	  - const: holtek,ht16k33
---
 .../bindings/auxdisplay/holtek,ht16k33.yaml   | 22 ++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
index 64ffff460026040f..4380a5177a67d2e2 100644
--- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
+++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
@@ -14,14 +14,23 @@ allOf:
 
 properties:
   compatible:
-    const: holtek,ht16k33
+    oneOf:
+      - items:
+          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment FeatherWing Display (Red)
+          - const: holtek,ht16k33
+
+      - items:
+          - const: adafruit,3130  # 0.54" Quad Alphanumeric FeatherWing Display (Red)
+          - const: holtek,ht16k33
+
+      - const: holtek,ht16k33     # Generic 16*8 LED controller with dot-matrix display
 
   reg:
     maxItems: 1
 
   refresh-rate-hz:
     maxItems: 1
-    description: Display update interval in Hertz
+    description: Display update interval in Hertz for dot-matrix displays
 
   interrupts:
     maxItems: 1
@@ -41,10 +50,17 @@ properties:
     default: 16
     description: Initial brightness level
 
+  color: true
+    description:
+      Color of the display.  Use one of the LED_COLOR_ID_* prefixed definitions
+      from the header include/dt-bindings/leds/common.h.  The default is red.
+    minimum: 0
+    maximum: 9
+    default: 1
+
 required:
   - compatible
   - reg
-  - refresh-rate-hz
 
 additionalProperties: false
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 03/17] auxdisplay: img-ascii-lcd: Fix lock-up when displaying empty string
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 01/17] uapi: Add <linux/map_to_14segment.h> Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 04/17] auxdisplay: img-ascii-lcd: Add helper variable dev Geert Uytterhoeven
                   ` (14 subsequent siblings)
  17 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

While writing an empty string to a device attribute is a no-op, and thus
does not need explicit safeguards, the user can still write a single
newline to an attribute file:

    echo > .../message

If that happens, img_ascii_lcd_display() trims the newline, yielding an
empty string, and causing an infinite loop in img_ascii_lcd_scroll().

Fix this by adding a check for empty strings.  Clear the display in case
one is encountered.

Fixes: 0cad855fbd083ee5 ("auxdisplay: img-ascii-lcd: driver for simple ASCII LCD displays")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Untested with img-ascii-lcd, but triggered with my initial version of
linedisp.
---
 drivers/auxdisplay/img-ascii-lcd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index 1cce409ce5cacbc8..e33ce0151cdfd150 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -280,6 +280,16 @@ static int img_ascii_lcd_display(struct img_ascii_lcd_ctx *ctx,
 	if (msg[count - 1] == '\n')
 		count--;
 
+	if (!count) {
+		/* clear the LCD */
+		devm_kfree(&ctx->pdev->dev, ctx->message);
+		ctx->message = NULL;
+		ctx->message_len = 0;
+		memset(ctx->curr, ' ', ctx->cfg->num_chars);
+		ctx->cfg->update(ctx);
+		return 0;
+	}
+
 	new_msg = devm_kmalloc(&ctx->pdev->dev, count + 1, GFP_KERNEL);
 	if (!new_msg)
 		return -ENOMEM;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 04/17] auxdisplay: img-ascii-lcd: Add helper variable dev
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (2 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 03/17] auxdisplay: img-ascii-lcd: Fix lock-up when displaying empty string Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 16:59   ` Miguel Ojeda
  2021-03-22 14:48 ` [PATCH 05/17] auxdisplay: img-ascii-lcd: Convert device attribute to sysfs_emit() Geert Uytterhoeven
                   ` (13 subsequent siblings)
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

img_ascii_lcd_probe() has many users of "pdev->dev".  Add a shorthand to
simplify the code.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/img-ascii-lcd.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index e33ce0151cdfd150..2b6e41ec57544faa 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -365,26 +365,25 @@ static int img_ascii_lcd_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match;
 	const struct img_ascii_lcd_config *cfg;
+	struct device *dev = &pdev->dev;
 	struct img_ascii_lcd_ctx *ctx;
 	int err;
 
-	match = of_match_device(img_ascii_lcd_matches, &pdev->dev);
+	match = of_match_device(img_ascii_lcd_matches, dev);
 	if (!match)
 		return -ENODEV;
 
 	cfg = match->data;
-	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx) + cfg->num_chars,
-			   GFP_KERNEL);
+	ctx = devm_kzalloc(dev, sizeof(*ctx) + cfg->num_chars, GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
 	if (cfg->external_regmap) {
-		ctx->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
+		ctx->regmap = syscon_node_to_regmap(dev->parent->of_node);
 		if (IS_ERR(ctx->regmap))
 			return PTR_ERR(ctx->regmap);
 
-		if (of_property_read_u32(pdev->dev.of_node, "offset",
-					 &ctx->offset))
+		if (of_property_read_u32(dev->of_node, "offset", &ctx->offset))
 			return -EINVAL;
 	} else {
 		ctx->base = devm_platform_ioremap_resource(pdev, 0);
@@ -408,7 +407,7 @@ static int img_ascii_lcd_probe(struct platform_device *pdev)
 	if (err)
 		goto out_del_timer;
 
-	err = device_create_file(&pdev->dev, &dev_attr_message);
+	err = device_create_file(dev, &dev_attr_message);
 	if (err)
 		goto out_del_timer;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 05/17] auxdisplay: img-ascii-lcd: Convert device attribute to sysfs_emit()
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (3 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 04/17] auxdisplay: img-ascii-lcd: Add helper variable dev Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 06/17] auxdisplay: Extract character line display core support Geert Uytterhoeven
                   ` (12 subsequent siblings)
  17 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Convert the "message" device attribute from sprintf() to sysfs_emit(),
as the latter is aware of the PAGE_SIZE buffer.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/img-ascii-lcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index 2b6e41ec57544faa..2b5640b638900a90 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -326,7 +326,7 @@ static ssize_t message_show(struct device *dev, struct device_attribute *attr,
 {
 	struct img_ascii_lcd_ctx *ctx = dev_get_drvdata(dev);
 
-	return sprintf(buf, "%s\n", ctx->message);
+	return sysfs_emit(buf, "%s\n", ctx->message);
 }
 
 /**
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 06/17] auxdisplay: Extract character line display core support
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (4 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 05/17] auxdisplay: img-ascii-lcd: Convert device attribute to sysfs_emit() Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-23  8:18   ` robin
  2021-03-22 14:48 ` [PATCH 07/17] auxdisplay: linedisp: Use kmemdup_nul() helper Geert Uytterhoeven
                   ` (11 subsequent siblings)
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Extract the character line display core support from the simple ASCII
LCD driver for the MIPS Boston, Malta & SEAD3 development boards into
its own subdriver, so it can be reused for other displays.

Note that this moves the "message" device attribute in sysfs in a
"linedisp.N" subdirectory.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Changes to img-ascii-lcd.c are untested due to lack of hardware.
---
 drivers/auxdisplay/Kconfig         |   8 +
 drivers/auxdisplay/Makefile        |   1 +
 drivers/auxdisplay/img-ascii-lcd.c | 198 +++----------------------
 drivers/auxdisplay/line-display.c  | 231 +++++++++++++++++++++++++++++
 drivers/auxdisplay/line-display.h  |  43 ++++++
 5 files changed, 304 insertions(+), 177 deletions(-)
 create mode 100644 drivers/auxdisplay/line-display.c
 create mode 100644 drivers/auxdisplay/line-display.h

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 1509cb74705a30ad..42fc7b155de09dbc 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -25,6 +25,12 @@ 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
@@ -155,6 +161,7 @@ config IMG_ASCII_LCD
 	depends on HAS_IOMEM
 	default y if MIPS_MALTA
 	select MFD_SYSCON
+	select LINEDISP
 	help
 	  Enable this to support the simple ASCII LCD displays found on
 	  development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
@@ -169,6 +176,7 @@ config HT16K33
 	select FB_SYS_IMAGEBLIT
 	select INPUT_MATRIXKMAP
 	select FB_BACKLIGHT
+	select LINEDISP
 	help
 	  Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
 	  LED controller driver with keyscan.
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 307771027c893a5b..6968ed4d3f0a8930 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -13,3 +13,4 @@ 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
diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index 2b5640b638900a90..4b6e4ac5ed977864 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -4,7 +4,6 @@
  * Author: Paul Burton <paul.burton@mips.com>
  */
 
-#include <generated/utsrelease.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
@@ -14,7 +13,8 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
-#include <linux/sysfs.h>
+
+#include "line-display.h"
 
 struct img_ascii_lcd_ctx;
 
@@ -27,36 +27,26 @@ struct img_ascii_lcd_ctx;
 struct img_ascii_lcd_config {
 	unsigned int num_chars;
 	bool external_regmap;
-	void (*update)(struct img_ascii_lcd_ctx *ctx);
+	void (*update)(struct linedisp *linedisp);
 };
 
 /**
  * struct img_ascii_lcd_ctx - Private data structure
- * @pdev: the ASCII LCD platform device
  * @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
- * @message: the full message to display or scroll on the LCD
- * @message_len: the length of the @message string
- * @scroll_pos: index of the first character of @message currently displayed
- * @scroll_rate: scroll interval in jiffies
- * @timer: timer used to implement scrolling
+ * @linedisp: line display structure
  * @curr: the string currently displayed on the LCD
  */
 struct img_ascii_lcd_ctx {
-	struct platform_device *pdev;
 	union {
 		void __iomem *base;
 		struct regmap *regmap;
 	};
 	u32 offset;
 	const struct img_ascii_lcd_config *cfg;
-	char *message;
-	unsigned int message_len;
-	unsigned int scroll_pos;
-	unsigned int scroll_rate;
-	struct timer_list timer;
+	struct linedisp linedisp;
 	char curr[] __aligned(8);
 };
 
@@ -64,8 +54,10 @@ struct img_ascii_lcd_ctx {
  * MIPS Boston development board
  */
 
-static void boston_update(struct img_ascii_lcd_ctx *ctx)
+static void boston_update(struct linedisp *linedisp)
 {
+	struct img_ascii_lcd_ctx *ctx =
+		container_of(linedisp, struct img_ascii_lcd_ctx, linedisp);
 	ulong val;
 
 #if BITS_PER_LONG == 64
@@ -90,12 +82,14 @@ static struct img_ascii_lcd_config boston_config = {
  * MIPS Malta development board
  */
 
-static void malta_update(struct img_ascii_lcd_ctx *ctx)
+static void malta_update(struct linedisp *linedisp)
 {
+	struct img_ascii_lcd_ctx *ctx =
+		container_of(linedisp, struct img_ascii_lcd_ctx, linedisp);
 	unsigned int i;
 	int err = 0;
 
-	for (i = 0; i < ctx->cfg->num_chars; i++) {
+	for (i = 0; i < linedisp->num_chars; i++) {
 		err = regmap_write(ctx->regmap,
 				   ctx->offset + (i * 8), ctx->curr[i]);
 		if (err)
@@ -173,12 +167,14 @@ static int sead3_wait_lcd_idle(struct img_ascii_lcd_ctx *ctx)
 	return 0;
 }
 
-static void sead3_update(struct img_ascii_lcd_ctx *ctx)
+static void sead3_update(struct linedisp *linedisp)
 {
+	struct img_ascii_lcd_ctx *ctx =
+		container_of(linedisp, struct img_ascii_lcd_ctx, linedisp);
 	unsigned int i;
 	int err = 0;
 
-	for (i = 0; i < ctx->cfg->num_chars; i++) {
+	for (i = 0; i < linedisp->num_chars; i++) {
 		err = sead3_wait_lcd_idle(ctx);
 		if (err)
 			break;
@@ -218,140 +214,6 @@ static const struct of_device_id img_ascii_lcd_matches[] = {
 };
 MODULE_DEVICE_TABLE(of, img_ascii_lcd_matches);
 
-/**
- * img_ascii_lcd_scroll() - scroll the display by a character
- * @t: really a pointer to the private data structure
- *
- * Scroll the current message along the LCD by one character, rearming the
- * timer if required.
- */
-static void img_ascii_lcd_scroll(struct timer_list *t)
-{
-	struct img_ascii_lcd_ctx *ctx = from_timer(ctx, t, timer);
-	unsigned int i, ch = ctx->scroll_pos;
-	unsigned int num_chars = ctx->cfg->num_chars;
-
-	/* update the current message string */
-	for (i = 0; i < num_chars;) {
-		/* copy as many characters from the string as possible */
-		for (; i < num_chars && ch < ctx->message_len; i++, ch++)
-			ctx->curr[i] = ctx->message[ch];
-
-		/* wrap around to the start of the string */
-		ch = 0;
-	}
-
-	/* update the LCD */
-	ctx->cfg->update(ctx);
-
-	/* move on to the next character */
-	ctx->scroll_pos++;
-	ctx->scroll_pos %= ctx->message_len;
-
-	/* rearm the timer */
-	if (ctx->message_len > ctx->cfg->num_chars)
-		mod_timer(&ctx->timer, jiffies + ctx->scroll_rate);
-}
-
-/**
- * img_ascii_lcd_display() - set the message to be displayed
- * @ctx: pointer to the private data structure
- * @msg: the message to display
- * @count: length of msg, or -1
- *
- * Display a new message @msg on the LCD. @msg can be longer than the number of
- * characters the LCD can display, in which case it will begin scrolling across
- * the LCD display.
- *
- * Return: 0 on success, -ENOMEM on memory allocation failure
- */
-static int img_ascii_lcd_display(struct img_ascii_lcd_ctx *ctx,
-			     const char *msg, ssize_t count)
-{
-	char *new_msg;
-
-	/* stop the scroll timer */
-	del_timer_sync(&ctx->timer);
-
-	if (count == -1)
-		count = strlen(msg);
-
-	/* if the string ends with a newline, trim it */
-	if (msg[count - 1] == '\n')
-		count--;
-
-	if (!count) {
-		/* clear the LCD */
-		devm_kfree(&ctx->pdev->dev, ctx->message);
-		ctx->message = NULL;
-		ctx->message_len = 0;
-		memset(ctx->curr, ' ', ctx->cfg->num_chars);
-		ctx->cfg->update(ctx);
-		return 0;
-	}
-
-	new_msg = devm_kmalloc(&ctx->pdev->dev, count + 1, GFP_KERNEL);
-	if (!new_msg)
-		return -ENOMEM;
-
-	memcpy(new_msg, msg, count);
-	new_msg[count] = 0;
-
-	if (ctx->message)
-		devm_kfree(&ctx->pdev->dev, ctx->message);
-
-	ctx->message = new_msg;
-	ctx->message_len = count;
-	ctx->scroll_pos = 0;
-
-	/* update the LCD */
-	img_ascii_lcd_scroll(&ctx->timer);
-
-	return 0;
-}
-
-/**
- * message_show() - read message via sysfs
- * @dev: the LCD device
- * @attr: the LCD message attribute
- * @buf: the buffer to read the message into
- *
- * Read the current message being displayed or scrolled across the LCD display
- * into @buf, for reads from sysfs.
- *
- * Return: the number of characters written to @buf
- */
-static ssize_t message_show(struct device *dev, struct device_attribute *attr,
-			    char *buf)
-{
-	struct img_ascii_lcd_ctx *ctx = dev_get_drvdata(dev);
-
-	return sysfs_emit(buf, "%s\n", ctx->message);
-}
-
-/**
- * message_store() - write a new message via sysfs
- * @dev: the LCD device
- * @attr: the LCD message attribute
- * @buf: the buffer containing the new message
- * @count: the size of the message in @buf
- *
- * Write a new message to display or scroll across the LCD display from sysfs.
- *
- * Return: the size of the message on success, else -ERRNO
- */
-static ssize_t message_store(struct device *dev, struct device_attribute *attr,
-			     const char *buf, size_t count)
-{
-	struct img_ascii_lcd_ctx *ctx = dev_get_drvdata(dev);
-	int err;
-
-	err = img_ascii_lcd_display(ctx, buf, count);
-	return err ?: count;
-}
-
-static DEVICE_ATTR_RW(message);
-
 /**
  * img_ascii_lcd_probe() - probe an LCD display device
  * @pdev: the LCD platform device
@@ -391,30 +253,13 @@ static int img_ascii_lcd_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->base);
 	}
 
-	ctx->pdev = pdev;
-	ctx->cfg = cfg;
-	ctx->message = NULL;
-	ctx->scroll_pos = 0;
-	ctx->scroll_rate = HZ / 2;
-
-	/* initialise a timer for scrolling the message */
-	timer_setup(&ctx->timer, img_ascii_lcd_scroll, 0);
-
-	platform_set_drvdata(pdev, ctx);
-
-	/* display a default message */
-	err = img_ascii_lcd_display(ctx, "Linux " UTS_RELEASE "       ", -1);
-	if (err)
-		goto out_del_timer;
-
-	err = device_create_file(dev, &dev_attr_message);
+	err = linedisp_register(&ctx->linedisp, dev, cfg->num_chars, ctx->curr,
+				cfg->update);
 	if (err)
-		goto out_del_timer;
+		return err;
 
+	platform_set_drvdata(pdev, ctx);
 	return 0;
-out_del_timer:
-	del_timer_sync(&ctx->timer);
-	return err;
 }
 
 /**
@@ -430,8 +275,7 @@ static int img_ascii_lcd_remove(struct platform_device *pdev)
 {
 	struct img_ascii_lcd_ctx *ctx = platform_get_drvdata(pdev);
 
-	device_remove_file(&pdev->dev, &dev_attr_message);
-	del_timer_sync(&ctx->timer);
+	linedisp_unregister(&ctx->linedisp);
 	return 0;
 }
 
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
new file mode 100644
index 0000000000000000..4b97c20ac0b381ee
--- /dev/null
+++ b/drivers/auxdisplay/line-display.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Character line display core support
+ *
+ * Copyright (C) 2016 Imagination Technologies
+ * Author: Paul Burton <paul.burton@mips.com>
+ *
+ * Copyright (C) 2021 Glider bv
+ */
+
+#include <generated/utsrelease.h>
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+#include <linux/timer.h>
+
+#include "line-display.h"
+
+/**
+ * linedisp_scroll() - scroll the display by a character
+ * @t: really a pointer to the private data structure
+ *
+ * Scroll the current message along the display by one character, rearming the
+ * timer if required.
+ */
+static void linedisp_scroll(struct timer_list *t)
+{
+	struct linedisp *linedisp = from_timer(linedisp, t, timer);
+	unsigned int i, ch = linedisp->scroll_pos;
+	unsigned int num_chars = linedisp->num_chars;
+
+	/* update the current message string */
+	for (i = 0; i < num_chars;) {
+		/* copy as many characters from the string as possible */
+		for (; i < num_chars && ch < linedisp->message_len; i++, ch++)
+			linedisp->buf[i] = linedisp->message[ch];
+
+		/* wrap around to the start of the string */
+		ch = 0;
+	}
+
+	/* update the display */
+	linedisp->update(linedisp);
+
+	/* move on to the next character */
+	linedisp->scroll_pos++;
+	linedisp->scroll_pos %= linedisp->message_len;
+
+	/* rearm the timer */
+	if (linedisp->message_len > num_chars)
+		mod_timer(&linedisp->timer, jiffies + linedisp->scroll_rate);
+}
+
+/**
+ * linedisp_display() - set the message to be displayed
+ * @linedisp: pointer to the private data structure
+ * @msg: the message to display
+ * @count: length of msg, or -1
+ *
+ * Display a new message @msg on the display. @msg can be longer than the
+ * number of characters the display can display, in which case it will begin
+ * scrolling across the display.
+ *
+ * Return: 0 on success, -ENOMEM on memory allocation failure
+ */
+static int linedisp_display(struct linedisp *linedisp, const char *msg,
+			    ssize_t count)
+{
+	char *new_msg;
+
+	/* stop the scroll timer */
+	del_timer_sync(&linedisp->timer);
+
+	if (count == -1)
+		count = strlen(msg);
+
+	/* if the string ends with a newline, trim it */
+	if (msg[count - 1] == '\n')
+		count--;
+
+	if (!count) {
+		/* Clear the display */
+		kfree(linedisp->message);
+		linedisp->message = NULL;
+		linedisp->message_len = 0;
+		memset(linedisp->buf, ' ', linedisp->num_chars);
+		linedisp->update(linedisp);
+		return 0;
+	}
+
+	new_msg = kmalloc(count + 1, GFP_KERNEL);
+	if (!new_msg)
+		return -ENOMEM;
+
+	memcpy(new_msg, msg, count);
+	new_msg[count] = 0;
+
+	kfree(linedisp->message);
+
+	linedisp->message = new_msg;
+	linedisp->message_len = count;
+	linedisp->scroll_pos = 0;
+
+	/* update the display */
+	linedisp_scroll(&linedisp->timer);
+
+	return 0;
+}
+
+/**
+ * message_show() - read message via sysfs
+ * @dev: the display device
+ * @attr: the display message attribute
+ * @buf: the buffer to read the message into
+ *
+ * Read the current message being displayed or scrolled across the display into
+ * @buf, for reads from sysfs.
+ *
+ * Return: the number of characters written to @buf
+ */
+static ssize_t message_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+
+	return sysfs_emit(buf, "%s\n", linedisp->message);
+}
+
+/**
+ * message_store() - write a new message via sysfs
+ * @dev: the display device
+ * @attr: the display message attribute
+ * @buf: the buffer containing the new message
+ * @count: the size of the message in @buf
+ *
+ * Write a new message to display or scroll across the display from sysfs.
+ *
+ * Return: the size of the message on success, else -ERRNO
+ */
+static ssize_t message_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	int err;
+
+	err = linedisp_display(linedisp, buf, count);
+	return err ?: count;
+}
+
+static DEVICE_ATTR_RW(message);
+
+static struct attribute *linedisp_attrs[] = {
+	&dev_attr_message.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(linedisp);
+
+static const struct device_type linedisp_type = {
+	.groups	= linedisp_groups,
+};
+
+/**
+ * linedisp_register - register a character line display
+ * @linedisp: pointer to character line display structure
+ * @parent: parent device
+ * @num_chars: the number of characters that can be displayed
+ * @buf: pointer to a buffer that can hold @num_chars characters
+ * @update: Function called to update the display.  This must not sleep!
+ *
+ * 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))
+{
+	static atomic_t linedisp_id = ATOMIC_INIT(-1);
+	int err;
+
+	memset(linedisp, 0, sizeof(*linedisp));
+	linedisp->dev.parent = parent;
+	linedisp->dev.type = &linedisp_type;
+	linedisp->update = update;
+	linedisp->buf = buf;
+	linedisp->num_chars = num_chars;
+	linedisp->scroll_rate = HZ / 2;
+
+	device_initialize(&linedisp->dev);
+	dev_set_name(&linedisp->dev, "linedisp.%lu",
+		     (unsigned long)atomic_inc_return(&linedisp_id));
+
+	/* initialise a timer for scrolling the message */
+	timer_setup(&linedisp->timer, linedisp_scroll, 0);
+
+	err = device_add(&linedisp->dev);
+	if (err)
+		goto out_del_timer;
+
+	/* display a default message */
+	err = linedisp_display(linedisp, "Linux " UTS_RELEASE "       ", -1);
+	if (err)
+		goto out_del_dev;
+
+	return 0;
+
+out_del_dev:
+	device_del(&linedisp->dev);
+out_del_timer:
+	del_timer_sync(&linedisp->timer);
+	put_device(&linedisp->dev);
+	return err;
+}
+EXPORT_SYMBOL_GPL(linedisp_register);
+
+/**
+ * linedisp_unregister - unregister a character line display
+ * @linedisp: pointer to character line display structure registered previously
+ *	      with linedisp_register()
+ */
+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);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
new file mode 100644
index 0000000000000000..0f5891d34c485871
--- /dev/null
+++ b/drivers/auxdisplay/line-display.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Character line display core support
+ *
+ * Copyright (C) 2016 Imagination Technologies
+ * Author: Paul Burton <paul.burton@mips.com>
+ *
+ * Copyright (C) 2021 Glider bv
+ */
+
+#ifndef _LINEDISP_H
+#define _LINEDISP_H
+
+/**
+ * struct linedisp - character line display private data structure
+ * @dev: the line display device
+ * @timer: timer used to implement scrolling
+ * @update: function called to update the display
+ * @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
+ * @message_len: the length of the @message string
+ * @scroll_pos: index of the first character of @message currently displayed
+ * @scroll_rate: scroll interval in jiffies
+ */
+struct linedisp {
+	struct device dev;
+	struct timer_list timer;
+	void (*update)(struct linedisp *linedisp);
+	char *buf;
+	char *message;
+	unsigned int num_chars;
+	unsigned int message_len;
+	unsigned int scroll_pos;
+	unsigned int scroll_rate;
+};
+
+int linedisp_register(struct linedisp *linedisp, struct device *parent,
+		      unsigned int num_chars, char *buf,
+		      void (*update)(struct linedisp *linedisp));
+void linedisp_unregister(struct linedisp *linedisp);
+
+#endif /* LINEDISP_H */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 07/17] auxdisplay: linedisp: Use kmemdup_nul() helper
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (5 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 06/17] auxdisplay: Extract character line display core support Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 08/17] auxdisplay: linedisp: Add support for changing scroll rate Geert Uytterhoeven
                   ` (10 subsequent siblings)
  17 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Use the existing kmemdup_nul() helper instead of open-coding the same
operation.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/line-display.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 4b97c20ac0b381ee..3f35199bc39f511f 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -91,13 +91,10 @@ static int linedisp_display(struct linedisp *linedisp, const char *msg,
 		return 0;
 	}
 
-	new_msg = kmalloc(count + 1, GFP_KERNEL);
+	new_msg = kmemdup_nul(msg, count, GFP_KERNEL);
 	if (!new_msg)
 		return -ENOMEM;
 
-	memcpy(new_msg, msg, count);
-	new_msg[count] = 0;
-
 	kfree(linedisp->message);
 
 	linedisp->message = new_msg;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 08/17] auxdisplay: linedisp: Add support for changing scroll rate
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (6 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 07/17] auxdisplay: linedisp: Use kmemdup_nul() helper Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 09/17] auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize() Geert Uytterhoeven
                   ` (9 subsequent siblings)
  17 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

If the message to display is longer than the number of characters that
the display can show, the message will be scrolled.  Currently the
scroll rate is fixed, moving every 500 ms.

Add support for changing the scroll rate through a "scroll_step_ms"
device attribute in sysfs.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/line-display.c | 37 +++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 3f35199bc39f511f..03e7f104aa1add32 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -19,6 +19,8 @@
 
 #include "line-display.h"
 
+#define DEFAULT_SCROLL_RATE	(HZ / 2)
+
 /**
  * linedisp_scroll() - scroll the display by a character
  * @t: really a pointer to the private data structure
@@ -50,7 +52,7 @@ static void linedisp_scroll(struct timer_list *t)
 	linedisp->scroll_pos %= linedisp->message_len;
 
 	/* rearm the timer */
-	if (linedisp->message_len > num_chars)
+	if (linedisp->message_len > num_chars && linedisp->scroll_rate)
 		mod_timer(&linedisp->timer, jiffies + linedisp->scroll_rate);
 }
 
@@ -149,8 +151,39 @@ static ssize_t message_store(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RW(message);
 
+static ssize_t scroll_step_ms_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+
+	return sysfs_emit(buf, "%u\n", jiffies_to_msecs(linedisp->scroll_rate));
+}
+
+static ssize_t scroll_step_ms_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	unsigned int ms;
+
+	if (kstrtouint(buf, 10, &ms) != 0)
+		return -EINVAL;
+
+	linedisp->scroll_rate = msecs_to_jiffies(ms);
+	if (linedisp->message && linedisp->message_len > linedisp->num_chars) {
+		del_timer_sync(&linedisp->timer);
+		if (linedisp->scroll_rate)
+			linedisp_scroll(&linedisp->timer);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(scroll_step_ms);
+
 static struct attribute *linedisp_attrs[] = {
 	&dev_attr_message.attr,
+	&dev_attr_scroll_step_ms.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(linedisp);
@@ -182,7 +215,7 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	linedisp->update = update;
 	linedisp->buf = buf;
 	linedisp->num_chars = num_chars;
-	linedisp->scroll_rate = HZ / 2;
+	linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
 
 	device_initialize(&linedisp->dev);
 	dev_set_name(&linedisp->dev, "linedisp.%lu",
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 09/17] auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize()
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (7 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 08/17] auxdisplay: linedisp: Add support for changing scroll rate Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-23  7:59   ` robin
  2021-03-22 14:48 ` [PATCH 10/17] auxdisplay: ht16k33: Remove unneeded error check in keypad probe() Geert Uytterhoeven
                   ` (8 subsequent siblings)
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Use the existing HT16K33_FB_SIZE definition instead of open-coding the
same calculation using an hardcoded value.
While at it, restore reverse Christmas tree variable declaration order.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/ht16k33.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 1e69cc6d21a0dca2..6d39f12054618fa5 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -168,9 +168,9 @@ static void ht16k33_fb_update(struct work_struct *work)
 
 static int ht16k33_initialize(struct ht16k33_priv *priv)
 {
+	uint8_t data[HT16K33_FB_SIZE];
 	uint8_t byte;
 	int err;
-	uint8_t data[HT16K33_MATRIX_LED_MAX_COLS * 2];
 
 	/* Clear RAM (8 * 16 bits) */
 	memset(data, 0, sizeof(data));
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 10/17] auxdisplay: ht16k33: Remove unneeded error check in keypad probe()
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (8 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 09/17] auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize() Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-23  8:08   ` robin
  2021-03-22 14:48 ` [PATCH 11/17] auxdisplay: ht16k33: Convert to simple i2c probe function Geert Uytterhoeven
                   ` (7 subsequent siblings)
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

There is no need to check the return code of input_register_device(),
just propagate it to the caller.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/ht16k33.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 6d39f12054618fa5..13d18f218b4d196e 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -378,11 +378,7 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
 
 	ht16k33_keypad_stop(keypad->dev);
 
-	err = input_register_device(keypad->dev);
-	if (err)
-		return err;
-
-	return 0;
+	return input_register_device(keypad->dev);
 }
 
 static int ht16k33_probe(struct i2c_client *client,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 11/17] auxdisplay: ht16k33: Convert to simple i2c probe function
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (9 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 10/17] auxdisplay: ht16k33: Remove unneeded error check in keypad probe() Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-23  8:31   ` robin
  2021-03-22 14:48 ` [PATCH 12/17] auxdisplay: ht16k33: Add helper variable dev Geert Uytterhoeven
                   ` (6 subsequent siblings)
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

ht16k33_probe() does not use the passed i2c_device_id, so the driver can
be converted trivially to the new-style of i2c probing.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/ht16k33.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 13d18f218b4d196e..1b67f38109bddba8 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -381,8 +381,7 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
 	return input_register_device(keypad->dev);
 }
 
-static int ht16k33_probe(struct i2c_client *client,
-				  const struct i2c_device_id *id)
+static int ht16k33_probe(struct i2c_client *client)
 {
 	int err;
 	uint32_t dft_brightness;
@@ -523,7 +522,7 @@ static const struct of_device_id ht16k33_of_match[] = {
 MODULE_DEVICE_TABLE(of, ht16k33_of_match);
 
 static struct i2c_driver ht16k33_driver = {
-	.probe		= ht16k33_probe,
+	.probe_new	= ht16k33_probe,
 	.remove		= ht16k33_remove,
 	.driver		= {
 		.name		= DRIVER_NAME,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 12/17] auxdisplay: ht16k33: Add helper variable dev
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (10 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 11/17] auxdisplay: ht16k33: Convert to simple i2c probe function Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-23  8:57   ` robin
  2021-03-22 14:48 ` [PATCH 13/17] auxdisplay: ht16k33: Move delayed work Geert Uytterhoeven
                   ` (5 subsequent siblings)
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

This driver has many users of "client->dev".  Add shorthands to simplify
the code.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/ht16k33.c | 43 ++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 1b67f38109bddba8..37fca1d44c3e73e1 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -316,7 +316,8 @@ static void ht16k33_keypad_stop(struct input_dev *dev)
 static int ht16k33_keypad_probe(struct i2c_client *client,
 				struct ht16k33_keypad *keypad)
 {
-	struct device_node *node = client->dev.of_node;
+	struct device *dev = &client->dev;
+	struct device_node *node = dev->of_node;
 	u32 rows = HT16K33_MATRIX_KEYPAD_MAX_ROWS;
 	u32 cols = HT16K33_MATRIX_KEYPAD_MAX_COLS;
 	int err;
@@ -324,7 +325,7 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
 	keypad->client = client;
 	init_waitqueue_head(&keypad->wait);
 
-	keypad->dev = devm_input_allocate_device(&client->dev);
+	keypad->dev = devm_input_allocate_device(dev);
 	if (!keypad->dev)
 		return -ENOMEM;
 
@@ -341,17 +342,17 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
 	err = of_property_read_u32(node, "debounce-delay-ms",
 				   &keypad->debounce_ms);
 	if (err) {
-		dev_err(&client->dev, "key debounce delay not specified\n");
+		dev_err(dev, "key debounce delay not specified\n");
 		return err;
 	}
 
-	err = matrix_keypad_parse_of_params(&client->dev, &rows, &cols);
+	err = matrix_keypad_parse_of_params(dev, &rows, &cols);
 	if (err)
 		return err;
 	if (rows > HT16K33_MATRIX_KEYPAD_MAX_ROWS ||
 	    cols > HT16K33_MATRIX_KEYPAD_MAX_COLS) {
-		dev_err(&client->dev, "%u rows or %u cols out of range in DT\n",
-			rows, cols);
+		dev_err(dev, "%u rows or %u cols out of range in DT\n", rows,
+			cols);
 		return -ERANGE;
 	}
 
@@ -362,17 +363,17 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
 	err = matrix_keypad_build_keymap(NULL, NULL, rows, cols, NULL,
 					 keypad->dev);
 	if (err) {
-		dev_err(&client->dev, "failed to build keymap\n");
+		dev_err(dev, "failed to build keymap\n");
 		return err;
 	}
 
-	err = devm_request_threaded_irq(&client->dev, client->irq,
-					NULL, ht16k33_keypad_irq_thread,
+	err = devm_request_threaded_irq(dev, client->irq, NULL,
+					ht16k33_keypad_irq_thread,
 					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
 					DRIVER_NAME, keypad);
 	if (err) {
-		dev_err(&client->dev, "irq request failed %d, error %d\n",
-			client->irq, err);
+		dev_err(dev, "irq request failed %d, error %d\n", client->irq,
+			err);
 		return err;
 	}
 
@@ -389,14 +390,15 @@ static int ht16k33_probe(struct i2c_client *client)
 	struct backlight_properties bl_props;
 	struct ht16k33_priv *priv;
 	struct ht16k33_fbdev *fbdev;
-	struct device_node *node = client->dev.of_node;
+	struct device *dev = &client->dev;
+	struct device_node *node = dev->of_node;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		dev_err(&client->dev, "i2c_check_functionality error\n");
+		dev_err(dev, "i2c_check_functionality error\n");
 		return -EIO;
 	}
 
-	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
@@ -414,13 +416,13 @@ static int ht16k33_probe(struct i2c_client *client)
 	if (!fbdev->buffer)
 		return -ENOMEM;
 
-	fbdev->cache = devm_kmalloc(&client->dev, HT16K33_FB_SIZE, GFP_KERNEL);
+	fbdev->cache = devm_kmalloc(dev, HT16K33_FB_SIZE, GFP_KERNEL);
 	if (!fbdev->cache) {
 		err = -ENOMEM;
 		goto err_fbdev_buffer;
 	}
 
-	fbdev->info = framebuffer_alloc(0, &client->dev);
+	fbdev->info = framebuffer_alloc(0, dev);
 	if (!fbdev->info) {
 		err = -ENOMEM;
 		goto err_fbdev_buffer;
@@ -429,7 +431,7 @@ static int ht16k33_probe(struct i2c_client *client)
 	err = of_property_read_u32(node, "refresh-rate-hz",
 		&fbdev->refresh_rate);
 	if (err) {
-		dev_err(&client->dev, "refresh rate not specified\n");
+		dev_err(dev, "refresh rate not specified\n");
 		goto err_fbdev_info;
 	}
 	fb_bl_default_curve(fbdev->info, 0, MIN_BRIGHTNESS, MAX_BRIGHTNESS);
@@ -460,11 +462,10 @@ static int ht16k33_probe(struct i2c_client *client)
 	bl_props.type = BACKLIGHT_RAW;
 	bl_props.max_brightness = MAX_BRIGHTNESS;
 
-	bl = devm_backlight_device_register(&client->dev, DRIVER_NAME"-bl",
-					    &client->dev, priv,
+	bl = devm_backlight_device_register(dev, DRIVER_NAME"-bl", dev, priv,
 					    &ht16k33_bl_ops, &bl_props);
 	if (IS_ERR(bl)) {
-		dev_err(&client->dev, "failed to register backlight\n");
+		dev_err(dev, "failed to register backlight\n");
 		err = PTR_ERR(bl);
 		goto err_fbdev_unregister;
 	}
@@ -474,7 +475,7 @@ static int ht16k33_probe(struct i2c_client *client)
 	if (err) {
 		dft_brightness = MAX_BRIGHTNESS;
 	} else if (dft_brightness > MAX_BRIGHTNESS) {
-		dev_warn(&client->dev,
+		dev_warn(dev,
 			 "invalid default brightness level: %u, using %u\n",
 			 dft_brightness, MAX_BRIGHTNESS);
 		dft_brightness = MAX_BRIGHTNESS;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 13/17] auxdisplay: ht16k33: Move delayed work
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (11 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 12/17] auxdisplay: ht16k33: Add helper variable dev Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 14/17] auxdisplay: ht16k33: Extract ht16k33_brightness_set() Geert Uytterhoeven
                   ` (4 subsequent siblings)
  17 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Move delayed_work from ht16k33_fbdev to ht16k33_priv, as it is not
specific to dot-matrix displays, but common to all display types.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/ht16k33.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 37fca1d44c3e73e1..91b039702270ab10 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -65,11 +65,11 @@ struct ht16k33_fbdev {
 	uint32_t refresh_rate;
 	uint8_t *buffer;
 	uint8_t *cache;
-	struct delayed_work work;
 };
 
 struct ht16k33_priv {
 	struct i2c_client *client;
+	struct delayed_work work;
 	struct ht16k33_keypad keypad;
 	struct ht16k33_fbdev fbdev;
 };
@@ -117,7 +117,7 @@ static void ht16k33_fb_queue(struct ht16k33_priv *priv)
 {
 	struct ht16k33_fbdev *fbdev = &priv->fbdev;
 
-	schedule_delayed_work(&fbdev->work, HZ / fbdev->refresh_rate);
+	schedule_delayed_work(&priv->work, HZ / fbdev->refresh_rate);
 }
 
 /*
@@ -125,10 +125,9 @@ static void ht16k33_fb_queue(struct ht16k33_priv *priv)
  */
 static void ht16k33_fb_update(struct work_struct *work)
 {
-	struct ht16k33_fbdev *fbdev =
-		container_of(work, struct ht16k33_fbdev, work.work);
-	struct ht16k33_priv *priv =
-		container_of(fbdev, struct ht16k33_priv, fbdev);
+	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
+						 work.work);
+	struct ht16k33_fbdev *fbdev = &priv->fbdev;
 
 	uint8_t *p1, *p2;
 	int len, pos = 0, first = -1;
@@ -436,7 +435,7 @@ static int ht16k33_probe(struct i2c_client *client)
 	}
 	fb_bl_default_curve(fbdev->info, 0, MIN_BRIGHTNESS, MAX_BRIGHTNESS);
 
-	INIT_DELAYED_WORK(&fbdev->work, ht16k33_fb_update);
+	INIT_DELAYED_WORK(&priv->work, ht16k33_fb_update);
 	fbdev->info->fbops = &ht16k33_fb_ops;
 	fbdev->info->screen_base = (char __iomem *) fbdev->buffer;
 	fbdev->info->screen_size = HT16K33_FB_SIZE;
@@ -502,7 +501,7 @@ static int ht16k33_remove(struct i2c_client *client)
 	struct ht16k33_priv *priv = i2c_get_clientdata(client);
 	struct ht16k33_fbdev *fbdev = &priv->fbdev;
 
-	cancel_delayed_work_sync(&fbdev->work);
+	cancel_delayed_work_sync(&priv->work);
 	unregister_framebuffer(fbdev->info);
 	framebuffer_release(fbdev->info);
 	free_page((unsigned long) fbdev->buffer);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 14/17] auxdisplay: ht16k33: Extract ht16k33_brightness_set()
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (12 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 13/17] auxdisplay: ht16k33: Move delayed work Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 15/17] auxdisplay: ht16k33: Extract frame buffer probing Geert Uytterhoeven
                   ` (3 subsequent siblings)
  17 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Extract brightness handling into a helper function, so it can be called
from multiple places.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/ht16k33.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 91b039702270ab10..7c34b7a7879deb1a 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -113,6 +113,22 @@ static int ht16k33_display_off(struct ht16k33_priv *priv)
 	return i2c_smbus_write_byte(priv->client, REG_DISPLAY_SETUP);
 }
 
+static int ht16k33_brightness_set(struct ht16k33_priv *priv,
+				  unsigned int brightness)
+{
+	int error;
+
+	if (brightness == 0)
+		return ht16k33_display_off(priv);
+
+	error = ht16k33_display_on(priv);
+	if (error)
+		return error;
+
+	return i2c_smbus_write_byte(priv->client,
+				    REG_BRIGHTNESS | (brightness - 1));
+}
+
 static void ht16k33_fb_queue(struct ht16k33_priv *priv)
 {
 	struct ht16k33_fbdev *fbdev = &priv->fbdev;
@@ -197,13 +213,10 @@ static int ht16k33_bl_update_status(struct backlight_device *bl)
 
 	if (bl->props.power != FB_BLANK_UNBLANK ||
 	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
-	    bl->props.state & BL_CORE_FBBLANK || brightness == 0) {
-		return ht16k33_display_off(priv);
-	}
+	    bl->props.state & BL_CORE_FBBLANK)
+		brightness = 0;
 
-	ht16k33_display_on(priv);
-	return i2c_smbus_write_byte(priv->client,
-				    REG_BRIGHTNESS | (brightness - 1));
+	return ht16k33_brightness_set(priv, brightness);
 }
 
 static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 15/17] auxdisplay: ht16k33: Extract frame buffer probing
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (13 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 14/17] auxdisplay: ht16k33: Extract ht16k33_brightness_set() Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-22 14:48 ` [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays Geert Uytterhoeven
                   ` (2 subsequent siblings)
  17 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Extract all frame buffer (including backlight) probing into
ht16k33_fbdev_probe().

Call ht16k33_fbdev_probe() after ht16k33_keypad_probe(), as the latter
does not need any manual cleanup in the probe error path.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/auxdisplay/ht16k33.c | 96 ++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 43 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 7c34b7a7879deb1a..11d86960fbc5d8c1 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -394,33 +394,15 @@ static int ht16k33_keypad_probe(struct i2c_client *client,
 	return input_register_device(keypad->dev);
 }
 
-static int ht16k33_probe(struct i2c_client *client)
+static int ht16k33_fbdev_probe(struct i2c_client *client,
+			       struct ht16k33_priv *priv, uint32_t brightness)
 {
-	int err;
-	uint32_t dft_brightness;
-	struct backlight_device *bl;
-	struct backlight_properties bl_props;
-	struct ht16k33_priv *priv;
-	struct ht16k33_fbdev *fbdev;
 	struct device *dev = &client->dev;
+	struct ht16k33_fbdev *fbdev = &priv->fbdev;
 	struct device_node *node = dev->of_node;
-
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		dev_err(dev, "i2c_check_functionality error\n");
-		return -EIO;
-	}
-
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	priv->client = client;
-	i2c_set_clientdata(client, priv);
-	fbdev = &priv->fbdev;
-
-	err = ht16k33_initialize(priv);
-	if (err)
-		return err;
+	struct backlight_properties bl_props;
+	struct backlight_device *bl;
+	int err;
 
 	/* Framebuffer (2 bytes per column) */
 	BUILD_BUG_ON(PAGE_SIZE < HT16K33_FB_SIZE);
@@ -462,13 +444,6 @@ static int ht16k33_probe(struct i2c_client *client)
 	if (err)
 		goto err_fbdev_info;
 
-	/* Keypad */
-	if (client->irq > 0) {
-		err = ht16k33_keypad_probe(client, &priv->keypad);
-		if (err)
-			goto err_fbdev_unregister;
-	}
-
 	/* Backlight */
 	memset(&bl_props, 0, sizeof(struct backlight_properties));
 	bl_props.type = BACKLIGHT_RAW;
@@ -482,18 +457,7 @@ static int ht16k33_probe(struct i2c_client *client)
 		goto err_fbdev_unregister;
 	}
 
-	err = of_property_read_u32(node, "default-brightness-level",
-				   &dft_brightness);
-	if (err) {
-		dft_brightness = MAX_BRIGHTNESS;
-	} else if (dft_brightness > MAX_BRIGHTNESS) {
-		dev_warn(dev,
-			 "invalid default brightness level: %u, using %u\n",
-			 dft_brightness, MAX_BRIGHTNESS);
-		dft_brightness = MAX_BRIGHTNESS;
-	}
-
-	bl->props.brightness = dft_brightness;
+	bl->props.brightness = brightness;
 	ht16k33_bl_update_status(bl);
 
 	ht16k33_fb_queue(priv);
@@ -509,6 +473,52 @@ static int ht16k33_probe(struct i2c_client *client)
 	return err;
 }
 
+static int ht16k33_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct device_node *node = dev->of_node;
+	struct ht16k33_priv *priv;
+	uint32_t dft_brightness;
+	int err;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(dev, "i2c_check_functionality error\n");
+		return -EIO;
+	}
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	i2c_set_clientdata(client, priv);
+
+	err = ht16k33_initialize(priv);
+	if (err)
+		return err;
+
+	err = of_property_read_u32(node, "default-brightness-level",
+				   &dft_brightness);
+	if (err) {
+		dft_brightness = MAX_BRIGHTNESS;
+	} else if (dft_brightness > MAX_BRIGHTNESS) {
+		dev_warn(dev,
+			 "invalid default brightness level: %u, using %u\n",
+			 dft_brightness, MAX_BRIGHTNESS);
+		dft_brightness = MAX_BRIGHTNESS;
+	}
+
+	/* Keypad */
+	if (client->irq > 0) {
+		err = ht16k33_keypad_probe(client, &priv->keypad);
+		if (err)
+			return err;
+	}
+
+	/* Frame Buffer Display */
+	return ht16k33_fbdev_probe(client, priv, dft_brightness);
+}
+
 static int ht16k33_remove(struct i2c_client *client)
 {
 	struct ht16k33_priv *priv = i2c_get_clientdata(client);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (14 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 15/17] auxdisplay: ht16k33: Extract frame buffer probing Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-29  7:08   ` Robin van der Gracht
  2021-03-22 14:48 ` [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support Geert Uytterhoeven
  2021-03-22 17:03 ` [PATCH 00/17] auxdisplay: ht16k33: Add character display support Miguel Ojeda
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

The Holtek HT16K33 LED controller is not only used for driving
dot-matrix displays, but also for driving segment displays.

Add support for 4-digit 7-segment and quad 14-segment alphanumeric
displays, like the Adafruit 7-segment and 14-segment display backpack
and FeatherWing expansion boards.  Use the character line display core
support to display a message, which will be scrolled if it doesn't fit.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
The 7-segment support is based on schematics, and untested on actual
hardware.
---
 drivers/auxdisplay/ht16k33.c | 198 +++++++++++++++++++++++++++++++++--
 1 file changed, 191 insertions(+), 7 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 11d86960fbc5d8c1..0b502a6039f89c6b 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -5,6 +5,7 @@
  * Author: Robin van der Gracht <robin@protonic.nl>
  *
  * Copyright: (C) 2016 Protonic Holland.
+ * Copyright (C) 2021 Glider bv
  */
 
 #include <linux/kernel.h>
@@ -17,9 +18,18 @@
 #include <linux/backlight.h>
 #include <linux/input.h>
 #include <linux/input/matrix_keypad.h>
+#include <linux/leds.h>
+#include <linux/of_device.h>
 #include <linux/workqueue.h>
 #include <linux/mm.h>
 
+#include <linux/map_to_7segment.h>
+#include <linux/map_to_14segment.h>
+
+#include <asm/unaligned.h>
+
+#include "line-display.h"
+
 /* Registers */
 #define REG_SYSTEM_SETUP		0x20
 #define REG_SYSTEM_SETUP_OSC_ON		BIT(0)
@@ -47,6 +57,12 @@
 #define BYTES_PER_ROW		(HT16K33_MATRIX_LED_MAX_ROWS / 8)
 #define HT16K33_FB_SIZE		(HT16K33_MATRIX_LED_MAX_COLS * BYTES_PER_ROW)
 
+enum display_type {
+	DISP_MATRIX = 0,
+	DISP_QUAD_7SEG,
+	DISP_QUAD_14SEG,
+};
+
 struct ht16k33_keypad {
 	struct i2c_client *client;
 	struct input_dev *dev;
@@ -67,11 +83,25 @@ struct ht16k33_fbdev {
 	uint8_t *cache;
 };
 
+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];
+};
+
 struct ht16k33_priv {
 	struct i2c_client *client;
 	struct delayed_work work;
 	struct ht16k33_keypad keypad;
-	struct ht16k33_fbdev fbdev;
+	union {
+		struct ht16k33_fbdev fbdev;
+		struct ht16k33_seg seg;
+	};
+	enum display_type type;
 };
 
 static const struct fb_fix_screeninfo ht16k33_fb_fix = {
@@ -101,6 +131,33 @@ 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;
@@ -325,6 +382,51 @@ 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 void ht16k33_seg7_update(struct work_struct *work)
+{
+	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
+						 work.work);
+	struct ht16k33_seg *seg = &priv->seg;
+	char *s = seg->curr;
+	uint8_t buf[9];
+
+	buf[0] = map_to_seg7(&seg->map.seg7, *s++);
+	buf[1] = 0;
+	buf[2] = map_to_seg7(&seg->map.seg7, *s++);
+	buf[3] = 0;
+	buf[4] = 0;
+	buf[5] = 0;
+	buf[6] = map_to_seg7(&seg->map.seg7, *s++);
+	buf[7] = 0;
+	buf[8] = map_to_seg7(&seg->map.seg7, *s++);
+
+	i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
+}
+
+static void ht16k33_seg14_update(struct work_struct *work)
+{
+	struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
+						 work.work);
+	struct ht16k33_seg *seg = &priv->seg;
+	char *s = seg->curr;
+	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);
+
+	i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
+}
+
 static int ht16k33_keypad_probe(struct i2c_client *client,
 				struct ht16k33_keypad *keypad)
 {
@@ -473,10 +575,57 @@ static int ht16k33_fbdev_probe(struct i2c_client *client,
 	return err;
 }
 
+static int ht16k33_seg_probe(struct i2c_client *client,
+			     struct ht16k33_priv *priv, uint32_t brightness)
+{
+	struct ht16k33_seg *seg = &priv->seg;
+	struct device *dev = &client->dev;
+	int err;
+
+	err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
+	if (err)
+		return err;
+
+	switch (priv->type) {
+	case DISP_MATRIX:
+		/* not handled here */
+		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_update);
+	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;
+}
+
 static int ht16k33_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct device_node *node = dev->of_node;
+	const struct of_device_id *id;
 	struct ht16k33_priv *priv;
 	uint32_t dft_brightness;
 	int err;
@@ -491,6 +640,9 @@ static int ht16k33_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	priv->client = client;
+	id = i2c_of_match_device(dev->driver->of_match_table, client);
+	if (id)
+		priv->type = (uintptr_t)id->data;
 	i2c_set_clientdata(client, priv);
 
 	err = ht16k33_initialize(priv);
@@ -515,8 +667,19 @@ static int ht16k33_probe(struct i2c_client *client)
 			return err;
 	}
 
-	/* Frame Buffer Display */
-	return ht16k33_fbdev_probe(client, priv, dft_brightness);
+	switch (priv->type) {
+	case DISP_MATRIX:
+		/* Frame Buffer Display */
+		err = ht16k33_fbdev_probe(client, priv, dft_brightness);
+		break;
+
+	case DISP_QUAD_7SEG:
+	case DISP_QUAD_14SEG:
+		/* Segment Display */
+		err = ht16k33_seg_probe(client, priv, dft_brightness);
+		break;
+	}
+	return err;
 }
 
 static int ht16k33_remove(struct i2c_client *client)
@@ -525,9 +688,21 @@ static int ht16k33_remove(struct i2c_client *client)
 	struct ht16k33_fbdev *fbdev = &priv->fbdev;
 
 	cancel_delayed_work_sync(&priv->work);
-	unregister_framebuffer(fbdev->info);
-	framebuffer_release(fbdev->info);
-	free_page((unsigned long) fbdev->buffer);
+
+	switch (priv->type) {
+	case DISP_MATRIX:
+		unregister_framebuffer(fbdev->info);
+		framebuffer_release(fbdev->info);
+		free_page((unsigned long)fbdev->buffer);
+		break;
+
+	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;
+	}
 
 	return 0;
 }
@@ -539,7 +714,16 @@ static const struct i2c_device_id ht16k33_i2c_match[] = {
 MODULE_DEVICE_TABLE(i2c, ht16k33_i2c_match);
 
 static const struct of_device_id ht16k33_of_match[] = {
-	{ .compatible = "holtek,ht16k33", },
+	{
+		/* 0.56" 4-Digit 7-Segment FeatherWing Display (Red) */
+		.compatible = "adafruit,3108", .data = (void *)DISP_QUAD_7SEG,
+	}, {
+		/* 0.54" Quad Alphanumeric FeatherWing Display (Red) */
+		.compatible = "adafruit,3130", .data = (void *)DISP_QUAD_14SEG,
+	}, {
+		/* Generic, assumed Dot-Matrix Display */
+		.compatible = "holtek,ht16k33", .data = (void *)DISP_MATRIX,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ht16k33_of_match);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (15 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays Geert Uytterhoeven
@ 2021-03-22 14:48 ` Geert Uytterhoeven
  2021-03-23 10:08   ` Robin van der Gracht
  2021-03-22 17:03 ` [PATCH 00/17] auxdisplay: ht16k33: Add character display support Miguel Ojeda
  17 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 14:48 UTC (permalink / raw)
  To: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman
  Cc: devicetree, linux-mips, linux-kernel, Geert Uytterhoeven

Instantiate a single LED for a segment display.  This allows the user to
control display brightness and blinking through the LED class API and
triggers, and exposes the display color.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
For setting display brightness, this could use the existing backlight
support for frame buffer devices instantiated for dot-matrix displays.
However, using the leds subsystem instead has the advantage that the
driver can make use of the HT16K33's hardware blinking support, and can
expose the display color.
---
 drivers/auxdisplay/ht16k33.c | 81 ++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 4 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 0b502a6039f89c6b..5821addd9aec5633 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -29,6 +29,7 @@
 #include <asm/unaligned.h>
 
 #include "line-display.h"
+#include "../leds/leds.h"		/* for led_colors[] */
 
 /* Registers */
 #define REG_SYSTEM_SETUP		0x20
@@ -36,6 +37,10 @@
 
 #define REG_DISPLAY_SETUP		0x80
 #define REG_DISPLAY_SETUP_ON		BIT(0)
+#define REG_DISPLAY_SETUP_BLINK_OFF	(0 << 1)
+#define REG_DISPLAY_SETUP_BLINK_2HZ	(1 << 1)
+#define REG_DISPLAY_SETUP_BLINK_1HZ	(2 << 1)
+#define REG_DISPLAY_SETUP_BLINK_0HZ5	(3 << 1)
 
 #define REG_ROWINT_SET			0xA0
 #define REG_ROWINT_SET_INT_EN		BIT(0)
@@ -57,6 +62,8 @@
 #define BYTES_PER_ROW		(HT16K33_MATRIX_LED_MAX_ROWS / 8)
 #define HT16K33_FB_SIZE		(HT16K33_MATRIX_LED_MAX_COLS * BYTES_PER_ROW)
 
+#define COLOR_DEFAULT		LED_COLOR_ID_RED
+
 enum display_type {
 	DISP_MATRIX = 0,
 	DISP_QUAD_7SEG,
@@ -85,6 +92,7 @@ struct ht16k33_fbdev {
 
 struct ht16k33_seg {
 	struct linedisp linedisp;
+	struct led_classdev led;
 	union {
 		struct seg7_conversion_map seg7;
 		struct seg14_conversion_map seg14;
@@ -102,6 +110,7 @@ struct ht16k33_priv {
 		struct ht16k33_seg seg;
 	};
 	enum display_type type;
+	uint8_t blink;
 };
 
 static const struct fb_fix_screeninfo ht16k33_fb_fix = {
@@ -160,7 +169,7 @@ 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;
+	uint8_t data = REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON | priv->blink;
 
 	return i2c_smbus_write_byte(priv->client, data);
 }
@@ -175,8 +184,11 @@ static int ht16k33_brightness_set(struct ht16k33_priv *priv,
 {
 	int error;
 
-	if (brightness == 0)
+	if (brightness == 0) {
+		// Disable blink mode
+		priv->blink = REG_DISPLAY_SETUP_BLINK_OFF;
 		return ht16k33_display_off(priv);
+	}
 
 	error = ht16k33_display_on(priv);
 	if (error)
@@ -186,6 +198,49 @@ static int ht16k33_brightness_set(struct ht16k33_priv *priv,
 				    REG_BRIGHTNESS | (brightness - 1));
 }
 
+static int ht16k33_brightness_set_blocking(struct led_classdev *led_cdev,
+					   enum led_brightness brightness)
+{
+	struct ht16k33_priv *priv = container_of(led_cdev, struct ht16k33_priv,
+						 seg.led);
+
+	return ht16k33_brightness_set(priv, brightness);
+}
+
+static int ht16k33_blink_set(struct led_classdev *led_cdev,
+			     unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct ht16k33_priv *priv = container_of(led_cdev, struct ht16k33_priv,
+						 seg.led);
+	unsigned int delay;
+	uint8_t blink;
+	int error;
+
+	if (!*delay_on && !*delay_off) {
+		blink = REG_DISPLAY_SETUP_BLINK_1HZ;
+		delay = 1000;
+	} else if (*delay_on <= 750) {
+		blink = REG_DISPLAY_SETUP_BLINK_2HZ;
+		delay = 500;
+	} else if (*delay_on <= 1500) {
+		blink = REG_DISPLAY_SETUP_BLINK_1HZ;
+		delay = 1000;
+	} else {
+		blink = REG_DISPLAY_SETUP_BLINK_0HZ5;
+		delay = 2000;
+	}
+
+	error = i2c_smbus_write_byte(priv->client,
+				     REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON |
+				     blink);
+	if (error)
+		return error;
+
+	priv->blink = blink;
+	*delay_on = *delay_off = delay;
+	return 0;
+}
+
 static void ht16k33_fb_queue(struct ht16k33_priv *priv)
 {
 	struct ht16k33_fbdev *fbdev = &priv->fbdev;
@@ -578,11 +633,29 @@ static int ht16k33_fbdev_probe(struct i2c_client *client,
 static int ht16k33_seg_probe(struct i2c_client *client,
 			     struct ht16k33_priv *priv, uint32_t brightness)
 {
-	struct ht16k33_seg *seg = &priv->seg;
 	struct device *dev = &client->dev;
+	struct device_node *node = dev->of_node;
+	struct ht16k33_seg *seg = &priv->seg;
+	u32 color = COLOR_DEFAULT;
 	int err;
 
-	err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
+	of_property_read_u32(node, "color", &color);
+	seg->led.name = devm_kasprintf(dev, GFP_KERNEL,
+			DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT,
+			color < LED_COLOR_ID_MAX ? led_colors[color] : "");
+	seg->led.brightness_set_blocking = ht16k33_brightness_set_blocking;
+	seg->led.blink_set = ht16k33_blink_set;
+	seg->led.flags = LED_CORE_SUSPENDRESUME;
+	seg->led.brightness = brightness;
+	seg->led.max_brightness = MAX_BRIGHTNESS;
+
+	err = devm_led_classdev_register(dev, &seg->led);
+	if (err) {
+		dev_err(dev, "Failed to register LED\n");
+		return err;
+	}
+
+	err = ht16k33_brightness_set(priv, seg->led.brightness);
 	if (err)
 		return err;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 43+ messages in thread

* Re: [PATCH 04/17] auxdisplay: img-ascii-lcd: Add helper variable dev
  2021-03-22 14:48 ` [PATCH 04/17] auxdisplay: img-ascii-lcd: Add helper variable dev Geert Uytterhoeven
@ 2021-03-22 16:59   ` Miguel Ojeda
  2021-03-22 17:53     ` Geert Uytterhoeven
  0 siblings, 1 reply; 43+ messages in thread
From: Miguel Ojeda @ 2021-03-22 16:59 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman, devicetree, linux-mips, linux-kernel

On Mon, Mar 22, 2021 at 3:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> +       struct device *dev = &pdev->dev;

Do we want helpers like this to be `*const` (assuming they can)? (same
for patch 12).

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 00/17] auxdisplay: ht16k33: Add character display support
  2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
                   ` (16 preceding siblings ...)
  2021-03-22 14:48 ` [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support Geert Uytterhoeven
@ 2021-03-22 17:03 ` Miguel Ojeda
  17 siblings, 0 replies; 43+ messages in thread
From: Miguel Ojeda @ 2021-03-22 17:03 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman, devicetree, linux-mips, linux-kernel

On Mon, Mar 22, 2021 at 3:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Thanks for your comments!

We could use this as a showcase on how to structure and present a
patch series! ;-)

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 01/17] uapi: Add <linux/map_to_14segment.h>
  2021-03-22 14:48 ` [PATCH 01/17] uapi: Add <linux/map_to_14segment.h> Geert Uytterhoeven
@ 2021-03-22 17:05   ` Miguel Ojeda
  2021-03-22 17:51     ` Geert Uytterhoeven
  0 siblings, 1 reply; 43+ messages in thread
From: Miguel Ojeda @ 2021-03-22 17:05 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman, devicetree, linux-mips, linux-kernel

On Mon, Mar 22, 2021 at 3:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> You can find an image of the full character set at
> https://drive.google.com/file/d/1h3EYFBWHIjh8B_cwPA5ocAD-lFYipRie/view

Should we put it in e.g. the Docs or just alongside this file?

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays
  2021-03-22 14:48 ` [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays Geert Uytterhoeven
@ 2021-03-22 17:38   ` Rob Herring
  2021-03-23  9:12   ` robin
  1 sibling, 0 replies; 43+ messages in thread
From: Rob Herring @ 2021-03-22 17:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Miguel Ojeda, devicetree, linux-mips, linux-kernel,
	Robin van der Gracht, Greg Kroah-Hartman, Paul Burton,
	Rob Herring

On Mon, 22 Mar 2021 15:48:33 +0100, Geert Uytterhoeven wrote:
> The Holtek HT16K33 LED controller is not only used for driving
> dot-matrix displays, but also for driving segment displays.
> 
> Document compatible values for the Adafruit 7-segment[1] and
> 14-segment[2] FeatherWing expansion boards with red displays.  According
> to the schematics, all other Adafruit 7-segment and 14-segment display
> backpack and FeatherWing expansion boards (including bare boards and
> boards fitted with displays) are compatible with these two boards.
> Add a "color" property to support the different color variants.
> 
> [1] https://www.adafruit.com/product/3108
> [2] https://www.adafruit.com/product/3130
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Alternatives I considered:
>   1. Document the attached display type in a child node.
>      I.e. specify segment type, number of characters, and wiring.
>      Especially the latter would become really complex, due to the sheer
>      amount of possible wiring combinations.
>      Using this method, you also loose the ability to just connect a
>      display to an i2c bus, and instantiate the device from sysfs,
>      without using DT:
> 
> 	echo adafruit,3130 0x70 > /sys/class/i2c/i2c-adapter/.../new_device
> 
>   2. Do not use the "color" property, but document all Adafruit
>      7-segment and 14-segment display backpack and FeatherWing expansion
>      boards.
>      This would lead to a myriad of compatible values:
> 
>       - items:
> 	  - enum:
> 	      - adafruit,878      # 0.56" 4-Digit 7-Segment Display Backpack (Red)
> 	      - adafruit,879      # 0.56" 4-Digit 7-Segment Display Backpack (Yellow)
> 	      - adafruit,880      # 0.56" 4-Digit 7-Segment Display Backpack (Green)
> 	      - adafruit,881      # 0.56" 4-Digit 7-Segment Display Backpack (Blue)
> 	      - adafruit,1002     # 0.56" 4-Digit 7-Segment Display Backpack (White)
> 	  - const: adafruit,877   # 0.56" 4-Digit 7-Segment Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,1268     # 1.2" 4-Digit 7-Segment Display Backpack (Green)
> 	      - adafruit,1269     # 1.2" 4-Digit 7-Segment Display Backpack (Yellow)
> 	      - adafruit,1270     # 1.2" 4-Digit 7-Segment Display Backpack (Red)
> 	  - const: adafruit,1271  # 1.2" 4-Digit 7-Segment Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,1911     # 0.54" Quad Alphanumeric Display Backpack (Red)
> 	      - adafruit,1912     # 0.54" Quad Alphanumeric Display Backpack (Blue)
> 	      - adafruit,2157     # 0.54" Quad Alphanumeric Display Backpack (White)
> 	      - adafruit,2158     # 0.54" Quad Alphanumeric Display Backpack (Yellow)
> 	      - adafruit,2159     # 0.54" Quad Alphanumeric Display Backpack (Yellow-Green)
> 	      - adafruit,2160     # 0.54" Quad Alphanumeric Display Backpack (Green)
> 	  - const: adafruit,1910  # 0.54" Quad 14-segment Alphanumeric Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,3106     # 0.56" 4-Digit 7-Segment FeatherWing Display (Blue)
> 	      - adafruit,3107     # 0.56" 4-Digit 7-Segment FeatherWing Display (Green)
> 	      - adafruit,3108     # 0.56" 4-Digit 7-Segment FeatherWing Display (Red)
> 	      - adafruit,3109     # 0.56" 4-Digit 7-Segment FeatherWing Display (White)
> 	      - adafruit,3110     # 0.56" 4-Digit 7-Segment FeatherWing Display (Yellow)
> 	  - const: adafruit,3088  # 0.56" 4-Digit 7-Segment FeatherWing
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,3127     # 0.54" Quad Alphanumeric FeatherWing Display (White)
> 	      - adafruit,3128     # 0.54" Quad Alphanumeric FeatherWing Display (Blue)
> 	      - adafruit,3129     # 0.54" Quad Alphanumeric FeatherWing Display (Green)
> 	      - adafruit,3130     # 0.54" Quad Alphanumeric FeatherWing Display (Red)
> 	      - adafruit,3131     # 0.54" Quad Alphanumeric FeatherWing Display (Yellow)
> 	      - adafruit,3132     # 0.54" Quad Alphanumeric FeatherWing Display (Yellow-Green)
> 	  - const: adafruit,3089  # 0.54" Quad 14-segment Alphanumeric FeatherWing
> 	  - const: holtek,ht16k33
> ---
>  .../bindings/auxdisplay/holtek,ht16k33.yaml   | 22 ++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml:54:16: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: mapping values are not allowed in this context
  in "<unicode string>", line 54, column 16
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml:  mapping values are not allowed in this context
  in "<unicode string>", line 54, column 16
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
make: *** [Makefile:1380: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1456639

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 01/17] uapi: Add <linux/map_to_14segment.h>
  2021-03-22 17:05   ` Miguel Ojeda
@ 2021-03-22 17:51     ` Geert Uytterhoeven
  0 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 17:51 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, linux-kernel

Hi Miguel,

On Mon, Mar 22, 2021 at 6:05 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Mon, Mar 22, 2021 at 3:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > You can find an image of the full character set at
> > https://drive.google.com/file/d/1h3EYFBWHIjh8B_cwPA5ocAD-lFYipRie/view
>
> Should we put it in e.g. the Docs or just alongside this file?

I don't know.  We don't have the image for the 7-segment display mapping
in the docs either.
Perhaps I should push the programs I wrote to create the images (for
7-seg and 14-seg) to github?

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 04/17] auxdisplay: img-ascii-lcd: Add helper variable dev
  2021-03-22 16:59   ` Miguel Ojeda
@ 2021-03-22 17:53     ` Geert Uytterhoeven
  0 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-22 17:53 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, linux-kernel

Hi Miguel,

On Mon, Mar 22, 2021 at 5:59 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Mon, Mar 22, 2021 at 3:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > +       struct device *dev = &pdev->dev;
>
> Do we want helpers like this to be `*const` (assuming they can)? (same
> for patch 12).

No, e.g. the devm_*() functions need to make changes to the device's
structure.

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 09/17] auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize()
  2021-03-22 14:48 ` [PATCH 09/17] auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize() Geert Uytterhoeven
@ 2021-03-23  7:59   ` robin
  0 siblings, 0 replies; 43+ messages in thread
From: robin @ 2021-03-23  7:59 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	devicetree, linux-mips, linux-kernel

On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> Use the existing HT16K33_FB_SIZE definition instead of open-coding the
> same calculation using an hardcoded value.
> While at it, restore reverse Christmas tree variable declaration order.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/auxdisplay/ht16k33.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c 
> b/drivers/auxdisplay/ht16k33.c
> index 1e69cc6d21a0dca2..6d39f12054618fa5 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -168,9 +168,9 @@ static void ht16k33_fb_update(struct work_struct 
> *work)
> 
>  static int ht16k33_initialize(struct ht16k33_priv *priv)
>  {
> +	uint8_t data[HT16K33_FB_SIZE];
>  	uint8_t byte;
>  	int err;
> -	uint8_t data[HT16K33_MATRIX_LED_MAX_COLS * 2];
> 
>  	/* Clear RAM (8 * 16 bits) */
>  	memset(data, 0, sizeof(data));

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 10/17] auxdisplay: ht16k33: Remove unneeded error check in keypad probe()
  2021-03-22 14:48 ` [PATCH 10/17] auxdisplay: ht16k33: Remove unneeded error check in keypad probe() Geert Uytterhoeven
@ 2021-03-23  8:08   ` robin
  0 siblings, 0 replies; 43+ messages in thread
From: robin @ 2021-03-23  8:08 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	devicetree, linux-mips, linux-kernel

On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> There is no need to check the return code of input_register_device(),
> just propagate it to the caller.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/auxdisplay/ht16k33.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c 
> b/drivers/auxdisplay/ht16k33.c
> index 6d39f12054618fa5..13d18f218b4d196e 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -378,11 +378,7 @@ static int ht16k33_keypad_probe(struct i2c_client 
> *client,
> 
>  	ht16k33_keypad_stop(keypad->dev);
> 
> -	err = input_register_device(keypad->dev);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return input_register_device(keypad->dev);
>  }
> 
>  static int ht16k33_probe(struct i2c_client *client,

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 06/17] auxdisplay: Extract character line display core support
  2021-03-22 14:48 ` [PATCH 06/17] auxdisplay: Extract character line display core support Geert Uytterhoeven
@ 2021-03-23  8:18   ` robin
  2021-03-23  8:52     ` Geert Uytterhoeven
  0 siblings, 1 reply; 43+ messages in thread
From: robin @ 2021-03-23  8:18 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	devicetree, linux-mips, linux-kernel

On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> Extract the character line display core support from the simple ASCII
> LCD driver for the MIPS Boston, Malta & SEAD3 development boards into
> its own subdriver, so it can be reused for other displays.
> 
> Note that this moves the "message" device attribute in sysfs in a
> "linedisp.N" subdirectory.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Changes to img-ascii-lcd.c are untested due to lack of hardware.
> ---
>  drivers/auxdisplay/Kconfig         |   8 +
>  drivers/auxdisplay/Makefile        |   1 +
>  drivers/auxdisplay/img-ascii-lcd.c | 198 +++----------------------
>  drivers/auxdisplay/line-display.c  | 231 +++++++++++++++++++++++++++++
>  drivers/auxdisplay/line-display.h  |  43 ++++++
>  5 files changed, 304 insertions(+), 177 deletions(-)
>  create mode 100644 drivers/auxdisplay/line-display.c
>  create mode 100644 drivers/auxdisplay/line-display.h
> 
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index 1509cb74705a30ad..42fc7b155de09dbc 100644
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -25,6 +25,12 @@ 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
> @@ -155,6 +161,7 @@ config IMG_ASCII_LCD
>  	depends on HAS_IOMEM
>  	default y if MIPS_MALTA
>  	select MFD_SYSCON
> +	select LINEDISP
>  	help
>  	  Enable this to support the simple ASCII LCD displays found on
>  	  development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
> @@ -169,6 +176,7 @@ config HT16K33
>  	select FB_SYS_IMAGEBLIT
>  	select INPUT_MATRIXKMAP
>  	select FB_BACKLIGHT
> +	select LINEDISP

At this point in your patch stack it's not used by the ht16k33 driver. I 
think it
would be nicer to add this dependency when the code actually starts 
depending on it.
So that when this patch stack gets applied partially or not in one go 
the chunks
would be independent.

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 11/17] auxdisplay: ht16k33: Convert to simple i2c probe function
  2021-03-22 14:48 ` [PATCH 11/17] auxdisplay: ht16k33: Convert to simple i2c probe function Geert Uytterhoeven
@ 2021-03-23  8:31   ` robin
  0 siblings, 0 replies; 43+ messages in thread
From: robin @ 2021-03-23  8:31 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	devicetree, linux-mips, linux-kernel

On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> ht16k33_probe() does not use the passed i2c_device_id, so the driver 
> can
> be converted trivially to the new-style of i2c probing.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/auxdisplay/ht16k33.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c 
> b/drivers/auxdisplay/ht16k33.c
> index 13d18f218b4d196e..1b67f38109bddba8 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -381,8 +381,7 @@ static int ht16k33_keypad_probe(struct i2c_client 
> *client,
>  	return input_register_device(keypad->dev);
>  }
> 
> -static int ht16k33_probe(struct i2c_client *client,
> -				  const struct i2c_device_id *id)
> +static int ht16k33_probe(struct i2c_client *client)
>  {
>  	int err;
>  	uint32_t dft_brightness;
> @@ -523,7 +522,7 @@ static const struct of_device_id ht16k33_of_match[] 
> = {
>  MODULE_DEVICE_TABLE(of, ht16k33_of_match);
> 
>  static struct i2c_driver ht16k33_driver = {
> -	.probe		= ht16k33_probe,
> +	.probe_new	= ht16k33_probe,
>  	.remove		= ht16k33_remove,
>  	.driver		= {
>  		.name		= DRIVER_NAME,

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 06/17] auxdisplay: Extract character line display core support
  2021-03-23  8:18   ` robin
@ 2021-03-23  8:52     ` Geert Uytterhoeven
  0 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-23  8:52 UTC (permalink / raw)
  To: Robin van der Gracht
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List

Hi Robin,

On Tue, Mar 23, 2021 at 9:18 AM robin <robin@protonic.nl> wrote:
> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> > Extract the character line display core support from the simple ASCII
> > LCD driver for the MIPS Boston, Malta & SEAD3 development boards into
> > its own subdriver, so it can be reused for other displays.
> >
> > Note that this moves the "message" device attribute in sysfs in a
> > "linedisp.N" subdirectory.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/drivers/auxdisplay/Kconfig
> > +++ b/drivers/auxdisplay/Kconfig
> > @@ -25,6 +25,12 @@ 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
> > @@ -155,6 +161,7 @@ config IMG_ASCII_LCD
> >       depends on HAS_IOMEM
> >       default y if MIPS_MALTA
> >       select MFD_SYSCON
> > +     select LINEDISP
> >       help
> >         Enable this to support the simple ASCII LCD displays found on
> >         development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
> > @@ -169,6 +176,7 @@ config HT16K33
> >       select FB_SYS_IMAGEBLIT
> >       select INPUT_MATRIXKMAP
> >       select FB_BACKLIGHT
> > +     select LINEDISP
>
> At this point in your patch stack it's not used by the ht16k33 driver. I
> think it
> would be nicer to add this dependency when the code actually starts
> depending on it.
> So that when this patch stack gets applied partially or not in one go
> the chunks
> would be independent.

Indeed, I messed up the order. Will fix.
Thanks for spotting!

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 12/17] auxdisplay: ht16k33: Add helper variable dev
  2021-03-22 14:48 ` [PATCH 12/17] auxdisplay: ht16k33: Add helper variable dev Geert Uytterhoeven
@ 2021-03-23  8:57   ` robin
  0 siblings, 0 replies; 43+ messages in thread
From: robin @ 2021-03-23  8:57 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	devicetree, linux-mips, linux-kernel

On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> This driver has many users of "client->dev".  Add shorthands to 
> simplify
> the code.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/auxdisplay/ht16k33.c | 43 ++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c 
> b/drivers/auxdisplay/ht16k33.c
> index 1b67f38109bddba8..37fca1d44c3e73e1 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -316,7 +316,8 @@ static void ht16k33_keypad_stop(struct input_dev 
> *dev)
>  static int ht16k33_keypad_probe(struct i2c_client *client,
>  				struct ht16k33_keypad *keypad)
>  {
> -	struct device_node *node = client->dev.of_node;
> +	struct device *dev = &client->dev;
> +	struct device_node *node = dev->of_node;
>  	u32 rows = HT16K33_MATRIX_KEYPAD_MAX_ROWS;
>  	u32 cols = HT16K33_MATRIX_KEYPAD_MAX_COLS;
>  	int err;
> @@ -324,7 +325,7 @@ static int ht16k33_keypad_probe(struct i2c_client 
> *client,
>  	keypad->client = client;
>  	init_waitqueue_head(&keypad->wait);
> 
> -	keypad->dev = devm_input_allocate_device(&client->dev);
> +	keypad->dev = devm_input_allocate_device(dev);
>  	if (!keypad->dev)
>  		return -ENOMEM;
> 
> @@ -341,17 +342,17 @@ static int ht16k33_keypad_probe(struct i2c_client 
> *client,
>  	err = of_property_read_u32(node, "debounce-delay-ms",
>  				   &keypad->debounce_ms);
>  	if (err) {
> -		dev_err(&client->dev, "key debounce delay not specified\n");
> +		dev_err(dev, "key debounce delay not specified\n");
>  		return err;
>  	}
> 
> -	err = matrix_keypad_parse_of_params(&client->dev, &rows, &cols);
> +	err = matrix_keypad_parse_of_params(dev, &rows, &cols);
>  	if (err)
>  		return err;
>  	if (rows > HT16K33_MATRIX_KEYPAD_MAX_ROWS ||
>  	    cols > HT16K33_MATRIX_KEYPAD_MAX_COLS) {
> -		dev_err(&client->dev, "%u rows or %u cols out of range in DT\n",
> -			rows, cols);
> +		dev_err(dev, "%u rows or %u cols out of range in DT\n", rows,
> +			cols);
>  		return -ERANGE;
>  	}
> 
> @@ -362,17 +363,17 @@ static int ht16k33_keypad_probe(struct i2c_client 
> *client,
>  	err = matrix_keypad_build_keymap(NULL, NULL, rows, cols, NULL,
>  					 keypad->dev);
>  	if (err) {
> -		dev_err(&client->dev, "failed to build keymap\n");
> +		dev_err(dev, "failed to build keymap\n");
>  		return err;
>  	}
> 
> -	err = devm_request_threaded_irq(&client->dev, client->irq,
> -					NULL, ht16k33_keypad_irq_thread,
> +	err = devm_request_threaded_irq(dev, client->irq, NULL,
> +					ht16k33_keypad_irq_thread,
>  					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>  					DRIVER_NAME, keypad);
>  	if (err) {
> -		dev_err(&client->dev, "irq request failed %d, error %d\n",
> -			client->irq, err);
> +		dev_err(dev, "irq request failed %d, error %d\n", client->irq,
> +			err);
>  		return err;
>  	}
> 
> @@ -389,14 +390,15 @@ static int ht16k33_probe(struct i2c_client 
> *client)
>  	struct backlight_properties bl_props;
>  	struct ht16k33_priv *priv;
>  	struct ht16k33_fbdev *fbdev;
> -	struct device_node *node = client->dev.of_node;
> +	struct device *dev = &client->dev;
> +	struct device_node *node = dev->of_node;
> 
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> -		dev_err(&client->dev, "i2c_check_functionality error\n");
> +		dev_err(dev, "i2c_check_functionality error\n");
>  		return -EIO;
>  	}
> 
> -	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
> 
> @@ -414,13 +416,13 @@ static int ht16k33_probe(struct i2c_client 
> *client)
>  	if (!fbdev->buffer)
>  		return -ENOMEM;
> 
> -	fbdev->cache = devm_kmalloc(&client->dev, HT16K33_FB_SIZE, 
> GFP_KERNEL);
> +	fbdev->cache = devm_kmalloc(dev, HT16K33_FB_SIZE, GFP_KERNEL);
>  	if (!fbdev->cache) {
>  		err = -ENOMEM;
>  		goto err_fbdev_buffer;
>  	}
> 
> -	fbdev->info = framebuffer_alloc(0, &client->dev);
> +	fbdev->info = framebuffer_alloc(0, dev);
>  	if (!fbdev->info) {
>  		err = -ENOMEM;
>  		goto err_fbdev_buffer;
> @@ -429,7 +431,7 @@ static int ht16k33_probe(struct i2c_client *client)
>  	err = of_property_read_u32(node, "refresh-rate-hz",
>  		&fbdev->refresh_rate);
>  	if (err) {
> -		dev_err(&client->dev, "refresh rate not specified\n");
> +		dev_err(dev, "refresh rate not specified\n");
>  		goto err_fbdev_info;
>  	}
>  	fb_bl_default_curve(fbdev->info, 0, MIN_BRIGHTNESS, MAX_BRIGHTNESS);
> @@ -460,11 +462,10 @@ static int ht16k33_probe(struct i2c_client 
> *client)
>  	bl_props.type = BACKLIGHT_RAW;
>  	bl_props.max_brightness = MAX_BRIGHTNESS;
> 
> -	bl = devm_backlight_device_register(&client->dev, DRIVER_NAME"-bl",
> -					    &client->dev, priv,
> +	bl = devm_backlight_device_register(dev, DRIVER_NAME"-bl", dev, priv,
>  					    &ht16k33_bl_ops, &bl_props);
>  	if (IS_ERR(bl)) {
> -		dev_err(&client->dev, "failed to register backlight\n");
> +		dev_err(dev, "failed to register backlight\n");
>  		err = PTR_ERR(bl);
>  		goto err_fbdev_unregister;
>  	}
> @@ -474,7 +475,7 @@ static int ht16k33_probe(struct i2c_client *client)
>  	if (err) {
>  		dft_brightness = MAX_BRIGHTNESS;
>  	} else if (dft_brightness > MAX_BRIGHTNESS) {
> -		dev_warn(&client->dev,
> +		dev_warn(dev,
>  			 "invalid default brightness level: %u, using %u\n",
>  			 dft_brightness, MAX_BRIGHTNESS);
>  		dft_brightness = MAX_BRIGHTNESS;

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays
  2021-03-22 14:48 ` [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays Geert Uytterhoeven
  2021-03-22 17:38   ` Rob Herring
@ 2021-03-23  9:12   ` robin
  2021-05-18 14:35     ` Geert Uytterhoeven
  1 sibling, 1 reply; 43+ messages in thread
From: robin @ 2021-03-23  9:12 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	devicetree, linux-mips, linux-kernel

On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> The Holtek HT16K33 LED controller is not only used for driving
> dot-matrix displays, but also for driving segment displays.
> 
> Document compatible values for the Adafruit 7-segment[1] and
> 14-segment[2] FeatherWing expansion boards with red displays.  
> According
> to the schematics, all other Adafruit 7-segment and 14-segment display
> backpack and FeatherWing expansion boards (including bare boards and
> boards fitted with displays) are compatible with these two boards.
> Add a "color" property to support the different color variants.
> 
> [1] https://www.adafruit.com/product/3108
> [2] https://www.adafruit.com/product/3130
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Alternatives I considered:
>   1. Document the attached display type in a child node.
>      I.e. specify segment type, number of characters, and wiring.
>      Especially the latter would become really complex, due to the 
> sheer
>      amount of possible wiring combinations.
>      Using this method, you also loose the ability to just connect a
>      display to an i2c bus, and instantiate the device from sysfs,
>      without using DT:
> 
> 	echo adafruit,3130 0x70 > /sys/class/i2c/i2c-adapter/.../new_device
> 
>   2. Do not use the "color" property, but document all Adafruit
>      7-segment and 14-segment display backpack and FeatherWing 
> expansion
>      boards.
>      This would lead to a myriad of compatible values:
> 
>       - items:
> 	  - enum:
> 	      - adafruit,878      # 0.56" 4-Digit 7-Segment Display Backpack 
> (Red)
> 	      - adafruit,879      # 0.56" 4-Digit 7-Segment Display Backpack 
> (Yellow)
> 	      - adafruit,880      # 0.56" 4-Digit 7-Segment Display Backpack 
> (Green)
> 	      - adafruit,881      # 0.56" 4-Digit 7-Segment Display Backpack 
> (Blue)
> 	      - adafruit,1002     # 0.56" 4-Digit 7-Segment Display Backpack 
> (White)
> 	  - const: adafruit,877   # 0.56" 4-Digit 7-Segment Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,1268     # 1.2" 4-Digit 7-Segment Display Backpack 
> (Green)
> 	      - adafruit,1269     # 1.2" 4-Digit 7-Segment Display Backpack 
> (Yellow)
> 	      - adafruit,1270     # 1.2" 4-Digit 7-Segment Display Backpack 
> (Red)
> 	  - const: adafruit,1271  # 1.2" 4-Digit 7-Segment Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,1911     # 0.54" Quad Alphanumeric Display Backpack 
> (Red)
> 	      - adafruit,1912     # 0.54" Quad Alphanumeric Display Backpack 
> (Blue)
> 	      - adafruit,2157     # 0.54" Quad Alphanumeric Display Backpack 
> (White)
> 	      - adafruit,2158     # 0.54" Quad Alphanumeric Display Backpack 
> (Yellow)
> 	      - adafruit,2159     # 0.54" Quad Alphanumeric Display Backpack
> (Yellow-Green)
> 	      - adafruit,2160     # 0.54" Quad Alphanumeric Display Backpack 
> (Green)
> 	  - const: adafruit,1910  # 0.54" Quad 14-segment Alphanumeric 
> Backpack
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,3106     # 0.56" 4-Digit 7-Segment FeatherWing 
> Display (Blue)
> 	      - adafruit,3107     # 0.56" 4-Digit 7-Segment FeatherWing 
> Display (Green)
> 	      - adafruit,3108     # 0.56" 4-Digit 7-Segment FeatherWing 
> Display (Red)
> 	      - adafruit,3109     # 0.56" 4-Digit 7-Segment FeatherWing 
> Display (White)
> 	      - adafruit,3110     # 0.56" 4-Digit 7-Segment FeatherWing
> Display (Yellow)
> 	  - const: adafruit,3088  # 0.56" 4-Digit 7-Segment FeatherWing
> 	  - const: holtek,ht16k33
> 
>       - items:
> 	  - enum:
> 	      - adafruit,3127     # 0.54" Quad Alphanumeric FeatherWing 
> Display (White)
> 	      - adafruit,3128     # 0.54" Quad Alphanumeric FeatherWing 
> Display (Blue)
> 	      - adafruit,3129     # 0.54" Quad Alphanumeric FeatherWing 
> Display (Green)
> 	      - adafruit,3130     # 0.54" Quad Alphanumeric FeatherWing 
> Display (Red)
> 	      - adafruit,3131     # 0.54" Quad Alphanumeric FeatherWing
> Display (Yellow)
> 	      - adafruit,3132     # 0.54" Quad Alphanumeric FeatherWing
> Display (Yellow-Green)
> 	  - const: adafruit,3089  # 0.54" Quad 14-segment Alphanumeric 
> FeatherWing
> 	  - const: holtek,ht16k33
> ---
>  .../bindings/auxdisplay/holtek,ht16k33.yaml   | 22 ++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git
> a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> index 64ffff460026040f..4380a5177a67d2e2 100644
> --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> @@ -14,14 +14,23 @@ allOf:
> 
>  properties:
>    compatible:
> -    const: holtek,ht16k33
> +    oneOf:
> +      - items:
> +          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment
> FeatherWing Display (Red)
> +          - const: holtek,ht16k33
> +
> +      - items:
> +          - const: adafruit,3130  # 0.54" Quad Alphanumeric
> FeatherWing Display (Red)
> +          - const: holtek,ht16k33
> +
> +      - const: holtek,ht16k33     # Generic 16*8 LED controller with
> dot-matrix display
> 
>    reg:
>      maxItems: 1
> 
>    refresh-rate-hz:
>      maxItems: 1
> -    description: Display update interval in Hertz
> +    description: Display update interval in Hertz for dot-matrix 
> displays

The above should be included in patch 16

> 
>    interrupts:
>      maxItems: 1
> @@ -41,10 +50,17 @@ properties:
>      default: 16
>      description: Initial brightness level
> 
> +  color: true
> +    description:
> +      Color of the display.  Use one of the LED_COLOR_ID_* prefixed 
> definitions
> +      from the header include/dt-bindings/leds/common.h.  The default 
> is red.
> +    minimum: 0
> +    maximum: 9
> +    default: 1
> +

The above should be included in patch 17

>  required:
>    - compatible
>    - reg
> -  - refresh-rate-hz

'refresh-rate-hz' is still a required property for the dot-matrix / 
fbdev setup.
If it can no longer be listed here than maybe its nice to mention that 
it's required
somewhere else (in it's description?).
Rob?

Groetjes / Kind regards,

Robin van der Gracht

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support
  2021-03-22 14:48 ` [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support Geert Uytterhoeven
@ 2021-03-23 10:08   ` Robin van der Gracht
  2021-03-23 12:32     ` Geert Uytterhoeven
  0 siblings, 1 reply; 43+ messages in thread
From: Robin van der Gracht @ 2021-03-23 10:08 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	devicetree, linux-mips, linux-kernel

On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> Instantiate a single LED for a segment display.  This allows the user 
> to
> control display brightness and blinking through the LED class API and
> triggers, and exposes the display color.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> For setting display brightness, this could use the existing backlight
> support for frame buffer devices instantiated for dot-matrix displays.
> However, using the leds subsystem instead has the advantage that the
> driver can make use of the HT16K33's hardware blinking support, and can
> expose the display color.
> ---
>  drivers/auxdisplay/ht16k33.c | 81 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/auxdisplay/ht16k33.c 
> b/drivers/auxdisplay/ht16k33.c
> index 0b502a6039f89c6b..5821addd9aec5633 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -29,6 +29,7 @@
>  #include <asm/unaligned.h>
> 
>  #include "line-display.h"
> +#include "../leds/leds.h"		/* for led_colors[] */
> 
>  /* Registers */
>  #define REG_SYSTEM_SETUP		0x20
> @@ -36,6 +37,10 @@
> 
>  #define REG_DISPLAY_SETUP		0x80
>  #define REG_DISPLAY_SETUP_ON		BIT(0)
> +#define REG_DISPLAY_SETUP_BLINK_OFF	(0 << 1)
> +#define REG_DISPLAY_SETUP_BLINK_2HZ	(1 << 1)
> +#define REG_DISPLAY_SETUP_BLINK_1HZ	(2 << 1)
> +#define REG_DISPLAY_SETUP_BLINK_0HZ5	(3 << 1)
> 
>  #define REG_ROWINT_SET			0xA0
>  #define REG_ROWINT_SET_INT_EN		BIT(0)
> @@ -57,6 +62,8 @@
>  #define BYTES_PER_ROW		(HT16K33_MATRIX_LED_MAX_ROWS / 8)
>  #define HT16K33_FB_SIZE		(HT16K33_MATRIX_LED_MAX_COLS * BYTES_PER_ROW)
> 
> +#define COLOR_DEFAULT		LED_COLOR_ID_RED
> +
>  enum display_type {
>  	DISP_MATRIX = 0,
>  	DISP_QUAD_7SEG,
> @@ -85,6 +92,7 @@ struct ht16k33_fbdev {
> 
>  struct ht16k33_seg {
>  	struct linedisp linedisp;
> +	struct led_classdev led;
>  	union {
>  		struct seg7_conversion_map seg7;
>  		struct seg14_conversion_map seg14;
> @@ -102,6 +110,7 @@ struct ht16k33_priv {
>  		struct ht16k33_seg seg;
>  	};
>  	enum display_type type;
> +	uint8_t blink;
>  };
> 
>  static const struct fb_fix_screeninfo ht16k33_fb_fix = {
> @@ -160,7 +169,7 @@ 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;
> +	uint8_t data = REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON | 
> priv->blink;
> 
>  	return i2c_smbus_write_byte(priv->client, data);
>  }
> @@ -175,8 +184,11 @@ static int ht16k33_brightness_set(struct
> ht16k33_priv *priv,
>  {
>  	int error;
> 
> -	if (brightness == 0)
> +	if (brightness == 0) {
> +		// Disable blink mode
> +		priv->blink = REG_DISPLAY_SETUP_BLINK_OFF;
>  		return ht16k33_display_off(priv);
> +	}
> 
>  	error = ht16k33_display_on(priv);
>  	if (error)
> @@ -186,6 +198,49 @@ static int ht16k33_brightness_set(struct
> ht16k33_priv *priv,
>  				    REG_BRIGHTNESS | (brightness - 1));
>  }
> 
> +static int ht16k33_brightness_set_blocking(struct led_classdev 
> *led_cdev,
> +					   enum led_brightness brightness)
> +{
> +	struct ht16k33_priv *priv = container_of(led_cdev, struct 
> ht16k33_priv,
> +						 seg.led);
> +
> +	return ht16k33_brightness_set(priv, brightness);
> +}
> +
> +static int ht16k33_blink_set(struct led_classdev *led_cdev,
> +			     unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct ht16k33_priv *priv = container_of(led_cdev, struct 
> ht16k33_priv,
> +						 seg.led);
> +	unsigned int delay;
> +	uint8_t blink;
> +	int error;
> +
> +	if (!*delay_on && !*delay_off) {
> +		blink = REG_DISPLAY_SETUP_BLINK_1HZ;
> +		delay = 1000;
> +	} else if (*delay_on <= 750) {
> +		blink = REG_DISPLAY_SETUP_BLINK_2HZ;
> +		delay = 500;
> +	} else if (*delay_on <= 1500) {
> +		blink = REG_DISPLAY_SETUP_BLINK_1HZ;
> +		delay = 1000;
> +	} else {
> +		blink = REG_DISPLAY_SETUP_BLINK_0HZ5;
> +		delay = 2000;
> +	}
> +
> +	error = i2c_smbus_write_byte(priv->client,
> +				     REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON |
> +				     blink);
> +	if (error)
> +		return error;
> +
> +	priv->blink = blink;
> +	*delay_on = *delay_off = delay;
> +	return 0;
> +}
> +
>  static void ht16k33_fb_queue(struct ht16k33_priv *priv)
>  {
>  	struct ht16k33_fbdev *fbdev = &priv->fbdev;
> @@ -578,11 +633,29 @@ static int ht16k33_fbdev_probe(struct i2c_client 
> *client,
>  static int ht16k33_seg_probe(struct i2c_client *client,
>  			     struct ht16k33_priv *priv, uint32_t brightness)
>  {
> -	struct ht16k33_seg *seg = &priv->seg;
>  	struct device *dev = &client->dev;
> +	struct device_node *node = dev->of_node;
> +	struct ht16k33_seg *seg = &priv->seg;
> +	u32 color = COLOR_DEFAULT;
>  	int err;
> 
> -	err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> +	of_property_read_u32(node, "color", &color);
> +	seg->led.name = devm_kasprintf(dev, GFP_KERNEL,
> +			DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT,
> +			color < LED_COLOR_ID_MAX ? led_colors[color] : "");
> +	seg->led.brightness_set_blocking = ht16k33_brightness_set_blocking;
> +	seg->led.blink_set = ht16k33_blink_set;
> +	seg->led.flags = LED_CORE_SUSPENDRESUME;
> +	seg->led.brightness = brightness;
> +	seg->led.max_brightness = MAX_BRIGHTNESS;
> +
> +	err = devm_led_classdev_register(dev, &seg->led);
> +	if (err) {
> +		dev_err(dev, "Failed to register LED\n");
> +		return err;
> +	}
> +
> +	err = ht16k33_brightness_set(priv, seg->led.brightness);
>  	if (err)
>  		return err;

The LED class can pretty much do what the backlight class can and more.

Maybe we can stop registering a backlight device in the fbdev case and
register a led device for both. This makes the code cleaner and drops
a dependency but will break backwards compatibility.

I'd prefer a single solution that covers both use cases, but I'm not
sure about the 'breaking backwards compatibility' consequence...

Groetjes / Kind regards,
Robin van der Gracht

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support
  2021-03-23 10:08   ` Robin van der Gracht
@ 2021-03-23 12:32     ` Geert Uytterhoeven
  2021-03-23 20:40       ` Pavel Machek
  0 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-23 12:32 UTC (permalink / raw)
  To: Robin van der Gracht
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List,
	linux-leds, Pavel Machek, Dan Murphy

CC linux-leds (which I intended, but forgot to add)

cover letter at
https://lore.kernel.org/linux-devicetree/20210322144848.1065067-1-geert@linux-m68k.org/

On Tue, Mar 23, 2021 at 11:08 AM Robin van der Gracht <robin@protonic.nl> wrote:
>
> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> > Instantiate a single LED for a segment display.  This allows the user
> > to
> > control display brightness and blinking through the LED class API and
> > triggers, and exposes the display color.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > For setting display brightness, this could use the existing backlight
> > support for frame buffer devices instantiated for dot-matrix displays.
> > However, using the leds subsystem instead has the advantage that the
> > driver can make use of the HT16K33's hardware blinking support, and can
> > expose the display color.
> > ---
> >  drivers/auxdisplay/ht16k33.c | 81 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 77 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/auxdisplay/ht16k33.c
> > b/drivers/auxdisplay/ht16k33.c
> > index 0b502a6039f89c6b..5821addd9aec5633 100644
> > --- a/drivers/auxdisplay/ht16k33.c
> > +++ b/drivers/auxdisplay/ht16k33.c
> > @@ -29,6 +29,7 @@
> >  #include <asm/unaligned.h>
> >
> >  #include "line-display.h"
> > +#include "../leds/leds.h"            /* for led_colors[] */
> >
> >  /* Registers */
> >  #define REG_SYSTEM_SETUP             0x20
> > @@ -36,6 +37,10 @@
> >
> >  #define REG_DISPLAY_SETUP            0x80
> >  #define REG_DISPLAY_SETUP_ON         BIT(0)
> > +#define REG_DISPLAY_SETUP_BLINK_OFF  (0 << 1)
> > +#define REG_DISPLAY_SETUP_BLINK_2HZ  (1 << 1)
> > +#define REG_DISPLAY_SETUP_BLINK_1HZ  (2 << 1)
> > +#define REG_DISPLAY_SETUP_BLINK_0HZ5 (3 << 1)
> >
> >  #define REG_ROWINT_SET                       0xA0
> >  #define REG_ROWINT_SET_INT_EN                BIT(0)
> > @@ -57,6 +62,8 @@
> >  #define BYTES_PER_ROW                (HT16K33_MATRIX_LED_MAX_ROWS / 8)
> >  #define HT16K33_FB_SIZE              (HT16K33_MATRIX_LED_MAX_COLS * BYTES_PER_ROW)
> >
> > +#define COLOR_DEFAULT                LED_COLOR_ID_RED
> > +
> >  enum display_type {
> >       DISP_MATRIX = 0,
> >       DISP_QUAD_7SEG,
> > @@ -85,6 +92,7 @@ struct ht16k33_fbdev {
> >
> >  struct ht16k33_seg {
> >       struct linedisp linedisp;
> > +     struct led_classdev led;
> >       union {
> >               struct seg7_conversion_map seg7;
> >               struct seg14_conversion_map seg14;
> > @@ -102,6 +110,7 @@ struct ht16k33_priv {
> >               struct ht16k33_seg seg;
> >       };
> >       enum display_type type;
> > +     uint8_t blink;
> >  };
> >
> >  static const struct fb_fix_screeninfo ht16k33_fb_fix = {
> > @@ -160,7 +169,7 @@ 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;
> > +     uint8_t data = REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON |
> > priv->blink;
> >
> >       return i2c_smbus_write_byte(priv->client, data);
> >  }
> > @@ -175,8 +184,11 @@ static int ht16k33_brightness_set(struct
> > ht16k33_priv *priv,
> >  {
> >       int error;
> >
> > -     if (brightness == 0)
> > +     if (brightness == 0) {
> > +             // Disable blink mode
> > +             priv->blink = REG_DISPLAY_SETUP_BLINK_OFF;
> >               return ht16k33_display_off(priv);
> > +     }
> >
> >       error = ht16k33_display_on(priv);
> >       if (error)
> > @@ -186,6 +198,49 @@ static int ht16k33_brightness_set(struct
> > ht16k33_priv *priv,
> >                                   REG_BRIGHTNESS | (brightness - 1));
> >  }
> >
> > +static int ht16k33_brightness_set_blocking(struct led_classdev
> > *led_cdev,
> > +                                        enum led_brightness brightness)
> > +{
> > +     struct ht16k33_priv *priv = container_of(led_cdev, struct
> > ht16k33_priv,
> > +                                              seg.led);
> > +
> > +     return ht16k33_brightness_set(priv, brightness);
> > +}
> > +
> > +static int ht16k33_blink_set(struct led_classdev *led_cdev,
> > +                          unsigned long *delay_on, unsigned long *delay_off)
> > +{
> > +     struct ht16k33_priv *priv = container_of(led_cdev, struct
> > ht16k33_priv,
> > +                                              seg.led);
> > +     unsigned int delay;
> > +     uint8_t blink;
> > +     int error;
> > +
> > +     if (!*delay_on && !*delay_off) {
> > +             blink = REG_DISPLAY_SETUP_BLINK_1HZ;
> > +             delay = 1000;
> > +     } else if (*delay_on <= 750) {
> > +             blink = REG_DISPLAY_SETUP_BLINK_2HZ;
> > +             delay = 500;
> > +     } else if (*delay_on <= 1500) {
> > +             blink = REG_DISPLAY_SETUP_BLINK_1HZ;
> > +             delay = 1000;
> > +     } else {
> > +             blink = REG_DISPLAY_SETUP_BLINK_0HZ5;
> > +             delay = 2000;
> > +     }
> > +
> > +     error = i2c_smbus_write_byte(priv->client,
> > +                                  REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON |
> > +                                  blink);
> > +     if (error)
> > +             return error;
> > +
> > +     priv->blink = blink;
> > +     *delay_on = *delay_off = delay;
> > +     return 0;
> > +}
> > +
> >  static void ht16k33_fb_queue(struct ht16k33_priv *priv)
> >  {
> >       struct ht16k33_fbdev *fbdev = &priv->fbdev;
> > @@ -578,11 +633,29 @@ static int ht16k33_fbdev_probe(struct i2c_client
> > *client,
> >  static int ht16k33_seg_probe(struct i2c_client *client,
> >                            struct ht16k33_priv *priv, uint32_t brightness)
> >  {
> > -     struct ht16k33_seg *seg = &priv->seg;
> >       struct device *dev = &client->dev;
> > +     struct device_node *node = dev->of_node;
> > +     struct ht16k33_seg *seg = &priv->seg;
> > +     u32 color = COLOR_DEFAULT;
> >       int err;
> >
> > -     err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> > +     of_property_read_u32(node, "color", &color);
> > +     seg->led.name = devm_kasprintf(dev, GFP_KERNEL,
> > +                     DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT,
> > +                     color < LED_COLOR_ID_MAX ? led_colors[color] : "");
> > +     seg->led.brightness_set_blocking = ht16k33_brightness_set_blocking;
> > +     seg->led.blink_set = ht16k33_blink_set;
> > +     seg->led.flags = LED_CORE_SUSPENDRESUME;
> > +     seg->led.brightness = brightness;
> > +     seg->led.max_brightness = MAX_BRIGHTNESS;
> > +
> > +     err = devm_led_classdev_register(dev, &seg->led);
> > +     if (err) {
> > +             dev_err(dev, "Failed to register LED\n");
> > +             return err;
> > +     }
> > +
> > +     err = ht16k33_brightness_set(priv, seg->led.brightness);
> >       if (err)
> >               return err;
>
> The LED class can pretty much do what the backlight class can and more.
>
> Maybe we can stop registering a backlight device in the fbdev case and
> register a led device for both. This makes the code cleaner and drops
> a dependency but will break backwards compatibility.
>
> I'd prefer a single solution that covers both use cases, but I'm not
> sure about the 'breaking backwards compatibility' consequence...
>
> Groetjes / Kind regards,
> Robin van der Gracht

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support
  2021-03-23 12:32     ` Geert Uytterhoeven
@ 2021-03-23 20:40       ` Pavel Machek
  2021-03-24  8:31         ` Geert Uytterhoeven
  0 siblings, 1 reply; 43+ messages in thread
From: Pavel Machek @ 2021-03-23 20:40 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List,
	linux-leds, Dan Murphy

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

Hi!

> CC linux-leds (which I intended, but forgot to add)
> 
> cover letter at
> https://lore.kernel.org/linux-devicetree/20210322144848.1065067-1-geert@linux-m68k.org/

Still does not tell me... riscv on fpga with 4 character display. What
is this? :-).


> On Tue, Mar 23, 2021 at 11:08 AM Robin van der Gracht <robin@protonic.nl> wrote:
> >
> > On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> > > Instantiate a single LED for a segment display.  This allows the user
> > > to
> > > control display brightness and blinking through the LED class API and
> > > triggers, and exposes the display color.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > ---
> > > For setting display brightness, this could use the existing backlight
> > > support for frame buffer devices instantiated for dot-matrix displays.
> > > However, using the leds subsystem instead has the advantage that the
> > > driver can make use of the HT16K33's hardware blinking support, and can
> > > expose the display color.

We have multicolor support now...

> > > -     err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> > > +     of_property_read_u32(node, "color", &color);
> > > +     seg->led.name = devm_kasprintf(dev, GFP_KERNEL,
> > > +                     DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT,
> > > +                     color < LED_COLOR_ID_MAX ? led_colors[color] : "");

And would prefer not to see driver_name as part of LED name.

> > > +     err = ht16k33_brightness_set(priv, seg->led.brightness);
> > >       if (err)
> > >               return err;
> >
> > The LED class can pretty much do what the backlight class can and more.
> >
> > Maybe we can stop registering a backlight device in the fbdev case and
> > register a led device for both. This makes the code cleaner and drops
> > a dependency but will break backwards compatibility.
> >
> > I'd prefer a single solution that covers both use cases, but I'm not
> > sure about the 'breaking backwards compatibility' consequence...

For new drivers, breaking compatibility should not be a problem.

Best regards,
									Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support
  2021-03-23 20:40       ` Pavel Machek
@ 2021-03-24  8:31         ` Geert Uytterhoeven
  2021-03-27 22:19           ` Pavel Machek
  2021-06-25 12:39           ` Geert Uytterhoeven
  0 siblings, 2 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-24  8:31 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List,
	linux-leds, Dan Murphy

Hi Pavel,

On Tue, Mar 23, 2021 at 9:40 PM Pavel Machek <pavel@ucw.cz> wrote:
> > CC linux-leds (which I intended, but forgot to add)
> >
> > cover letter at
> > https://lore.kernel.org/linux-devicetree/20210322144848.1065067-1-geert@linux-m68k.org/
>
> Still does not tell me... riscv on fpga with 4 character display. What
> is this? :-).

https://gregdavill.github.io/OrangeCrab/r0.2/
https://github.com/litex-hub/linux-on-litex-vexriscv
https://www.adafruit.com/product/3130

> > On Tue, Mar 23, 2021 at 11:08 AM Robin van der Gracht <robin@protonic.nl> wrote:
> > > On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> > > > Instantiate a single LED for a segment display.  This allows the user
> > > > to
> > > > control display brightness and blinking through the LED class API and
> > > > triggers, and exposes the display color.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > ---
> > > > For setting display brightness, this could use the existing backlight
> > > > support for frame buffer devices instantiated for dot-matrix displays.
> > > > However, using the leds subsystem instead has the advantage that the
> > > > driver can make use of the HT16K33's hardware blinking support, and can
> > > > expose the display color.
>
> We have multicolor support now...

The color is fixed by the LED color, so there is no need for multicolor
(yet, one day someone might connect RGB 7-segment LED displays
(https://www.rgbdigit.com/rgbdigit/ ;-)

> > > > -     err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> > > > +     of_property_read_u32(node, "color", &color);
> > > > +     seg->led.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > +                     DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT,
> > > > +                     color < LED_COLOR_ID_MAX ? led_colors[color] : "");
>
> And would prefer not to see driver_name as part of LED name.

OK. So what should I use instead? Or just leave the part before the
first colon empty?

> > > > +     err = ht16k33_brightness_set(priv, seg->led.brightness);
> > > >       if (err)
> > > >               return err;
> > >
> > > The LED class can pretty much do what the backlight class can and more.
> > >
> > > Maybe we can stop registering a backlight device in the fbdev case and
> > > register a led device for both. This makes the code cleaner and drops
> > > a dependency but will break backwards compatibility.
> > >
> > > I'd prefer a single solution that covers both use cases, but I'm not
> > > sure about the 'breaking backwards compatibility' consequence...
>
> For new drivers, breaking compatibility should not be a problem.

The dot-matrix support is part of the existing driver, thus subject to
backwards compatibility.
Perhaps we can register the LED device for both, and build the backlight
device on top of the LED device, like "led-backlight" does.  Would that
work? Or can't the LED no longer be controlled from sysfs (e.g.
triggers) if it is in use by a backlight driver?

Thanks!

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support
  2021-03-24  8:31         ` Geert Uytterhoeven
@ 2021-03-27 22:19           ` Pavel Machek
  2021-06-25 12:39           ` Geert Uytterhoeven
  1 sibling, 0 replies; 43+ messages in thread
From: Pavel Machek @ 2021-03-27 22:19 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List,
	linux-leds, Dan Murphy

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

Hi!

> > > > > -     err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> > > > > +     of_property_read_u32(node, "color", &color);
> > > > > +     seg->led.name = devm_kasprintf(dev, GFP_KERNEL,
> > > > > +                     DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT,
> > > > > +                     color < LED_COLOR_ID_MAX ? led_colors[color] : "");
> >
> > And would prefer not to see driver_name as part of LED name.
> 
> OK. So what should I use instead? Or just leave the part before the
> first colon empty?

I'd suggest auxdisplay:...:backlight.

And we should start documenting this somewhere.

Best regards,
									Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays
  2021-03-22 14:48 ` [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays Geert Uytterhoeven
@ 2021-03-29  7:08   ` Robin van der Gracht
  2021-03-29  7:15     ` Geert Uytterhoeven
  0 siblings, 1 reply; 43+ messages in thread
From: Robin van der Gracht @ 2021-03-29  7:08 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	devicetree, linux-mips, linux-kernel

Hello Geert,

On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> The Holtek HT16K33 LED controller is not only used for driving
> dot-matrix displays, but also for driving segment displays.
> 
> Add support for 4-digit 7-segment and quad 14-segment alphanumeric
> displays, like the Adafruit 7-segment and 14-segment display backpack
> and FeatherWing expansion boards.  Use the character line display core
> support to display a message, which will be scrolled if it doesn't fit.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> The 7-segment support is based on schematics, and untested on actual
> hardware.
> ---
>  drivers/auxdisplay/ht16k33.c | 198 +++++++++++++++++++++++++++++++++--
>  1 file changed, 191 insertions(+), 7 deletions(-)
> 
...
> 
> +static int ht16k33_seg_probe(struct i2c_client *client,
> +			     struct ht16k33_priv *priv, uint32_t brightness)
> +{
> +	struct ht16k33_seg *seg = &priv->seg;
> +	struct device *dev = &client->dev;
> +	int err;
> +
> +	err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> +	if (err)
> +		return err;
> +
> +	switch (priv->type) {
> +	case DISP_MATRIX:
> +		/* not handled here */
> +		break;

This 'case' shouldn't happen. Having said that, the break here will 
still
cause the linedisp_register() function to be called for the DISP_MATRIX 
type.
If you'd like to handle this case, a return (or setting 'err') should
prevent this.

> +
> +	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_update);
> +	if (err)
> +		goto err_remove_map_file;
> +
> +	return 0;

Groetjes/Kind regards,
Robin van der Gracht

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays
  2021-03-29  7:08   ` Robin van der Gracht
@ 2021-03-29  7:15     ` Geert Uytterhoeven
  2021-03-29  7:30       ` Robin van der Gracht
  0 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-03-29  7:15 UTC (permalink / raw)
  To: Robin van der Gracht
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List

Hoi Robin,

On Mon, Mar 29, 2021 at 9:09 AM Robin van der Gracht <robin@protonic.nl> wrote:
> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> > The Holtek HT16K33 LED controller is not only used for driving
> > dot-matrix displays, but also for driving segment displays.
> >
> > Add support for 4-digit 7-segment and quad 14-segment alphanumeric
> > displays, like the Adafruit 7-segment and 14-segment display backpack
> > and FeatherWing expansion boards.  Use the character line display core
> > support to display a message, which will be scrolled if it doesn't fit.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > The 7-segment support is based on schematics, and untested on actual
> > hardware.
> > ---
> >  drivers/auxdisplay/ht16k33.c | 198 +++++++++++++++++++++++++++++++++--
> >  1 file changed, 191 insertions(+), 7 deletions(-)
> >
> ...
> >
> > +static int ht16k33_seg_probe(struct i2c_client *client,
> > +                          struct ht16k33_priv *priv, uint32_t brightness)
> > +{
> > +     struct ht16k33_seg *seg = &priv->seg;
> > +     struct device *dev = &client->dev;
> > +     int err;
> > +
> > +     err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
> > +     if (err)
> > +             return err;
> > +
> > +     switch (priv->type) {
> > +     case DISP_MATRIX:
> > +             /* not handled here */
> > +             break;
>
> This 'case' shouldn't happen. Having said that, the break here will
> still
> cause the linedisp_register() function to be called for the DISP_MATRIX
> type.
> If you'd like to handle this case, a return (or setting 'err') should
> prevent this.

This function is never called if priv->type == DISP_MATRIX, so this
cannot happen.  However, gcc complains if not all enum values are
handled in a switch() statement, hence the dummy case.

Is there a better way to handle this?

> > +     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_update);
> > +     if (err)
> > +             goto err_remove_map_file;
> > +
> > +     return 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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays
  2021-03-29  7:15     ` Geert Uytterhoeven
@ 2021-03-29  7:30       ` Robin van der Gracht
  0 siblings, 0 replies; 43+ messages in thread
From: Robin van der Gracht @ 2021-03-29  7:30 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List

On 2021-03-29 09:15, Geert Uytterhoeven wrote:
> Hoi Robin,
> 
> On Mon, Mar 29, 2021 at 9:09 AM Robin van der Gracht 
> <robin@protonic.nl> wrote:
>> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
>> > The Holtek HT16K33 LED controller is not only used for driving
>> > dot-matrix displays, but also for driving segment displays.
>> >
>> > Add support for 4-digit 7-segment and quad 14-segment alphanumeric
>> > displays, like the Adafruit 7-segment and 14-segment display backpack
>> > and FeatherWing expansion boards.  Use the character line display core
>> > support to display a message, which will be scrolled if it doesn't fit.
>> >
>> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> > ---
>> > The 7-segment support is based on schematics, and untested on actual
>> > hardware.
>> > ---
>> >  drivers/auxdisplay/ht16k33.c | 198 +++++++++++++++++++++++++++++++++--
>> >  1 file changed, 191 insertions(+), 7 deletions(-)
>> >
>> ...
>> >
>> > +static int ht16k33_seg_probe(struct i2c_client *client,
>> > +                          struct ht16k33_priv *priv, uint32_t brightness)
>> > +{
>> > +     struct ht16k33_seg *seg = &priv->seg;
>> > +     struct device *dev = &client->dev;
>> > +     int err;
>> > +
>> > +     err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS);
>> > +     if (err)
>> > +             return err;
>> > +
>> > +     switch (priv->type) {
>> > +     case DISP_MATRIX:
>> > +             /* not handled here */
>> > +             break;
>> 
>> This 'case' shouldn't happen. Having said that, the break here will
>> still
>> cause the linedisp_register() function to be called for the 
>> DISP_MATRIX
>> type.
>> If you'd like to handle this case, a return (or setting 'err') should
>> prevent this.
> 
> This function is never called if priv->type == DISP_MATRIX, so this
> cannot happen.  However, gcc complains if not all enum values are
> handled in a switch() statement, hence the dummy case.

I see. But if it's there it should work right? :P

> Is there a better way to handle this?

How about adding the default case:

default:
     /* not handled */
     err = -EINVAL;

Groetjes/Kind regards,
Robin van der Gracht

-- 
Protonic Holland
Factorij 36
1689AL Zwaag
+31 (0)229 212928
https://www.protonic.nl

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays
  2021-03-23  9:12   ` robin
@ 2021-05-18 14:35     ` Geert Uytterhoeven
  2021-05-19 10:37       ` Robin van der Gracht
  0 siblings, 1 reply; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-05-18 14:35 UTC (permalink / raw)
  To: Robin van der Gracht
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List

Hoi Robin,

On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote:
> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> > The Holtek HT16K33 LED controller is not only used for driving
> > dot-matrix displays, but also for driving segment displays.
> >
> > Document compatible values for the Adafruit 7-segment[1] and
> > 14-segment[2] FeatherWing expansion boards with red displays.
> > According
> > to the schematics, all other Adafruit 7-segment and 14-segment display
> > backpack and FeatherWing expansion boards (including bare boards and
> > boards fitted with displays) are compatible with these two boards.
> > Add a "color" property to support the different color variants.
> >
> > [1] https://www.adafruit.com/product/3108
> > [2] https://www.adafruit.com/product/3130
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> > @@ -14,14 +14,23 @@ allOf:
> >
> >  properties:
> >    compatible:
> > -    const: holtek,ht16k33
> > +    oneOf:
> > +      - items:
> > +          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment
> > FeatherWing Display (Red)
> > +          - const: holtek,ht16k33
> > +
> > +      - items:
> > +          - const: adafruit,3130  # 0.54" Quad Alphanumeric
> > FeatherWing Display (Red)
> > +          - const: holtek,ht16k33
> > +
> > +      - const: holtek,ht16k33     # Generic 16*8 LED controller with
> > dot-matrix display
> >
> >    reg:
> >      maxItems: 1
> >
> >    refresh-rate-hz:
> >      maxItems: 1
> > -    description: Display update interval in Hertz
> > +    description: Display update interval in Hertz for dot-matrix
> > displays
>
> The above should be included in patch 16

I disagree: bindings are independent from the driver implementation.

> >    interrupts:
> >      maxItems: 1
> > @@ -41,10 +50,17 @@ properties:
> >      default: 16
> >      description: Initial brightness level
> >
> > +  color: true
> > +    description:
> > +      Color of the display.  Use one of the LED_COLOR_ID_* prefixed
> > definitions
> > +      from the header include/dt-bindings/leds/common.h.  The default
> > is red.
> > +    minimum: 0
> > +    maximum: 9
> > +    default: 1
> > +
>
> The above should be included in patch 17

Same here.

> >  required:
> >    - compatible
> >    - reg
> > -  - refresh-rate-hz
>
> 'refresh-rate-hz' is still a required property for the dot-matrix /
> fbdev setup.

True.

> If it can no longer be listed here than maybe its nice to mention that
> it's required
> somewhere else (in it's description?).

    if:
      properties:
        compatible:
          const: holtek,ht16k33
    then:
      required:
        - refresh-rate-hz

Thanks!

Gr{oetje,eeting}s,

                        Geert

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

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays
  2021-05-18 14:35     ` Geert Uytterhoeven
@ 2021-05-19 10:37       ` Robin van der Gracht
  2021-05-19 11:02         ` Geert Uytterhoeven
  0 siblings, 1 reply; 43+ messages in thread
From: Robin van der Gracht @ 2021-05-19 10:37 UTC (permalink / raw)
  To: Rob Herring
  Cc: Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List,
	Geert Uytterhoeven

Hi Rob,

On 2021-05-18 16:35, Geert Uytterhoeven wrote:
> Hoi Robin,
> 
> On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote:
>> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
>> > The Holtek HT16K33 LED controller is not only used for driving
>> > dot-matrix displays, but also for driving segment displays.
>> >
>> > Document compatible values for the Adafruit 7-segment[1] and
>> > 14-segment[2] FeatherWing expansion boards with red displays.
>> > According
>> > to the schematics, all other Adafruit 7-segment and 14-segment display
>> > backpack and FeatherWing expansion boards (including bare boards and
>> > boards fitted with displays) are compatible with these two boards.
>> > Add a "color" property to support the different color variants.
>> >
>> > [1] https://www.adafruit.com/product/3108
>> > [2] https://www.adafruit.com/product/3130
>> >
>> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
>> > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
>> > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
>> > @@ -14,14 +14,23 @@ allOf:
>> >
>> >  properties:
>> >    compatible:
>> > -    const: holtek,ht16k33
>> > +    oneOf:
>> > +      - items:
>> > +          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment
>> > FeatherWing Display (Red)
>> > +          - const: holtek,ht16k33
>> > +
>> > +      - items:
>> > +          - const: adafruit,3130  # 0.54" Quad Alphanumeric
>> > FeatherWing Display (Red)
>> > +          - const: holtek,ht16k33
>> > +
>> > +      - const: holtek,ht16k33     # Generic 16*8 LED controller with
>> > dot-matrix display
>> >
>> >    reg:
>> >      maxItems: 1
>> >
>> >    refresh-rate-hz:
>> >      maxItems: 1
>> > -    description: Display update interval in Hertz
>> > +    description: Display update interval in Hertz for dot-matrix
>> > displays
>> 
>> The above should be included in patch 16
> 
> I disagree: bindings are independent from the driver implementation.
> 
>> >    interrupts:
>> >      maxItems: 1
>> > @@ -41,10 +50,17 @@ properties:
>> >      default: 16
>> >      description: Initial brightness level
>> >
>> > +  color: true
>> > +    description:
>> > +      Color of the display.  Use one of the LED_COLOR_ID_* prefixed
>> > definitions
>> > +      from the header include/dt-bindings/leds/common.h.  The default
>> > is red.
>> > +    minimum: 0
>> > +    maximum: 9
>> > +    default: 1
>> > +
>> 
>> The above should be included in patch 17
> 
> Same here.

My thought was that one without the other makes no sense. But if it's common
practice to create a separate patch for device tree bindings (it's a 
patch-set
after all) than that's fine with me.

@Rob what do you think?

Best regards,
Met vriendelijke groet,

Robin van der Gracht

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays
  2021-05-19 10:37       ` Robin van der Gracht
@ 2021-05-19 11:02         ` Geert Uytterhoeven
  0 siblings, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-05-19 11:02 UTC (permalink / raw)
  To: Robin van der Gracht
  Cc: Rob Herring, Miguel Ojeda, Paul Burton, Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List

Hoi Robin,

On Wed, May 19, 2021 at 12:37 PM Robin van der Gracht <robin@protonic.nl> wrote:
> On 2021-05-18 16:35, Geert Uytterhoeven wrote:
> > On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote:
> >> On 2021-03-22 15:48, Geert Uytterhoeven wrote:
> >> > The Holtek HT16K33 LED controller is not only used for driving
> >> > dot-matrix displays, but also for driving segment displays.
> >> >
> >> > Document compatible values for the Adafruit 7-segment[1] and
> >> > 14-segment[2] FeatherWing expansion boards with red displays.
> >> > According
> >> > to the schematics, all other Adafruit 7-segment and 14-segment display
> >> > backpack and FeatherWing expansion boards (including bare boards and
> >> > boards fitted with displays) are compatible with these two boards.
> >> > Add a "color" property to support the different color variants.
> >> >
> >> > [1] https://www.adafruit.com/product/3108
> >> > [2] https://www.adafruit.com/product/3130
> >> >
> >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> >> > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> >> > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
> >> > @@ -14,14 +14,23 @@ allOf:
> >> >
> >> >  properties:
> >> >    compatible:
> >> > -    const: holtek,ht16k33
> >> > +    oneOf:
> >> > +      - items:
> >> > +          - const: adafruit,3108  # 0.56" 4-Digit 7-Segment
> >> > FeatherWing Display (Red)
> >> > +          - const: holtek,ht16k33
> >> > +
> >> > +      - items:
> >> > +          - const: adafruit,3130  # 0.54" Quad Alphanumeric
> >> > FeatherWing Display (Red)
> >> > +          - const: holtek,ht16k33
> >> > +
> >> > +      - const: holtek,ht16k33     # Generic 16*8 LED controller with
> >> > dot-matrix display
> >> >
> >> >    reg:
> >> >      maxItems: 1
> >> >
> >> >    refresh-rate-hz:
> >> >      maxItems: 1
> >> > -    description: Display update interval in Hertz
> >> > +    description: Display update interval in Hertz for dot-matrix
> >> > displays
> >>
> >> The above should be included in patch 16
> >
> > I disagree: bindings are independent from the driver implementation.
> >
> >> >    interrupts:
> >> >      maxItems: 1
> >> > @@ -41,10 +50,17 @@ properties:
> >> >      default: 16
> >> >      description: Initial brightness level
> >> >
> >> > +  color: true
> >> > +    description:
> >> > +      Color of the display.  Use one of the LED_COLOR_ID_* prefixed
> >> > definitions
> >> > +      from the header include/dt-bindings/leds/common.h.  The default
> >> > is red.
> >> > +    minimum: 0
> >> > +    maximum: 9
> >> > +    default: 1
> >> > +
> >>
> >> The above should be included in patch 17
> >
> > Same here.
>
> My thought was that one without the other makes no sense. But if it's common
> practice to create a separate patch for device tree bindings (it's a
> patch-set
> after all) than that's fine with me.

scripts/checkpatch.pl even has a check for that:

    WARN("DT_SPLIT_BINDING_PATCH",
         "DT binding docs and includes should be a separate patch.
See: Documentation/devicetree/bindings/submitting-patches.rst\n");

See also rule #5 in the doc referred above:

  5) The Documentation/ portion of the patch should come in the series before
     the code implementing the binding.

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

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support
  2021-03-24  8:31         ` Geert Uytterhoeven
  2021-03-27 22:19           ` Pavel Machek
@ 2021-06-25 12:39           ` Geert Uytterhoeven
  1 sibling, 0 replies; 43+ messages in thread
From: Geert Uytterhoeven @ 2021-06-25 12:39 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Robin van der Gracht, Rob Herring, Miguel Ojeda, Paul Burton,
	Greg Kroah-Hartman,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:BROADCOM NVRAM DRIVER, Linux Kernel Mailing List,
	linux-leds

On Wed, Mar 24, 2021 at 9:31 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Mar 23, 2021 at 9:40 PM Pavel Machek <pavel@ucw.cz> wrote:
> > > CC linux-leds (which I intended, but forgot to add)
> > >
> > > cover letter at
> > > https://lore.kernel.org/linux-devicetree/20210322144848.1065067-1-geert@linux-m68k.org/

> > > > > +     err = ht16k33_brightness_set(priv, seg->led.brightness);
> > > > >       if (err)
> > > > >               return err;
> > > >
> > > > The LED class can pretty much do what the backlight class can and more.
> > > >
> > > > Maybe we can stop registering a backlight device in the fbdev case and
> > > > register a led device for both. This makes the code cleaner and drops
> > > > a dependency but will break backwards compatibility.
> > > >
> > > > I'd prefer a single solution that covers both use cases, but I'm not
> > > > sure about the 'breaking backwards compatibility' consequence...
> >
> > For new drivers, breaking compatibility should not be a problem.
>
> The dot-matrix support is part of the existing driver, thus subject to
> backwards compatibility.
> Perhaps we can register the LED device for both, and build the backlight
> device on top of the LED device, like "led-backlight" does.  Would that
> work? Or can't the LED no longer be controlled from sysfs (e.g.
> triggers) if it is in use by a backlight driver?

Using "led-backlight", the backlight can no longer be controlled from
sysfs, precluding the use of other triggers incl. hardware blinking.
But a normal LED can be used as a backlight, with ledtrig-backlight,
so that is the most flexible option, but only if no backwards
compatibility is to be considered.


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

^ permalink raw reply	[flat|nested] 43+ messages in thread

end of thread, other threads:[~2021-06-25 12:39 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-22 14:48 [PATCH 00/17] auxdisplay: ht16k33: Add character display support Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 01/17] uapi: Add <linux/map_to_14segment.h> Geert Uytterhoeven
2021-03-22 17:05   ` Miguel Ojeda
2021-03-22 17:51     ` Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 02/17] dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays Geert Uytterhoeven
2021-03-22 17:38   ` Rob Herring
2021-03-23  9:12   ` robin
2021-05-18 14:35     ` Geert Uytterhoeven
2021-05-19 10:37       ` Robin van der Gracht
2021-05-19 11:02         ` Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 03/17] auxdisplay: img-ascii-lcd: Fix lock-up when displaying empty string Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 04/17] auxdisplay: img-ascii-lcd: Add helper variable dev Geert Uytterhoeven
2021-03-22 16:59   ` Miguel Ojeda
2021-03-22 17:53     ` Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 05/17] auxdisplay: img-ascii-lcd: Convert device attribute to sysfs_emit() Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 06/17] auxdisplay: Extract character line display core support Geert Uytterhoeven
2021-03-23  8:18   ` robin
2021-03-23  8:52     ` Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 07/17] auxdisplay: linedisp: Use kmemdup_nul() helper Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 08/17] auxdisplay: linedisp: Add support for changing scroll rate Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 09/17] auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize() Geert Uytterhoeven
2021-03-23  7:59   ` robin
2021-03-22 14:48 ` [PATCH 10/17] auxdisplay: ht16k33: Remove unneeded error check in keypad probe() Geert Uytterhoeven
2021-03-23  8:08   ` robin
2021-03-22 14:48 ` [PATCH 11/17] auxdisplay: ht16k33: Convert to simple i2c probe function Geert Uytterhoeven
2021-03-23  8:31   ` robin
2021-03-22 14:48 ` [PATCH 12/17] auxdisplay: ht16k33: Add helper variable dev Geert Uytterhoeven
2021-03-23  8:57   ` robin
2021-03-22 14:48 ` [PATCH 13/17] auxdisplay: ht16k33: Move delayed work Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 14/17] auxdisplay: ht16k33: Extract ht16k33_brightness_set() Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 15/17] auxdisplay: ht16k33: Extract frame buffer probing Geert Uytterhoeven
2021-03-22 14:48 ` [PATCH 16/17] auxdisplay: ht16k33: Add support for segment displays Geert Uytterhoeven
2021-03-29  7:08   ` Robin van der Gracht
2021-03-29  7:15     ` Geert Uytterhoeven
2021-03-29  7:30       ` Robin van der Gracht
2021-03-22 14:48 ` [PATCH 17/17] auxdisplay: ht16k33: Add segment display LED support Geert Uytterhoeven
2021-03-23 10:08   ` Robin van der Gracht
2021-03-23 12:32     ` Geert Uytterhoeven
2021-03-23 20:40       ` Pavel Machek
2021-03-24  8:31         ` Geert Uytterhoeven
2021-03-27 22:19           ` Pavel Machek
2021-06-25 12:39           ` Geert Uytterhoeven
2021-03-22 17:03 ` [PATCH 00/17] auxdisplay: ht16k33: Add character display support Miguel Ojeda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).