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

[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" ?

>>
>>>          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 <linux/kernel.h>
>>> -#include <linux/init.h>
>>> -#include <linux/module.h>
>>> -#include <linux/device.h>
>>>    #include <linux/leds.h>
>>> -
>>> -#include <mach/hardware.h>
>>> -#include <asm/hardware/locomo.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/mfd/locomo.h>
>>
>>
>> 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