Messages in this thread | | | From | Fabio Estevam <> | Date | Thu, 21 Apr 2022 17:42:34 -0300 | Subject | Re: [PATCH v2 2/2] ARM: dts: Add bosch acc board |
| |
Hi Philip,
On Thu, Apr 21, 2022 at 9:26 AM Philip Oberfichtner <pro@denx.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 = <®_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@10000000 { > + 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@2000000 {};
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 = <ðphy>; > + > + 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 = <®_usb_h2_vbus>; > + osc-clkgate-delay = <0x3>;
Not a valid property.
> + pad-supply = <&vgen2_reg>;
Not a valid property.
| |