From: Simon Arlott <simon@fire.lp0.eu>
To: "Jacek Anaszewski" <j.anaszewski@samsung.com>,
"Álvaro Fernández Rojas" <noltari@gmail.com>
Cc: Jonas Gorski <jogo@openwrt.org>,
Richard Purdie <rpurdie@rpsys.net>,
linux-leds@vger.kernel.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: [PATCH 2/2] leds-bcm6328: Swap LED ON and OFF definitions
Date: Sun, 15 Nov 2015 13:34:37 +0000 [thread overview]
Message-ID: <564889ED.4070204@simon.arlott.org.uk> (raw)
In-Reply-To: <56488968.3070103@simon.arlott.org.uk>
The values of BCM6328_LED_MODE_ON and BCM6328_LED_MODE_OFF were named
for active low LEDs. These should be swapped so that they are named for
the default case of active high LEDs.
Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
On 05/11/15 10:41, Jacek Anaszewski wrote:
> On 11/04/2015 04:46 PM, Álvaro Fernández Rojas wrote:
>> BCM6328_LED_MODE_ON and BCM6328_LED_MODE_OFF values were extracted from
>> Broadcom's GPL code, in which they assume leds are active low by default.
>> I can confirm the code is correct as it is right now, since those values
>> match the active high / low values of the LEDs managed by GPIO instead
>> of by using this driver.
>
> BCM6328_LED_MODE_ON and BCM6328_LED_MODE_OFF should represent the values
> that actually set the LED state according to the current logic.
> Otherwise it will confuse people who will be analyzing this code.
> We are interested in the logic as it is seen from this driver's
> perspective and not GPIO perspective.
>
> IMO the values should be swapped.
drivers/leds/leds-bcm6328.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c
index 95d0cf9..0329dee 100644
--- a/drivers/leds/leds-bcm6328.c
+++ b/drivers/leds/leds-bcm6328.c
@@ -48,10 +48,10 @@
BCM6328_SERIAL_LED_SHIFT_DIR)
#define BCM6328_LED_MODE_MASK 3
-#define BCM6328_LED_MODE_OFF 0
+#define BCM6328_LED_MODE_ON 0
#define BCM6328_LED_MODE_FAST 1
#define BCM6328_LED_MODE_BLINK 2
-#define BCM6328_LED_MODE_ON 3
+#define BCM6328_LED_MODE_OFF 3
#define BCM6328_LED_SHIFT(X) ((X) << 1)
/**
@@ -126,9 +126,9 @@ static void bcm6328_led_set(struct led_classdev *led_cdev,
*(led->blink_leds) &= ~BIT(led->pin);
if ((led->active_low && value == LED_OFF) ||
(!led->active_low && value != LED_OFF))
- bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
- else
bcm6328_led_mode(led, BCM6328_LED_MODE_ON);
+ else
+ bcm6328_led_mode(led, BCM6328_LED_MODE_OFF);
spin_unlock_irqrestore(led->lock, flags);
}
@@ -303,8 +303,8 @@ static int bcm6328_led(struct device *dev, struct device_node *nc, u32 reg,
val = bcm6328_led_read(mode) >>
BCM6328_LED_SHIFT(shift % 16);
val &= BCM6328_LED_MODE_MASK;
- if ((led->active_low && val == BCM6328_LED_MODE_ON) ||
- (!led->active_low && val == BCM6328_LED_MODE_OFF))
+ if ((led->active_low && val == BCM6328_LED_MODE_OFF) ||
+ (!led->active_low && val == BCM6328_LED_MODE_ON))
led->cdev.brightness = LED_FULL;
else
led->cdev.brightness = LED_OFF;
--
2.1.4
--
Simon Arlott
next prev parent reply other threads:[~2015-11-15 13:34 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-24 16:53 [PATCH] leds: bcm6328: Handle default-state of LEDs correctly Simon Arlott
2015-10-26 8:45 ` Jacek Anaszewski
2015-10-26 12:36 ` Simon Arlott
2015-10-28 10:56 ` Jacek Anaszewski
2015-10-29 19:48 ` [PATCH] leds-bcm6328: Reuse bcm6328_led_set() instead of copying its functionality Simon Arlott
2015-11-04 15:41 ` Jacek Anaszewski
2015-11-04 15:46 ` Álvaro Fernández Rojas
2015-11-05 10:41 ` Jacek Anaszewski
2015-11-15 13:32 ` [PATCH 1/2] " Simon Arlott
2015-11-15 13:34 ` Simon Arlott [this message]
2015-11-23 10:20 ` [PATCH 2/2] leds-bcm6328: Swap LED ON and OFF definitions Jacek Anaszewski
2015-11-16 14:38 ` [PATCH 1/2] leds-bcm6328: Reuse bcm6328_led_set() instead of copying its functionality Jacek Anaszewski
2015-11-16 20:24 ` [PATCH 1/2 (v2)] " Simon Arlott
2015-11-16 21:33 ` Álvaro Fernández Rojas
2015-11-17 7:42 ` Simon Arlott
2015-11-17 8:06 ` Jacek Anaszewski
2015-11-17 8:15 ` Jacek Anaszewski
2015-11-22 20:40 ` [PATCH 1/2 (v3)] " Simon Arlott
2015-11-23 10:19 ` Jacek Anaszewski
2015-11-23 10:20 ` [PATCH 1/2 (v2)] " Jacek Anaszewski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=564889ED.4070204@simon.arlott.org.uk \
--to=simon@fire.lp0.eu \
--cc=j.anaszewski@samsung.com \
--cc=jogo@openwrt.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=noltari@gmail.com \
--cc=rpurdie@rpsys.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).