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

[Krzysztof Kozlowski]

On Thu, 24 Dec 2020 at 11:08, Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> 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

It's not the problem of including more than one DTSI. It's the problem
of creating fake DTS or DTSI files whose purpose is only to include
others. Keep it simple. Don't create unnecessary files. "Entities
should not be multiplied without necessity."

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

Which was wrong as well. Don't create unnecessary files.

>
> /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

The files represent real devices or their components. So you have a
SOM - a DTSI file. You have a carrier board - a DTS file. That's
simple design which is mostly followed, unless something over
complicated passes the review.

> 1. More verbose for which IP's are available in the carrier board

No difference when carrier DTSI is the DTS. Exactly the same.

> 2. Easy to extend if someone can create another SoM with a similar Carrier.

Not really, if they include carrier DTSI they need to override a lot.
So usually (including practice - I did it) they *copy* the carrier to
create their own design.

>
> 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.

NAK from my side. I explained my reasoning. You created a fake, empty
DTSI which included only other DTSI. After review, you agreed to fix
it. However you still create a fake DTS which includes only a DTSI.

"Entities should not be multiplied without necessity."

Best regards,
Krzysztof