Re: [PATCH v3 4/6] arm64: dts: imx8mm: Add Engicam i.Core MX8M Mini C.TOUCH 2.0

[Jagan Teki]

On Thu, Dec 24, 2020 at 2:48 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On Wed, 23 Dec 2020 at 13:07, Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Dec 23, 2020 at 5:29 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Dec 23, 2020 at 04:33:41PM +0530, Jagan Teki wrote:
> > > > Engicam C.TOUCH 2.0 is an EDIMM compliant general purpose Carrier
> > > > board.
> > > >
> > > > Genaral features:
> > > > - Ethernet 10/100
> > > > - Wifi/BT
> > > > - USB Type A/OTG
> > > > - Audio Out
> > > > - CAN
> > > > - LVDS panel connector
> > > >
> > > > i.Core MX8M Mini is an EDIMM SoM based on NXP i.MX8M Mini from Engicam.
> > > >
> > > > i.Core MX8M Mini needs to mount on top of this Carrier board for
> > > > creating complete i.Core MX8M Mini C.TOUCH 2.0 board.
> > > >
> > > > Add support for it.
> > > >
> > > > Signed-off-by: Matteo Lisi <matteo.lisi@xxxxxxxxxxx>
> > > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > Changes for v3:
> > > > - don't maintain common nodes and include it, if no feature diff
> > > > Changes for v2:
> > > > - enabled fec1 node
> > > > - updated commit message
> > > > - dropped engicam from filename since it aligned with imx6 engicam
> > > >   dts files naming conventions.
> > > > - add i2c nodes
> > > > - fixed v1 comments
> > > >
> > > >  arch/arm64/boot/dts/freescale/Makefile        |  1 +
> > > >  .../dts/freescale/imx8mm-engicam-ctouch2.dtsi | 82 +++++++++++++++++++
> > > >  .../freescale/imx8mm-icore-mx8mm-ctouch2.dts  | 21 +++++
> > > >  3 files changed, 104 insertions(+)
> > > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi
> > >
> > > You split some common part to ctouch2.dtsi so it can be reused in
> > > multiple places. I saw so far only one usage, where are the others?
> >
> > To be clear, ctouch2.dtsi not mean for common it is C.TOUCH2 carrier
> > board dtsi. The other carrier is C.TOUCH2 10.1" Open Frame(display),
> > since DSI is not yet mainlined, I didn't add this yet.
>
> If I understand correctly: it is a DTSI which is included only by one
> DTS... and DTS does not have any other nodes. This as well is not the

This is not mandatory as per my understanding, including exiting DTS
topologies in Mainline.

There are several places where more than one dtsi has been included,
Simple example of imx8mm tree is

arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dts

/dts-v1/;

#include "imx8mm.dtsi"
#include "imx8mm-beacon-som.dtsi"
#include "imx8mm-beacon-baseboard.dtsi"

(SoC dtsi, SoM dtsi, Carrier board dtsi)

> design which makes any sense. We do not create empty DTS files which
> only include one more DTSI. The contents of
> imx8mm-engicam-ctouch2.dtsi should be directly in
> imx8mm-icore-mx8mm-ctouch2.dts. That's the same problem as with v1 -
> you overcomplicate simple stuff. It really looks like you ignored the
> comments from v1 in multiple places.

As explained above, the design is pretty much the same as the existing SoM's.

imx8mm-engicam-ctouch2.dtsi is not just a dtsi file where nodes are
enabled. It has nodes enabled for Carrier board, so keeping nodes
separately will

1. More verbose for which IP's are available in the carrier board
2. Easy to extend if someone can create another SoM with a similar Carrier.

Ie is the whole idea to keep carrier board dtsi and includes them in dts.

As I suggest, if you can look into px30 you can understand more easily.

Jagan.