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

From: "Andreas Färber" <afaerber@suse.de>
To: Heiner Kallweit <hkallweit1@gmail.com>,
	Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
	Miguel Ojeda <ojeda@kernel.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	"linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"open list:ARM/Amlogic Meson..."
	<linux-amlogic@lists.infradead.org>
Subject: Re: [PATCH v2 5/6] auxdisplay: add support for Titanmec TM1628 7 segment display controller
Date: Tue, 22 Feb 2022 13:16:49 +0100	[thread overview]
Message-ID: <f0eeb2f4-b47d-f0d8-2169-388899d1cfdf@suse.de> (raw)
In-Reply-To: <288225b9-85e0-0c46-5d8b-c91f76c796e5@gmail.com>

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@gmail.com> 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@gmail.com>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>
>> 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

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Ivo Totev
HRB 36809 (AG Nürnberg)