From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753434AbbJZIpo (ORCPT ); Mon, 26 Oct 2015 04:45:44 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:26427 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753170AbbJZIpm (ORCPT ); Mon, 26 Oct 2015 04:45:42 -0400 X-AuditID: cbfec7f4-f79c56d0000012ee-c6-562de8331e99 Message-id: <562DE832.6070903@samsung.com> Date: Mon, 26 Oct 2015 09:45:38 +0100 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 MIME-version: 1.0 To: Simon Arlott Cc: Richard Purdie , linux-leds@vger.kernel.org, Linux Kernel Mailing List , =?UTF-8?B?w4FsdmFybyBGZXJuw6FuZGV6IFJvamFz?= , Jonas Gorski Subject: Re: [PATCH] leds: bcm6328: Handle default-state of LEDs correctly References: <562BB799.7000708@simon.arlott.org.uk> In-reply-to: <562BB799.7000708@simon.arlott.org.uk> Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrOLMWRmVeSWpSXmKPExsVy+t/xq7rGL3TDDD7u57O4ve4Um8XlXXPY LLa+WcdosebOIVaL3bueslpcOP+Z2YHNY3H/EkaPnbPusntsWZzhsWf+D1aPz5vkAlijuGxS UnMyy1KL9O0SuDK6/t5gLGhSqDi9eBVrA+NiyS5GTg4JAROJWdOes0DYYhIX7q1n62Lk4hAS WMoosW5tNwuE84xRoq/7PjNIFa+AlsS/pfuYQGwWAVWJg4t+gHWzCRhK/HzxGiwuKhAh8ef0 PlaIekGJH5PvgdWICKhIXLjVzgoylFngKaPE5svfwYqEBbwkvu1uBSsSEjCS2DrvLtggTgFj iY8HD7KD2MwCZhKPWtYxQ9jyEpvXvGWewCgwC8mOWUjKZiEpW8DIvIpRNLU0uaA4KT3XUK84 Mbe4NC9dLzk/dxMjJLi/7GBcfMzqEKMAB6MSD+8LHt0wIdbEsuLK3EOMEhzMSiK8vTOAQrwp iZVVqUX58UWlOanFhxilOViUxHnn7nofIiSQnliSmp2aWpBaBJNl4uCUamBccvFkzdu3Pkv6 cjbVr3W6t2Z9rviC+/UF/1U1Sx7HmLu/YvoZs8/+lEfCH1X3i4dsNq1/9WeRqXTMdoFvyybW vKkMrc26c0yeaZN2k9yRG1NfOT4xcyjp+RBl2u3BXNj4/eYT81/THT12r39tuJDj1Zb4wyzf xI5MvJMts+woa42sSlKzAO8MJZbijERDLeai4kQA7aNp7GoCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > .../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