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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05/12/2015 05:35 PM, Dmitry Eremin-Solenikov wrote:
2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>:
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@xxxxxxxxx>
---
   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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux