Re: [PATCH/RFC v3 1/5] leds: Add sysfs and kernel internal API for flash LEDs
Hi Bryan, Thanks for the review. On 04/26/2014 01:17 AM, Bryan Wu wrote: On Fri, Apr 11, 2014 at 7:56 AM, Jacek Anaszewski j.anaszew...@samsung.com wrote: Some LED devices support two operation modes - torch and flash. Do we have a method to look up the capabilities from LED devices driver? For example, the LED device supports Torch/Flash then LED device driver should set a flag like LED_DEV_CAP_TORCH or LED_DEV_CAP_FLASH. If it doesn't support those functions, it won't set those flags. It is assumed that torch led is always available. For declaring the existence of the flash led there is 'has_flash_led' flag in the struct led_flash. LED Flash class core can check those flags for further usage. 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_brightness, flash_strobe, flash_timeout, max_flash_timeout, max_flash_brightness, flash_fault and optional external_strobe, indicator_brightness and max_indicator_btightness. All the flash related features typo here, it should max_indicator_btightness - max_indicator_brightness are placed in a separate module. Please add one empty line here. The modifications aim to be compatible with V4L2 framework requirements related to the flash devices management. The design assumes that V4L2 sub-device can take of the LED class device control and communicate with it through the kernel internal interface. When V4L2 Flash sub-device file is opened, the LED class device sysfs interface is made unavailable. I don't quite understand the last sentence here. Looks like the LED flash class interface binds to V4L2 Flash sub-device, then why we need to export sysfs for user space if the only user is V4L2 which can talk through kernel internal API here. It has been agreed that the two types of interfaces should be available for the users for operating on LED flash devices. Currently on open the V4L2 flash sub-device sets the lock flag to disable LED sysfs interface which was exported when the LED device was created. Do you suggest that attributes should be removed each time V4L2 takes control of the LED flash device and re-created after the device is released? Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com Acked-by: Kyungmin Park kyungmin.p...@samsung.com Cc: Bryan Wu coolo...@gmail.com Cc: Richard Purdie rpur...@rpsys.net --- drivers/leds/Kconfig|8 + drivers/leds/Makefile |1 + drivers/leds/led-class.c| 36 ++- drivers/leds/led-flash.c| 627 +++ If we go for the LED Flash class, I prefer to use led-class-flash.c rather than led-flash.c OK. drivers/leds/led-triggers.c | 16 +- drivers/leds/leds.h |6 + include/linux/leds.h| 50 +++- include/linux/leds_flash.h | 252 + leds_flash.h - led-class-flash.h 8 files changed, 982 insertions(+), 14 deletions(-) create mode 100644 drivers/leds/led-flash.c create mode 100644 include/linux/leds_flash.h diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 2062682..1e1c81f 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -19,6 +19,14 @@ config LEDS_CLASS This option enables the led sysfs class in /sys/class/leds. You'll need this to do anything useful with LEDs. If unsure, say N. +config LEDS_CLASS_FLASH + tristate Flash LEDs Support LED Flash Class Support + depends on LEDS_CLASS + help + This option enables support for flash LED devices. Say Y if you + want to use flash specific features of a LED device, if they + are supported. + This help message is not very accurate, please take a look at LEDS_CLASS. And I prefer this driver can be a module, so it should be mentioned here. Doesn't 'tristate' property suffice for indicating that the driver can be a module? comment LED drivers config LEDS_88PM860X diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 3cd76db..8861b86 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -2,6 +2,7 @@ # LED Core obj-$(CONFIG_NEW_LEDS) += led-core.o obj-$(CONFIG_LEDS_CLASS) += led-class.o +obj-$(CONFIG_LEDS_CLASS_FLASH) += led-flash.o obj-$(CONFIG_LEDS_TRIGGERS)+= led-triggers.o # LED Platform Drivers diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index f37d63c..58f16c3 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -9,15 +9,16 @@ * published by the Free Software Foundation. */ -#include linux/module.h -#include linux/kernel.h +#include linux/ctype.h +#include linux/device.h +#include linux/err.h #include linux/init.h +#include linux/kernel.h #include linux/list.h +#include linux/module.h +#include linux/slab.h #include linux/spinlock.h -#include linux/device.h #include
Re: [PATCH/RFC v3 1/5] leds: Add sysfs and kernel internal API for flash LEDs
On Fri, Apr 11, 2014 at 7:56 AM, Jacek Anaszewski
j.anaszew...@samsung.com wrote:
Some LED devices support two operation modes - torch and
flash.
Do we have a method to look up the capabilities from LED devices driver?
For example, the LED device supports Torch/Flash then LED device
driver should set a flag like LED_DEV_CAP_TORCH or LED_DEV_CAP_FLASH.
If it doesn't support those functions, it won't set those flags.
LED Flash class core can check those flags for further usage.
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_brightness, flash_strobe, flash_timeout,
max_flash_timeout, max_flash_brightness, flash_fault and
optional external_strobe, indicator_brightness and
max_indicator_btightness. All the flash related features
typo here, it should max_indicator_btightness - max_indicator_brightness
are placed in a separate module.
Please add one empty line here.
The modifications aim to be compatible with V4L2 framework
requirements related to the flash devices management. The
design assumes that V4L2 sub-device can take of the LED class
device control and communicate with it through the kernel
internal interface. When V4L2 Flash sub-device file is
opened, the LED class device sysfs interface is made
unavailable.
I don't quite understand the last sentence here. Looks like the LED
flash class interface binds to V4L2 Flash sub-device, then why we need
to export sysfs for user space if the only user is V4L2 which can talk
through kernel internal API here.
Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Bryan Wu coolo...@gmail.com
Cc: Richard Purdie rpur...@rpsys.net
---
drivers/leds/Kconfig|8 +
drivers/leds/Makefile |1 +
drivers/leds/led-class.c| 36 ++-
drivers/leds/led-flash.c| 627
+++
If we go for the LED Flash class, I prefer to use led-class-flash.c
rather than led-flash.c
drivers/leds/led-triggers.c | 16 +-
drivers/leds/leds.h |6 +
include/linux/leds.h| 50 +++-
include/linux/leds_flash.h | 252 +
leds_flash.h - led-class-flash.h
8 files changed, 982 insertions(+), 14 deletions(-)
create mode 100644 drivers/leds/led-flash.c
create mode 100644 include/linux/leds_flash.h
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2062682..1e1c81f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -19,6 +19,14 @@ config LEDS_CLASS
This option enables the led sysfs class in /sys/class/leds. You'll
need this to do anything useful with LEDs. If unsure, say N.
+config LEDS_CLASS_FLASH
+ tristate Flash LEDs Support
LED Flash Class Support
+ depends on LEDS_CLASS
+ help
+ This option enables support for flash LED devices. Say Y if you
+ want to use flash specific features of a LED device, if they
+ are supported.
+
This help message is not very accurate, please take a look at
LEDS_CLASS. And I prefer this driver can be a module, so it should be
mentioned here.
comment LED drivers
config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3cd76db..8861b86 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -2,6 +2,7 @@
# LED Core
obj-$(CONFIG_NEW_LEDS) += led-core.o
obj-$(CONFIG_LEDS_CLASS) += led-class.o
+obj-$(CONFIG_LEDS_CLASS_FLASH) += led-flash.o
obj-$(CONFIG_LEDS_TRIGGERS)+= led-triggers.o
# LED Platform Drivers
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f37d63c..58f16c3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -9,15 +9,16 @@
* published by the Free Software Foundation.
*/
-#include linux/module.h
-#include linux/kernel.h
+#include linux/ctype.h
+#include linux/device.h
+#include linux/err.h
#include linux/init.h
+#include linux/kernel.h
#include linux/list.h
+#include linux/module.h
+#include linux/slab.h
#include linux/spinlock.h
-#include linux/device.h
#include linux/timer.h
-#include linux/err.h
-#include linux/ctype.h
#include linux/leds.h
#include leds.h
I believe this change is kind of cleanup, could you please split them
out? For this patch we just need add those LED Flash class related
code.
@@ -45,28 +46,38 @@ 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 unlock;
+ }
ret = kstrtoul(buf, 10, state);
if (ret)
-
Re: [PATCH/RFC v3 1/5] leds: Add sysfs and kernel internal API for flash LEDs
Hi Jacek,
Thanks for the update! Some comments below. I'll try to reply to the rest
during the coming days.
On Fri, Apr 11, 2014 at 04:56:52PM +0200, 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_brightness, flash_strobe, flash_timeout,
max_flash_timeout, max_flash_brightness, flash_fault and
optional external_strobe, indicator_brightness and
max_indicator_btightness. All the flash related features
are placed in a separate module.
The modifications aim to be compatible with V4L2 framework
requirements related to the flash devices management. The
design assumes that V4L2 sub-device can take of the LED class
device control and communicate with it through the kernel
internal interface. When V4L2 Flash sub-device file is
opened, the LED class device sysfs interface is made
unavailable.
Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Bryan Wu coolo...@gmail.com
Cc: Richard Purdie rpur...@rpsys.net
---
drivers/leds/Kconfig|8 +
drivers/leds/Makefile |1 +
drivers/leds/led-class.c| 36 ++-
drivers/leds/led-flash.c| 627
+++
drivers/leds/led-triggers.c | 16 +-
drivers/leds/leds.h |6 +
include/linux/leds.h| 50 +++-
include/linux/leds_flash.h | 252 +
8 files changed, 982 insertions(+), 14 deletions(-)
create mode 100644 drivers/leds/led-flash.c
create mode 100644 include/linux/leds_flash.h
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2062682..1e1c81f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -19,6 +19,14 @@ config LEDS_CLASS
This option enables the led sysfs class in /sys/class/leds. You'll
need this to do anything useful with LEDs. If unsure, say N.
+config LEDS_CLASS_FLASH
+ tristate Flash LEDs Support
+ depends on LEDS_CLASS
+ help
+ This option enables support for flash LED devices. Say Y if you
+ want to use flash specific features of a LED device, if they
+ are supported.
+
comment LED drivers
config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3cd76db..8861b86 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -2,6 +2,7 @@
# LED Core
obj-$(CONFIG_NEW_LEDS) += led-core.o
obj-$(CONFIG_LEDS_CLASS) += led-class.o
+obj-$(CONFIG_LEDS_CLASS_FLASH) += led-flash.o
obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
# LED Platform Drivers
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f37d63c..58f16c3 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -9,15 +9,16 @@
* published by the Free Software Foundation.
*/
-#include linux/module.h
-#include linux/kernel.h
+#include linux/ctype.h
+#include linux/device.h
+#include linux/err.h
#include linux/init.h
+#include linux/kernel.h
#include linux/list.h
+#include linux/module.h
+#include linux/slab.h
#include linux/spinlock.h
-#include linux/device.h
#include linux/timer.h
-#include linux/err.h
-#include linux/ctype.h
#include linux/leds.h
#include leds.h
@@ -45,28 +46,38 @@ 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 unlock;
+ }
ret = kstrtoul(buf, 10, state);
if (ret)
- return ret;
+ goto unlock;
if (state == LED_OFF)
led_trigger_remove(led_cdev);
__led_set_brightness(led_cdev, state);
+ ret = size;
- return size;
+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);
#ifdef CONFIG_LEDS_TRIGGERS
static DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
@@ -174,6 +185,8 @@ EXPORT_SYMBOL_GPL(led_classdev_suspend);
void led_classdev_resume(struct led_classdev *led_cdev)
{
led_cdev-brightness_set(led_cdev, led_cdev-brightness);
+ if
