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

[Robin Murphy]

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 at 0 {
>>          compatible = "titanmec,tm1628";
>>          ...
>>
>>          leds {
>>              #address-cells = <2>;
>>              #size-cells = <0>;
>>
>>              alarm at 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