[PATCH] leds: leds-gpio: Add a condition check for active low leds.

[PATCH] leds: leds-gpio: Add a condition check for active low leds.

[Song Qiang]

Some leds on our board are active low leds, which means these leds
are lighted when the corresponding gpio line is low, while the
original leds-gpio driver default all leds are active high leds.
This patch adds a devicetree node "light-state", whose value should
be "high" for active high leds and "low" for active low leds.
The default value is "high" for compatible for the original driver.

Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
---
 .../devicetree/bindings/leds/leds-gpio.txt    | 15 +++++++++++
 drivers/leds/leds-gpio.c                      | 25 +++++++++++++++++--
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
index a48dda268f81..0a8fad75c704 100644
--- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
+++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
@@ -23,6 +23,9 @@ LED sub-node properties:
   remains up.
 - panic-indicator : (optional)
   see Documentation/devicetree/bindings/leds/common.txt
+- light-state: (optional) Values should be "high" or "low", which indicates
+	the state of the GPIO pin when the led is on.
+  see Documentation/devicetree/bindings/leds/common.txt
 
 Examples:
 
@@ -64,3 +67,15 @@ leds {
 		retain-state-suspended;
 	};
 };
+
+leds {
+	compatible = "gpio-leds";
+
+	led0 {
+		label = "led0";
+		gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
+		linux,default-trigger = "heartbeat";
+		default-state = "off";
+		light-state = "low";
+	};
+}
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 764c31301f90..463ded0c71ac 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -26,9 +26,13 @@ struct gpio_led_data {
 	struct gpio_desc *gpiod;
 	u8 can_sleep;
 	u8 blinking;
+	u8 light_state;
 	gpio_blink_set_t platform_gpio_blink_set;
 };
 
