Simplify the use of SPI displays now that fbtft_device is gone

[notro]

I have known for some time that many find it difficult to use SPI displays that don't have an existing DT overlay.
Before Linux 5.4 it was possible to use fbtft_device to support nearly every SPI DBI display, but this is now gone.

When the issue came to my attention again recently I decided to see if I can do something about it.

I see 3 ways to solve this:

1. Resurrect fbtft_device and make it work again.
   Pros: All those existing blog posts will be relevant again.
   Cons: It can't go into mainline so have to stay downstream. And it's not my favorite.

2. Make a DT overlay that behaves like fbtft_device. I see that the overlays have come a long way since I looked at it in detail a few years back so I don't know its capabilities. Some properties like reset-gpios, led-gpios and init need to be optional and can only be enabled when the argument is used, the other properties can have a default value. The compatible property/argument picks the wanted driver and thus its default initialization sequence. The init property can override this.
   Pros: No need to add any code, just an overlay.

The downside of these 2 is that fbdev is deprecated so it's not an optimal solution going forward.

3. I have a generic MIPI DBI SPI DRM driver that will work with all DBI compatible SPI displays:
   drivers/gpu/drm/tiny/mipi_dbi_drv.c (wiki)
   Pros: It's a very small and simple DRM driver which makes it easy to maintain. All the difficult stuffs happens in DRM libraries. Fully configurable from DT.
   Cons: One more deviation from mainline that needs to be carried "forever" since this driver can't go into mainline (not allowed to set arbitrary registers from Device Tree).

Thoughts?

Cc: @6by9

[6by9]

Can we make a driver more akin to panel-simple or panel-ilitek-ili9881 where each compatible string looks up a configuration that can then be used with the display?

OK it means that each new panel means a code (and dtbinding!) change rather than overlay, but in theory it should be acceptable to mainline. You have the basis of it already with your existing tiny/mipi_dbi_drv driver, but need the configuration structs to be added rather than putting them in an overlay.

[notro]

Indeed that's possible having the driver support many displays, drivers/gpu/drm/tiny/st7735r.c supports 2 displays.

Zephyr has properties for some of the commands which seems like a good idea. This avoids panel specifics in the driver.
Example: https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/display/sitronix%2Cst7735r.yaml
(one challenge with this approach is that the fbtft driver already uses the controller DT compatible)

Unfortunately getting stuff into mainline is more work than I can spend, so that's not something I can take on.

[pelwell]

I'm not against supporting another downstream driver if it will be widely used (as I think an fbtft replacement might be). I also like the sound of option 2, but I don't see how that could work without a driver that gets settings from DT - is there such a thing?

And welcome back.

[6by9]

Unfortunately getting stuff into mainline is more work than I can spend, so that's not something I can take on.

Understood. I fully recognise the pain - I also have a load of patches I really ought to submit to mainline.
If we can work within a scheme that ought to be acceptable to mainline (although you never can quite predict it), then it means that others can take it on.

I'm not sure that Zephyr's bindings would be acceptable. They differ significantly to mainline, and seems to be specifying configuration instead of describing the hardware, and that's normally the big no-no.
Switching on the compatible, or parameters that genuinely describe the hardware seem to be the only things that are acceptable.

[notro]

I'm not sure that Zephyr's bindings would be acceptable. They differ significantly to mainline, and seems to be specifying configuration instead of describing the hardware, and that's normally the big no-no. Switching on the compatible, or parameters that genuinely describe the hardware seem to be the only things that are acceptable.

Your comment jogged my memory and I remembered seeing something similar in mainline: bindings/display/solomon%2Cssd1307fb.yaml
So I think it should be fine, but as you say one can never be sure, so adding something like this would have to go through mainline.

[notro]

I also like the sound of option 2, but I don't see how that could work without a driver that gets settings from DT - is there such a thing?

