Message-ID: <5fc186a889036da5aa11a3ef609bfcc4@denx.de>
Date:   Wed, 27 Apr 2022 15:52:06 +0200
From:   pro@...x.de
To:     Fabio Estevam <festevam@...il.com>
Cc:     "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        NXP Linux Team <linux-imx@....com>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Winker Matthias <Matthias.Winker@...bosch.com>
Subject: Re: [PATCH v2 2/2] ARM: dts: Add bosch acc board

Hi Fabio,

thank you for the review. All comments will be incorporated in v3.

On 2022-04-21 22:42, Fabio Estevam wrote:
> Hi Philip,
> 
> On Thu, Apr 21, 2022 at 9:26 AM Philip Oberfichtner <pro@...x.de> 
> wrote:
>> 
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/leds/common.h>
>> +#include "imx6q.dtsi"
>> +
>> +/ {
>> +       model = "Bosch ACC";
>> +       compatible = "bosch,imx6q-acc", "fsl,imx6q";
>> +
>> +       aliases {
>> +               serial0 = &uart2;
>> +               serial1 = &uart1;
>> +
> 
> Unneeded blank line. Please remove it.
> 
>> +               i2c0 = &i2c1;
>> +               i2c1 = &i2c2;
>> +               i2c2 = &i2c3;
>> +               /* eMMC is connected to USDHC interface 4, but shall 
>> get the name 0 */
> 
> Unneeded comment. Please remove it.
> 
>> +               mmc0 = &usdhc4;
>> +               /* SC-Cards is connected to USDHC interface 2, but 
>> shall get the name 1 */
> 
> Ditto.
> 
>> +               mmc1 = &usdhc2;
>> +       };
>> +
>> +       backlight {
>> +               compatible = "pwm-backlight";
>> +               /* The last value is the PWM period in nano-seconds!
>> +                * -> 5 kHz = 200 µS = 200.000 ns
>> +                */
> 
> This is not the correct style for multi-line comments.
> 
> It should be:
> 
> /*
>  * bla bla
>  * bla bla
>  */
> 
> Actually, I think you could just remove these comments.
> 
>> +               pwms = <&pwm1 0 200000>;
>> +               brightness-levels = <0 61 499 1706 4079 8022 13938 
>> 22237 33328 47623 65535>;
>> +               num-interpolated-steps = <10>;
>> +               default-brightness-level = <60>;
>> +               power-supply = <&reg_lcd0_pwr>;
>> +       };
>> +
>> +       refclk: refclk {
>> +               compatible = "fixed-factor-clock";
>> +               #clock-cells = <0>;
>> +
> 
> Unnecessary blank line.
> 
>> +               clocks = <&clks IMX6QDL_CLK_CKO2>;
>> +               clock-div = <1>;
>> +               clock-mult = <1>;
>> +               clock-output-names = "12mhz_refclk";
> 
> Ditto.
> 
>> +
>> +       gpio-leds {
>> +               compatible = "gpio-leds";
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&pinctrl_reset_gpio_led>;
>> +
>> +               led-2 {
>> +                       color = <LED_COLOR_ID_RED>;
>> +                       gpios = <&gpio5 18 0>;
> 
> Better use GPIO label:
> 
> gpios = <&gpio5 18 GPIO_ACTIVE_HIGH>;
> 
>> +       memory@...00000 {
>> +               device_type = "memory";
>> +               reg = <0x10000000 0x40000000>;
>> +       };
> 
> Please put the memory node below the alias.
> 
>> +
>> +       supply_5P0: regulator-0 {
> 
> Please check arch/arm/boot/dts/imx6qdl-sabresd.dtsi for a reference
> on how to name the regulators.
> 
>> +       soc {
>> +               aips1: bus@...0000 {};
> 
> Why is this needed?
> 
>> +&audmux {
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&pinctrl_audmux>;
>> +       status = "okay";
> 
> Is this needed? I don't see audio support present on this board.
> 
>> +&fec {
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&pinctrl_enet>;
>> +       status = "okay";
> 
> Please move the status property as the last one.
> 
> No need for blank lines.
>> +
>> +       clocks = <&clks IMX6QDL_CLK_ENET>,
>> +               <&clks IMX6QDL_CLK_ENET>,
>> +               <&clks IMX6QDL_CLK_ENET>,
>> +               <&clks IMX6QDL_CLK_ENET_REF>;
>> +       clock-names = "ipg", "ahb", "ptp", "enet_out";
>> +       phy-mode = "rmii";
>> +       phy-supply = <&supply_sw4_3V3>;
>> +       phy-handle = <&ethphy>;
>> +
>> +       mdio {
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               ethphy: ethernet-phy@0 {
>> +                       compatible = "ethernet-phy-ieee802.3-c22";
>> +                       reg = <0>;
>> +                       interrupt-parent = <&gpio1>;
>> +                       interrupts = <23 IRQ_TYPE_EDGE_FALLING>;
>> +                       smsc,disable-energy-detect;
>> +               };
>> +       };
>> +};
> 
>> +       lm75: sensor@49 {
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&pinctrl_lm75>;
>> +
> 
> Uneeded blank line.
> 
>> +       pmic: pmic@8 {
>> +               compatible = "fsl,pfuze100";
>> +               reg = <0x08>;
>> +
>> +               regulators {
>> +                       /*
>> +                        * VDD_CORE is connected to SW1 ABC
>> +                        * We need to define sw1ab and sw1c, but later 
>> it is controlled solely with
>> +                        * sw1c and therefore only this is named 
>> "VDD_SOC".
>> +                        * See PMIC datasheet Rev. 18, chapter 
>> 6.4.4.3.1: "The feedback and all
>> +                        * other controls are accomplished by use of 
>> pin SW1CFB and SW1C control
>> +                        * registers, respectively."
>> +                        * Setting min and max according to SOC 
>> datasheet
>> +                        */
>> +                       pmic_sw1abc: sw1c {
>> +                               regulator-name = "VDD_SOC (sw1abc)";
>> +                               regulator-min-microvolt = <1275000>;
>> +                               regulator-max-microvolt = <1500000>;
>> +                               regulator-boot-on;
>> +                               regulator-always-on;
>> +                               regulator-ramp-delay = <6250>;
>> +
>> +                               default-voltage = <1300000>;
> 
> This is not a valid property.
> 
>> +
>> +                       pmic_sw2: sw2 {
>> +                               regulator-name = "VDD_ARM (sw2)";
>> +                               regulator-min-microvolt = <1050000>;
>> +                               regulator-max-microvolt = <1500000>;
>> +                               regulator-boot-on;
>> +                               regulator-always-on;
>> +                               regulator-ramp-delay = <6250>;
>> +
> 
> Unneeded blank line.
> 
>> +                               default-voltage = <1300000>;
> 
> Invalid property.
> 
>> +&i2c3 {
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&pinctrl_i2c3>;
>> +       clock-frequency = <400000>;
>> +       status = "okay";
>> +
>> +       exc3000: touchscreen@2a {
>> +               compatible = "eeti,exc3000";
>> +               reg = <0x2a>;
>> +
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&pinctrl_ctouch>;
>> +
>> +               interrupt-parent = <&gpio4>;
>> +               interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
>> +
>> +               touchscreen-size-x = <4096>;
>> +               touchscreen-size-y = <4096>;
> 
> Remove all the blank lines.
> 
>> +&iomuxc {
> 
> We usually put the iomuxc node as the last one.
> 
>> +       pinctrl_gpio_export_gpio_fixed_in: 
>> pinctrl-gpio-export-gpio-fixed-in-grp {
> 
> Not referenced anywhere.
> 
>> +               fsl,pins = <
>> +                       MX6QDL_PAD_KEY_COL2__GPIO4_IO10         
>> 0x80000000      /* CLEAR ALL */
>> +                       MX6QDL_PAD_CSI0_DAT4__GPIO5_IO22        
>> 0x80000000      /* DIG_IN_1 */
>> +                       MX6QDL_PAD_CSI0_DAT5__GPIO5_IO23        
>> 0x80000000      /* DIG_IN_2 */
>> +                       MX6QDL_PAD_SD3_CMD__GPIO7_IO02          
>> 0x80000000      /* PoE */
>> +                       MX6QDL_PAD_SD3_CLK__GPIO7_IO03          
>> 0x80000000      /* PoE T2P */
>> +               >;
>> +       };
>> +
>> +       pinctrl_reset_gpio_led: pinctrl-reset-gpio-led-pin {
> 
> pinctrl_reset_gpio_led: pinctrl-reset-gpio-led-grp
> 
>> +               fsl,pins = <
>> +                       MX6QDL_PAD_CSI0_PIXCLK__GPIO5_IO18             
>>  0x80000000
> 
> Please avoid 0x80000000. Use the real pad setting instead.
> 
>> +               >;
>> +       };
>> +
>> +       pinctrl_gpio_export_gpio_fixed_out: 
>> pinctrl-gpio-export-gpio-fixed-out-grp {
> 
> This is not called anywhere. Please check globally.
> 
>> +               fsl,pins = <
>> +                       MX6QDL_PAD_CSI0_DAT6__GPIO5_IO24        
>> 0x0001B0B0      /*  DIG_OUT_1 */
> 
> 0x1b0b0 for consistency. Please check globally.
> 
> 
>> +       pinctrl_i2c2: i2c2grp {
>> +               fsl,pins = <
>> +                       MX6QDL_PAD_KEY_COL3__I2C2_SCL 0x4001b810
>> +                       MX6QDL_PAD_KEY_ROW3__I2C2_SDA 0x4001b810
>> +                       /* NO SRE | 130 Ohm | SPEED LOW | Open Drain | 
>> PKE | PUE | 100k PU | HYS  */
> 
> No need for comment.
> 
>> +       pinctrl_i2c2_gpio: i2c2-gpiogrp {
>> +               fsl,pins = <
>> +                       MX6QDL_PAD_KEY_COL3__GPIO4_IO12 0x80000000
> 
> Avoid  0x80000000. Please check globally.
> 
>> +       lvds0: lvds-channel@0 {
>> +               fsl,data-mapping = "spwg";
>> +               fsl,data-width = <24>;
>> +
>> +               display-timings {
>> +                       native-mode = <&lvds0_timing0>;
>> +                       lvds0_timing0: timing0 {
>> +                               clock-frequency = <79479000>;
> 
> Please add this panel support in drivers/gpu/drm/panel/panel-simple.c
> and then reference its compatible here.
> 
>> +&sdma {
>> +       fsl,sdma-ram-script-name = "imx/sdma/sdma-imx6q.bin";
> 
> Why is this needed?
> 
>> +       iram = <&ocram>;
> 
> Not a valid property for sdma.
> 
> 
>> +&uart1 {
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&pinctrl_uart1>;
>> +       status = "okay";
>> +
>> +       rts-gpios = <&gpio7 8 0>;
> 
> Use GPIO label.
> 
>> +       linux,rs485-enabled-at-boot-time;
>> +       rs485-rx-during-tx;
>> +};
>> +
>> +&uart2 {
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&pinctrl_uart2>;
>> +       fsl,uart-has-rtscts;
> 
> s/ fsl,uart-has-rtscts/uart-has-rtscts;
> 
>> +&usbh2 {
>> +       pinctrl-names = "idle", "active";
>> +       pinctrl-0 = <&pinctrl_usbh2_idle>;
>> +       pinctrl-1 = <&pinctrl_usbh2_active>;
>> +       status = "okay";
>> +
>> +       vbus-supply = <&reg_usb_h2_vbus>;
>> +       osc-clkgate-delay = <0x3>;
> 
> Not a valid property.
> 
>> +       pad-supply = <&vgen2_reg>;
> 
> Not a valid property.

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-22 Fax: +49-8142-66989-80    Email: pro@...x.de
=====================================================================

Powered by blists - more mailing lists