From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Date: Wed, 13 May 2015 14:53:03 +0000 Subject: Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core Message-Id: <5553654F.4010608@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> <555327EA.5060402@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@vger.kernel.org" , linux-i2c@vger.kernel.org, linux-input , linux-leds On 05/13/2015 04:14 PM, Dmitry Eremin-Solenikov wrote: > 2015-05-13 13:31 GMT+03:00 Jacek Anaszewski : >> 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" ? > > No. Maybe I should add "select REGMAP" instead. REGMAP is enabled by default if REGMAP_MMIO is enabled. Having REGMAP_MMIO selected would also allow to notice at first glance which regmap backend is used. This would in turn provide a straightforward explanation on why brightness_set op is not delegated to work queue. >>>>> 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 > > It works without any "might sleep/sleeping in atomic context" warnings. > > Technically I'd agree with you. If I'm using regmaps, ideally I should not > depend on the exact backend and put working with it to the work queue. > However as this is a driver for quite old IP block, it is not used with > regmap backends other than MMIO, I'd deduce, it's ok to do things > in a more relaxed way and to call regmap_write even from atomic > contexts. Ack. -- Best Regards, Jacek Anaszewski