Re: [PATCH v2 5/6] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Heiner Kallweit <hkallweit1@xxxxxxxxx>]

On 22.02.2022 13:16, Andreas Färber wrote:
> On 22.02.22 10:44, Heiner Kallweit wrote:
>> On 22.02.2022 09:19, Geert Uytterhoeven wrote:
>>> Hi Heiner,
>>>
>>> On Mon, Feb 21, 2022 at 9:26 PM Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>>>> This patch 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.
>>>>
>>>> Tested-by: Christian Hewitt <christianshewitt@xxxxxxxxx>
>>>> Signed-off-by: Andreas Färber <afaerber@xxxxxxx>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>
>>> Thanks for your patch!
>>>
>>>> --- /dev/null
>>>> +++ b/drivers/auxdisplay/tm1628.c
>>>
>>>> +static int tm1628_show_text(struct tm1628 *s)
>>>> +{
>>>> +       static SEG7_CONVERSION_MAP(map_seg7, MAP_ASCII7SEG_ALPHANUM);
>>>
>>> This mapping can not be overridden by the user.  Is there any
>>> specific reason you didn't make the mapping configurable from sysfs,
>>> cfr. map_seg7_{show,store}() in include/uapi/linux/map_to_7segment.h?
>>>
>>
>> The more features an initial driver version includes, the more discussion
>> topics pop up and make it less likely that we end up with at least something.
>> I think there's a reason why the driver was resting since the initial
>> attempt 2 yrs ago. Therefore I'd like to keep it as a MVP. 
>> If somebody should have the need for add-on features, then they can be
>> added later.
> 
> As I pointed out in your v1, I did implement all that already, as was
> requested by Geert on my RFC:
> 
> https://github.com/afaerber/linux/commit/bbecf951348c7de8ba922c6c002a09369b717d82
> 
I don't want to prove how many API's I master, but give users what they need.
And that's simply:
- control symbols from kernel or user space -> LED class sysfs attributes / triggers
- write text (mainly numbers) to display -> sysfs attribute
And this with a minimum of code to facilitate maintenance.
As confirmed by Christian the proposed version gives him what he needs.

If there should be actually a demand for features like dimming control, then it
can be added later (e.g. you could add it based on the code you wrote already).

> Regards,
> Andreas
> 
Heiner