Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller

On 16.03.2022 01:38, Robin Murphy wrote:

On 2022-02-25 21:09, Heiner Kallweit wrote:

quoted

This series adds support for the Titanmec TM1628 7 segment display
controller. It's based on previous RFC work from Andreas Färber.
The RFC version placed the driver in the LED subsystem, but this was
NAK'ed by the LED maintainer. Therefore I moved the driver to
/drivers/auxdisplay what seems most reasonable to me.

Further changes to the RFC version:
- Driver can be built also w/o LED class support, for displays that
   don't have any symbols to be exposed as LED's.
- Simplified the code and rewrote a lot of it.
- Driver is now kind of a MVP, but functionality should be sufficient
   for most use cases.
- Use the existing 7 segment support in uapi/linux/map_to_7segment.h
   as suggested by Geert Uytterhoeven.

Note: There's a number of chips from other manufacturers that are
       almost identical, e.g. FD628, SM1628. Only difference I saw so
       far is that they partially support other display modes.
       TM1628: 6x12, 7x11
       SM1628C: 4x13, 5x12, 6x11, 7x10
       For typical displays on devices using these chips this
       difference shouldn't matter.

Successfully tested on a TX3 Mini TV box that has an SM1628C and a
display with 4 digits and 7 symbols.

FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so:

    titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>;
    titanmec,grid = /bits/ 8 <5 4 2 1>;

(grid 3 segment 2 is used for a colon in the middle)

If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used?

Definitely this could be one future extension. It could also consider that there's a number of more or less
identical chips from other vendors that differ primarily in the supported display modes.

I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...)

With regards to inputs at least I have no plans because I have no hw supporting input.
Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result.
Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version
may result in longer discussions until maintainer or I give up.
Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break
existing users. And I think that's the case.

Cheers,
Robin.

Heiner

quoted

v2:
- (re-)add Andreas' SoB to two patches
- fix YAML issues
- include ctype.h explicitly
- add info message in probe()

v3:
- remove patch 1 because it has been applied via the SPI tree already
- fix remaining YAML issues in patch 2
- follow Miguel's suggestion on usage of Co-Developed-by

v4:
- add patch for MAINTAINERS entry
- incorporate Miguel's review comments
- Replace Co-Developed-by with Co-developed-by (checkpatch)
v5:
- add vendor prefix to driver-specific dt properties

Andreas Färber (1):
   dt-bindings: vendor-prefixes: Add Titan Micro Electronics

Heiner Kallweit (5):
   dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
   docs: ABI: document tm1628 attribute display-text
   auxdisplay: add support for Titanmec TM1628 7 segment display
     controller
   arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment
     display
   MAINTAINERS: Add entry for tm1628 auxdisplay driver

  .../testing/sysfs-devices-auxdisplay-tm1628   |   7 +
  .../bindings/auxdisplay/titanmec,tm1628.yaml  |  92 +++++
  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
  MAINTAINERS                                   |   7 +
  .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts  |  59 +++
  drivers/auxdisplay/Kconfig                    |  11 +
  drivers/auxdisplay/Makefile                   |   1 +
  drivers/auxdisplay/tm1628.c                   | 376 ++++++++++++++++++
  8 files changed, 555 insertions(+)
  create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
  create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
  create mode 100644 drivers/auxdisplay/tm1628.c