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

[Krzysztof Kozlowski <krzk@kernel.org>]

On Tue, Dec 22, 2020 at 02:20:55PM +0530, Jagan Teki wrote:
> On Tue, Dec 22, 2020 at 2:36 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > 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@kernel.org> 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@engicam.com>
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > > > 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?
> 
> imx8mm-engicam-edimm2.2.dtsi for EDIMM2.2 Carrier
> imx8mm-engicam-ctouch2.dtsi for C.TOUCH2 Carrier
> imx8mm-engicam-common.dtsi for common nodes for above 2 carrier boards.
> 
> Yes, imx8mm-engicam-edimm2.2.dtsi is empty now

Then that's the answer. We do not create empty files.

> but nodes like PCIe,
> CSI, DSI will support once the respective drivers are part of Mainline
> but those are not supported in C.TOUCH2 carrier board dtsi. There are
> some GPIO pins differences between EDIMM2.2 and C.TOUCH2 carriers on
> WiFi/BT so those will be part of the respective carrier dtsi.

It's the same clear as before. Don't create empty files. Once you decide
to bring new features, you create a new file.

Best regards,
Krzysztof