Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220407210411.8167-1-tharvey@gateworks.com> <20220407210411.8167-2-tharvey@gateworks.com>
 <be7fd7be-c65a-215c-df96-5a6eed52b5e7@linaro.org>
In-Reply-To: <be7fd7be-c65a-215c-df96-5a6eed52b5e7@linaro.org>
From:   Tim Harvey <tharvey@gateworks.com>
Date:   Fri, 8 Apr 2022 08:52:32 -0700
Message-ID: <CAJ+vNU1D5GowKH97mpS+zT97XTavbKfijt6P8KEHN8-DD+gtYA@mail.gmail.com>
Subject: Re: [PATCH 2/2] arm64: dts: imx: Add i.MX8M Plus Gateworks gw7400 dts support
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Device Tree Mailing List <devicetree@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Linux ARM Mailing List <linux-arm-kernel@lists.infradead.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Krzysztof Kozlowski <krzk+dt@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 8, 2022 at 12:13 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 07/04/2022 23:04, Tim Harvey wrote:
> > The Gateworks GW7400 is an ARM based single board computer (SBC)
> > featuring:
> >  - i.MX8M Plus SoC
> >  - LPDDR4 DRAM
> >  - eMMC FLASH
>
> Thank you for your patch. There is something to discuss/improve.
>

Krzysztof,

Thanks for the review!

> (...)
>
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/input/linux-event-codes.h>
> > +#include <dt-bindings/leds/common.h>
> > +
> > +#include "imx8mp.dtsi"
> > +
> > +/ {
> > +     model = "Gateworks Venice GW74xx i.MX8MP board";
> > +     compatible = "gw,imx8mp-gw74xx", "fsl,imx8mp";
>
> Deprecated vendor prefix.

thanks - I wasn't aware 'gw' got deprecated. I will update to 'gateworks'.

I'm in the habit of using 'make dtbs W=1' to check for errors. Is
there something better I should be using that would catch this?

>
> > +
> > +     aliases {
> > +             ethernet0 = &eqos;
> > +             ethernet1 = &fec;
> > +             ethernet2 = &lan1;
> > +             ethernet3 = &lan2;
> > +             ethernet4 = &lan3;
> > +             ethernet5 = &lan4;
> > +             ethernet6 = &lan5;
> > +     };
> > +
> > +     chosen {
> > +             stdout-path = &uart2;
> > +     };
> > +
> > +     memory@40000000 {
> > +             device_type = "memory";
> > +             reg = <0x0 0x40000000 0 0x80000000>;
> > +     };
> > +
> > +     gpio-keys {
> > +             compatible = "gpio-keys";
> > +
> > +             user-pb {
>
> Generic node names please, so "key-0" or "user-pb-key" (although adding
> specific parts is really not needed, because you have the label).

ok

>
> > +                     label = "user_pb";
> > +                     gpios = <&gpio 2 GPIO_ACTIVE_LOW>;
> > +                     linux,code = <BTN_0>;
> > +             };
> > +
> > +             user-pb1x {
> > +                     label = "user_pb1x";
> > +                     linux,code = <BTN_1>;
> > +                     interrupt-parent = <&gsc>;
> > +                     interrupts = <0>;
> > +             };
> > +
> > +             key-erased {
> > +                     label = "key_erased";
> > +                     linux,code = <BTN_2>;
> > +                     interrupt-parent = <&gsc>;
> > +                     interrupts = <1>;
> > +             };
> > +
> > +             eeprom-wp {
> > +                     label = "eeprom_wp";
> > +                     linux,code = <BTN_3>;
> > +                     interrupt-parent = <&gsc>;
> > +                     interrupts = <2>;
> > +             };
> > +
> > +             tamper {
> > +                     label = "tamper";
> > +                     linux,code = <BTN_4>;
> > +                     interrupt-parent = <&gsc>;
> > +                     interrupts = <5>;
> > +             };
> > +
> > +             switch-hold {
> > +                     label = "switch_hold";
> > +                     linux,code = <BTN_5>;
> > +                     interrupt-parent = <&gsc>;
> > +                     interrupts = <7>;
> > +             };
> > +     };
> > +
> > +     led-controller {
> > +             compatible = "gpio-leds";
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&pinctrl_gpio_leds>;
> > +
> > +             led-0 {
> > +                     function = LED_FUNCTION_STATUS;
>
> LED_FUNCTION_HEARTBEAT

yes, thanks for catching this

>
> > +                     color = <LED_COLOR_ID_GREEN>;
> > +                     gpios = <&gpio2 15 GPIO_ACTIVE_HIGH>;
> > +                     default-state = "on";
> > +                     linux,default-trigger = "heartbeat";
> > +             };
> > +
> > +             led-1 {
> > +                     function = LED_FUNCTION_STATUS;
> > +                     color = <LED_COLOR_ID_RED>;
> > +                     gpios = <&gpio2 16 GPIO_ACTIVE_HIGH>;
> > +                     default-state = "off";
> > +             };
> > +     };
> > +
> > +     pps {
> > +             compatible = "pps-gpio";
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&pinctrl_pps>;
> > +             gpios = <&gpio1 12 GPIO_ACTIVE_HIGH>;
> > +             status = "okay";
>
> No need for status.

ok. What is the best way to know if a binding uses status or not?

Documentation/devicetree/bindings/net/fsl,fec.yaml for example does
not mention it uses status, yet it does.

Best Regards,

Tim