From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Date: Wed, 13 May 2015 10:31:06 +0000 Subject: Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core Message-Id: <555327EA.5060402@samsung.com> List-Id: References: <1430178954-11138-1-git-send-email-dbaryshkov@gmail.com> <1430178954-11138-3-git-send-email-dbaryshkov@gmail.com> <554A2DB4.3020000@samsung.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dmitry Eremin-Solenikov Cc: Russell King , Daniel Mack , Robert Jarzmik , Linus Walleij , Alexandre Courbot , Wolfram Sang , Dmitry Torokhov , Bryan Wu , Richard Purdie , Samuel Ortiz , Lee Jones , Mark Brown , Jingoo Han , Jean-Christophe Plagniol-Villard , Tomi Valkeinen , Liam Girdwood , Andrea Adami , linux-arm-kernel , "linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-input , linux-leds On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote: > 2015-05-06 18:05 GMT+03:00 Jacek Anaszewski : >> On 04/28/2015 01:55 AM, Dmitry Eremin-Solenikov wrote: >>> >>> Adapt locomo leds driver to new locomo core setup. >>> >>> Signed-off-by: Dmitry Eremin-Solenikov >>> --- >>> drivers/leds/Kconfig | 1 - >>> drivers/leds/leds-locomo.c | 119 >>> +++++++++++++++++++++++---------------------- >>> 2 files changed, 61 insertions(+), 59 deletions(-) >>> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 966b960..4b4650b 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -79,7 +79,6 @@ config LEDS_LM3642 >>> config LEDS_LOCOMO >>> tristate "LED Support for Locomo device" >>> depends on LEDS_CLASS >>> - depends on SHARP_LOCOMO >> >> >> Why do you remove this dependency? > > Because SHARP_LOCOMO is a Kconfig symbol for the old driver. New driver > uses MFD_LOCOMO Kconfig entry. Also the driver now uses generic platform > device and regmap interfaces, so there is no direct dependency on main > LoCoMo driver. And the policy (IIRC) was not to have such dependencies. Ack. Shouldn't you also need "select REGMAP_MMIO" ? >> >>> help >>> This option enables support for the LEDs on Sharp Locomo. >>> Zaurus models SL-5500 and SL-5600. >>> diff --git a/drivers/leds/leds-locomo.c b/drivers/leds/leds-locomo.c >>> index 80ba048..7fde812 100644 >>> --- a/drivers/leds/leds-locomo.c >>> +++ b/drivers/leds/leds-locomo.c >>> @@ -9,89 +9,92 @@ >>> */ >>> >>> #include >>> -#include >>> -#include >>> -#include >>> #include >>> - >>> -#include >>> -#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> >> Please keep alphabetical order. > > Ack > >> >>> + >>> +struct locomo_led { >>> + struct led_classdev led; >>> + struct regmap *regmap; >>> + unsigned int reg; >>> +}; >>> >>> static void locomoled_brightness_set(struct led_classdev *led_cdev, >>> - enum led_brightness value, int offset) >>> + enum led_brightness value) >>> { >>> - struct locomo_dev *locomo_dev = LOCOMO_DEV(led_cdev->dev->parent); >>> - unsigned long flags; >>> + struct locomo_led *led = container_of(led_cdev, struct locomo_led, >>> led); >>> >>> - local_irq_save(flags); >>> - if (value) >>> - locomo_writel(LOCOMO_LPT_TOFH, locomo_dev->mapbase + >>> offset); >>> - else >>> - locomo_writel(LOCOMO_LPT_TOFL, locomo_dev->mapbase + >>> offset); >>> - local_irq_restore(flags); >>> + regmap_write(led->regmap, led->reg, >>> + value ? LOCOMO_LPT_TOFH : LOCOMO_LPT_TOFL); >>> } >> >> >> Please use work queue for setting brightness. This is required for the >> driver to be compatible with led triggers. You can refer to the >> existing LED drivers on how to implement this. > > Hmm. Why? The regmap here uses MMIO access, so it is atomic operation > and doesn't need to be wrapped into work queue, does it? Triggers can call brightness_set op in the interrupt context, so it cannot sleep. I see "map->lock(map->lock_arg)" in regmap_write, thus I am inferring that it can sleep. I am not sure if regmap implements its 'lock' op when using MMIO. The best way to figure this out is turning "LED Timer Trigger" on in the config and execute: echo "timer" > trigger -- Best Regards, Jacek Anaszewski