diff mbox series

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

Message ID 20201223110343.126638-5-jagan@amarulasolutions.com
State New
Headers
Series arm64: dts: imx8mm: Add Engicam i.Core MX8M Mini |

Commit Message

Jagan Teki Dec. 23, 2020, 11:03 a.m. UTC 
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
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-icore-mx8mm-ctouch2.dts

Comments

Krzysztof Kozlowski Dec. 23, 2020, 11:59 a.m. UTC | #1 
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?

Best regards,
Krzysztof
Jagan Teki Dec. 23, 2020, 12:06 p.m. UTC | #2 
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.

Jagan.
Krzysztof Kozlowski Dec. 24, 2020, 9:18 a.m. UTC | #3 
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
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.

The same applies to imx8mm-engicam-edimm2.2.dtsi.

Best regards,
Krzysztof
Jagan Teki Dec. 24, 2020, 10:07 a.m. UTC | #4 
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

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.
Krzysztof Kozlowski Dec. 24, 2020, 10:21 a.m. UTC | #5 
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.

>
> 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
Jagan Teki Dec. 28, 2020, 8:20 a.m. UTC | #6 
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.
Krzysztof Kozlowski Dec. 28, 2020, 8:34 a.m. UTC | #7 
On Mon, 28 Dec 2020 at 09:21, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > #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?

If you submit another DTS using the imx8mm-engicam-ctouch2.dtsi - with
its own differences of course (not copying other DTS...) - then having
a DTSI makes sense. In current form, still NAK for all the reasons I
explained more than once.

Best regards,
Krzysztof
Jagan Teki Dec. 28, 2020, 8:44 a.m. UTC | #8 
On Mon, Dec 28, 2020 at 2:04 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, 28 Dec 2020 at 09:21, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > #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?
>
> If you submit another DTS using the imx8mm-engicam-ctouch2.dtsi - with
> its own differences of course (not copying other DTS...) - then having
> a DTSI makes sense. In current form, still NAK for all the reasons I
> explained more than once.

Okay, thanks for the review.

Jagan.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 6f0777ee6cd6..8d49a2c74604 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -32,6 +32,7 @@  dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2162a-qds.dtb
 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-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-ctouch2.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi
new file mode 100644
index 000000000000..f7870efd9dab
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi
@@ -0,0 +1,82 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2020 Engicam srl
+ * Copyright (c) 2020 Amarula Solutions(India)
+ */
+
+&fec1 {
+	status = "okay";
+};
+
+&i2c2 {
+	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2>;
+	status = "okay";
+};
+
+&i2c4 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c4>;
+	status = "okay";
+};
+
+&iomuxc {
+	pinctrl_i2c2: i2c2grp {
+		fsl,pins = <
+			MX8MM_IOMUXC_I2C2_SCL_I2C2_SCL		0x400001c3
+			MX8MM_IOMUXC_I2C2_SDA_I2C2_SDA		0x400001c3
+		>;
+	};
+
+	pinctrl_i2c4: i2c4grp {
+		fsl,pins = <
+			MX8MM_IOMUXC_I2C4_SCL_I2C4_SCL		0x400001c3
+			MX8MM_IOMUXC_I2C4_SDA_I2C4_SDA		0x400001c3
+		>;
+	};
+
+	pinctrl_uart2: uart2grp {
+		fsl,pins = <
+			MX8MM_IOMUXC_UART2_RXD_UART2_DCE_RX	0x140
+			MX8MM_IOMUXC_UART2_TXD_UART2_DCE_TX	0x140
+		>;
+	};
+
+	pinctrl_usdhc1_gpio: usdhc1gpiogrp {
+		fsl,pins = <
+			MX8MM_IOMUXC_GPIO1_IO06_GPIO1_IO6	0x41
+		>;
+	};
+
+	pinctrl_usdhc1: usdhc1grp {
+		fsl,pins = <
+			MX8MM_IOMUXC_SD1_CLK_USDHC1_CLK		0x190
+			MX8MM_IOMUXC_SD1_CMD_USDHC1_CMD		0x1d0
+			MX8MM_IOMUXC_SD1_DATA0_USDHC1_DATA0	0x1d0
+			MX8MM_IOMUXC_SD1_DATA1_USDHC1_DATA1	0x1d0
+			MX8MM_IOMUXC_SD1_DATA2_USDHC1_DATA2	0x1d0
+			MX8MM_IOMUXC_SD1_DATA3_USDHC1_DATA3	0x1d0
+		>;
+	};
+};
+
+&uart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart2>;
+	status = "okay";
+};
+
+/* SD */
+&usdhc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_usdhc1_gpio>;
+	cd-gpios = <&gpio1 6 GPIO_ACTIVE_LOW>;
+	max-frequency = <50000000>;
+	bus-width = <4>;
+	no-1-8-v;
+	pm-ignore-notify;
+	keep-power-in-suspend;
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-icore-mx8mm-ctouch2.dts b/arch/arm64/boot/dts/freescale/imx8mm-icore-mx8mm-ctouch2.dts
new file mode 100644
index 000000000000..8eb01b1f882a
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mm-icore-mx8mm-ctouch2.dts
@@ -0,0 +1,21 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2019 NXP
+ * Copyright (c) 2019 Engicam srl
+ * Copyright (c) 2020 Amarula Solutions(India)
+ */
+
+/dts-v1/;
+#include "imx8mm.dtsi"
+#include "imx8mm-engicam-ctouch2.dtsi"
+#include "imx8mm-icore-mx8mm.dtsi"
+
+/ {
+	model = "Engicam i.Core MX8M Mini C.TOUCH 2.0";
+	compatible = "engicam,icore-mx8mm-ctouch2", "engicam,icore-mx8mm",
+		     "fsl,imx8mm";
+
+	chosen {
+		stdout-path = &uart2;
+	};
+};