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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 21.02.2022 07:32, Christian Hewitt wrote:
> resend from correct mail account:
> 
>> On 19 Feb 2022, at 5:13 pm, Heiner Kallweit <hkallweit1@gmail.com> 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.
>>
>> To be decided is through which tree this series should go.
>> I'd think SPI would be most suited, but that's a decision I
>> leave up to the respective maintainers.
>>
>> 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.
> 
> Thanks for dusting off sources and working on this! - it’s another piece
> of the upstream puzzle for distros that install on Android boxes.
> 
> I needed the following patch to address compile issues (missing include,
> and the recent void/int change in linux-next (I’m using 5.17.y):
> 
> diff --git a/drivers/auxdisplay/tm1628.c b/drivers/auxdisplay/tm1628.c
> index a39b638282c1..ab3557f8b330 100644
> --- a/drivers/auxdisplay/tm1628.c
> +++ b/drivers/auxdisplay/tm1628.c
> @@ -5,6 +5,7 @@
> * Copyright (c) 2019 Andreas Färber
> */
> 
> +#include <linux/ctype.h>
> #include <linux/delay.h>
> #include <linux/leds.h>
> #include <linux/module.h>
> @@ -327,10 +328,11 @@ static int tm1628_spi_probe(struct spi_device *spi)
>       return device_create_file(&spi->dev, &dev_attr_display_text);
> }
> 
> -static void tm1628_spi_remove(struct spi_device *spi)
> +static int tm1628_spi_remove(struct spi_device *spi)
> {
>       device_remove_file(&spi->dev, &dev_attr_display_text);
>       tm1628_set_display_ctrl(spi, false);
> +       return 0;
> }
> 
> static void tm1628_spi_shutdown(struct spi_device *spi)
> 
> I also needed CONFIG_SPI_GPIO=y in kernel config. With this added the
> driver probes on my TX3 mini box and the display goes dark overwriting
> the default ‘boot’ text. The following systemd service and script sets
> the clock and flashes the colon separator on/off to count seconds:
> 
> https://github.com/chewitt/LibreELEC.tv/commit/c8f1ebe6f6c366188f18f9d2b401de6c2979fdd7
> 
> With the include fixup and maybe a Kconfig tweak, for the series:
> 
> Tested-by: Christian Hewitt <christianshewitt@gmail.com>

Thanks for testing! 
On some systems the display controller may be connected to a HW SPI
interface not using GPIO's. Therefore I'd prefer to not make
the driver dependent on CONFIG_SPI_GPIO.