Re: [PATCH] arm64: dts: freescale: add initial support for Google i.MX 8MQ Phanbell

[Fabio Estevam]

Hi Marco,

On Thu, Nov 14, 2019 at 4:56 PM Marco Antonio Franchi
<marco.franchi@xxxxxxx> 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