Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030408AbbKDPqg (ORCPT ); Wed, 4 Nov 2015 10:46:36 -0500 Received: from mail-wi0-f175.google.com ([209.85.212.175]:33361 "EHLO mail-wi0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932323AbbKDPqe (ORCPT ); Wed, 4 Nov 2015 10:46:34 -0500 Subject: Re: [PATCH] leds-bcm6328: Reuse bcm6328_led_set() instead of copying its functionality To: Jacek Anaszewski , Simon Arlott , Jonas Gorski References: <562BB799.7000708@simon.arlott.org.uk> <562DE832.6070903@samsung.com> <5630A9C1.5060907@samsung.com> <56327821.8020508@simon.arlott.org.uk> <563A2731.40204@samsung.com> Cc: Richard Purdie , linux-leds@vger.kernel.org, Linux Kernel Mailing List From: =?UTF-8?Q?=c3=81lvaro_Fern=c3=a1ndez_Rojas?= Message-ID: <563A2850.5000506@gmail.com> Date: Wed, 4 Nov 2015 16:46:24 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <563A2731.40204@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5804 Lines: 159 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 >> --- >> 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; >> > > -- 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/