Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1255228 > unrolled thread

[PATCH] leds: bcm6328: Handle default-state of LEDs correctly

Started bySimon Arlott <simon@fire.lp0.eu>
First post2015-10-24 19:00 +0200
Last post2015-10-29 20:50 +0100
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] leds: bcm6328: Handle default-state of LEDs correctly Simon Arlott <simon@fire.lp0.eu> - 2015-10-24 19:00 +0200
    Re: [PATCH] leds: bcm6328: Handle default-state of LEDs correctly Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-10-26 09:50 +0100
      Re: [PATCH] leds: bcm6328: Handle default-state of LEDs correctly "Simon Arlott" <simon@fire.lp0.eu> - 2015-10-26 13:40 +0100
        Re: [PATCH] leds: bcm6328: Handle default-state of LEDs correctly Jacek Anaszewski <j.anaszewski@samsung.com> - 2015-10-28 12:00 +0100
          [PATCH] leds-bcm6328: Reuse bcm6328_led_set() instead of copying its  functionality Simon Arlott <simon@fire.lp0.eu> - 2015-10-29 20:50 +0100

#1255228 — [PATCH] leds: bcm6328: Handle default-state of LEDs correctly

FromSimon Arlott <simon@fire.lp0.eu>
Date2015-10-24 19:00 +0200
Subject[PATCH] leds: bcm6328: Handle default-state of LEDs correctly
Message-ID<qnakF-6c3-1@gated-at.bofh.it>
The default-state handler assumes that the LED is active low and omits
use of the shift macro causing "keep" to misdetect the state.

Determine the brightness and then use the led set function to apply it.

Update the documentation to indicate that this driver works for the
BCM63168 (which has many active high LEDs) too.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 .../devicetree/bindings/leds/leds-bcm6328.txt      |  2 +-
 drivers/leds/leds-bcm6328.c                        | 23 ++++++++++++----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-bcm6328.txt b/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
index f9e36ad..d00260d 100644
--- a/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
+++ b/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
@@ -1,6 +1,6 @@
 LEDs connected to Broadcom BCM6328 controller
 
-This controller is present on BCM6318, BCM6328, BCM6362 and BCM63268.
+This controller is present on BCM6318, BCM6328, BCM6362, BCM63168 and BCM63268.
 In these SoCs it's possible to control LEDs both as GPIOs or by hardware.
 However, on some devices there are Serial LEDs (LEDs connected to a 74x164
 controller), which can either be controlled by software (exporting the 74x164
diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c
index 1793727..771c171 100644
--- a/drivers/leds/leds-bcm6328.c
+++ b/drivers/leds/leds-bcm6328.c
@@ -259,7 +259,6 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 		       unsigned long *blink_leds, unsigned long *blink_delay)
 {
 	struct bcm6328_led *led;
-	unsigned long flags;
 	const char *state;
 	int rc;
 
@@ -282,13 +281,12 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 						    NULL);
 
 	if (!of_property_read_string(nc, "default-state", &state)) {
-		spin_lock_irqsave(lock, flags);
 		if (!strcmp(state, "on")) {
 			led->cdev.brightness = LED_FULL;
-			bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
 		} else if (!strcmp(state, "keep")) {
 			void __iomem *mode;
 			unsigned long val, shift;
+			unsigned long flags;
 
 			shift = bcm6328_pin2shift(led->pin);
 			if (shift / 16)
@@ -296,19 +294,24 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 			else
 				mode = mem + BCM6328_REG_MODE_LO;
 
-			val = bcm6328_led_read(mode) >> (shift % 16);
+			spin_lock_irqsave(lock, flags);
+			val = bcm6328_led_read(mode);
+			spin_unlock_irqrestore(lock, flags);
+
+			val >>= BCM6328_LED_SHIFT(shift % 16);
 			val &= BCM6328_LED_MODE_MASK;
-			if (val == BCM6328_LED_MODE_ON)
+
+			dev_info(dev, "pin %lu = %08lx\n", led->pin, val);
+			if ((led->active_low && val == BCM6328_LED_MODE_ON) ||
+			    (!led->active_low && val == BCM6328_LED_MODE_OFF))
 				led->cdev.brightness = LED_FULL;
-			else {
+			else
 				led->cdev.brightness = LED_OFF;
-				bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
-			}
 		} else {
 			led->cdev.brightness = LED_OFF;
-			bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
 		}
-		spin_unlock_irqrestore(lock, flags);
+
+		bcm6328_led_set(&led->cdev, led->cdev.brightness);
 	}
 
 	led->cdev.brightness_set = bcm6328_led_set;
