[PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Sebastian Reichel]

From: Ian Ray <ian.ray@ge.com>

Use kernel LED interface for the alarm LEDs.

Signed-off-by: Ian Ray <ian.ray@ge.com>
[Rebased]
Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
[Rebased]
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm/boot/dts/imx53-ppd.dts | 49 +++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/imx53-ppd.dts b/arch/arm/boot/dts/imx53-ppd.dts
index 5ff9a179c83c..34ce41600098 100644
--- a/arch/arm/boot/dts/imx53-ppd.dts
+++ b/arch/arm/boot/dts/imx53-ppd.dts
@@ -176,7 +176,7 @@ pwm_bl: backlight {
 		power-supply = <&reg_3v3_lcd>;
 	};
 
-	leds {
+	leds-brightness {
 		compatible = "pwm-leds";
 
 		alarm-brightness {
@@ -185,6 +185,32 @@ alarm-brightness {
 		};
 	};
 
+	leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_alarmled_pins>;
+
+		alarm1 {
+			label = "alarm:red";
+			gpios = <&gpio7 3 GPIO_ACTIVE_HIGH>;
+		};
+
+		alarm2 {
+			label = "alarm:yellow";
+			gpios = <&gpio7 7 GPIO_ACTIVE_HIGH>;
+		};
+
+		alarm3 {
+			label = "alarm:blue";
+			gpios = <&gpio7 8 GPIO_ACTIVE_HIGH>;
+		};
+
+		alarm4 {
+			label = "alarm:silenced";
+			gpios = <&gpio7 13 GPIO_ACTIVE_HIGH>;
+		};
+	};
+
 	gpio-poweroff {
 		compatible = "gpio-poweroff";
 		gpios = <&gpio3 9 GPIO_ACTIVE_HIGH>;
@@ -909,18 +935,10 @@ MX53_PAD_NANDF_CS2__GPIO6_15		0x0
 			MX53_PAD_NANDF_CS3__GPIO6_16		0x0
 			/* POWER_AND_BOOT_STATUS_INDICATOR */
 			MX53_PAD_PATA_INTRQ__GPIO7_2		0x1e4
-			/* ACTIVATE_ALARM_LIGHT_RED */
-			MX53_PAD_PATA_DIOR__GPIO7_3		0x0
-			/* ACTIVATE_ALARM_LIGHT_YELLOW */
-			MX53_PAD_PATA_DA_1__GPIO7_7		0x0
-			/* ACTIVATE_ALARM_LIGHT_CYAN */
-			MX53_PAD_PATA_DA_2__GPIO7_8		0x0
 			/* RUNNING_ON_BATTERY_INDICATOR_GREEN */
 			MX53_PAD_GPIO_16__GPIO7_11		0x0
 			/* BATTERY_STATUS_INDICATOR_AMBER */
 			MX53_PAD_GPIO_17__GPIO7_12		0x0
-			/* AUDIO_ALARMS_SILENCED_INDICATOR */
-			MX53_PAD_GPIO_18__GPIO7_13		0x0
 		>;
 	};
 
@@ -1080,4 +1098,17 @@ pinctrl_usb_otg: usbotggrp {
 			MX53_PAD_KEY_COL4__USBOH3_USBOTG_OC	0x180
 		>;
 	};
+
+	pinctrl_alarmled_pins: qmx6alarmledgrp {
+		fsl,pins = <
+			/* ACTIVATE_ALARM_LIGHT_RED */
+			MX53_PAD_PATA_DIOR__GPIO7_3		0x0
+			/* ACTIVATE_ALARM_LIGHT_YELLOW */
+			MX53_PAD_PATA_DA_1__GPIO7_7		0x0
+			/* ACTIVATE_ALARM_LIGHT_CYAN */
+			MX53_PAD_PATA_DA_2__GPIO7_8		0x0
+			/* AUDIO_ALARMS_SILENCED_INDICATOR */
+			MX53_PAD_GPIO_18__GPIO7_13		0x0
+		>;
+	};
 };
-- 
2.25.1

