[PATCH/RFC,1/8] leds: Add sysfs and kernel internal API for flash LEDs

Commit Message

Jacek Anaszewski March 20, 2014, 2:51 p.m. UTC

Some LED devices support two operation modes - torch and
flash. This patch provides support for flash LED devices
in the LED subsystem by introducing new sysfs attributes
and kernel internal interface. The attributes being
introduced are: flash_mode, flash_timeout, max_flash_timeout,
flash_fault and hw_triggered.
The modifications aim to be compatible with V4L2 framework
requirements related to the flash devices management. The
design assumes that V4L2 driver can take of the LED class
device control and communicate with it through the kernel
internal interface. The LED sysfs interface is made
unavailable then.

Signed-off-by: Jacek Anaszewski <[e-mail]>
Acked-by: Kyungmin Park <[e-mail]>
Cc: Bryan Wu <[e-mail]>
Cc: Richard Purdie <[e-mail]>
---
 drivers/leds/led-class.c    |  216 +++++++++++++++++++++++++++++++++++++++++--
 drivers/leds/led-core.c     |  124 +++++++++++++++++++++++--
 drivers/leds/led-triggers.c |   17 +++-
 drivers/leds/leds.h         |    9 ++
 include/linux/leds.h        |  136 +++++++++++++++++++++++++++
 5 files changed, 486 insertions(+), 16 deletions(-)

Comments

Richard Purdie March 20, 2014, 3:28 p.m. UTC | #1

On Thu, 2014-03-20 at 15:51 +0100, Jacek Anaszewski wrote:
> Some LED devices support two operation modes - torch and
> flash. This patch provides support for flash LED devices
> in the LED subsystem by introducing new sysfs attributes
> and kernel internal interface. The attributes being
> introduced are: flash_mode, flash_timeout, max_flash_timeout,
> flash_fault and hw_triggered.
> The modifications aim to be compatible with V4L2 framework
> requirements related to the flash devices management. The
> design assumes that V4L2 driver can take of the LED class
> device control and communicate with it through the kernel
> internal interface. The LED sysfs interface is made
> unavailable then.
> 
> Signed-off-by: Jacek Anaszewski <[e-mail]>
> Acked-by: Kyungmin Park <[e-mail]>
> Cc: Bryan Wu <[e-mail]>
> Cc: Richard Purdie <[e-mail]>
> ---
>  drivers/leds/led-class.c    |  216 +++++++++++++++++++++++++++++++++++++++++--
>  drivers/leds/led-core.c     |  124 +++++++++++++++++++++++--
>  drivers/leds/led-triggers.c |   17 +++-
>  drivers/leds/leds.h         |    9 ++
>  include/linux/leds.h        |  136 +++++++++++++++++++++++++++
>  5 files changed, 486 insertions(+), 16 deletions(-)

It seems rather sad to have to insert that amount of code into the core
LED files for something which only a small number of LEDs actually use.
This will increase the footprint of the core LED code significantly.

Is it not possible to add this as a module/extension to the LED core
rather than completely entangling them?

Cheers,

Richard

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jacek Anaszewski March 21, 2014, 8:27 a.m. UTC | #2

On 03/20/2014 04:28 PM, Richard Purdie wrote:
> On Thu, 2014-03-20 at 15:51 +0100, Jacek Anaszewski wrote:
>> Some LED devices support two operation modes - torch and
>> flash. This patch provides support for flash LED devices
>> in the LED subsystem by introducing new sysfs attributes
>> and kernel internal interface. The attributes being
>> introduced are: flash_mode, flash_timeout, max_flash_timeout,
>> flash_fault and hw_triggered.
>> The modifications aim to be compatible with V4L2 framework
>> requirements related to the flash devices management. The
>> design assumes that V4L2 driver can take of the LED class
>> device control and communicate with it through the kernel
>> internal interface. The LED sysfs interface is made
>> unavailable then.
>>
>> Signed-off-by: Jacek Anaszewski <[e-mail]>
>> Acked-by: Kyungmin Park <[e-mail]>
>> Cc: Bryan Wu <[e-mail]>
>> Cc: Richard Purdie <[e-mail]>
>> ---
>>   drivers/leds/led-class.c    |  216 +++++++++++++++++++++++++++++++++++++++++--
>>   drivers/leds/led-core.c     |  124 +++++++++++++++++++++++--
>>   drivers/leds/led-triggers.c |   17 +++-
>>   drivers/leds/leds.h         |    9 ++
>>   include/linux/leds.h        |  136 +++++++++++++++++++++++++++
>>   5 files changed, 486 insertions(+), 16 deletions(-)
>
> It seems rather sad to have to insert that amount of code into the core
> LED files for something which only a small number of LEDs actually use.
> This will increase the footprint of the core LED code significantly.
>
> Is it not possible to add this as a module/extension to the LED core
> rather than completely entangling them?

OK, I'll try to decouple it.

Thanks,
Jacek Anaszewski

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sakari Ailus March 23, 2014, 11:18 p.m. UTC | #3

Hi Jacek,

Thanks for the patchset. It's very nice in general. I have a few comments
below.

On Thu, Mar 20, 2014 at 03:51:03PM +0100, Jacek Anaszewski wrote:
> Some LED devices support two operation modes - torch and
> flash. This patch provides support for flash LED devices
> in the LED subsystem by introducing new sysfs attributes
> and kernel internal interface. The attributes being
> introduced are: flash_mode, flash_timeout, max_flash_timeout,
> flash_fault and hw_triggered.
> The modifications aim to be compatible with V4L2 framework
> requirements related to the flash devices management. The
> design assumes that V4L2 driver can take of the LED class
> device control and communicate with it through the kernel
> internal interface. The LED sysfs interface is made
> unavailable then.
> 
> Signed-off-by: Jacek Anaszewski <[e-mail]>
> Acked-by: Kyungmin Park <[e-mail]>
> Cc: Bryan Wu <[e-mail]>
> Cc: Richard Purdie <[e-mail]>
> ---
>  drivers/leds/led-class.c    |  216 +++++++++++++++++++++++++++++++++++++++++--
>  drivers/leds/led-core.c     |  124 +++++++++++++++++++++++--
>  drivers/leds/led-triggers.c |   17 +++-
>  drivers/leds/leds.h         |    9 ++
>  include/linux/leds.h        |  136 +++++++++++++++++++++++++++
>  5 files changed, 486 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index f37d63c..0510532 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -4,6 +4,9 @@
>   * Copyright (C) 2005 John Lenz <[e-mail]>
>   * Copyright (C) 2005-2007 Richard Purdie <[e-mail]>
>   *
> + * Copyright (C) 2014 Samsung Electronics Co., Ltd.
> + * Author: Jacek Anaszewski <[e-mail]>
> + *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
> @@ -13,6 +16,7 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/list.h>
> +#include <linux/slab.h>

The list seems to be in quite a disarray. Could you order it as you're
adding a new header to it?