+#define LEDS_GPIO_LIGHTSTATE_HIGH	0
+#define LEDS_GPIO_LIGHTSTATE_LOW	1
+
 static inline struct gpio_led_data *
 			cdev_to_gpio_led_data(struct led_classdev *led_cdev)
 {
@@ -42,9 +46,15 @@ static void gpio_led_set(struct led_classdev *led_cdev,
 	int level;
 
 	if (value == LED_OFF)
-		level = 0;
+		if (led_dat->light_state == LEDS_GPIO_LIGHTSTATE_HIGH)
+			level = 0;
+		else
+			level = 1;
 	else
-		level = 1;
+		if (led_dat->light_state == LEDS_GPIO_LIGHTSTATE_HIGH)
+			level = 1;
+		else
+			level = 0;
 
 	if (led_dat->blinking) {
 		led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
@@ -205,6 +215,17 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 				led.default_state = LEDS_GPIO_DEFSTATE_OFF;
 		}
 
+		if (!fwnode_property_read_string(child, "light-state",
+						 &state)) {
+			if (!strcmp(state, "low"))
+				led_dat->light_state =
+					LEDS_GPIO_LIGHTSTATE_LOW;
+			else{
+				led_dat->light_state =
+					LEDS_GPIO_LIGHTSTATE_HIGH;
+			}
+		}
+
 		if (fwnode_property_present(child, "retain-state-suspended"))
 			led.retain_state_suspended = 1;
 		if (fwnode_property_present(child, "retain-state-shutdown"))
-- 
2.17.1

Re: [PATCH] leds: leds-gpio: Add a condition check for active low leds.

[Michal Vokáč]

Hi Song,
On 5.9.2018 04:34, Song Qiang wrote:
> Some leds on our board are active low leds, which means these leds
> are lighted when the corresponding gpio line is low, while the
> original leds-gpio driver default all leds are active high leds.
> This patch adds a devicetree node "light-state", whose value should
> be "high" for active high leds and "low" for active low leds.
> The default value is "high" for compatible for the original driver.
> 
> Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> ---
>   .../devicetree/bindings/leds/leds-gpio.txt    | 15 +++++++++++
>   drivers/leds/leds-gpio.c                      | 25 +++++++++++++++++--
>   2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> index a48dda268f81..0a8fad75c704 100644
> --- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> @@ -23,6 +23,9 @@ LED sub-node properties:
>     remains up.
>   - panic-indicator : (optional)
>     see Documentation/devicetree/bindings/leds/common.txt
> +- light-state: (optional) Values should be "high" or "low", which indicates
> +	the state of the GPIO pin when the led is on.
> +  see Documentation/devicetree/bindings/leds/common.txt
>   
>   Examples:
>   
> @@ -64,3 +67,15 @@ leds {
>   		retain-state-suspended;
>   	};
>   };
> +
> +leds {
> +	compatible = "gpio-leds";
> +
> +	led0 {
> +		label = "led0";
> +		gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;

With this patch you are introducing one more way to invert the logic.
If your LED is active LOW, you should use GPIO_ACTIVE_LOW in your DT.
All should work as expected then.

Best regards,
Michal

> +		linux,default-trigger = "heartbeat";
> +		default-state = "off";
> +		light-state = "low";
> +	};
> +}
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index 764c31301f90..463ded0c71ac 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -26,9 +26,13 @@ struct gpio_led_data {
>   	struct gpio_desc *gpiod;
>   	u8 can_sleep;
>   	u8 blinking;
> +	u8 light_state;
>   	gpio_blink_set_t platform_gpio_blink_set;
>   };
>   
> +#define LEDS_GPIO_LIGHTSTATE_HIGH	0
> +#define LEDS_GPIO_LIGHTSTATE_LOW	1
> +
>   static inline struct gpio_led_data *
>   			cdev_to_gpio_led_data(struct led_classdev *led_cdev)
>   {
> @@ -42,9 +46,15 @@ static void gpio_led_set(struct led_classdev *led_cdev,
>   	int level;
>   
>   	if (value == LED_OFF)
> -		level = 0;
> +		if (led_dat->light_state == LEDS_GPIO_LIGHTSTATE_HIGH)
> +			level = 0;
> +		else
> +			level = 1;
>   	else
> -		level = 1;
> +		if (led_dat->light_state == LEDS_GPIO_LIGHTSTATE_HIGH)
> +			level = 1;
> +		else
> +			level = 0;
>   
>   	if (led_dat->blinking) {
>   		led_dat->platform_gpio_blink_set(led_dat->gpiod, level,
> @@ -205,6 +215,17 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>   				led.default_state = LEDS_GPIO_DEFSTATE_OFF;
>   		}
>   
> +		if (!fwnode_property_read_string(child, "light-state",
> +						 &state)) {
> +			if (!strcmp(state, "low"))
> +				led_dat->light_state =
> +					LEDS_GPIO_LIGHTSTATE_LOW;
> +			else{
> +				led_dat->light_state =
> +					LEDS_GPIO_LIGHTSTATE_HIGH;
> +			}
> +		}
> +
>   		if (fwnode_property_present(child, "retain-state-suspended"))
>   			led.retain_state_suspended = 1;
>   		if (fwnode_property_present(child, "retain-state-shutdown"))
> 

Re: [PATCH] leds: leds-gpio: Add a condition check for active low leds.

[Pavel Machek]

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

On Wed 2018-09-05 09:44:04, Michal Vokáč wrote:
> Hi Song,
> On 5.9.2018 04:34, Song Qiang wrote:
> >Some leds on our board are active low leds, which means these leds
> >are lighted when the corresponding gpio line is low, while the
> >original leds-gpio driver default all leds are active high leds.
> >This patch adds a devicetree node "light-state", whose value should
> >be "high" for active high leds and "low" for active low leds.
> >The default value is "high" for compatible for the original driver.
> >
> >Signed-off-by: Song Qiang <songqiang.1304521@gmail.com>
> >---
> >  .../devicetree/bindings/leds/leds-gpio.txt    | 15 +++++++++++
> >  drivers/leds/leds-gpio.c                      | 25 +++++++++++++++++--
> >  2 files changed, 38 insertions(+), 2 deletions(-)
> >
> >diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> >index a48dda268f81..0a8fad75c704 100644
> >--- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
> >+++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> >@@ -23,6 +23,9 @@ LED sub-node properties:
> >    remains up.
> >  - panic-indicator : (optional)
> >    see Documentation/devicetree/bindings/leds/common.txt
> >+- light-state: (optional) Values should be "high" or "low", which indicates
> >+	the state of the GPIO pin when the led is on.
> >+  see Documentation/devicetree/bindings/leds/common.txt
> >  Examples:
> >@@ -64,3 +67,15 @@ leds {
> >  		retain-state-suspended;
> >  	};
> >  };
> >+
> >+leds {
> >+	compatible = "gpio-leds";
> >+
> >+	led0 {
> >+		label = "led0";
> >+		gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
> 
> With this patch you are introducing one more way to invert the logic.
> If your LED is active LOW, you should use GPIO_ACTIVE_LOW in your DT.
> All should work as expected then.

Exactly, GPIO subsystem already has ways to specify active-low and
active-high.

NAK.
									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 --]