Re: [PATCH 2/3] arm64: dts: freescale: Add devicetree support for Thor96 board

From: Shawn Guo <shawnguo@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: s.hauer@pengutronix.de, robh+dt@kernel.org,
	kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
	darshak.patel@einfochips.com, prajose.john@einfochips.com,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, pavel@ucw.cz,
	linux-leds@vger.kernel.org
Subject: Re: [PATCH 2/3] arm64: dts: freescale: Add devicetree support for Thor96 board
Date: Mon, 28 Oct 2019 22:32:42 +0800	[thread overview]
Message-ID: <20191028143240.GO16985@dragon> (raw)
In-Reply-To: <20191028142732.GA29312@Mani-XPS-13-9360>

On Mon, Oct 28, 2019 at 07:57:32PM +0530, Manivannan Sadhasivam wrote:
> Hi Shawn,
> 
> On Mon, Oct 28, 2019 at 07:51:12PM +0800, Shawn Guo wrote:
> > On Thu, Oct 24, 2019 at 08:12:34PM +0530, Manivannan Sadhasivam wrote:
> > > Add devicetree support for Thor96 board from Einfochips. This board is
> > > one of the 96Boards Consumer Edition platform powered by the NXP
> > > i.MX8MQ SoC.
> > > 
> > > Following are the features supported currently:
> > > 
> > > 1. uSD
> > > 2. WiFi/BT
> > > 3. Ethernet
> > > 4. EEPROM (M24256)
> > > 5. NOR Flash (W25Q256JW)
> > > 6. 2xUSB3.0 ports and 1xUSB2.0 port at HS expansion
> > > 
> > > More information about this board can be found in Arrow website:
> > > https://www.arrow.com/en/products/i.imx8-thor96/arrow-development-tools
> > > 
> > > Link to 96Boards CE Specification: https://linaro.co/ce-specification
> > > 
> > > Signed-off-by: Darshak Patel <darshak.patel@einfochips.com>
> > > [Mani: cleaned up for upstream]
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/freescale/Makefile        |   1 +
> > >  .../boot/dts/freescale/imx8mq-thor96.dts      | 581 ++++++++++++++++++
> > >  2 files changed, 582 insertions(+)
> > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-thor96.dts
> > > 
> > > diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> > > index 93fce8f0c66d..dec1662019be 100644
> > > --- a/arch/arm64/boot/dts/freescale/Makefile
> > > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > > @@ -28,6 +28,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mq-hummingboard-pulse.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-nitrogen.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-pico-pi.dtb
> > > +dtb-$(CONFIG_ARCH_MXC) += imx8mq-thor96.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb
> > >  dtb-$(CONFIG_ARCH_MXC) += imx8qxp-ai_ml.dtb
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-thor96.dts b/arch/arm64/boot/dts/freescale/imx8mq-thor96.dts
> > > new file mode 100644
> > > index 000000000000..e1adf24c2602
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mq-thor96.dts
> > > @@ -0,0 +1,581 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2019 Einfochips
> > > + * Copyright 2019 Linaro Ltd.
> > > + */
> > > +
> > > +/dts-v1/;
> > > +
> > > +#include "imx8mq.dtsi"
> > > +
> > > +/ {
> > > +	model = "Einfochips i.MX8MQ Thor96";
> > > +	compatible = "einfochips,imx8mq-thor96", "fsl,imx8mq";
> > > +
> > > +	chosen {
> > > +		stdout-path = &uart1;
> > > +	};
> > > +
> > > +	memory@40000000 {
> > > +		device_type = "memory";
> > > +		reg = <0x00000000 0x40000000 0 0x80000000>;
> > > +	};
> > > +
> > > +	leds {
> > > +		compatible = "gpio-leds";
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pinctrl_leds>;
> > > +
> > > +		user-led1 {
> > > +			label = "green:user1";
> > > +			gpios = <&gpio4 21 GPIO_ACTIVE_HIGH>;
> > > +			linux,default-trigger = "heartbeat";
> > > +		};
> > > +
> > > +		user-led2 {
> > > +			label = "green:user2";
> > > +			gpios = <&gpio4 22 GPIO_ACTIVE_HIGH>;
> > > +			linux,default-trigger = "none";
> > 
> > I see linux,default-trigger is defined in Documentation/devicetree/bindings/leds/common.txt
> > with given string values. "none" and the following ones are not there.
> > I'm not sure how this works.
> > 
> 
> The devicetree binding is not updated. I think folks just added triggers
> to the respective places but didn't bother to update the binding.

Can you please bring the bindings up to date?  It will help people like
me a lot.

Shawn

> 
> For instance, mmc* trigger is registered here:
> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/mmc/core/host.c#L476
> 
> For `none`, it is like removing the trigger. This is the default mode if the
> trigger property is not present:
> https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/leds/led-triggers.c#L113
> 
> You can see the list of LED triggers by `cat`ing this file:
> /sys/class/leds/<led>/trigger
> 
> Copied Pavel and LED list for reference.