All fbtft drivers support the same set of properties (except bgr), I was thinking something like this: https://gist.github.com/notro/0c1cead85dfb406d5d9b2d8cef07f542

Using that overlay, these two configs will create identical SPI devices (ignoring touchcontroller):

dtoverlay=rpi-display
dtoverlay=fbtft,ili9341,bgr,reset_pin=23,dc_pin=24,led_pin=18,rotate=270

The one thing I didn't find out how to do, was to create a 32-bit array from a parameter:

		//init        = <&display>,"init"; // 32-bit array

Example overlay using the init property: mz61581-overlay.dts

And welcome back.

Thanks Phil

[notro]

I discovered that fbtft gpio backlight is broken: staging/fbtft: Fix backlight
Strange that no one has complained, at least I haven't heard anything. It's been like that since 5.0.

[pelwell]

There are a couple of ways in which you could build up an integer array from a parameter - patching in each integer at a time (properties can be extended with offsets beyond the existing length), or as a byte array - but they are neither elegant nor efficient. My advice would be to put each variant of the init property in its own fragment targeting the display node, but to mark them all as disabled (__dormant__). The parameters that select each display type by setting the compatible string can then also select the appropriate init by enabling the relevant fragment using the overlay/fragment parameter mechanism, indicated with a zero phandle and a string of operations on numbered fragments.

For example:

    ...

    fragment@2 {
        target = <&display>;
        __dormant__ {
            init = <0x1234 0x5678 ...>;
        };
    };

    fragment@3 {
        target = <&display>;
        __dormant__ {
            init = <0x4321 0x8765 ...>;
        };
    };

    ...

    __overrides__ {
        typea = <&display>, "compatible=fbtft,typea",
            <0>, "+2";
        typeb = <&display>, "compatible=fbtft,typeb",
            <0>, "+3";
    };

Fragments 2 and 3 are intra-overlay fragments because they target another part of the overlay. The firmware is careful to apply intra- fragments before those the others targetting the base DTB (inter-, I suppose), but that is the limit of its intelligence. For example, if fragment 1 targets the base DTB, 2 targets 1 and 3 targets 2, 2 will correctly be applied to 1 before it is merged, but 3 will be applied too and late and will be lost.

In general it's safest to write the fragments in the order in which they should be applied, but having a base fragment at the start of the file that is then populated by the others is also fine.

[notro]

What am I doing wrong here, I have this in the overlay:

		init          = <&display>,"init[";

config.txt:

dtoverlay=fbtft,ili9341,bgr,reset_pin=23,dc_pin=24,led_pin=18,rotate=270,init=[01 00 01 00]

log:

007688.106: dtparam: init=[01 00 01 00]
007688.517: dterror: invalid bytestring '[01 00 01 00]'

[pelwell]

Try without the brackets and spaces - I can check the exact rules later.

[pelwell]

Ah - whitespace and colons are legal separators between pairs of hex digits, but separators are optional.

[notro]

Luckily for my use case the property values are stored as big-endian, so it's not so bad to use a byte string.

But it looks like there's something wrong with the parser?

init=01000003
007718.204: dtparam: init=01000003

init=01000003 01000005
007682.346: dtparam: init=01000003 0100

init=0100000301000005
007684.426: dtparam: init=0100000301000
007684.840: dterror: invalid bytestring '0100000301000'

init=01 00 00 03 01 00 00 05
007684.964: dtparam: init=01 00 00 03 0
007685.378: dterror: invalid bytestring '01 00 00 03 0'

init=01:00:00:03:01:00:00:05
007684.103: dtparam: init=01:00:00:03:0
007684.517: dterror: invalid bytestring '01:00:00:03:0'

init=01:00:00:03 01:00:00:05
007685.455: dtparam: init=01:00:00:03 0
007685.869: dterror: invalid bytestring '01:00:00:03 0'

Hm, it looks like the string is capped at len=13.

[notro]

Oh, it's the total length of the statement that is too long:

dtoverlay=fbtft,ili9341,bgr,reset_pin=23,dc_pin=24,led_pin=18,rotate=270,init=01000003 01000005
007715.214: dtparam: init=01000003 01000005

dtoverlay=fbtft,ili9341,bgr,reset_pin=23,dc_pin=24,led_pin=18,rotate=270,debug=3,init=01000003 01000005
007684.064: dtparam: init=01000003 0100

[notro]

This it what I would like to do, would it be possible to increase the max length to 1024?

dtoverlay=fbtft,ili9341,bgr,reset_pin=23,dc_pin=24,led_pin=18,rotate=270,debug=3
dtparam=init=01000028 010000cf 00000000 00000083 00000030 010000ed 00000064 00000003 00000012 00000081 010000e8 00000085 00000001 00000079 010000cb 00000039 0000002c 00000000 00000034 00000002 010000f7 00000020 010000ea 00000000 00000000 010000c0 00000026 010000c1 00000011 010000c5 00000035 0000003e 010000c7 000000be 0100003a 00000055 01000036 00000028 010000b1 00000000 0000001b 01000026 00000001 010000f2 00000008 01000026 00000001 010000e0 0000001f 0000001a 00000018 0000000a 0000000f 00000006 00000045 00000087 00000032 0000000a 00000007 00000002 00000007 00000005 00000000 010000e1 00000000 00000025 00000027 00000005 00000010 00000009 0000003a 00000078 0000004d 00000005 00000018 0000000d 00000038 0000003a 0000001f 010000b7 00000007 010000b6 0000000a 00000082 00000027 00000000 01000011 02000064 01000029 02000064

If this would be possible a DT overlay can perform all that fbtft_device could do (except handle a panel with an offset on the address).

[pelwell]

There's a 78-character line length limit (the buffer holds 80 characters, including newline and NUL), which has been raised to 98 with the very latest firmwares. Increasing the maximum length would also affect all the other readers of config.txt, including bootcode.bin which is memory-limited because it runs before SDRAM has been enabled, so I'm reluctant to do that.

The byte string feature was added with MAC addresses in mind, and it fell out nicely that single byte/integer parameters had an offset after the separator, and the other types (booleans, strings, byte strings) didn't. That prevents us from having multiple init properties that start at different offsets - init0, init1, etc.

One option which would allow long byte strings like this is to say that assigning to the same byte string property repeatedly (or consecutively) causes the new content to be appended. You would then (with the increased line length limit) be able to split the assignment into groups of 8 words:

dtparam=init=01000028 010000cf 00000000 00000083 00000030 010000ed 00000064 00000003
dtparam=init=00000012 00000081 010000e8 00000085 00000001 00000079 010000cb 00000039
dtparam=init=0000002c 00000000 00000034 00000002 010000f7 00000020 010000ea 00000000
dtparam=init=00000000 010000c0 00000026 010000c1 00000011 010000c5 00000035 0000003e
dtparam=init=010000c7 000000be 0100003a 00000055 01000036 00000028 010000b1 00000000
dtparam=init=0000001b 01000026 00000001 010000f2 00000008 01000026 00000001 010000e0
dtparam=init=0000001f 0000001a 00000018 0000000a 0000000f 00000006 00000045 00000087
dtparam=init=00000032 0000000a 00000007 00000002 00000007 00000005 00000000 010000e1
dtparam=init=00000000 00000025 00000027 00000005 00000010 00000009 0000003a 00000078
dtparam=init=0000004d 00000005 00000018 0000000d 00000038 0000003a 0000001f 010000b7
dtparam=init=00000007 010000b6 0000000a 00000082 00000027 00000000 01000011 02000064
dtparam=init=01000029 02000064

[pelwell]

I would like to go back and explore my earlier comment:

My advice would be to put each variant of the init property in its own fragment targeting the display node