-- 
2.1.4

-- 
Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1255752

FromJacek Anaszewski <j.anaszewski@samsung.com>
Date2015-10-26 09:50 +0100
Message-ID<qnLDz-60G-17@gated-at.bofh.it>
In reply to#1255228
Hi Simon,

Thanks for the patch. There are conflicts when applying
it to the LED tree:

git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git

Please use it as a code base for your LED development.

When patching existing drivers, please always copy its author(s).

Cc Alvaro and Jonas.

On 10/24/2015 06:53 PM, Simon Arlott wrote:
> The default-state handler assumes that the LED is active low and omits
> use of the shift macro causing "keep" to misdetect the state.
>
> Determine the brightness and then use the led set function to apply it.
>
> Update the documentation to indicate that this driver works for the
> BCM63168 (which has many active high LEDs) too.
>
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> ---
>   .../devicetree/bindings/leds/leds-bcm6328.txt      |  2 +-
>   drivers/leds/leds-bcm6328.c                        | 23 ++++++++++++----------
>   2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-bcm6328.txt b/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
> index f9e36ad..d00260d 100644
> --- a/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
> @@ -1,6 +1,6 @@
>   LEDs connected to Broadcom BCM6328 controller
>
> -This controller is present on BCM6318, BCM6328, BCM6362 and BCM63268.
> +This controller is present on BCM6318, BCM6328, BCM6362, BCM63168 and BCM63268.
>   In these SoCs it's possible to control LEDs both as GPIOs or by hardware.
>   However, on some devices there are Serial LEDs (LEDs connected to a 74x164
>   controller), which can either be controlled by software (exporting the 74x164

Is this change related to the modifications made to the driver?
If yes, please explain the relationship in the commit message, and if
not, it should be split into a separate patch.

> diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c
> index 1793727..771c171 100644
> --- a/drivers/leds/leds-bcm6328.c
> +++ b/drivers/leds/leds-bcm6328.c
> @@ -259,7 +259,6 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
>   		       unsigned long *blink_leds, unsigned long *blink_delay)
>   {
>   	struct bcm6328_led *led;
> -	unsigned long flags;
>   	const char *state;
>   	int rc;
>
> @@ -282,13 +281,12 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
>   						    NULL);
>
>   	if (!of_property_read_string(nc, "default-state", &state)) {
> -		spin_lock_irqsave(lock, flags);
>   		if (!strcmp(state, "on")) {
>   			led->cdev.brightness = LED_FULL;
> -			bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
>   		} else if (!strcmp(state, "keep")) {
>   			void __iomem *mode;
>   			unsigned long val, shift;
> +			unsigned long flags;
>
>   			shift = bcm6328_pin2shift(led->pin);
>   			if (shift / 16)
> @@ -296,19 +294,24 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
>   			else
>   				mode = mem + BCM6328_REG_MODE_LO;
>
> -			val = bcm6328_led_read(mode) >> (shift % 16);
> +			spin_lock_irqsave(lock, flags);
> +			val = bcm6328_led_read(mode);
> +			spin_unlock_irqrestore(lock, flags);
> +
> +			val >>= BCM6328_LED_SHIFT(shift % 16);
>   			val &= BCM6328_LED_MODE_MASK;
> -			if (val == BCM6328_LED_MODE_ON)
> +
> +			dev_info(dev, "pin %lu = %08lx\n", led->pin, val);
> +			if ((led->active_low && val == BCM6328_LED_MODE_ON) ||
> +			    (!led->active_low && val == BCM6328_LED_MODE_OFF))
>   				led->cdev.brightness = LED_FULL;
> -			else {
> +			else
>   				led->cdev.brightness = LED_OFF;
> -				bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
> -			}
>   		} else {
>   			led->cdev.brightness = LED_OFF;
> -			bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
>   		}
> -		spin_unlock_irqrestore(lock, flags);
> +
> +		bcm6328_led_set(&led->cdev, led->cdev.brightness);
>   	}
>
>   	led->cdev.brightness_set = bcm6328_led_set;
>

-- 
Best Regards,
Jacek Anaszewski


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1255869

