Re: [PATCH] ARM: dts: imx6qdl-sabresd.dtsi: Add heartbeat led

Re: [PATCH] ARM: dts: imx6qdl-sabresd.dtsi: Add heartbeat led

[Shawn Guo]

On Tue, Mar 04, 2014 at 03:34:45PM +0100, Vincent Stehlé wrote:
> Use the red gpio led for heartbeat.

Some people think it's annoying to have a LED blinking all the time.
Since it's a user defined LED, let's leave it to users for their
particular use case.

Shawn

> 
> Signed-off-by: Vincent Stehlé <vincent.stehle-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> 
> Hi,
> 
> This has been tested on Sabre SD i.MX6 Quad board, with linux v3.14-rc5+ and
> imx_v6_v7_defconfig.
> 
> As a side note, checkpatch.pl complains about gpio-leds being undocumented, but
> I am not sure what to do about that.
> 
> Best regards,
> 
> V.
> 
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index e75e11b..cf4d736 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -88,6 +88,16 @@
>  		default-brightness-level = <7>;
>  		status = "okay";
>  	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		heartbeat-led {
> +		        label = "Heartbeat";
> +		        gpios = <&gpio1 2 0>;
> +		        linux,default-trigger = "heartbeat";
> +		};
> +	};
>  };
>  
>  &audmux {
> @@ -182,6 +192,7 @@
>  				MX6QDL_PAD_ENET_TXD1__GPIO1_IO29 0x80000000
>  				MX6QDL_PAD_EIM_D22__GPIO3_IO22  0x80000000
>  				MX6QDL_PAD_ENET_CRS_DV__GPIO1_IO25 0x80000000
> +				MX6QDL_PAD_GPIO_2__GPIO1_IO02 0x80000000
>  			>;
>  		};
>  	};
> -- 
> 1.9.0
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] ARM: dts: imx6qdl-sabresd.dtsi: Add red led

[Shawn Guo]

On Wed, Mar 05, 2014 at 09:16:23AM +0100, Vincent Stehlé wrote:
> Make the red gpio led available to the user.
> 
> This can be toggled with the sysfs for example, or used as a heartbeat or mmc
> activity light by changing the trigger.
> 
> Signed-off-by: Vincent Stehlé <vincent.stehle@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> 
> ---
> 
> Hi Shawn,
> 
> Thanks for your feedback.
> 
> If you don't want to make the led a heartbeat by default, here is v2 of the
> patch, which only makes the led available to the user. One is then free to use
> it as a heartbeat, or something else. Do you prefer it this way?
> 
> Best regards,
> 
> V.
> 
> 
> Changes since v1:
> - Do not make it a heartbeat by default.
> 
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index e75e11b..210a8df 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -88,6 +88,16 @@
>  		default-brightness-level = <7>;
>  		status = "okay";
>  	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		red {
> +		        gpios = <&gpio1 2 0>;
> +		        linux,default-trigger = "none";

Per Documentation/devicetree/bindings/leds/common.txt, 'none' is not a
valid value of property 'linux,default-trigger'.  I suggest you simply
drop the property.

> +		        default-state = "on";
> +		};
> +	};
>  };
>  
>  &audmux {
> @@ -182,6 +192,7 @@
>  				MX6QDL_PAD_ENET_TXD1__GPIO1_IO29 0x80000000
>  				MX6QDL_PAD_EIM_D22__GPIO3_IO22  0x80000000
>  				MX6QDL_PAD_ENET_CRS_DV__GPIO1_IO25 0x80000000
> +				MX6QDL_PAD_GPIO_2__GPIO1_IO02 0x80000000

It's not a hog pin, so shouldn't be added here.  (Right, most of the
existing pins shouldn't be here from the beginning)

The patch from Liu Ying [1] could be a good example on this regard.

Shawn

[1] http://www.spinics.net/lists/arm-kernel/msg308999.html

>  			>;
>  		};
>  	};
> -- 
> 1.9.0
> 
> 

Re: [PATCH v2] ARM: dts: imx6qdl-sabresd.dtsi: Add red led

[Russell King - ARM Linux]

On Wed, Mar 05, 2014 at 07:42:28PM +0800, Shawn Guo wrote:
> It's not a hog pin, so shouldn't be added here.  (Right, most of the
> existing pins shouldn't be here from the beginning)

On the subject of that kind of thing... I have an issue with the existing
pinmux stuff.  This is a wider issue than your discussion above...

I regard the idea of requiring the pinmux to be configured as part of
the driver probe a rather broken concept - yes, it makes sense in some
scenarios, but for the vast majority of cases, it doesn't.

Let's take an example.  A pin is wired to a SPDIF transceiver.  The
out-of-reset SoC configuration results in the pin turning the LED on
in the SPDIF transceiver.

With the way stuff is setup, with the driver having the pinmux settings
for itself, the point at which that pin gets configured is when the
driver loads, which may be a module - so that can happen quite late in
the boot sequence.  A failure to boot can result in it not happening at
all.

So, this brings up the obvious question: is this proper behaviour of the
hardware, or is this something we should do better with - such as where
we know what the settings should be for a platform, and these settings
never change, we arrange for that to happen at kernel boot time as a
once-off.

For example, in the above case, we know it's board XYZ, we know that this
pin is connected to the SPDIF transceiver, so maybe we should configure
it early on in the boot sequence whether or not the driver has been
configured and/or loaded.

Comments?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] ARM: dts: imx6qdl-sabresd.dtsi: Add red led

