Messages in this thread | | | Subject | Re: [PATCH] leds-bcm6328: Reuse bcm6328_led_set() instead of copying its functionality | From | Álvaro Fernández Rojas <> | Date | Wed, 4 Nov 2015 16:46:24 +0100 |
| |
Hello Jacek,
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.
Regards, Álvaro.
El 04/11/2015 a las 16:41, Jacek Anaszewski escribió: > Hi Simon, > > Thanks for the patch. Generally this patch touches two > areas - replacement of redundant code with bcm6328_led_set, > and locking reorganization. These should be split into > two separate patches. Nonetheless, I've noticed some > issues in the code, please refer below. > > On 10/29/2015 08:48 PM, Simon Arlott wrote: >> 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); > > There are some problems with active_low mode here, I didn't recognize > earlier. > > I'd expect that active_low implies reverse logic, i.e.: > > LED_FULL -> bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); > LED_OFF -> bcm6328_led_mode(led, BCM6328_LED_MODE_ON); > > Let's take a look at bcm6328_led_set: > > 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); > > which, for active_low case, boils down to: > > LED_FULL -> bcm6328_led_mode(led, BCM6328_LED_MODE_ON); > LED_OFF -> bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); > > and for !active_low case to: > > LED_FULL -> bcm6328_led_mode(led, BCM6328_LED_MODE_OFF); > LED_OFF -> bcm6328_led_mode(led, BCM6328_LED_MODE_ON); > > so, this is the other way round. > > In bcm6328_led we have: > > if ((led->active_low && val == BCM6328_LED_MODE_ON) || > (!led->active_low && val == BCM6328_LED_MODE_OFF)) > led->cdev.brightness = LED_FULL; > else > led->cdev.brightness = LED_OFF; > > which, for active_low case, boils down to: > > BCM6328_LED_MODE_ON -> led->cdev.brightness = LED_FULL > BCM6328_LED_MODE_OFF -> led->cdev.brightness = LED_OFF > > and for !active_low case to: > > BCM6328_LED_MODE_ON -> led->cdev.brightness = LED_OFF > BCM6328_LED_MODE_OFF -> led->cdev.brightness = LED_FULL > > again, the other way round. > > All this looks like active_low really means active high. > Correct me if I'm wrong. > > Alvaro, Jonas, could you also help to clarify this discrepancy? > > >> - 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; >> > >
| |