Re: [PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

On Thu 2020-04-16 16:51:23, Sebastian Reichel wrote:
> From: Ian Ray <ian.ray@ge.com>
> 
> Use kernel LED interface for the alarm LEDs.

Could we get these changes cced to LED maintainers?

> +		alarm1 {
> +			label = "alarm:red";
> +			gpios = <&gpio7 3 GPIO_ACTIVE_HIGH>;
> +		};

So... What is function of these leds, and can we get naming more
consistent with rest of the kernel?
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

Hi,

On Fri, Apr 24, 2020 at 11:32:26AM +0200, Pavel Machek wrote:
> On Thu 2020-04-16 16:51:23, Sebastian Reichel wrote:
> > From: Ian Ray <ian.ray@ge.com>
> > 
> > Use kernel LED interface for the alarm LEDs.
> 
> Could we get these changes cced to LED maintainers?

Sorry, you are not turning up via get_maintainer.pl and usually
subsystem maintainers are not CC'd for every DT device instance.
E.g. I do not want to be always CC'd for DT board file containing
a battery/charger. I'm quite surprised you want to be CC'd for
them, just looking at ARM DT files there are over 1000 instances
of leds.

> > +		alarm1 {
> > +			label = "alarm:red";
> > +			gpios = <&gpio7 3 GPIO_ACTIVE_HIGH>;
> > +		};
> 
> So... What is function of these leds, and can we get naming more
> consistent with rest of the kernel?

The device is a medical patient monitor and these are alarm LEDs
informing about critical device or patient status. They are
referenced by their color (those are discrete LEDs, not a
multi-color one) basically everywhere. The only exception is
"silenced", which means that audible alarm is surpressed. I
don't think we have something comparable for any of those LEDs
in the mainline tree.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

Hi,

ping?

-- Sebastian

On Fri, Apr 24, 2020 at 02:44:23PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Apr 24, 2020 at 11:32:26AM +0200, Pavel Machek wrote:
> > On Thu 2020-04-16 16:51:23, Sebastian Reichel wrote:
> > > From: Ian Ray <ian.ray@ge.com>
> > > 
> > > Use kernel LED interface for the alarm LEDs.
> > 
> > Could we get these changes cced to LED maintainers?
> 
> Sorry, you are not turning up via get_maintainer.pl and usually
> subsystem maintainers are not CC'd for every DT device instance.
> E.g. I do not want to be always CC'd for DT board file containing
> a battery/charger. I'm quite surprised you want to be CC'd for
> them, just looking at ARM DT files there are over 1000 instances
> of leds.
> 
> > > +		alarm1 {
> > > +			label = "alarm:red";
> > > +			gpios = <&gpio7 3 GPIO_ACTIVE_HIGH>;
> > > +		};
> > 
> > So... What is function of these leds, and can we get naming more
> > consistent with rest of the kernel?
> 
> The device is a medical patient monitor and these are alarm LEDs
> informing about critical device or patient status. They are
> referenced by their color (those are discrete LEDs, not a
> multi-color one) basically everywhere. The only exception is
> "silenced", which means that audible alarm is surpressed. I
> don't think we have something comparable for any of those LEDs
> in the mainline tree.
> 
> -- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1839 bytes --]

Hi!

> ping?

Well, I thought that we maybe do not need standard LEDs on medical hardware.

> On Fri, Apr 24, 2020 at 02:44:23PM +0200, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Fri, Apr 24, 2020 at 11:32:26AM +0200, Pavel Machek wrote:
> > > On Thu 2020-04-16 16:51:23, Sebastian Reichel wrote:
> > > > From: Ian Ray <ian.ray@ge.com>
> > > > 
> > > > Use kernel LED interface for the alarm LEDs.
> > > 
> > > Could we get these changes cced to LED maintainers?
> > 
> > Sorry, you are not turning up via get_maintainer.pl and usually
> > subsystem maintainers are not CC'd for every DT device instance.
> > E.g. I do not want to be always CC'd for DT board file containing
> > a battery/charger. I'm quite surprised you want to be CC'd for
> > them, just looking at ARM DT files there are over 1000 instances
> > of leds.

Well, we have mess in the naming; I'd like to clear it up.

> > > > +		alarm1 {
> > > > +			label = "alarm:red";
> > > > +			gpios = <&gpio7 3 GPIO_ACTIVE_HIGH>;
> > > > +		};
> > > 
> > > So... What is function of these leds, and can we get naming more
> > > consistent with rest of the kernel?
> > 
> > The device is a medical patient monitor and these are alarm LEDs
> > informing about critical device or patient status. They are
> > referenced by their color (those are discrete LEDs, not a
> > multi-color one) basically everywhere. The only exception is
> > "silenced", which means that audible alarm is surpressed. I
> > don't think we have something comparable for any of those LEDs
> > in the mainline tree.

Actually, we have "platform:*:mute" LEDs, that could be used for
"silenced".

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2664 bytes --]

Hi,

On Fri, May 29, 2020 at 06:02:04PM +0200, Pavel Machek wrote:
> > ping?
> 
> Well, I thought that we maybe do not need standard LEDs on medical hardware.

