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

Jacek Anaszewski
2015-05-13T10:31:06Z

On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote:
> 2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
>> 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@gmail.com>
>>> ---
>>> 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