Given that you are already proposing to build knowledge of each of the displays into the overlay (e.g. ili9341), why not put the init settings in there as well? That way, if a mistake is found or if the driver changes in some way then we can update everyone more easily.

[6by9]

I've pinged Maxime as maintainer of solomon-ssd1307fb.yaml to get his view on config in DT vs code.

[notro]

There's a 78-character line length limit (the buffer holds 80 characters, including newline and NUL), which has been raised to 98 with the very latest firmwares. Increasing the maximum length would also affect all the other readers of config.txt, including bootcode.bin which is memory-limited because it runs before SDRAM has been enabled, so I'm reluctant to do that.

Ok, so it's not trivial to change.

Even without the init property an fbtft overlay will be a big improvement on the current situation. And it doesn't take much effort to do.
I can expand on the fbtft wiki and add an example on how to make and an overlay with an init property.

I would like to go back and explore my earlier comment:

My advice would be to put each variant of the init property in its own fragment targeting the display node

Given that you are already proposing to build knowledge of each of the displays into the overlay (e.g. ili9341), why not put the init settings in there as well? That way, if a mistake is found or if the driver changes in some way then we can update everyone more easily.

I can do that for the displays that are supported in fbtft_device.

An init property would have made it easy to support any display if the inititialization sequence is known, without needing to understand Device Tree files. I have just installed Home Assistant and are faced with cryptic yaml files, so I can relate to how it feels for someone that just wants to use a display they bought (without doing research to see if it's supported).

Not having an init property excludes option 3 as well (generic DRM driver), but it's not a big loss. It's best avoided to carry around things that can't go into mainline. It could be possible to change the format of the init property to be bytes instead of u32's which would shorten the line considerly, but making mainline drivers more generic is preferrable. Let's see what Maxime has to say about using properties in that way.

[pelwell]

I am prepared to add byte string concatenation as an option of last resort (as long as it is unlikely to happen accidentally), but as you say Maxime may have other ideas.

[6by9]

Comment from Maxime:

It was merged very early in the DT conversions era, and we didn't have as much experience (and a general feeling of good practices) back then. It probably wouldn't be merged today.
So I'm not sure it's something that should be seen as some example or some kind of prior-art argument.
That doesn't mean that "it's never going to be merged" though, more like "if you think it's going to be easy because of the ssd1307 binding, it's not"

I'll let @mripard expand on that if required.

[notro]

I think I'll make a PR adding the fbtft overlay and add a step-by-step guide on the fbtft wiki on how to create an overlay with an init property. That will cover most of the fbtft_device use cases.

Long term the first step would be to make a patch and add controller properties to drivers/gpu/drm/tiny/ili9341.c (most common controller) and see what the DT maintainer thinks of that.

Thanks for helping me explore the options.

[notro]

Unfortunately getting stuff into mainline is more work than I can spend, so that's not something I can take on.

Understood. I fully recognise the pain - I also have a load of patches I really ought to submit to mainline.

I have commit rights in DRM so I can apply patches, but I can only apply my own patches after someone else has reviewed them. And since I haven't partnered up with someone else cross reviewing patches, I can't be certain that someone will review them or how long it will take for me to get them in.

I'm not sure that Zephyr's bindings would be acceptable. They differ significantly to mainline, and seems to be specifying configuration instead of describing the hardware, and that's normally the big no-no. Switching on the compatible, or parameters that genuinely describe the hardware seem to be the only things that are acceptable.

@6by9 would you be interested in reviewing the driver patches if I take a stab at this and the DT maintainer accepts the binding?

[6by9]

I claim no expertise with these SPI displays, but can give things a review, and test where I have hardware (I've got about 6 of these displays kicking around).

@mripard does have a contract with Raspberry Pi for vc4 DRM work, and I have no issues with him reviewing your patches under that contract if he's willing to do so.

[notro]

Thanks, I'll give it try then. I'll do the st7735r since doing the ili9341 would require me to convert the binding to yaml as well.