The discussion died and the patch was not applied :) In general
IDK how worthwhile it is to use standard LED names for them. I
suppose the number of people planning to create something like
OpenWRT for medical devices is not so big.

> > On Fri, Apr 24, 2020 at 02:44:23PM +0200, Sebastian Reichel wrote:
> > > On Fri, Apr 24, 2020 at 11:32:26AM +0200, Pavel Machek wrote:
> > > > On Thu 2020-04-16 16:51:23, Sebastian Reichel wrote:
> > > > > From: Ian Ray <ian.ray@ge.com>
> > > > > 
> > > > > Use kernel LED interface for the alarm LEDs.
> > > > 
> > > > Could we get these changes cced to LED maintainers?
> > > 
> > > Sorry, you are not turning up via get_maintainer.pl and usually
> > > subsystem maintainers are not CC'd for every DT device instance.
> > > E.g. I do not want to be always CC'd for DT board file containing
> > > a battery/charger. I'm quite surprised you want to be CC'd for
> > > them, just looking at ARM DT files there are over 1000 instances
> > > of leds.
> 
> Well, we have mess in the naming; I'd like to clear it up.

I understand.

> > > > > +		alarm1 {
> > > > > +			label = "alarm:red";
> > > > > +			gpios = <&gpio7 3 GPIO_ACTIVE_HIGH>;
> > > > > +		};
> > > > 
> > > > So... What is function of these leds, and can we get naming more
> > > > consistent with rest of the kernel?
> > > 
> > > The device is a medical patient monitor and these are alarm LEDs
> > > informing about critical device or patient status. They are
> > > referenced by their color (those are discrete LEDs, not a
> > > multi-color one) basically everywhere. The only exception is
> > > "silenced", which means that audible alarm is surpressed. I
> > > don't think we have something comparable for any of those LEDs
> > > in the mainline tree.
> 
> Actually, we have "platform:*:mute" LEDs, that could be used for
> "silenced".

I see you point, but wonder if mute is the right choice. The LED
signals a silenced alarm, which IMHO is not the same:

* The alarm silencing is temporary and system unsilences after
  1-2 minutes.
* LED is usually blinking instead of solid like a laptop mute LED
  (so that operator is aware of silenced alarm)
* Device usually cannot be put into silenced mode before the alarm
  appears
* Some medical devices still generate perodic beeps

AFAIK this is named alarm silencing by basically everyone for
medical devices. So I think naming this platfrom:*:mute would
increase the mess.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

Hi!

> On Fri, May 29, 2020 at 06:02:04PM +0200, Pavel Machek wrote:
> > > ping?
> > 
> > Well, I thought that we maybe do not need standard LEDs on medical hardware.
> 
> The discussion died and the patch was not applied :) In general
> IDK how worthwhile it is to use standard LED names for them. I
> suppose the number of people planning to create something like
> OpenWRT for medical devices is not so big.

:-)

> > > > The device is a medical patient monitor and these are alarm LEDs
> > > > informing about critical device or patient status. They are
> > > > referenced by their color (those are discrete LEDs, not a
> > > > multi-color one) basically everywhere. The only exception is
> > > > "silenced", which means that audible alarm is surpressed. I
> > > > don't think we have something comparable for any of those LEDs
> > > > in the mainline tree.
> > 
> > Actually, we have "platform:*:mute" LEDs, that could be used for
> > "silenced".
> 
> I see you point, but wonder if mute is the right choice. The LED
> signals a silenced alarm, which IMHO is not the same:
> 
> * The alarm silencing is temporary and system unsilences after
>   1-2 minutes.
> * LED is usually blinking instead of solid like a laptop mute LED
>   (so that operator is aware of silenced alarm)
> * Device usually cannot be put into silenced mode before the alarm
>   appears
> * Some medical devices still generate perodic beeps
> 
> AFAIK this is named alarm silencing by basically everyone for
> medical devices. So I think naming this platfrom:*:mute would
> increase the mess.

Ok, I guess it does not matter much. Generally no two LEDs behave
exactly the same, and I'd believe this would be close enough, but
... it really does not matter.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3001 bytes --]

Hi,

Can this be applied please? There were no changes in 5.8 cycle to
PPD's DT file, so it still applies.

Thanks,

-- Sebastian

