Re: [PATCH v2 6/6] arm64: dts: imx8mm: Add Engicam i.Core MX8M Mini EDIMM2.2 Starter Kit

[Krzysztof Kozlowski]

On Tue, Dec 22, 2020 at 01:03:07AM +0530, Jagan Teki wrote:
> On Mon, Dec 21, 2020 at 7:36 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> >
> > On Mon, Dec 21, 2020 at 05:01:51PM +0530, Jagan Teki wrote:
> > > Engicam EDIMM2.2 Starter Kit is an EDIMM 2.2 Form Factor Capacitive
> > > Evaluation Board.
> > >
> > > Genaral features:
> > > - LCD 7" C.Touch
> > > - microSD slot
> > > - Ethernet 1Gb
> > > - Wifi/BT
> > > - 2x LVDS Full HD interfaces
> > > - 3x USB 2.0
> > > - 1x USB 3.0
> > > - HDMI Out
> > > - Mini PCIe
> > > - MIPI CSI
> > > - 2x CAN
> > > - Audio Out
> > >
> > > 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 Evaluation board for
> > > creating complete i.Core MX8M Mini EDIMM2.2 Starter Kit.
> > >
> > > PCIe, DSI, CSI nodes will add it into imx8mm-engicam-edimm2.2.dtsi once
> > > Mainline Linux supported.
> > >
> > > Add support for it.
> > >
> > > Signed-off-by: Matteo Lisi <matteo.lisi@xxxxxxxxxxx>
> > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > Changes for v2:
> > > - updated commit message
> > > - dropped engicam from filename since it aligned with imx6 engicam
> > >   dts files naming conventions.
> > >
> > >  arch/arm64/boot/dts/freescale/Makefile        |  1 +
> > >  .../freescale/imx8mm-engicam-edimm2.2.dtsi    |  7 +++++++
> > >  .../freescale/imx8mm-icore-mx8mm-edimm2.2.dts | 21 +++++++++++++++++++
> > >  3 files changed, 29 insertions(+)
> > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-engicam-edimm2.2.dtsi
> > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-icore-mx8mm-edimm2.2.dts
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> > > index 8d49a2c74604..43783076f856 100644
> > > --- a/arch/arm64/boot/dts/freescale/Makefile
> > > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > > @@ -33,6 +33,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-beacon-kit.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mm-ddr4-evk.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mm-icore-mx8mm-ctouch2.dtb
> > > +dtb-$(CONFIG_ARCH_MXC) += imx8mm-icore-mx8mm-edimm2.2.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mm-kontron-n801x-s.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mm-var-som-symphony.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mn-evk.dtb
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-engicam-edimm2.2.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-engicam-edimm2.2.dtsi
> > > new file mode 100644
> > > index 000000000000..294df07289a2
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-engicam-edimm2.2.dtsi
> > > @@ -0,0 +1,7 @@
> > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +/*
> > > + * Copyright (c) 2020 Engicam srl
> > > + * Copyright (c) 2020 Amarula Solutions(India)
> > > + */
> > > +
> > > +#include "imx8mm-engicam-common.dtsi"
> >
> > It seems you ignored my comments from previous email. That's not how we
> > go with the process.
> >
> > Don't create confusing or overcomplicated hierarchy of includes. Don't
> > create files which do nothing.
> 
> Idea is to move common nodes in separate dtsi instead of adding
> redundant nodes into respective areas. let me know if it still
> confusing.

A file which *only* includes another file does not fulfill this idea of
moving common nodes to a separate DTSI file. Or if I still miss
something, please point me, what common nodes are stored in
imx8mm-engicam-edimm2.2.dtsi?

Best regards,
Krzysztof