mbox series

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

Message ID 4172e59f-b9d5-d87d-9dbd-a6f683a2173c@gmail.com
Headers show
Series auxdisplay: Add support for the Titanmec TM1628 7 segment display controller | expand

Message

Heiner Kallweit Feb. 19, 2022, 1:13 p.m. UTC
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.

Andreas Färber (2):
  spi: gpio: Implement LSB First bitbang support
  dt-bindings: vendor-prefixes: Add Titan Micro Electronics

Heiner Kallweit (4):
  dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
  docs: ABI: document tm1628 attribute display-text
  auxdisplay: add support for Titanmec TM1628 7 segment display
    controller
  arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment
    display

 .../testing/sysfs-devices-auxdisplay-tm1628   |   7 +
 .../bindings/auxdisplay/titanmec,tm1628.yaml  |  82 ++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts  |  59 +++
 drivers/auxdisplay/Kconfig                    |  10 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/tm1628.c                   | 368 ++++++++++++++++++
 drivers/spi/spi-bitbang-txrx.h                |  66 ++++
 drivers/spi/spi-gpio.c                        |  42 +-
 9 files changed, 628 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
 create mode 100644 drivers/auxdisplay/tm1628.c

Comments

Miguel Ojeda Feb. 19, 2022, 1:27 p.m. UTC | #1
On Sat, Feb 19, 2022 at 2: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.

Could you please link to the discussion and/or summarize the rationale
behind the NAK?

Cheers,
Miguel
Heiner Kallweit Feb. 19, 2022, 1:37 p.m. UTC | #2
On 19.02.2022 14:27, Miguel Ojeda wrote:
> On Sat, Feb 19, 2022 at 2: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.
> 
> Could you please link to the discussion and/or summarize the rationale
> behind the NAK?
> 

+Pavel

I didn't find an explicit reason, but I suppose Pavel sees this driver as
one that makes use of the LED subsystem, but doesn't belong to it.
In the following mail he's expressing his opinion that the driver should
be best placed under auxdisplay.

https://lore.kernel.org/linux-arm-kernel/20200226130300.GB2800@duo.ucw.cz/

> Cheers,
> Miguel

Heiner
Miguel Ojeda Feb. 19, 2022, 2:11 p.m. UTC | #3
On Sat, Feb 19, 2022 at 2:37 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> I didn't find an explicit reason, but I suppose Pavel sees this driver as
> one that makes use of the LED subsystem, but doesn't belong to it.
> In the following mail he's expressing his opinion that the driver should
> be best placed under auxdisplay.

Ah, OK -- thanks!

Cheers,
Miguel
Andreas Färber Feb. 19, 2022, 4:07 p.m. UTC | #4
Hi,

On 19.02.22 14:37, Heiner Kallweit wrote:
> On 19.02.2022 14:27, Miguel Ojeda wrote:
>> On Sat, Feb 19, 2022 at 2: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.
>>
>> Could you please link to the discussion and/or summarize the rationale
>> behind the NAK?
>>
> 
> +Pavel
> 
> I didn't find an explicit reason, but I suppose Pavel sees this driver as
> one that makes use of the LED subsystem, but doesn't belong to it.
> In the following mail he's expressing his opinion that the driver should
> be best placed under auxdisplay.
> 
> https://lore.kernel.org/linux-arm-kernel/20200226130300.GB2800@duo.ucw.cz/

And I disagreed. It does not fit with the other drivers in auxdisplay
that were operating on a much higher level.

I'd also like to point out that I did implement the map_to_7segment API,
as was suggested, as you will find in my tree - which you may have
missed, referencing only the RFC patchset and putting your authorship on
it exclusively? A move from one directory to another should not warrant
my author and SoB getting removed from the actual driver.

Given that we need to manage a buffer with bits per segment or LED
symbol, one idea that I haven't found time for yet was to implement it
as framebuffer or drm device instead. (And most Realtek platforms got
broken by removing the adjustable text base defines.)

