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

[Robin Murphy <robin.murphy@xxxxxxx>]

On 2022-03-17 21:49, Heiner Kallweit wrote:
> On 17.03.2022 21:08, Robin Murphy wrote:
> > On 2022-03-16 21:19, Heiner Kallweit wrote:
> > > On 16.03.2022 01:38, Robin Murphy wrote:
> > > > On 2022-02-25 21:09, Heiner Kallweit wrote:
> > > > > 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.
> >
> > FWIW, if you've got a board with exposed GPIO/SPI headers, searching "TM1638" on ebay/aliexpress/etc. should find the cheapo breakout boards. I believe they're quite popular with the Arduino crowd, so I expect that may well carry over to the Raspberry Pi crowd once they get wind of a kernel driver that can be driven by DT overlays.
> >
> > > 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.
> >
> > Unfortunately the principle is that DT bindings describe the device, not whatever the current level of Linux driver support for it might be. Perhaps I'm a little sensitised since I'm currently feeling the pain of extending a decade-old binding with functionality that was overlooked at the time, and not breaking compatibility is now rather awkward.
> >
> > I'm not suggesting that there needs to be any support implemented in the driver, just to be certain that we're not painting ourselves into a corner with the binding.
> >
> > > 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.
> >
> > May I ask what you have in mind? I figure that inputs would most likely want to be described individually, similarly to the gpio-keys binding, which would lend itself to having them as child nodes, except that doesn't fit with the current scheme of child nodes having to be LEDs addressed by (grid,segment). I suppose there is a possible escape hatch of abusing unused addresses, e.g. saying a node at address (0,n) is input n rather than an LED segment, but that seems pretty horrid (and I'm not sure how well schema could validate it). Or possibly pretending to also be a GPIO controller to reference from a separate gpio-keys node, but again that seems ugly and more like something to only do if there's no other option.
>
> Not being an expert in OF stuff I'm just focused on getting support for the hw I own.
> I tried to do this in the most simple and generic way so that others can follow-up
> and add additional functionality.

Sure, I appreciate that, and what I'm saying is that while what we 
currently have is pleasantly simple, I think it's actually a little 
*too* simple and not generic enough to extend easily. I'm more than 
happy to send patches adding the functionality I'm interested in to the 
driver once it's merged, but I can't make significant changes to the 
binding at that point and break it for early adopters. But let me go 
make proper review comments on the patch rather than confusing 
meta-review here...

Thanks,
Robin.

> > IMO it would be cleanest just to have an extra level of hierarchy, e.g.:
> >
> >
> >      led-controller@0 {
> >          compatible = "titanmec,tm1628";
> >          ...
> >
> >          leds {
> >              #address-cells = <2>;
> >              #size-cells = <0>;
> >
> >              alarm@5,4 {
> >                  ...
> >              };
> >          };
> >      };
> >
> > That way there's clearly almost no risk of breakage if an additional "inputs" node with its own children turns up later. Plus it should also be a trivial change to the current driver, compared to having to implement trick special cases or whole other APIs down the line - of course bindings should not be designed expressly for ease of driver implementation, but if they do work out that way it's usually a good sign :)
> >
> > Thanks,
> > Robin.
>
> Heiner