>  #include <linux/spinlock.h>
>  #include <linux/device.h>
>  #include <linux/timer.h>
> @@ -45,28 +49,186 @@ static ssize_t brightness_store(struct device *dev,
>  {
>  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>  	unsigned long state;
> -	ssize_t ret = -EINVAL;
> +	ssize_t ret;
> +
> +	mutex_lock(&led_cdev->led_lock);
> +
> +	if (led_sysfs_is_locked(led_cdev)) {
> +		ret = -EBUSY;
> +		goto exit_unlock;
> +	}
>  
>  	ret = kstrtoul(buf, 10, &state);
>  	if (ret)
> -		return ret;
> +		goto exit_unlock;
>  
>  	if (state == LED_OFF)
>  		led_trigger_remove(led_cdev);
> -	__led_set_brightness(led_cdev, state);
> +	led_set_brightness(led_cdev, state);
> +	ret = size;
>  
> -	return size;
> +exit_unlock:
> +	mutex_unlock(&led_cdev->led_lock);
> +	return ret;
>  }
>  static DEVICE_ATTR_RW(brightness);
>  
> -static ssize_t led_max_brightness_show(struct device *dev,
> +static ssize_t max_brightness_show(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>  
>  	return sprintf(buf, "%u\n", led_cdev->max_brightness);
>  }
> -static DEVICE_ATTR(max_brightness, 0444, led_max_brightness_show, NULL);
> +static DEVICE_ATTR_RO(max_brightness);
> +
> +static ssize_t flash_mode_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	unsigned long flash_mode;
> +	ssize_t ret;
> +
> +	mutex_lock(&led_cdev->led_lock);
> +
> +	if (led_sysfs_is_locked(led_cdev)) {
> +		ret = -EBUSY;
> +		goto exit_unlock;
> +	}
> +
> +	ret = kstrtoul(buf, 10, &flash_mode);
> +	if (ret)
> +		goto exit_unlock;
> +
> +	if (flash_mode < 0 && flash_mode > 1)
> +		return -EINVAL;
> +
> +	if (led_is_flash_mode(led_cdev) == flash_mode) {
> +		ret = size;
> +		goto exit_unlock;
> +	}
> +
> +	led_trigger_remove(led_cdev);
> +
> +	led_set_flash_mode(led_cdev, flash_mode);
> +	ret = size;
> +
> +exit_unlock:
> +	mutex_unlock(&led_cdev->led_lock);
> +	return ret;
> +}
> +
> +static ssize_t flash_mode_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", led_is_flash_mode(led_cdev));
> +}
> +static DEVICE_ATTR_RW(flash_mode);
> +
> +static ssize_t flash_timeout_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	unsigned long flash_timeout;
> +	ssize_t ret;
> +
> +	mutex_lock(&led_cdev->led_lock);
> +
> +	if (led_sysfs_is_locked(led_cdev)) {
> +		ret = -EBUSY;
> +		goto exit_unlock;
> +	}
> +
> +	ret = kstrtoul(buf, 10, &flash_timeout);
> +	if (ret)
> +		goto exit_unlock;
> +
> +	led_set_flash_timeout(led_cdev, flash_timeout);
> +	ret = size;
> +
> +exit_unlock:
> +	mutex_unlock(&led_cdev->led_lock);
> +	return ret;
> +}
> +
> +static ssize_t flash_timeout_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_flash *flash = led_cdev->flash;
> +
> +	return sprintf(buf, "%lu\n", flash->timeout);
> +}
> +static DEVICE_ATTR_RW(flash_timeout);
> +
> +static ssize_t max_flash_timeout_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_flash *flash = led_cdev->flash;
> +
> +	return sprintf(buf, "%lu\n", flash->max_timeout);
> +}
> +static DEVICE_ATTR_RO(max_flash_timeout);
> +
> +static ssize_t flash_fault_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	unsigned int fault;
> +	int ret;
> +
> +	ret = led_get_flash_fault(led_cdev, &fault);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	return sprintf(buf, "%x\n", fault);

How about ...0x%8.8x... or such?

