Message-ID: <20140323231833.GA2054@valkosipuli.retiisi.org.uk>
Date:	Mon, 24 Mar 2014 01:18:33 +0200
From:	Sakari Ailus <sakari.ailus@....fi>
To:	Jacek Anaszewski <j.anaszewski@...sung.com>
Cc:	linux-media@...r.kernel.org, linux-leds@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	s.nawrocki@...sung.com, a.hajda@...sung.com,
	kyungmin.park@...sung.com, Bryan Wu <cooloney@...il.com>,
	Richard Purdie <rpurdie@...ys.net>
Subject: Re: [PATCH/RFC 1/8] leds: Add sysfs and kernel internal API for
 flash LEDs

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 <j.anaszewski@...sung.com>
> Acked-by: Kyungmin Park <kyungmin.park@...sung.com>
> Cc: Bryan Wu <cooloney@...il.com>
> Cc: Richard Purdie <rpurdie@...ys.net>
> ---
>  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 <lenz@...wisc.edu>
>   * Copyright (C) 2005-2007 Richard Purdie <rpurdie@...nedhand.com>
>   *
> + * Copyright (C) 2014 Samsung Electronics Co., Ltd.
> + * Author: Jacek Anaszewski <j.anaszewski@...sung.com>
> + *
>   * 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 <j.anaszewski@...sung.com>");
>  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 <rpurdie@...nedhand.com>
>   *
> + * Copyright (C) 2014 Samsung Electronics Co., Ltd.
> + * Author: Jacek Anaszewski <j.anaszewski@...sung.com>
> + *
>   * 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 <gshark.jeong@...il.com>
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 <gshark.jeong@...il.com>
    Signed-off-by: Sakari Ailus <sakari.ailus@....fi>
    Signed-off-by: Mauro Carvalho Chehab <m.chehab@...sung.com>

>  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

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@....fi	XMPP: sailus@...iisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists