2015-05-13 13:31 GMT+03:00 Jacek Anaszewski j.anaszewski@samsung.com:

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

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.