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

[Dmitry Eremin-Solenikov <dbaryshkov@xxxxxxxxx>]

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.

>
>>         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]

-- 
With best wishes
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html