Regards,
Andreas
Heiner Kallweit Feb. 19, 2022, 5:16 p.m. UTC | #5
On 19.02.2022 17:07, Andreas Färber wrote:
> Hi,
> 
> On 19.02.22 14:37, Heiner Kallweit wrote:
>> On 19.02.2022 14:27, Miguel Ojeda wrote:
>>> On Sat, Feb 19, 2022 at 2: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.
>>>
>>> Could you please link to the discussion and/or summarize the rationale
>>> behind the NAK?
>>>
>>
>> +Pavel
>>
>> I didn't find an explicit reason, but I suppose Pavel sees this driver as
>> one that makes use of the LED subsystem, but doesn't belong to it.
>> In the following mail he's expressing his opinion that the driver should
>> be best placed under auxdisplay.
>>
>> https://lore.kernel.org/linux-arm-kernel/20200226130300.GB2800@duo.ucw.cz/
> 
> And I disagreed. It does not fit with the other drivers in auxdisplay
> that were operating on a much higher level.
> 

We need to find a place. And if Pavel has good reasons that it doesn't
fit into the LED subsystem, and Miguel should be fine with having
it in auxdisplay, then I'd see no reason to not go this way.

> I'd also like to point out that I did implement the map_to_7segment API,

Looking at the history of include/uapi/linux/map_to_7segment.h I see no
commit from you. Seems I'm missing something here.

> as was suggested, as you will find in my tree - which you may have
> missed, referencing only the RFC patchset and putting your authorship on
> it exclusively? A move from one directory to another should not warrant
> my author and SoB getting removed from the actual driver.
> 
The driver includes major changes and I mentioned your work in the commit
message. Also your still listed as MODULE_AUTHOR. My intention is to
get a driver upstream, not to earn credits for something.
So sure, your SoB can be (re-)added.

> Given that we need to manage a buffer with bits per segment or LED
> symbol, one idea that I haven't found time for yet was to implement it
> as framebuffer or drm device instead. (And most Realtek platforms got
> broken by removing the adjustable text base defines.)
> 
I'm not aware of the Realtek platform issue, do you have a link to a
related discussion? And wouldn't you think it's overengineering to
write a DRM driver for a 7 segment display with 4 digits?
Framebuffer seems to be deprecated based on my experience with
pygame / SDL2.

> Regards,
> Andreas
> 

Heiner
Christian Hewitt Feb. 21, 2022, 6:31 a.m. UTC | #6
> 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>
Christian Hewitt Feb. 21, 2022, 6:32 a.m. UTC | #7
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>
Heiner Kallweit Feb. 21, 2022, 7:57 p.m. UTC | #8
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.
Andreas Färber Feb. 22, 2022, 12:12 p.m. UTC | #9
On 19.02.22 18:16, Heiner Kallweit wrote:
> On 19.02.2022 17:07, Andreas Färber wrote:
>> Hi,
>>
>> On 19.02.22 14:37, Heiner Kallweit wrote:
>>> On 19.02.2022 14:27, Miguel Ojeda wrote:
>>>> On Sat, Feb 19, 2022 at 2: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.
>>>>
>>>> Could you please link to the discussion and/or summarize the rationale
>>>> behind the NAK?
>>>>
>>>
>>> +Pavel
>>>
>>> I didn't find an explicit reason, but I suppose Pavel sees this driver as
>>> one that makes use of the LED subsystem, but doesn't belong to it.
>>> In the following mail he's expressing his opinion that the driver should
>>> be best placed under auxdisplay.
>>>
>>> https://lore.kernel.org/linux-arm-kernel/20200226130300.GB2800@duo.ucw.cz/
>>
>> And I disagreed. It does not fit with the other drivers in auxdisplay
>> that were operating on a much higher level.
>>
> 
> We need to find a place. And if Pavel has good reasons that it doesn't
> fit into the LED subsystem, and Miguel should be fine with having
> it in auxdisplay, then I'd see no reason to not go this way.
> 
>> I'd also like to point out that I did implement the map_to_7segment API,
> 
> Looking at the history of include/uapi/linux/map_to_7segment.h I see no
> commit from you. Seems I'm missing something here.

You're replying inline too early:

>> as was suggested, as you will find in my tree

As I said, I implemented it in my driver:

