Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191114195609.30222-1-marco.franchi@nxp.com>
In-Reply-To: <20191114195609.30222-1-marco.franchi@nxp.com>
From:   Fabio Estevam <festevam@gmail.com>
Date:   Thu, 14 Nov 2019 17:13:36 -0300
Message-ID: <CAOMZO5Asp-m7zyY6dp72_VKZs0OisxX4B-PJtP4=GuE_-XDBsg@mail.gmail.com>
Subject: Re: [PATCH] arm64: dts: freescale: add initial support for Google
 i.MX 8MQ Phanbell
To:     Marco Antonio Franchi <marco.franchi@nxp.com>
Cc:     "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "marcofrk@gmail.com" <marcofrk@gmail.com>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Marco,

On Thu, Nov 14, 2019 at 4:56 PM Marco Antonio Franchi
<marco.franchi@nxp.com> wrote:
>
> This patch adds the device tree to support Google Coral Edge TPU,
> historicaly named as fsl-imx8mq-phanbell, a computer on module
> which can be used for AI/ML propose.
>
> It introduces a minimal enablement support for this module and

What are the features that have been tested?

Also, is the schematics available?

> was totally based on the NXP i.MX 8MQ EVK board and i.MX 8MQ Phanbell
> Google Source Code for Coral Edge TPU Mendel release:
> https://coral.googlesource.com/linux-imx/
>
> This patch was tested using the U-Boot 2017-03-1-release-chef,
> which is supported by the Coral Edge TPU Mendel release:
> https://coral.googlesource.com/uboot-imx/

I would suggest removing this paragraph from the commit log as it is
not relevant to the dts itself.

> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> index 38e344a2f0ff..cc7e02a30ed1 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -21,6 +21,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-ls2088a-rdb.dtb
>  dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-qds.dtb
>  dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb
>
> +dtb-$(CONFIG_ARCH_MXC) += fsl-imx8mq-phanbell.dtb

Please remove the fsl prefix and call it mx8mq-phanbell.dtb instead to
align with the other imx8mq dtbs.

> +&i2c1 {
> +       clock-frequency = <400000>;
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_i2c1>;
> +       status = "okay";
> +
> +       pmic: pmic@4b {
> +               reg = <0x4b>;
> +               compatible = "rohm,bd71837";
> +               pinctrl-0 = <&pinctrl_pmic>;
> +               gpio_intr = <&gpio1 3 GPIO_ACTIVE_LOW>;

This property does not exist upstream.

You should describe the interrupt like this instead:

interrupt-parent = <&gpio1>;
interrupts = <3 GPIO_ACTIVE_LOW>;

> +
> +               gpo {
> +                       rohm,drv = <0x0C>;

This property does not exist upstream.

> +&sai2 {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_sai2>;
> +       assigned-clocks =
> +               <&clk IMX8MQ_AUDIO_PLL1_BYPASS>, <&clk IMX8MQ_CLK_SAI2>;

Please don't split the lines as it gets harder to read.

> +       assigned-clock-parents =
> +               <&clk IMX8MQ_AUDIO_PLL1>, <&clk IMX8MQ_AUDIO_PLL1_OUT>;

Same here.

> +       assigned-clock-rates = <0>, <24576000>;
> +       status = "okay";
> +};
> +
> +&wdog1 {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_wdog>;
> +       fsl,ext-reset-output;
> +       status = "okay";
> +};
> +
> +&iomuxc {
> +       pinctrl-names = "default";
> +
> +       imx8mq-evk {

No need for this imx8mq-evk container.

> +               pinctrl_pmic: pmicirq {
> +                       fsl,pins = <
> +                               MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3       0x41 /*0x17059*/

This comment looks confusing. I would suggest removing it.

Regards,

Fabio Estevam