From"Simon Arlott" <simon@fire.lp0.eu>
Date2015-10-26 13:40 +0100
Message-ID<qnPe9-8ey-5@gated-at.bofh.it>
In reply to#1255752
On Mon, October 26, 2015 08:45, Jacek Anaszewski wrote:
> Hi Simon,
>
> Thanks for the patch. There are conflicts when applying
> it to the LED tree:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
>
> Please use it as a code base for your LED development.

This has already been fixed in the devel branch by d19e6b6a0062c86a75365f0715fd079245f6aab9,
although I think bcm6328_led() should adjust its use of the spinlock
to only cover reading the register and call bcm6328_led_set() instead
of copying the code from it.

-- 
Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1257905

FromJacek Anaszewski <j.anaszewski@samsung.com>
Date2015-10-28 12:00 +0100
Message-ID<qowCu-1y1-19@gated-at.bofh.it>
In reply to#1255869
Hi Simon,

On 10/26/2015 01:36 PM, Simon Arlott wrote:
> On Mon, October 26, 2015 08:45, Jacek Anaszewski wrote:
>> Hi Simon,
>>
>> Thanks for the patch. There are conflicts when applying
>> it to the LED tree:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
>>
>> Please use it as a code base for your LED development.
>
> This has already been fixed in the devel branch by d19e6b6a0062c86a75365f0715fd079245f6aab9,
> although I think bcm6328_led() should adjust its use of the spinlock
> to only cover reading the register and call bcm6328_led_set() instead
> of copying the code from it.
>

It is also on the for-next branch. Feel free to submit the patch,
if you see the room for improvement.

-- 
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258967 — [PATCH] leds-bcm6328: Reuse bcm6328_led_set() instead of copying its functionality

FromSimon Arlott <simon@fire.lp0.eu>
Date2015-10-29 20:50 +0100
Subject[PATCH] leds-bcm6328: Reuse bcm6328_led_set() instead of copying its functionality
Message-ID<qp1mW-4sA-1@gated-at.bofh.it>
In reply to#1257905
When ensuring a consistent initial LED state in bcm6328_led (as they may
be blinking instead of on/off), the LED register is set using a copy of
bcm6328_led_set(). To avoid further errors relating to active low handling,
call this function directly instead.

As bcm6328_led_set() expects to acquire the spinlock, narrow the locking
to only cover reading of the current state. There is no need to hold the
spinlock between reading the current value and setting it again because
the LED device has not yet been registered.

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 drivers/leds/leds-bcm6328.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c
index c7ea5c6..db327bd 100644
--- a/drivers/leds/leds-bcm6328.c
+++ b/drivers/leds/leds-bcm6328.c
@@ -264,7 +264,6 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 		       unsigned long *blink_leds, unsigned long *blink_delay)
 {
 	struct bcm6328_led *led;
-	unsigned long flags;
 	const char *state;
 	int rc;
 
@@ -286,13 +285,12 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 						    "linux,default-trigger",
 						    NULL);
 
-	spin_lock_irqsave(lock, flags);
 	if (!of_property_read_string(nc, "default-state", &state)) {
 		if (!strcmp(state, "on")) {
 			led->cdev.brightness = LED_FULL;
 		} else if (!strcmp(state, "keep")) {
 			void __iomem *mode;
-			unsigned long val, shift;
+			unsigned long val, shift, flags;
 
 			shift = bcm6328_pin2shift(led->pin);
 			if (shift / 16)
@@ -300,9 +298,12 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 			else
 				mode = mem + BCM6328_REG_MODE_LO;
 
+			spin_lock_irqsave(lock, flags);
 			val = bcm6328_led_read(mode) >>
 			      BCM6328_LED_SHIFT(shift % 16);
 			val &= BCM6328_LED_MODE_MASK;
+			spin_unlock_irqrestore(lock, flags);
+
 			if ((led->active_low && val == BCM6328_LED_MODE_ON) ||
 			    (!led->active_low && val == BCM6328_LED_MODE_OFF))
 				led->cdev.brightness = LED_FULL;
@@ -315,12 +316,7 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
 		led->cdev.brightness = LED_OFF;
 	}
 
-	if ((led->active_low && led->cdev.brightness == LED_FULL) ||
-	    (!led->active_low && led->cdev.brightness == LED_OFF))
-		bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
-	else
-		bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
-	spin_unlock_irqrestore(lock, flags);
+	bcm6328_led_set(&led->cdev, led->cdev.brightness);
 
 	led->cdev.brightness_set = bcm6328_led_set;
 	led->cdev.blink_set = bcm6328_blink_set;
-- 
2.1.4

-- 
Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web