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

[Heiner Kallweit <hkallweit1@xxxxxxxxx>]

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.


> 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