diff mbox

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

Message ID 1430178954-11138-3-git-send-email-dbaryshkov@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dmitry Baryshkov April 27, 2015, 11:55 p.m. UTC
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(-)

Comments

Dmitry Baryshkov May 12, 2015, 3:35 p.m. UTC | #1
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]
Dmitry Baryshkov May 13, 2015, 2:14 p.m. UTC | #2
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.
Dmitry Torokhov May 13, 2015, 4:42 p.m. UTC | #3
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.
Jacek Anaszewski May 14, 2015, 6:35 a.m. UTC | #4
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 mbox

Patch

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");