| Message ID | 1430178954-11138-3-git-send-email-dbaryshkov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > >> 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? [skipped]
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.
On Wed, May 13, 2015 at 04:53:03PM +0200, Jacek Anaszewski wrote: > On 05/13/2015 04:14 PM, Dmitry Eremin-Solenikov wrote: > >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. > > REGMAP is enabled by default if REGMAP_MMIO is enabled. Having That is unfortunately not how select works: it does not automatically select parents for the selected symbol. Thanks.
On 05/13/2015 06:42 PM, Dmitry Torokhov wrote: > On Wed, May 13, 2015 at 04:53:03PM +0200, Jacek Anaszewski wrote: >> On 05/13/2015 04:14 PM, Dmitry Eremin-Solenikov wrote: >>> 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. >> >> REGMAP is enabled by default if REGMAP_MMIO is enabled. Having > > That is unfortunately not how select works: it does not automatically > select parents for the selected symbol. Please look at config REGMAP declaration in the file drivers/base/regmap/Kconfig: config REGMAP default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ) select LZO_COMPRESS select LZO_DECOMPRESS select IRQ_DOMAIN if REGMAP_IRQ bool
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 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> + +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); } -static void locomoled_brightness_set0(struct led_classdev *led_cdev, - enum led_brightness value) +static int locomo_led_register( + struct device *dev, + struct locomo_led *led, + const char *name, + const char *trigger, + struct regmap *regmap, + unsigned int reg) { - locomoled_brightness_set(led_cdev, value, LOCOMO_LPT0); + led->led.name = name; + led->led.flags = LED_CORE_SUSPENDRESUME; + led->led.default_trigger = trigger; + led->led.brightness_set = locomoled_brightness_set; + led->regmap = regmap; + led->reg = reg; + + return devm_led_classdev_register(dev, &led->led); } -static void locomoled_brightness_set1(struct led_classdev *led_cdev, - enum led_brightness value) -{ - locomoled_brightness_set(led_cdev, value, LOCOMO_LPT1); -} - -static struct led_classdev locomo_led0 = { - .name = "locomo:amber:charge", - .default_trigger = "main-battery-charging", - .brightness_set = locomoled_brightness_set0, -}; - -static struct led_classdev locomo_led1 = { - .name = "locomo:green:mail", - .default_trigger = "nand-disk", - .brightness_set = locomoled_brightness_set1, -}; - -static int locomoled_probe(struct locomo_dev *ldev) +static int locomoled_probe(struct platform_device *pdev) { int ret; - - ret = led_classdev_register(&ldev->dev, &locomo_led0); + struct locomo_led *leds; + struct regmap *regmap; + + leds = devm_kzalloc(&pdev->dev, 2 * sizeof(*leds), GFP_KERNEL); + if (!leds) + return -ENOMEM; + + regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!regmap) + return -ENODEV; + + ret = locomo_led_register( + &pdev->dev, + leds, + "locomo:amber:charge", + "main-battery-charging", + regmap, + LOCOMO_LPT0); if (ret < 0) return ret; - ret = led_classdev_register(&ldev->dev, &locomo_led1); + ret = locomo_led_register( + &pdev->dev, + leds + 1, + "locomo:green:mail", + "mmc0", + regmap, + LOCOMO_LPT1); if (ret < 0) - led_classdev_unregister(&locomo_led0); - - return ret; -} + return ret; -static int locomoled_remove(struct locomo_dev *dev) -{ - led_classdev_unregister(&locomo_led0); - led_classdev_unregister(&locomo_led1); return 0; } -static struct locomo_driver locomoled_driver = { - .drv = { - .name = "locomoled" +static struct platform_driver locomoled_driver = { + .driver = { + .name = "locomo-led" }, - .devid = LOCOMO_DEVID_LED, .probe = locomoled_probe, - .remove = locomoled_remove, }; -static int __init locomoled_init(void) -{ - return locomo_driver_register(&locomoled_driver); -} -module_init(locomoled_init); +module_platform_driver(locomoled_driver); MODULE_AUTHOR("John Lenz <lenz@cs.wisc.edu>"); MODULE_DESCRIPTION("Locomo LED driver"); MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:locomo-led");
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(-)