Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core

Dmitry Eremin-Solenikov
2015-05-13T14:14:01Z

2015-05-13 13:31 GMT+03:00 Jacek Anaszewski <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>:
> On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote:
>>
>> 2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>:
>>>
>>> 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 <dbaryshkov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>> 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.

>>>> 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.

--
With best wishes
Dmitry