> +}
> +static DEVICE_ATTR_RO(flash_fault);
> +
> +static ssize_t hw_triggered_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	unsigned long hw_triggered;
> +	ssize_t ret;
> +
> +	mutex_lock(&led_cdev->led_lock);
> +
> +	if (led_sysfs_is_locked(led_cdev)) {
> +		ret = -EBUSY;
> +		goto unlock;
> +	}
> +
> +	ret = kstrtoul(buf, 10, &hw_triggered);
> +	if (ret)
> +		goto unlock;
> +
> +	if (hw_triggered > 1) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	ret = led_set_hw_triggered(led_cdev, hw_triggered);
> +	if (ret < 0)
> +		goto unlock;
> +	ret = size;
> +
> +unlock:
> +	mutex_unlock(&led_cdev->led_lock);
> +	return ret;
> +}
> +
> +static ssize_t hw_triggered_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", led_cdev->hw_triggered);
> +}
> +static DEVICE_ATTR_RW(hw_triggered);
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
>  static DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
> @@ -82,6 +244,11 @@ static const struct attribute_group led_trigger_group = {
>  static struct attribute *led_class_attrs[] = {
>  	&dev_attr_brightness.attr,
>  	&dev_attr_max_brightness.attr,
> +	&dev_attr_flash_mode.attr,
> +	&dev_attr_flash_timeout.attr,
> +	&dev_attr_max_flash_timeout.attr,
> +	&dev_attr_flash_fault.attr,
> +	&dev_attr_hw_triggered.attr,
>  	NULL,
>  };
>  
> @@ -204,20 +371,54 @@ static const struct dev_pm_ops leds_class_dev_pm_ops = {
>  };
>  
>  /**
> + * led_classdev_init_flash - add support for flash led
> + * @led_cdev: the device to add flash led support to
> + *
> + * Returns: 0 on success, error code on failure.
> + */
> +int led_classdev_init_flash(struct led_classdev *led_cdev)
> +{
> +	if (led_cdev->flash)
> +		return -EINVAL;
> +
> +	led_cdev->flash = kzalloc(sizeof(struct led_flash), GFP_KERNEL);
> +	if (!led_cdev->flash)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_init_flash);
> +
> +/**
>   * led_classdev_register - register a new object of led_classdev class.
>   * @parent: The device to register.
>   * @led_cdev: the led_classdev structure for this device.
>   */
>  int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
>  {
> +	struct led_flash_ops *ops;
> +
> +	if (led_cdev->flash) {
> +		ops = &led_cdev->flash->ops;
> +		if (!ops || !ops->strobe_set || !ops->mode_set ||
> +			!ops->fault_get) {
> +			dev_dbg(parent,
> +				"Flash LED ops validation failed for the %s\n"
> +				"LED device", led_cdev->name);

Splitting strings breaks grep... I think the 80 character limit rule is
a lesser problem in this case.

> +			return -EINVAL;
> +		}
> +	}
> +
>  	led_cdev->dev = device_create(leds_class, parent, 0, led_cdev,
>  				      "%s", led_cdev->name);
>  	if (IS_ERR(led_cdev->dev))
>  		return PTR_ERR(led_cdev->dev);
>  
> +
>  #ifdef CONFIG_LEDS_TRIGGERS
>  	init_rwsem(&led_cdev->trigger_lock);
>  #endif
> +	mutex_init(&led_cdev->led_lock);
>  	/* add to the list of leds */
>  	down_write(&leds_list_lock);
>  	list_add_tail(&led_cdev->node, &leds_list);
> @@ -271,6 +472,8 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
>  	down_write(&leds_list_lock);
>  	list_del(&led_cdev->node);
>  	up_write(&leds_list_lock);
> +
> +	kfree(led_cdev->flash);

mutex_destroy() here as well?

>  }
>  EXPORT_SYMBOL_GPL(led_classdev_unregister);
>  
> @@ -293,5 +496,6 @@ subsys_initcall(leds_init);
>  module_exit(leds_exit);
>  
>  MODULE_AUTHOR("John Lenz, Richard Purdie");
> +MODULE_AUTHOR("Jacek Anaszewski <[e-mail]>");
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("LED Class Interface");
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 71b40d3..093703c 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -5,6 +5,9 @@
>   *
>   * Author: Richard Purdie <[e-mail]>
>   *
> + * Copyright (C) 2014 Samsung Electronics Co., Ltd.
> + * Author: Jacek Anaszewski <[e-mail]>
> + *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
> @@ -71,10 +74,27 @@ static void led_blink_setup(struct led_classdev *led_cdev,
>  	led_set_software_blink(led_cdev, *delay_on, *delay_off);
>  }
>  
> +static int led_flash_strobe_set(struct led_classdev *led_cdev,
> +			enum led_brightness brightness,
> +			unsigned long *timeout)
> +{
> +	if (!get_flash_op(led_cdev, strobe_set))
> +		return -EINVAL;
> +
> +	if (brightness > led_cdev->max_brightness)
> +		brightness = led_cdev->max_brightness;
> +	call_flash_op(strobe_set, led_cdev, brightness, timeout);
> +
> +	return 0;
> +}
> +
>  void led_blink_set(struct led_classdev *led_cdev,
>  		   unsigned long *delay_on,
>  		   unsigned long *delay_off)
>  {
> +	if (led_is_flash_mode(led_cdev))
> +		return;
> +
>  	del_timer_sync(&led_cdev->blink_timer);
>  
>  	led_cdev->flags &= ~LED_BLINK_ONESHOT;
> @@ -89,6 +109,9 @@ void led_blink_set_oneshot(struct led_classdev *led_cdev,
>  			   unsigned long *delay_off,
>  			   int invert)
>  {
> +	if (led_is_flash_mode(led_cdev))
> +		return;
> +
>  	if ((led_cdev->flags & LED_BLINK_ONESHOT) &&
>  	     timer_pending(&led_cdev->blink_timer))
>  		return;
> @@ -116,13 +139,100 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink);
>  void led_set_brightness(struct led_classdev *led_cdev,
>  			enum led_brightness brightness)
>  {
> -	/* delay brightness setting if need to stop soft-blink timer */
> -	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> -		led_cdev->delayed_set_value = brightness;
> -		schedule_work(&led_cdev->set_brightness_work);
> -		return;
> +	struct led_flash *flash = led_cdev->flash;
> +	int ret;
> +
> +	if (led_is_flash_mode(led_cdev)) {
> +		if (brightness > 0) {
> +			ret = led_flash_strobe_set(led_cdev,
> +					brightness,
> +					&flash->timeout);

Indentation. Could you align the rest of the arguments to the opening
parenthesis?

> +			if (ret < 0)
> +				dev_err(led_cdev->dev,
> +					"Failed to setup flash strobe (%d)",
> +					ret);
> +		} else {
> +			__led_set_brightness(led_cdev, 0);
> +		}
> +	} else {
> +		/* delay brightness setting if need to stop soft-blink timer */
> +		if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
> +			led_cdev->delayed_set_value = brightness;
> +			schedule_work(&led_cdev->set_brightness_work);
> +			return;
> +		}
> +		__led_set_brightness(led_cdev, brightness);
>  	}
> -
> -	__led_set_brightness(led_cdev, brightness);
>  }
>  EXPORT_SYMBOL(led_set_brightness);
> +
> +int led_set_flash_mode(struct led_classdev *led_cdev,
> +			bool flash_mode)
> +{
> +	if (!get_flash_op(led_cdev, mode_set))
> +		return -EINVAL;
> +
> +	call_flash_op(mode_set, led_cdev, flash_mode);
> +
> +	if (flash_mode)
> +		led_cdev->flags |= LED_MODE_FLASH;
> +	else
> +		led_cdev->flags &= ~LED_MODE_FLASH;
> +
> +	led_set_brightness(led_cdev, 0);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(led_set_flash_mode);
> +
> +/* Caller must ensure led_cdev->led_lock held */
> +void led_sysfs_lock(struct led_classdev *led_cdev)
> +{
> +	led_cdev->flags |= LED_SYSFS_LOCK;
> +}
> +EXPORT_SYMBOL(led_sysfs_lock);
> +
> +/* Caller must ensure led_cdev->led_lock held */
> +void led_sysfs_unlock(struct led_classdev *led_cdev)
> +{
> +	led_cdev->flags &= ~LED_SYSFS_LOCK;
> +}
> +EXPORT_SYMBOL(led_sysfs_unlock);
> +
> +void led_set_flash_timeout(struct led_classdev *led_cdev, unsigned long timeout)
> +{
> +	struct led_flash *flash = led_cdev->flash;
> +
> +	/*
> +	 * Flash timeout is passed to a flash LED driver
> +	 * through the strobe_set callback - here we only
> +	 * cache the value.
> +	 */
> +	if (timeout > flash->max_timeout)

You could use the min() macro here.

> +		flash->timeout = flash->max_timeout;
> +	else
> +		flash->timeout = timeout;
> +}
> +EXPORT_SYMBOL(led_set_flash_timeout);
> +
> +int led_get_flash_fault(struct led_classdev *led_cdev, unsigned int *fault)
> +{
> +	if (!get_flash_op(led_cdev, fault_get))
> +		return -EINVAL;
> +
> +	return call_flash_op(fault_get, led_cdev, fault);
> +}
> +EXPORT_SYMBOL(led_get_flash_fault);
> +
> +int led_set_hw_triggered(struct led_classdev *led_cdev, bool enable)
> +{
> +	if (led_cdev->has_hw_trig) {
> +		__led_set_brightness(led_cdev, 0);

...why?

> +		led_cdev->hw_triggered = enable;
> +	} else if (enable) {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(led_set_hw_triggered);
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index df1a7c1..448e7c8 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -37,6 +37,15 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>  	char trigger_name[TRIG_NAME_MAX];
>  	struct led_trigger *trig;
>  	size_t len;
> +	int ret = count;
> +
> +	mutex_lock(&led_cdev->led_lock);
> +
> +	if (led_sysfs_is_locked(led_cdev) ||
> +	    led_is_flash_mode(led_cdev)) {

Fits on the previous line.

> +		ret = -EBUSY;
> +		goto exit_unlock;
> +	}
>  
>  	trigger_name[sizeof(trigger_name) - 1] = '\0';
>  	strncpy(trigger_name, buf, sizeof(trigger_name) - 1);
> @@ -47,7 +56,7 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>  
>  	if (!strcmp(trigger_name, "none")) {
>  		led_trigger_remove(led_cdev);
> -		return count;
> +		goto exit_unlock;
>  	}
>  
>  	down_read(&triggers_list_lock);
> @@ -58,12 +67,14 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
>  			up_write(&led_cdev->trigger_lock);
>  
>  			up_read(&triggers_list_lock);
> -			return count;
> +			goto exit_unlock;
>  		}
>  	}
>  	up_read(&triggers_list_lock);
>  
> -	return -EINVAL;
> +exit_unlock:
> +	mutex_unlock(&led_cdev->led_lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_store);
>  
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index 4c50365..44cc384 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -17,6 +17,15 @@
>  #include <linux/rwsem.h>
>  #include <linux/leds.h>
>  
> +

I'd probably spare a newline here.

> +#define call_flash_op(op, args...)		\
> +	((led_cdev)->flash->ops.op(args))
> +
> +#define get_flash_op(led_cdev, op)		\
> +	((led_cdev)->flash ?			\
> +		(led_cdev)->flash->ops.op :	\
> +		0)
> +
>  static inline void __led_set_brightness(struct led_classdev *led_cdev,
>  					enum led_brightness value)
>  {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 0287ab2..1bf0ab3 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -17,6 +17,14 @@
>  #include <linux/rwsem.h>
>  #include <linux/timer.h>
>  #include <linux/workqueue.h>
> +#include <linux/mutex.h>

mutex.h should be earlier in the list of included files.

> +#include <media/v4l2-device.h>
> +
> +#define LED_FAULT_OVER_VOLTAGE		(1 << 0)
> +#define LED_FAULT_TIMEOUT		(1 << 1)
> +#define LED_FAULT_OVER_TEMPERATURE	(1 << 2)
> +#define LED_FAULT_SHORT_CIRCUIT		(1 << 3)
> +#define LED_FAULT_OVER_CURRENT		(1 << 4)

This patch went in to the media-tree some time ago. I wonder if the relevant
bits should be added here now as well.

commit 935aa6b2e8a911e81baecec0537dd7e478dc8c91
Author: Daniel Jeong <[e-mail]>
Date:   Mon Mar 3 06:52:08 2014 -0300

    [media] v4l2-controls.h: Add addtional Flash fault bits
    
    Three Flash fault are added. V4L2_FLASH_FAULT_UNDER_VOLTAGE for the case low
    voltage below the min. limit. V4L2_FLASH_FAULT_INPUT_VOLTAGE for the case
    falling input voltage and chip adjust flash current not occur under voltage
    event. V4L2_FLASH_FAULT_LED_OVER_TEMPERATURE for the case the temperature
    exceed the maximun limit
    
    Signed-off-by: Daniel Jeong <[e-mail]>
    Signed-off-by: Sakari Ailus <[e-mail]>
    Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

>  struct device;
>  /*
> @@ -29,6 +37,33 @@ enum led_brightness {
>  	LED_FULL	= 255,
>  };
>  
> +struct led_classdev;
> +
> +struct led_flash_ops {
> +	/* Set torch/flash LED mode */
> +	void	(*mode_set)(struct led_classdev *led_cdev,
> +					bool flash_mode);
> +	/* Setup flash strobe */
> +	void	(*strobe_set)(struct led_classdev *led_cdev,
> +					enum led_brightness brightness,
> +					unsigned long *timeout);

Can't the above operations fail?

Does this also assume that strobing the flash will always configure the
brightness and timeout as well? It'd be rather nice if it didn't: strobing
the flash is time critical, and as the I2C bus is quite slow, additional
device access should be avoided especially when timing is critical.

> +	/* Get the flash LED fault */
> +	int	(*fault_get)(struct led_classdev *led_cdev,
> +					unsigned int *fault);
> +};
> +
> +struct led_flash {
> +	/* flash led specific ops */
> +	struct led_flash_ops	ops;

const?

> +	/*
> +	 * maximum allowed flash timeout - it is read only and
> +	 * should be initialized by the driver

s/should/must/ ?

> +	 */
> +	unsigned long		max_timeout;
> +	/* current flash timeout */
> +	unsigned long		timeout;
> +};
> +
>  struct led_classdev {
>  	const char		*name;
>  	int			 brightness;
> @@ -42,6 +77,8 @@ struct led_classdev {
>  #define LED_BLINK_ONESHOT	(1 << 17)
>  #define LED_BLINK_ONESHOT_STOP	(1 << 18)
>  #define LED_BLINK_INVERT	(1 << 19)
> +#define LED_MODE_FLASH		(1 << 20)
> +#define LED_SYSFS_LOCK		(1 << 21)
>  
>  	/* Set LED brightness level */
>  	/* Must not sleep, use a workqueue if needed */
> @@ -69,6 +106,19 @@ struct led_classdev {
>  	unsigned long		 blink_delay_on, blink_delay_off;
>  	struct timer_list	 blink_timer;
>  	int			 blink_brightness;
> +	struct led_flash	*flash;
> +	/*
> +	 * Determines whether a device supports triggering a flash led
> +	 * with use of a dedicated hardware pin
> +	 */
> +	bool			has_hw_trig;
> +	/* If true then hardware pin triggers flash strobe */
> +	bool			hw_triggered;
> +	/*
> +	 * Ensures consistent LED sysfs access and protects
> +	 * LED sysfs locking mechanism
> +	 */
> +	struct mutex		led_lock;
>  
>  	struct work_struct	set_brightness_work;
>  	int			delayed_set_value;
> @@ -90,6 +140,7 @@ extern int led_classdev_register(struct device *parent,
>  extern void led_classdev_unregister(struct led_classdev *led_cdev);
>  extern void led_classdev_suspend(struct led_classdev *led_cdev);
>  extern void led_classdev_resume(struct led_classdev *led_cdev);
> +extern int led_classdev_init_flash(struct led_classdev *led_cdev);
>  
>  /**
>   * led_blink_set - set blinking with software fallback
> @@ -138,6 +189,91 @@ extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
>   */
>  extern void led_set_brightness(struct led_classdev *led_cdev,
>  			       enum led_brightness brightness);
> +/**

I think the above line should be moved to the surplus comment stash or
something? :-)

> +/**
> + * led_set_flash_mode - set LED flash mode
> + * @led_cdev: the LED to set
> + * @flash_mode: true - flash mode, false - torch mode
> + *
> + * Returns: 0 on success, -EINVAL on failure
> + *
> + * Switch an LED's flash mode and set brightness to 0.
> + */
> +extern int led_set_flash_mode(struct led_classdev *led_cdev,
> +			      bool flash_mode);
> +
> +/**
> + * led_set_flash_timeout - set flash LED timeout
> + * @led_cdev: the LED to set
> + * @timeout: the flash timeout to be set
> + *
> + * Set the flash strobe duration.
> + */
> +extern void led_set_flash_timeout(struct led_classdev *led_cdev,
> +				  unsigned long timeout);
> +
> +/**
> + * led_get_flash_fault - get the flash LED fault
> + * @led_cdev: the LED to query
> + * @fault: bitmask containing flash faults
> + *
> + * Returns: 0 on success, -EINVAL on failure
> + *
> + * Get the flash LED fault.
> + */
> +extern int led_get_flash_fault(struct led_classdev *led_cdev,
> +			       unsigned int *fault);
> +
> +/**
> + * led_set_hw_triggered - set the flash LED hw_triggered mode
> + * @led_cdev: the LED to set
> + * @enable: the state to set
> + *
> + * Returns: 0 on success, -EINVAL on failure
> + *
> + * Enable/disable triggering the flash LED via hardware pin
> + */
> +extern int led_set_hw_triggered(struct led_classdev *led_cdev, bool enable);
> +
> +/**
> + * led_sysfs_lock - lock LED sysfs interface
> + * @led_cdev: the LED to set
> + *
> + * Lock the LED's sysfs interface
> + */
> +extern void led_sysfs_lock(struct led_classdev *led_cdev);
> +
> +/**
> + * led_sysfs_unlock - unlock LED sysfs interface
> + * @led_cdev: the LED to set
> + *
> + * Unlock the LED's sysfs interface
> + */
> +extern void led_sysfs_unlock(struct led_classdev *led_cdev);
> +
> +/**
> + * led_is_flash_mode
> + * @led_cdev: the LED query
> + *
> + * Returns: true if a led device is in the flash mode, false if it is
> +	    is in the torch mode
> + */
> +static inline bool led_is_flash_mode(struct led_classdev *led_cdev)
> +{
> +	return !!(led_cdev->flags & LED_MODE_FLASH);

I don't think you necessarily need !!() --- converting any non-zero integer
to bool is true. Same below.

> +}
> +
> +/**
> + * led_sysfs_is_locked
> + * @led_cdev: the LED query about
> + *
> + * Returns: true if the sysfs interface of the led is disabled,
> + *	    false otherwise
> + */
> +static inline bool led_sysfs_is_locked(struct led_classdev *led_cdev)
> +{
> +	return !!(led_cdev->flags & LED_SYSFS_LOCK);
> +}
>  
>  /*
>   * LED Triggers

Jacek Anaszewski March 28, 2014, 3:30 p.m. UTC | #4

Hi Sakari,

Thanks for the review.

On 03/24/2014 12:18 AM, Sakari Ailus wrote:
> Hi Jacek,
>
> Thanks for the patchset. It's very nice in general. I have a few comments
> below.

[...]

>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index 0287ab2..1bf0ab3 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -17,6 +17,14 @@
>>   #include <linux/rwsem.h>
>>   #include <linux/timer.h>
>>   #include <linux/workqueue.h>
>> +#include <linux/mutex.h>
>
> mutex.h should be earlier in the list of included files.
>
>> +#include <media/v4l2-device.h>
>> +
>> +#define LED_FAULT_OVER_VOLTAGE		(1 << 0)
>> +#define LED_FAULT_TIMEOUT		(1 << 1)
>> +#define LED_FAULT_OVER_TEMPERATURE	(1 << 2)
>> +#define LED_FAULT_SHORT_CIRCUIT		(1 << 3)
>> +#define LED_FAULT_OVER_CURRENT		(1 << 4)
>
> This patch went in to the media-tree some time ago. I wonder if the relevant
> bits should be added here now as well.
>
> commit 935aa6b2e8a911e81baecec0537dd7e478dc8c91
> Author: Daniel Jeong <[e-mail]>
> Date:   Mon Mar 3 06:52:08 2014 -0300
>
>      [media] v4l2-controls.h: Add addtional Flash fault bits
>
>      Three Flash fault are added. V4L2_FLASH_FAULT_UNDER_VOLTAGE for the case low
>      voltage below the min. limit. V4L2_FLASH_FAULT_INPUT_VOLTAGE for the case
>      falling input voltage and chip adjust flash current not occur under voltage
>      event. V4L2_FLASH_FAULT_LED_OVER_TEMPERATURE for the case the temperature
>      exceed the maximun limit
>
>      Signed-off-by: Daniel Jeong <[e-mail]>
>      Signed-off-by: Sakari Ailus <[e-mail]>
>      Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

As it will not cause a build break and any runtime problems, even if
the patch is not merged, I added these bits to my implementation.

BTW I have doubts about V4L2_FLASH_FAULT_INDICATOR and 
V4L2_CID_FLASH_INDICATOR_INTENSITY control. I did not take them
into account in my implementation because it is not clear for
me how an indicator led is related to a torch led. There is
a control for setting indicator intensity but there is not
one for enabling it. Could you shed some light on this issue?

Regards,
Jacek Anaszewski

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sakari Ailus March 31, 2014, 9:26 a.m. UTC | #5

Hi Jacek,

On Fri, Mar 28, 2014 at 04:30:19PM +0100, Jacek Anaszewski wrote:
> Hi Sakari,
> 
> Thanks for the review.
> 
> On 03/24/2014 12:18 AM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >Thanks for the patchset. It's very nice in general. I have a few comments
> >below.
> 
> [...]
> 
> >>diff --git a/include/linux/leds.h b/include/linux/leds.h
> >>index 0287ab2..1bf0ab3 100644
> >>--- a/include/linux/leds.h
> >>+++ b/include/linux/leds.h
> >>@@ -17,6 +17,14 @@
> >>  #include <linux/rwsem.h>
> >>  #include <linux/timer.h>
> >>  #include <linux/workqueue.h>
> >>+#include <linux/mutex.h>
> >
> >mutex.h should be earlier in the list of included files.
> >
> >>+#include <media/v4l2-device.h>
> >>+
> >>+#define LED_FAULT_OVER_VOLTAGE		(1 << 0)
> >>+#define LED_FAULT_TIMEOUT		(1 << 1)
> >>+#define LED_FAULT_OVER_TEMPERATURE	(1 << 2)
> >>+#define LED_FAULT_SHORT_CIRCUIT		(1 << 3)
> >>+#define LED_FAULT_OVER_CURRENT		(1 << 4)
> >
> >This patch went in to the media-tree some time ago. I wonder if the relevant
> >bits should be added here now as well.
> >
> >commit 935aa6b2e8a911e81baecec0537dd7e478dc8c91
> >Author: Daniel Jeong <[e-mail]>
> >Date:   Mon Mar 3 06:52:08 2014 -0300
> >
> >     [media] v4l2-controls.h: Add addtional Flash fault bits
> >
> >     Three Flash fault are added. V4L2_FLASH_FAULT_UNDER_VOLTAGE for the case low
> >     voltage below the min. limit. V4L2_FLASH_FAULT_INPUT_VOLTAGE for the case
> >     falling input voltage and chip adjust flash current not occur under voltage
> >     event. V4L2_FLASH_FAULT_LED_OVER_TEMPERATURE for the case the temperature
> >     exceed the maximun limit
> >
> >     Signed-off-by: Daniel Jeong <[e-mail]>
> >     Signed-off-by: Sakari Ailus <[e-mail]>
> >     Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> 
> As it will not cause a build break and any runtime problems, even if
> the patch is not merged, I added these bits to my implementation.
> 
> BTW I have doubts about V4L2_FLASH_FAULT_INDICATOR and
> V4L2_CID_FLASH_INDICATOR_INTENSITY control. I did not take them
> into account in my implementation because it is not clear for
> me how an indicator led is related to a torch led. There is
> a control for setting indicator intensity but there is not
> one for enabling it. Could you shed some light on this issue?

The indicator isn't related to the torch mode in any way. Some flash
controllers contain an additional output for indicator, also called privacy
led. These faults are related to that.

Patch

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f37d63c..0510532 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -4,6 +4,9 @@ 
  * Copyright (C) 2005 John Lenz <[e-mail]>
  * Copyright (C) 2005-2007 Richard Purdie <[e-mail]>
  *
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ * Author: Jacek Anaszewski <[e-mail]>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
@@ -13,6 +16,7 @@ 
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/device.h>
 #include <linux/timer.h>
@@ -45,28 +49,186 @@  static ssize_t brightness_store(struct device *dev,
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	unsigned long state;
-	ssize_t ret = -EINVAL;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_lock);
+
+	if (led_sysfs_is_locked(led_cdev)) {
+		ret = -EBUSY;
+		goto exit_unlock;
+	}
 
 	ret = kstrtoul(buf, 10, &state);
 	if (ret)
-		return ret;
+		goto exit_unlock;
 
 	if (state == LED_OFF)
 		led_trigger_remove(led_cdev);
-	__led_set_brightness(led_cdev, state);
+	led_set_brightness(led_cdev, state);
+	ret = size;
 
-	return size;
+exit_unlock:
+	mutex_unlock(&led_cdev->led_lock);
+	return ret;
 }
 static DEVICE_ATTR_RW(brightness);
 
-static ssize_t led_max_brightness_show(struct device *dev,
+static ssize_t max_brightness_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", led_cdev->max_brightness);
 }
-static DEVICE_ATTR(max_brightness, 0444, led_max_brightness_show, NULL);
+static DEVICE_ATTR_RO(max_brightness);
+
+static ssize_t flash_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long flash_mode;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_lock);
+
+	if (led_sysfs_is_locked(led_cdev)) {
+		ret = -EBUSY;
+		goto exit_unlock;
+	}
+
+	ret = kstrtoul(buf, 10, &flash_mode);
+	if (ret)
+		goto exit_unlock;
+
+	if (flash_mode < 0 && flash_mode > 1)
+		return -EINVAL;
+
+	if (led_is_flash_mode(led_cdev) == flash_mode) {
+		ret = size;
+		goto exit_unlock;
+	}
+
+	led_trigger_remove(led_cdev);
+
+	led_set_flash_mode(led_cdev, flash_mode);
+	ret = size;
+
+exit_unlock:
+	mutex_unlock(&led_cdev->led_lock);
+	return ret;
+}
+
+static ssize_t flash_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", led_is_flash_mode(led_cdev));
+}
+static DEVICE_ATTR_RW(flash_mode);
+
+static ssize_t flash_timeout_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long flash_timeout;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_lock);
+
+	if (led_sysfs_is_locked(led_cdev)) {
+		ret = -EBUSY;
+		goto exit_unlock;
+	}
+
+	ret = kstrtoul(buf, 10, &flash_timeout);
+	if (ret)
+		goto exit_unlock;
+
+	led_set_flash_timeout(led_cdev, flash_timeout);
+	ret = size;
+
+exit_unlock:
+	mutex_unlock(&led_cdev->led_lock);
+	return ret;
+}
+
+static ssize_t flash_timeout_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct led_flash *flash = led_cdev->flash;
+
+	return sprintf(buf, "%lu\n", flash->timeout);
+}
+static DEVICE_ATTR_RW(flash_timeout);
+
+static ssize_t max_flash_timeout_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct led_flash *flash = led_cdev->flash;
+
+	return sprintf(buf, "%lu\n", flash->max_timeout);
+}
+static DEVICE_ATTR_RO(max_flash_timeout);
+
+static ssize_t flash_fault_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned int fault;
+	int ret;
+
+	ret = led_get_flash_fault(led_cdev, &fault);
+	if (ret < 0)
+		return -EINVAL;
+
+	return sprintf(buf, "%x\n", fault);
+}
+static DEVICE_ATTR_RO(flash_fault);
+
+static ssize_t hw_triggered_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	unsigned long hw_triggered;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_lock);
+
+	if (led_sysfs_is_locked(led_cdev)) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	ret = kstrtoul(buf, 10, &hw_triggered);
+	if (ret)
+		goto unlock;
+
+	if (hw_triggered > 1) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	ret = led_set_hw_triggered(led_cdev, hw_triggered);
+	if (ret < 0)
+		goto unlock;
+	ret = size;
+
+unlock:
+	mutex_unlock(&led_cdev->led_lock);
+	return ret;
+}
+
+static ssize_t hw_triggered_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", led_cdev->hw_triggered);
+}
+static DEVICE_ATTR_RW(hw_triggered);
 
 #ifdef CONFIG_LEDS_TRIGGERS
 static DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
@@ -82,6 +244,11 @@  static const struct attribute_group led_trigger_group = {
 static struct attribute *led_class_attrs[] = {
 	&dev_attr_brightness.attr,
 	&dev_attr_max_brightness.attr,
+	&dev_attr_flash_mode.attr,
+	&dev_attr_flash_timeout.attr,
+	&dev_attr_max_flash_timeout.attr,
+	&dev_attr_flash_fault.attr,
+	&dev_attr_hw_triggered.attr,
 	NULL,
 };
 
@@ -204,20 +371,54 @@  static const struct dev_pm_ops leds_class_dev_pm_ops = {
 };
 
 /**
+ * led_classdev_init_flash - add support for flash led
+ * @led_cdev: the device to add flash led support to
+ *
+ * Returns: 0 on success, error code on failure.
+ */
+int led_classdev_init_flash(struct led_classdev *led_cdev)
+{
+	if (led_cdev->flash)
+		return -EINVAL;
+
+	led_cdev->flash = kzalloc(sizeof(struct led_flash), GFP_KERNEL);
+	if (!led_cdev->flash)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(led_classdev_init_flash);
+
+/**
  * led_classdev_register - register a new object of led_classdev class.
  * @parent: The device to register.
  * @led_cdev: the led_classdev structure for this device.
  */
 int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 {
+	struct led_flash_ops *ops;
+
+	if (led_cdev->flash) {
+		ops = &led_cdev->flash->ops;
+		if (!ops || !ops->strobe_set || !ops->mode_set ||
+			!ops->fault_get) {
+			dev_dbg(parent,
+				"Flash LED ops validation failed for the %s\n"
+				"LED device", led_cdev->name);
+			return -EINVAL;
+		}
+	}
+
 	led_cdev->dev = device_create(leds_class, parent, 0, led_cdev,
 				      "%s", led_cdev->name);
 	if (IS_ERR(led_cdev->dev))
 		return PTR_ERR(led_cdev->dev);
 
+
 #ifdef CONFIG_LEDS_TRIGGERS
 	init_rwsem(&led_cdev->trigger_lock);
 #endif
+	mutex_init(&led_cdev->led_lock);
 	/* add to the list of leds */
 	down_write(&leds_list_lock);
 	list_add_tail(&led_cdev->node, &leds_list);
@@ -271,6 +472,8 @@  void led_classdev_unregister(struct led_classdev *led_cdev)
 	down_write(&leds_list_lock);
 	list_del(&led_cdev->node);
 	up_write(&leds_list_lock);
+
+	kfree(led_cdev->flash);
 }
 EXPORT_SYMBOL_GPL(led_classdev_unregister);
 
@@ -293,5 +496,6 @@  subsys_initcall(leds_init);
 module_exit(leds_exit);
 
 MODULE_AUTHOR("John Lenz, Richard Purdie");
+MODULE_AUTHOR("Jacek Anaszewski <[e-mail]>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("LED Class Interface");
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 71b40d3..093703c 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -5,6 +5,9 @@ 
  *
  * Author: Richard Purdie <[e-mail]>
  *
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ * Author: Jacek Anaszewski <[e-mail]>
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
@@ -71,10 +74,27 @@  static void led_blink_setup(struct led_classdev *led_cdev,
 	led_set_software_blink(led_cdev, *delay_on, *delay_off);
 }
 
+static int led_flash_strobe_set(struct led_classdev *led_cdev,
+			enum led_brightness brightness,
+			unsigned long *timeout)
+{
+	if (!get_flash_op(led_cdev, strobe_set))
+		return -EINVAL;
+
+	if (brightness > led_cdev->max_brightness)
+		brightness = led_cdev->max_brightness;
+	call_flash_op(strobe_set, led_cdev, brightness, timeout);
+
+	return 0;
+}
+
 void led_blink_set(struct led_classdev *led_cdev,
 		   unsigned long *delay_on,
 		   unsigned long *delay_off)
 {
+	if (led_is_flash_mode(led_cdev))
+		return;
+
 	del_timer_sync(&led_cdev->blink_timer);
 
 	led_cdev->flags &= ~LED_BLINK_ONESHOT;
@@ -89,6 +109,9 @@  void led_blink_set_oneshot(struct led_classdev *led_cdev,
 			   unsigned long *delay_off,
 			   int invert)
 {
+	if (led_is_flash_mode(led_cdev))
+		return;
+
 	if ((led_cdev->flags & LED_BLINK_ONESHOT) &&
 	     timer_pending(&led_cdev->blink_timer))
 		return;
@@ -116,13 +139,100 @@  EXPORT_SYMBOL_GPL(led_stop_software_blink);
 void led_set_brightness(struct led_classdev *led_cdev,
 			enum led_brightness brightness)
 {
-	/* delay brightness setting if need to stop soft-blink timer */
-	if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
-		led_cdev->delayed_set_value = brightness;
-		schedule_work(&led_cdev->set_brightness_work);
-		return;
+	struct led_flash *flash = led_cdev->flash;
+	int ret;
+
+	if (led_is_flash_mode(led_cdev)) {
+		if (brightness > 0) {
+			ret = led_flash_strobe_set(led_cdev,
+					brightness,
+					&flash->timeout);
+			if (ret < 0)
+				dev_err(led_cdev->dev,
+					"Failed to setup flash strobe (%d)",
+					ret);
+		} else {
+			__led_set_brightness(led_cdev, 0);
+		}
+	} else {
+		/* delay brightness setting if need to stop soft-blink timer */
+		if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
+			led_cdev->delayed_set_value = brightness;
+			schedule_work(&led_cdev->set_brightness_work);
+			return;
+		}
+		__led_set_brightness(led_cdev, brightness);
 	}
-
-	__led_set_brightness(led_cdev, brightness);
 }
 EXPORT_SYMBOL(led_set_brightness);
+
+int led_set_flash_mode(struct led_classdev *led_cdev,
+			bool flash_mode)
+{
+	if (!get_flash_op(led_cdev, mode_set))
+		return -EINVAL;
+
+	call_flash_op(mode_set, led_cdev, flash_mode);
+
+	if (flash_mode)
+		led_cdev->flags |= LED_MODE_FLASH;
+	else
+		led_cdev->flags &= ~LED_MODE_FLASH;
+
+	led_set_brightness(led_cdev, 0);
+
+	return 0;
+}
+EXPORT_SYMBOL(led_set_flash_mode);
+
+/* Caller must ensure led_cdev->led_lock held */
+void led_sysfs_lock(struct led_classdev *led_cdev)
+{
+	led_cdev->flags |= LED_SYSFS_LOCK;
+}
+EXPORT_SYMBOL(led_sysfs_lock);
+
+/* Caller must ensure led_cdev->led_lock held */
+void led_sysfs_unlock(struct led_classdev *led_cdev)
+{
+	led_cdev->flags &= ~LED_SYSFS_LOCK;
+}
+EXPORT_SYMBOL(led_sysfs_unlock);
+
+void led_set_flash_timeout(struct led_classdev *led_cdev, unsigned long timeout)
+{
+	struct led_flash *flash = led_cdev->flash;
+
+	/*
+	 * Flash timeout is passed to a flash LED driver
+	 * through the strobe_set callback - here we only
+	 * cache the value.
+	 */
+	if (timeout > flash->max_timeout)
+		flash->timeout = flash->max_timeout;
+	else
+		flash->timeout = timeout;
+}
+EXPORT_SYMBOL(led_set_flash_timeout);
+
+int led_get_flash_fault(struct led_classdev *led_cdev, unsigned int *fault)
+{
+	if (!get_flash_op(led_cdev, fault_get))
+		return -EINVAL;
+
+	return call_flash_op(fault_get, led_cdev, fault);
+}
+EXPORT_SYMBOL(led_get_flash_fault);
+
+int led_set_hw_triggered(struct led_classdev *led_cdev, bool enable)
+{
+	if (led_cdev->has_hw_trig) {
+		__led_set_brightness(led_cdev, 0);
+		led_cdev->hw_triggered = enable;
+	} else if (enable) {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(led_set_hw_triggered);
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index df1a7c1..448e7c8 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -37,6 +37,15 @@  ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
 	char trigger_name[TRIG_NAME_MAX];
 	struct led_trigger *trig;
 	size_t len;
+	int ret = count;
+
+	mutex_lock(&led_cdev->led_lock);
+
+	if (led_sysfs_is_locked(led_cdev) ||
+	    led_is_flash_mode(led_cdev)) {
+		ret = -EBUSY;
+		goto exit_unlock;
+	}
 
 	trigger_name[sizeof(trigger_name) - 1] = '\0';
 	strncpy(trigger_name, buf, sizeof(trigger_name) - 1);
@@ -47,7 +56,7 @@  ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
 
 	if (!strcmp(trigger_name, "none")) {
 		led_trigger_remove(led_cdev);
-		return count;
+		goto exit_unlock;
 	}
 
 	down_read(&triggers_list_lock);
@@ -58,12 +67,14 @@  ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr,
 			up_write(&led_cdev->trigger_lock);
 
 			up_read(&triggers_list_lock);
-			return count;
+			goto exit_unlock;
 		}
 	}
 	up_read(&triggers_list_lock);
 
-	return -EINVAL;
+exit_unlock:
+	mutex_unlock(&led_cdev->led_lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(led_trigger_store);
 
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 4c50365..44cc384 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -17,6 +17,15 @@ 
 #include <linux/rwsem.h>
 #include <linux/leds.h>
 
+
+#define call_flash_op(op, args...)		\
+	((led_cdev)->flash->ops.op(args))
+
+#define get_flash_op(led_cdev, op)		\
+	((led_cdev)->flash ?			\
+		(led_cdev)->flash->ops.op :	\
+		0)
+
 static inline void __led_set_brightness(struct led_classdev *led_cdev,
 					enum led_brightness value)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 0287ab2..1bf0ab3 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -17,6 +17,14 @@ 
 #include <linux/rwsem.h>
 #include <linux/timer.h>
 #include <linux/workqueue.h>
+#include <linux/mutex.h>
+#include <media/v4l2-device.h>
+
+#define LED_FAULT_OVER_VOLTAGE		(1 << 0)
+#define LED_FAULT_TIMEOUT		(1 << 1)
+#define LED_FAULT_OVER_TEMPERATURE	(1 << 2)
+#define LED_FAULT_SHORT_CIRCUIT		(1 << 3)
+#define LED_FAULT_OVER_CURRENT		(1 << 4)
 
 struct device;
 /*
@@ -29,6 +37,33 @@  enum led_brightness {
 	LED_FULL	= 255,
 };
 
+struct led_classdev;
+
+struct led_flash_ops {
+	/* Set torch/flash LED mode */
+	void	(*mode_set)(struct led_classdev *led_cdev,
+					bool flash_mode);
+	/* Setup flash strobe */
+	void	(*strobe_set)(struct led_classdev *led_cdev,
+					enum led_brightness brightness,
+					unsigned long *timeout);
+	/* Get the flash LED fault */
+	int	(*fault_get)(struct led_classdev *led_cdev,
+					unsigned int *fault);
+};
+
+struct led_flash {
+	/* flash led specific ops */
+	struct led_flash_ops	ops;
+	/*
+	 * maximum allowed flash timeout - it is read only and
+	 * should be initialized by the driver
+	 */
+	unsigned long		max_timeout;
+	/* current flash timeout */
+	unsigned long		timeout;
+};
+
 struct led_classdev {
 	const char		*name;
 	int			 brightness;
@@ -42,6 +77,8 @@  struct led_classdev {
 #define LED_BLINK_ONESHOT	(1 << 17)
 #define LED_BLINK_ONESHOT_STOP	(1 << 18)
 #define LED_BLINK_INVERT	(1 << 19)
+#define LED_MODE_FLASH		(1 << 20)
+#define LED_SYSFS_LOCK		(1 << 21)
 
 	/* Set LED brightness level */
 	/* Must not sleep, use a workqueue if needed */
@@ -69,6 +106,19 @@  struct led_classdev {
 	unsigned long		 blink_delay_on, blink_delay_off;
 	struct timer_list	 blink_timer;
 	int			 blink_brightness;
+	struct led_flash	*flash;
+	/*
+	 * Determines whether a device supports triggering a flash led
+	 * with use of a dedicated hardware pin
+	 */
+	bool			has_hw_trig;
+	/* If true then hardware pin triggers flash strobe */
+	bool			hw_triggered;
+	/*
+	 * Ensures consistent LED sysfs access and protects
+	 * LED sysfs locking mechanism
+	 */
+	struct mutex		led_lock;
 
 	struct work_struct	set_brightness_work;
 	int			delayed_set_value;
@@ -90,6 +140,7 @@  extern int led_classdev_register(struct device *parent,
 extern void led_classdev_unregister(struct led_classdev *led_cdev);
 extern void led_classdev_suspend(struct led_classdev *led_cdev);
 extern void led_classdev_resume(struct led_classdev *led_cdev);
+extern int led_classdev_init_flash(struct led_classdev *led_cdev);
 
 /**
  * led_blink_set - set blinking with software fallback
@@ -138,6 +189,91 @@  extern void led_blink_set_oneshot(struct led_classdev *led_cdev,
  */
 extern void led_set_brightness(struct led_classdev *led_cdev,
 			       enum led_brightness brightness);
+/**
+/**
+ * led_set_flash_mode - set LED flash mode
+ * @led_cdev: the LED to set
+ * @flash_mode: true - flash mode, false - torch mode
+ *
+ * Returns: 0 on success, -EINVAL on failure
+ *
+ * Switch an LED's flash mode and set brightness to 0.
+ */
+extern int led_set_flash_mode(struct led_classdev *led_cdev,
+			      bool flash_mode);
+
+/**
+ * led_set_flash_timeout - set flash LED timeout
+ * @led_cdev: the LED to set
+ * @timeout: the flash timeout to be set
+ *
+ * Set the flash strobe duration.
+ */
+extern void led_set_flash_timeout(struct led_classdev *led_cdev,
+				  unsigned long timeout);
+
+/**
+ * led_get_flash_fault - get the flash LED fault
+ * @led_cdev: the LED to query
+ * @fault: bitmask containing flash faults
+ *
+ * Returns: 0 on success, -EINVAL on failure
+ *
+ * Get the flash LED fault.
+ */
+extern int led_get_flash_fault(struct led_classdev *led_cdev,
+			       unsigned int *fault);
+
+/**
+ * led_set_hw_triggered - set the flash LED hw_triggered mode
+ * @led_cdev: the LED to set
+ * @enable: the state to set
+ *
+ * Returns: 0 on success, -EINVAL on failure
+ *
+ * Enable/disable triggering the flash LED via hardware pin
+ */
+extern int led_set_hw_triggered(struct led_classdev *led_cdev, bool enable);
+
+/**
+ * led_sysfs_lock - lock LED sysfs interface
+ * @led_cdev: the LED to set
+ *
+ * Lock the LED's sysfs interface
+ */
+extern void led_sysfs_lock(struct led_classdev *led_cdev);
+
+/**
+ * led_sysfs_unlock - unlock LED sysfs interface
+ * @led_cdev: the LED to set
+ *
+ * Unlock the LED's sysfs interface
+ */
+extern void led_sysfs_unlock(struct led_classdev *led_cdev);
+
+/**
+ * led_is_flash_mode
+ * @led_cdev: the LED query
+ *
+ * Returns: true if a led device is in the flash mode, false if it is
+	    is in the torch mode
+ */
+static inline bool led_is_flash_mode(struct led_classdev *led_cdev)
+{
+	return !!(led_cdev->flags & LED_MODE_FLASH);
+}
+
+/**
+ * led_sysfs_is_locked
+ * @led_cdev: the LED query about
+ *
+ * Returns: true if the sysfs interface of the led is disabled,
+ *	    false otherwise
+ */
+static inline bool led_sysfs_is_locked(struct led_classdev *led_cdev)
+{
+	return !!(led_cdev->flags & LED_SYSFS_LOCK);
+}
 
 /*
  * LED Triggers