https://github.com/afaerber/linux/commit/bbecf951348c7de8ba922c6c002a09369b717d82

Thus me saying you are unnecessarily duplicating work that I already
did, without ping'ing the thread or me and claiming the credit for an
implementation change which I already did myself.

>> - which you may have
>> missed, referencing only the RFC patchset and putting your authorship on
>> it exclusively? A move from one directory to another should not warrant
>> my author and SoB getting removed from the actual driver.
>>
> The driver includes major changes and I mentioned your work in the commit
> message. Also your still listed as MODULE_AUTHOR. My intention is to
> get a driver upstream, not to earn credits for something.
> So sure, your SoB can be (re-)added.

https://github.com/afaerber/linux/commits/rtd1295-next

Also note this 5-in-4 optimization:

https://github.com/afaerber/linux/commit/ff8284b6ed9dc1e354c35840afdaf50b1cd97fea

And several more chipsets being covered.

>> Given that we need to manage a buffer with bits per segment or LED
>> symbol, one idea that I haven't found time for yet was to implement it
>> as framebuffer or drm device instead. (And most Realtek platforms got
>> broken by removing the adjustable text base defines.)
>>
> I'm not aware of the Realtek platform issue, do you have a link to a
> related discussion?

Realtek has a boot ROM at the beginning of memory space, which has been
a problem from the first RFC and for most bootloaders required to tweak
the kernel's text offset for successful boot. (Some not Open Source (LK)
and/or not openly flashable.)

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/487718.html

In 2020 that arm64 feature got removed without any further discussion:

https://lore.kernel.org/all/20200825135440.11288-1-ardb@kernel.org/

I've tried to revert it, but that's been a pain:

https://github.com/afaerber/linux/commit/0d2c647781bc89ee95bfa7b80d71237c7ebea230

> And wouldn't you think it's overengineering to
> write a DRM driver for a 7 segment display with 4 digits?
> Framebuffer seems to be deprecated based on my experience with
> pygame / SDL2.

Is there any other API that would allow userspace to write to the buffer
and bitblt parts to the SPI device?

Thinking of some optimizations I implemented in my driver to avoid
unnecessary SPI transfers:

https://github.com/afaerber/linux/commit/46c40209db163a81474c6894ebbd90b5e238ce60

Regards,
Andreas
Neil Armstrong Feb. 22, 2022, 2:48 p.m. UTC | #10
On 22/02/2022 13:12, Andreas Färber wrote:
> On 19.02.22 18:16, Heiner Kallweit wrote:
>> On 19.02.2022 17:07, Andreas Färber wrote:
>>> Hi,
>>>
>>> On 19.02.22 14:37, Heiner Kallweit wrote:
>>>> On 19.02.2022 14:27, Miguel Ojeda wrote:
>>>>> On Sat, Feb 19, 2022 at 2: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.
>>>>>
>>>>> Could you please link to the discussion and/or summarize the rationale
>>>>> behind the NAK?
>>>>>
>>>>
>>>> +Pavel
>>>>
>>>> I didn't find an explicit reason, but I suppose Pavel sees this driver as
>>>> one that makes use of the LED subsystem, but doesn't belong to it.
>>>> In the following mail he's expressing his opinion that the driver should
>>>> be best placed under auxdisplay.
>>>>
>>>> https://lore.kernel.org/linux-arm-kernel/20200226130300.GB2800@duo.ucw.cz/
>>>
>>> And I disagreed. It does not fit with the other drivers in auxdisplay
>>> that were operating on a much higher level.
>>>
>>
>> We need to find a place. And if Pavel has good reasons that it doesn't
>> fit into the LED subsystem, and Miguel should be fine with having
>> it in auxdisplay, then I'd see no reason to not go this way.
>>
>>> I'd also like to point out that I did implement the map_to_7segment API,
>>
>> Looking at the history of include/uapi/linux/map_to_7segment.h I see no
>> commit from you. Seems I'm missing something here.
> 
> You're replying inline too early:
> 
>>> as was suggested, as you will find in my tree
> 
> As I said, I implemented it in my driver:
> 
> https://github.com/afaerber/linux/commit/bbecf951348c7de8ba922c6c002a09369b717d82
> 
> Thus me saying you are unnecessarily duplicating work that I already
> did, without ping'ing the thread or me and claiming the credit for an
> implementation change which I already did myself.
> 
>>> - which you may have
>>> missed, referencing only the RFC patchset and putting your authorship on
>>> it exclusively? A move from one directory to another should not warrant
>>> my author and SoB getting removed from the actual driver.
>>>
>> The driver includes major changes and I mentioned your work in the commit
>> message. Also your still listed as MODULE_AUTHOR. My intention is to
>> get a driver upstream, not to earn credits for something.
>> So sure, your SoB can be (re-)added.
> 
> https://github.com/afaerber/linux/commits/rtd1295-next
> 
> Also note this 5-in-4 optimization:
> 
> https://github.com/afaerber/linux/commit/ff8284b6ed9dc1e354c35840afdaf50b1cd97fea
> 
> And several more chipsets being covered.
> 
>>> Given that we need to manage a buffer with bits per segment or LED
>>> symbol, one idea that I haven't found time for yet was to implement it
>>> as framebuffer or drm device instead. (And most Realtek platforms got
>>> broken by removing the adjustable text base defines.)
>>>
>> I'm not aware of the Realtek platform issue, do you have a link to a
>> related discussion?
> 
> Realtek has a boot ROM at the beginning of memory space, which has been
> a problem from the first RFC and for most bootloaders required to tweak
> the kernel's text offset for successful boot. (Some not Open Source (LK)
> and/or not openly flashable.)
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/487718.html
> 
> In 2020 that arm64 feature got removed without any further discussion:
> 
> https://lore.kernel.org/all/20200825135440.11288-1-ardb@kernel.org/