[Shawn Guo]

On Wed, Mar 05, 2014 at 07:39:26PM +0000, Russell King - ARM Linux wrote:
> On Wed, Mar 05, 2014 at 07:42:28PM +0800, Shawn Guo wrote:
> > It's not a hog pin, so shouldn't be added here.  (Right, most of the
> > existing pins shouldn't be here from the beginning)
> 
> On the subject of that kind of thing... I have an issue with the existing
> pinmux stuff.  This is a wider issue than your discussion above...
> 
> I regard the idea of requiring the pinmux to be configured as part of
> the driver probe a rather broken concept - yes, it makes sense in some
> scenarios, but for the vast majority of cases, it doesn't.

>From what I've seen, it's the opposite - the concept works for the
majority, and only some scenarios needs special handling.

> 
> Let's take an example.  A pin is wired to a SPDIF transceiver.  The
> out-of-reset SoC configuration results in the pin turning the LED on
> in the SPDIF transceiver.
> 
> With the way stuff is setup, with the driver having the pinmux settings
> for itself, the point at which that pin gets configured is when the
> driver loads, which may be a module - so that can happen quite late in
> the boot sequence.  A failure to boot can result in it not happening at
> all.
> 
> So, this brings up the obvious question: is this proper behaviour of the
> hardware, or is this something we should do better with - such as where
> we know what the settings should be for a platform, and these settings
> never change, we arrange for that to happen at kernel boot time as a
> once-off.
> 
> For example, in the above case, we know it's board XYZ, we know that this
> pin is connected to the SPDIF transceiver, so maybe we should configure
> it early on in the boot sequence whether or not the driver has been
> configured and/or loaded.

For such special case, I'm fine with using hog group to handle it.  But
IMO, bootloader is even a better place to correct such unreasonable
out-of-reset SoC configuration for particular board.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] ARM: dts: imx6qdl-sabresd.dtsi: Add red led

[Sascha Hauer]

On Wed, Mar 05, 2014 at 07:39:26PM +0000, Russell King - ARM Linux wrote:
> On Wed, Mar 05, 2014 at 07:42:28PM +0800, Shawn Guo wrote:
> > It's not a hog pin, so shouldn't be added here.  (Right, most of the
> > existing pins shouldn't be here from the beginning)
> 
> On the subject of that kind of thing... I have an issue with the existing
> pinmux stuff.  This is a wider issue than your discussion above...
> 
> I regard the idea of requiring the pinmux to be configured as part of
> the driver probe a rather broken concept - yes, it makes sense in some
> scenarios, but for the vast majority of cases, it doesn't.
> 
> Let's take an example.  A pin is wired to a SPDIF transceiver.  The
> out-of-reset SoC configuration results in the pin turning the LED on
> in the SPDIF transceiver.
> 
> With the way stuff is setup, with the driver having the pinmux settings
> for itself, the point at which that pin gets configured is when the
> driver loads, which may be a module - so that can happen quite late in
> the boot sequence.  A failure to boot can result in it not happening at
> all.
> 
> So, this brings up the obvious question: is this proper behaviour of the
> hardware, or is this something we should do better with - such as where
> we know what the settings should be for a platform, and these settings
> never change, we arrange for that to happen at kernel boot time as a
> once-off.
> 
> For example, in the above case, we know it's board XYZ, we know that this
> pin is connected to the SPDIF transceiver, so maybe we should configure
> it early on in the boot sequence whether or not the driver has been
> configured and/or loaded.

I have a similar case here on my desk. Our customer provided us a
complete list of pinmux and drive strength settings which must just be
written into registers. It's very convenient to skip the entire kernel
pinmux setup. With the driver core setting up the pins this is easily
possible, we only have to skip the pinctrl nodes from the devices.
The only exception currently is the imx-esdhci which manually tries
to setup its pinmux.

So with the current pinctrl stuff it's possible to do pinctrl completely
in the bootloader or as Shawn suggested, you can also put it in the hog
group.

Maybe we just have to state somewhere that doing so is not just a
byproduct but instead can and should be done if necessary.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] ARM: dts: imx6qdl-sabresd.dtsi: Add red led

[Shawn Guo]

On Wed, Mar 05, 2014 at 07:58:39PM +0100, Vincent Stehlé wrote:
> Make the red gpio led available to the user.
> 
> This can be toggled with the sysfs for example, or used as a heartbeat or mmc
> activity light by changing the trigger.
> 
> Signed-off-by: Vincent Stehlé <vincent.stehle@freescale.com>

Applied, thanks.