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

[Robin Murphy <robin.murphy@arm.com>]

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.

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel