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

[Jagan Teki <jagan@amarulasolutions.com>]

On Thu, Dec 24, 2020 at 3:51 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Thu, 24 Dec 2020 at 11:08, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Thu, Dec 24, 2020 at 2:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On Wed, 23 Dec 2020 at 13:07, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > On Wed, Dec 23, 2020 at 5:29 PM Krzysztof Kozlowski <krzk@kernel.org> 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@engicam.com>
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > ---
> > > > > > 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.

But what if the new board has slite change to use exiting carrier like
what ctouch2 10" OF. Can we add ctouch2 dtsi as a separate file for
this case?

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

Not sure. I have updated the series according to comments by dropping
-common.dtsi ie what I was thought of  "empty DTSI inclusion" you have
pointed at previous versions.

Jagan.