Re: [PATCH v2 2/2] ARM: dts: imx6ul-kontron-n6310: Add Kontron i.MX6UL N6310 SoM and boards

[Krzysztof Kozlowski <krzk@kernel.org>]

On Fri, Jul 26, 2019 at 02:54:20PM -0300, Fabio Estevam wrote:
> Hi Krzysztof,
> 
> On Fri, Jul 26, 2019 at 3:17 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
> > index 7294ac36f4c0..afb61a55e26f 100644
> > --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> > @@ -161,6 +161,10 @@ properties:
> >          items:
> >            - enum:
> >                - fsl,imx6ul-14x14-evk      # i.MX6 UltraLite 14x14 EVK Board
> > +              - kontron,n6310-som         # Kontron N6310 SOM
> > +              - kontron,n6310-s           # Kontron N6310 S Board
> > +              - kontron,n6310-s-43        # Kontron N6310 S 43 Board
> > +              - kontron,n6310-s-50        # Kontron N6310 S 50 Board
> 
> These entries should be:
>        imx6ul-kontron-n6310-s.dtb
>        imx6ul-kontron-n6310-s-43.dtb
>        imx6ul-kontron-n6310-s-50.dtb

OK

(I'll apply all your suggestions without and just reply here where there
is some discussion)

> 
> > +       panel {
> > +               compatible = "admatec,t043c004800272t2a";
> 
> I do not find this binding documented.

Because they are not... I mentioned it in commit msg - there are no
driver and bindings for them. I put them for completness of HW
description.

> 
> > +&i2c4 {
> > +       gt911@5d {
> 
> Node names should be generic according to the devicetree spec, so:
> 
> touchscreen@5d
> 
> > +               compatible = "goodix,gt928";
> > +               reg = <0x5d>;
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&pinctrl_cap_touch>;
> > +               interrupt-parent = <&gpio5>;
> > +               interrupts = <6 8>;
> 
> It would be better to use a laber to indicate the irq type:
> 
> interrupts = <6 IRQ_TYPE_LEVEL_LOW>;

Indeed.

> 
> > +               reset-gpios = <&gpio5 8 GPIO_ACTIVE_HIGH>;
> > +               irq-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
> 
> Active high?
> 
> Above you use "interrupts = <6 8>;" which means IRQ_TYPE_LEVEL_LOW.

Yes, it is confusing but it looks correct. The driver does not use the
GPIO flag so ACTIVE_HIGH or any other setting does not have effect.
Driver uses this pin (as active high) after disabling the interrupts as
an additional reset pin during resume. After this additional reset, it
serves back as interrupt pin.

> 
> > +       };
> > +};
> > +
> > +&iomuxc {
> 
> We tend to prefer putting iomuxc as the last node.
> 
> > +       pinctrl_lcdif_dat: lcdifdatgrp {
> > +               fsl,pins = <
> > +                       MX6UL_PAD_LCD_DATA00__LCDIF_DATA00      0x79
> > +                       MX6UL_PAD_LCD_DATA01__LCDIF_DATA01      0x79
> > +                       MX6UL_PAD_LCD_DATA02__LCDIF_DATA02      0x79
> > +                       MX6UL_PAD_LCD_DATA03__LCDIF_DATA03      0x79
> > +                       MX6UL_PAD_LCD_DATA04__LCDIF_DATA04      0x79
> > +                       MX6UL_PAD_LCD_DATA05__LCDIF_DATA05      0x79
> > +                       MX6UL_PAD_LCD_DATA06__LCDIF_DATA06      0x79
> > +                       MX6UL_PAD_LCD_DATA07__LCDIF_DATA07      0x79
> > +                       MX6UL_PAD_LCD_DATA08__LCDIF_DATA08      0x79
> > +                       MX6UL_PAD_LCD_DATA09__LCDIF_DATA09      0x79
> > +                       MX6UL_PAD_LCD_DATA10__LCDIF_DATA10      0x79
> > +                       MX6UL_PAD_LCD_DATA11__LCDIF_DATA11      0x79
> > +                       MX6UL_PAD_LCD_DATA12__LCDIF_DATA12      0x79
> > +                       MX6UL_PAD_LCD_DATA13__LCDIF_DATA13      0x79
> > +                       MX6UL_PAD_LCD_DATA14__LCDIF_DATA14      0x79
> > +                       MX6UL_PAD_LCD_DATA15__LCDIF_DATA15      0x79
> > +                       MX6UL_PAD_LCD_DATA16__LCDIF_DATA16      0x79
> > +                       MX6UL_PAD_LCD_DATA17__LCDIF_DATA17      0x79
> > +                       MX6UL_PAD_LCD_DATA18__LCDIF_DATA18      0x79
> > +                       MX6UL_PAD_LCD_DATA19__LCDIF_DATA19      0x79
> > +                       MX6UL_PAD_LCD_DATA20__LCDIF_DATA20      0x79
> > +                       MX6UL_PAD_LCD_DATA21__LCDIF_DATA21      0x79
> > +                       MX6UL_PAD_LCD_DATA22__LCDIF_DATA22      0x79
> > +                       MX6UL_PAD_LCD_DATA23__LCDIF_DATA23      0x79
> > +               >;
> > +       };
> > +
> > +       pinctrl_lcdif_ctrl: lcdifctrlgrp {
> > +               fsl,pins = <
> > +                       MX6UL_PAD_LCD_CLK__LCDIF_CLK            0x79
> > +                       MX6UL_PAD_LCD_ENABLE__LCDIF_ENABLE      0x79
> > +                       MX6UL_PAD_LCD_HSYNC__LCDIF_HSYNC        0x79
> > +                       MX6UL_PAD_LCD_VSYNC__LCDIF_VSYNC        0x79
> > +                       MX6UL_PAD_LCD_RESET__LCDIF_RESET        0x79
> > +               >;
> > +       };
> > +
> > +       pinctrl_cap_touch: captouchgrp {
> > +               fsl,pins = <
> > +                       MX6UL_PAD_SNVS_TAMPER6__GPIO5_IO06      0x1b0b0 /* Touch Interrupt */
> > +                       MX6UL_PAD_SNVS_TAMPER7__GPIO5_IO07      0x1b0b0 /* Touch Reset */
> > +                       MX6UL_PAD_SNVS_TAMPER8__GPIO5_IO08      0x1b0b0 /* Touch Wake */
> > +               >;
> > +       };
> > +
> > +       pinctrl_pwm7: pwm7grp {
> > +               fsl,pins = <
> > +                       MX6UL_PAD_CSI_VSYNC__PWM7_OUT           0x110b0
> > +               >;
> > +       };
> > +};
> > +
> > +&lcdif {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_lcdif_dat
> > +                    &pinctrl_lcdif_ctrl>;
> 
> Could fit into a single line.
> 
> > +       panel {
> > +               compatible = "admatec,t070p133t0s301";
> 
> Same here. Undocumented binding.
> 
> > +               backlight = <&backlight>;
> > +
> > +               port {
> > +                       panel_in: endpoint {
> > +                               remote-endpoint = <&display_out>;
> > +                       };
> > +               };
> > +       };
> > +};
> > +
> > +&i2c4 {
> > +       gt911@5d {
> 
> Same comments as previously apply.
> 
> > +
> > +       regulators {
> 
> No need to have this regulators indent level.
> 
> > +               reg_3v3: regulator1 {
> 
> You can place this one directly. The preferred format is:
> 
> reg_3v3: regulator-reg-3v3 {
> 
> > +&ecspi1 {
> > +       fsl,spi-num-chipselects = <1>;
> 
> This property is obsoleted. Please remove it.
> 
> > +       cs-gpios = <&gpio4 26 GPIO_ACTIVE_HIGH>;
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_ecspi1>;
> > +       status = "okay";
> > +
> > +       fram@0 {
> 
> Generic name please. eeprom@0
> 
> > +               compatible = "atmel,at25";
> 
> Please use the recommended compatible scheme as per
> Documentation/devicetree/bindings/eeprom/at25.txt.
> 
> > +               reg = <0>;
> > +               spi-max-frequency = <20000000>;
> > +               spi-cpha;
> > +               spi-cpol;
> > +               pagesize = <1>;
> > +               size = <8192>;
> > +               address-width = <16>;
> > +       };
> > +};
> 
> 
> > +&usbotg1 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_usbotg1>;
> > +       dr_mode = "otg";
> > +       status = "okay";
> 
> We prefer to put the 'status' property as the last one.
> 
> > +       srp-disable;
> > +       hnp-disable;
> > +       adp-disable;
> > +       vbus-supply = <&reg_usb_otg1_vbus>;
> > +};
> 
> > +/ {
> > +       model = "Kontron N6310 SOM";
> > +       compatible = "kontron,n6310-som", "fsl,imx6ul";
> > +
> > +       memory@80000000 {
> 
> device_type = "memory"; is missing here.
> 
> > +               reg = <0x80000000 0x10000000>;
> > +       };
> > +};
> > +
> > +&cpu0 {
> > +       clock-frequency = <528000000>;
> 
> Is this one really needed?

I'll check.

> 
> > +&ecspi2 {
> > +       cs-gpios = <&gpio4 22 GPIO_ACTIVE_HIGH>;
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_ecspi2>;
> > +       status = "okay";
> > +
> > +       flash: mx25v80@0 {
> 
> spi-flash@0
> 
> > +&qspi {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_qspi>;
> > +       status = "okay";
> > +
> > +       flash0: w25m02gv@0 {
> 
> generic name, please.

Thanks for review!

Best regards,
Krzysztof