Note the TEXT_OFFSET is only an issue with Amlogic vendor bootloader,
it has never been an issue with mainline U-Boot.

Neil

> 
> I've tried to revert it, but that's been a pain:
> 
> https://github.com/afaerber/linux/commit/0d2c647781bc89ee95bfa7b80d71237c7ebea230
> 
>> And wouldn't you think it's overengineering to
>> write a DRM driver for a 7 segment display with 4 digits?
>> Framebuffer seems to be deprecated based on my experience with
>> pygame / SDL2.
> 
> Is there any other API that would allow userspace to write to the buffer
> and bitblt parts to the SPI device?
> 
> Thinking of some optimizations I implemented in my driver to avoid
> unnecessary SPI transfers:
> 
> https://github.com/afaerber/linux/commit/46c40209db163a81474c6894ebbd90b5e238ce60
> 
> Regards,
> Andreas
>
Andreas Färber Feb. 22, 2022, 3:31 p.m. UTC | #11
On 22.02.22 15:48, Neil Armstrong wrote:
> On 22/02/2022 13:12, Andreas Färber wrote:
>> On 19.02.22 18:16, Heiner Kallweit wrote:
>>> On 19.02.2022 17:07, Andreas Färber wrote:
>>>> [...] (And most Realtek platforms got
>>>> broken by removing the adjustable text base defines.)
>>>>
>>> I'm not aware of the Realtek platform issue, do you have a link to a
>>> related discussion?
>>
>> Realtek has a boot ROM at the beginning of memory space, which has been
>> a problem from the first RFC and for most bootloaders required to tweak
>> the kernel's text offset for successful boot. (Some not Open Source (LK)
>> and/or not openly flashable.)
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/487718.html
>>
>>
>> In 2020 that arm64 feature got removed without any further discussion:
>>
>> https://lore.kernel.org/all/20200825135440.11288-1-ardb@kernel.org/
> 
> Note the TEXT_OFFSET is only an issue with Amlogic vendor bootloader,
> it has never been an issue with mainline U-Boot.

There is no mainline U-Boot for Realtek DHC (!= Amlogic Meson) though!

More important drivers than LED got blocked here, too, like MMC and USB
and pinctrl and clk.

And as hinted above, some Realtek boards come with a vendor LK that I
can't even patch a downstream version of.

Regards,
Andreas