On Thu, Apr 16, 2020 at 04:51:23PM +0200, Sebastian Reichel wrote:
> From: Ian Ray <ian.ray@ge.com>
> 
> Use kernel LED interface for the alarm LEDs.
> 
> Signed-off-by: Ian Ray <ian.ray@ge.com>
> [Rebased]
> Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
> [Rebased]
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  arch/arm/boot/dts/imx53-ppd.dts | 49 +++++++++++++++++++++++++++------
>  1 file changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx53-ppd.dts b/arch/arm/boot/dts/imx53-ppd.dts
> index 5ff9a179c83c..34ce41600098 100644
> --- a/arch/arm/boot/dts/imx53-ppd.dts
> +++ b/arch/arm/boot/dts/imx53-ppd.dts
> @@ -176,7 +176,7 @@ pwm_bl: backlight {
>  		power-supply = <&reg_3v3_lcd>;
>  	};
>  
> -	leds {
> +	leds-brightness {
>  		compatible = "pwm-leds";
>  
>  		alarm-brightness {
> @@ -185,6 +185,32 @@ alarm-brightness {
>  		};
>  	};
>  
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_alarmled_pins>;
> +
> +		alarm1 {
> +			label = "alarm:red";
> +			gpios = <&gpio7 3 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		alarm2 {
> +			label = "alarm:yellow";
> +			gpios = <&gpio7 7 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		alarm3 {
> +			label = "alarm:blue";
> +			gpios = <&gpio7 8 GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		alarm4 {
> +			label = "alarm:silenced";
> +			gpios = <&gpio7 13 GPIO_ACTIVE_HIGH>;
> +		};
> +	};
> +
>  	gpio-poweroff {
>  		compatible = "gpio-poweroff";
>  		gpios = <&gpio3 9 GPIO_ACTIVE_HIGH>;
> @@ -909,18 +935,10 @@ MX53_PAD_NANDF_CS2__GPIO6_15		0x0
>  			MX53_PAD_NANDF_CS3__GPIO6_16		0x0
>  			/* POWER_AND_BOOT_STATUS_INDICATOR */
>  			MX53_PAD_PATA_INTRQ__GPIO7_2		0x1e4
> -			/* ACTIVATE_ALARM_LIGHT_RED */
> -			MX53_PAD_PATA_DIOR__GPIO7_3		0x0
> -			/* ACTIVATE_ALARM_LIGHT_YELLOW */
> -			MX53_PAD_PATA_DA_1__GPIO7_7		0x0
> -			/* ACTIVATE_ALARM_LIGHT_CYAN */
> -			MX53_PAD_PATA_DA_2__GPIO7_8		0x0
>  			/* RUNNING_ON_BATTERY_INDICATOR_GREEN */
>  			MX53_PAD_GPIO_16__GPIO7_11		0x0
>  			/* BATTERY_STATUS_INDICATOR_AMBER */
>  			MX53_PAD_GPIO_17__GPIO7_12		0x0
> -			/* AUDIO_ALARMS_SILENCED_INDICATOR */
> -			MX53_PAD_GPIO_18__GPIO7_13		0x0
>  		>;
>  	};
>  
> @@ -1080,4 +1098,17 @@ pinctrl_usb_otg: usbotggrp {
>  			MX53_PAD_KEY_COL4__USBOH3_USBOTG_OC	0x180
>  		>;
>  	};
> +
> +	pinctrl_alarmled_pins: qmx6alarmledgrp {
> +		fsl,pins = <
> +			/* ACTIVATE_ALARM_LIGHT_RED */
> +			MX53_PAD_PATA_DIOR__GPIO7_3		0x0
> +			/* ACTIVATE_ALARM_LIGHT_YELLOW */
> +			MX53_PAD_PATA_DA_1__GPIO7_7		0x0
> +			/* ACTIVATE_ALARM_LIGHT_CYAN */
> +			MX53_PAD_PATA_DA_2__GPIO7_8		0x0
> +			/* AUDIO_ALARMS_SILENCED_INDICATOR */
> +			MX53_PAD_GPIO_18__GPIO7_13		0x0
> +		>;
> +	};
>  };
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ARM: dts: imx53: ppd: alarm LEDs use kernel LED interface

[Shawn Guo]

On Thu, Apr 16, 2020 at 04:51:23PM +0200, Sebastian Reichel wrote:
> From: Ian Ray <ian.ray@ge.com>
> 
> Use kernel LED interface for the alarm LEDs.
> 
> Signed-off-by: Ian Ray <ian.ray@ge.com>
> [Rebased]
> Signed-off-by: Samu Nuutamo <samu.nuutamo@vincit.fi>
> [Rebased]
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

s/imx53: ppd/imx53-ppd in subject.

Applied, thanks.