Re: [RFC PATCH v2 3/3] ARM: dto: Add bcm2711-rpi-7-inches-ts.dts overlay

[Rob Herring]

On Thu, Apr 28, 2022 at 08:44:17AM +0200, Geert Uytterhoeven wrote:
> Hi Rob,
> 
> On Wed, Apr 27, 2022 at 11:23 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> > On Wed, Apr 27, 2022 at 02:52:43PM -0400, Detlev Casanova wrote:
> > > Add a device tree overlay to support the official Raspberrypi 7" touchscreen for
> > > the bcm2711 devices.
> > >
> > > The panel is connected on the DSI 1 port and uses the simple-panel
> > > driver.
> > >
> > > The device tree also makes sure to activate the pixelvalve[0-4] CRTC modules
> > >
> > > Signed-off-by: Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx>
> > > ---
> > >  arch/arm/boot/dts/Makefile                    |   4 +
> > >  arch/arm/boot/dts/overlays/Makefile           |   3 +
> > >  .../dts/overlays/bcm2711-rpi-7-inches-ts.dts  | 125 ++++++++++++++++++
> >
> > .dtso is preferred. I think... It was discussed, but I never got an
> > updated patch to switch.
> 
> Unfortunately that switch indeed hasn't happened yet.
> My main gripe with .dts for overlays is that you cannot know whether
> it's an overlay or not without reading the file's contents.
> Hence tools like make also cannot know, and you need to e.g. list
> all files explicitly in a Makefile.

See my reply in the other thread for that.

> > >  arch/arm64/boot/dts/broadcom/Makefile         |   4 +
> > >  .../arm64/boot/dts/broadcom/overlays/Makefile |   3 +
> > >  .../overlays/bcm2711-rpi-7-inches-ts.dts      |   2 +
> > >  6 files changed, 141 insertions(+)
> > >  create mode 100644 arch/arm/boot/dts/overlays/Makefile
> > >  create mode 100644 arch/arm/boot/dts/overlays/bcm2711-rpi-7-inches-ts.dts
> >
> > A global (to arm) 'overlays' directory will create the same mess that we
> > have in arch/arm/boot/dts/. IMO, first you should move all the Broadcom
> > dts files to a 'broadcom' subdirectory like we have for arm64.
> 
> As I believe this display is not only used with real Raspberry Pi
> devices, it makes sense to not have it a broadcom directory.

Then at a minimum 'bcm2711' in the name is not appropriate.

I'm doubtful that as-is the overlay would apply to boards outside of 
RPi's. For this to work (well), there needs to be a connector node to 
translate between connector resources and the base board resources. See 
the recent mikrobus thread[2].

> In fact it may be used on other architectures than arm, too, so I
> think we need an arch-agnostic directory for overlays[1]?

Probably so.

Personally, I would prefer no DTs under /arch.

> This may need remapping of labels. I'm aware the rpi infrastructure has
> support for remapping labels when applying overlays during boot, but
> AFAIK this is not yet supported by fdtoverlay (or perhaps by a fork?)?
> Note that the remapping is also needed if you want to apply two
> instances of the same overlay.

First I've heard of label remapping... I have a lot of concerns about 
using labels for overlays. For starters, with a flip of a switch (-@), 
they all become an ABI when they were not previously. I think at a 
minimum, we need an annotation so that a subset can be exported. 
Anything that's an ABI, we should be documenting and reviewing.

The requirement for overlays upstream is that they are applied at build 
time to a base DT. Otherwise, we can't validate them completely. So if 
there's a label remapping dependency on these, sounds like there is some 
more work to do. The first being getting agreement that label remapping 
is the right approach.

Common label names or some remapping for targets kind of works, but 
easily falls apart. For example, GPIO (or any provider with identifier 
cells) numbering or SPI CS numbering would be different. 

Rob

[2] https://lore.kernel.org/all/YmFo+EntwxIsco%2Ft@xxxxxxxxxxxxxxxxxx/