[PATCH -next 0/2] leds: add substitutes for /sys/class/leds/<led>/trigger

[PATCH -next 0/2] leds: add substitutes for /sys/class/leds/<led>/trigger

[Akinobu Mita]

Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
However, this violates the "one value per file" rule of sysfs.

This series provides a new /sys/devices/virtual/led-trigger/ directory and
/sys/class/leds/<led>/current-trigger. The new api follows the "one value
per file" rule of sysfs.

This series was previously developed as a part of the series "leds: fix
/sys/class/leds/<led>/trigger and add new api" [1].  Now this version
only contains the new api part.

[1] https://lore.kernel.org/r/1567946472-10075-1-git-send-email-akinobu.mita@gmail.com

Akinobu Mita (2):
  leds: add /sys/devices/virtual/led-trigger/
  leds: add /sys/class/leds/<led>/current-trigger

 Documentation/ABI/testing/sysfs-class-led          | 13 +++
 .../ABI/testing/sysfs-devices-virtual-led-trigger  |  8 ++
 drivers/leds/led-class.c                           | 10 +++
 drivers/leds/led-triggers.c                        | 95 +++++++++++++++++++++-
 drivers/leds/leds.h                                |  5 ++
 include/linux/leds.h                               |  3 +
 6 files changed, 130 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-virtual-led-trigger

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
-- 
2.7.4

[PATCH -next 1/2] leds: add /sys/devices/virtual/led-trigger/

[Akinobu Mita]

Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
However, this violates the "one value per file" rule of sysfs.

This makes led_triggers "real" devices and provides an
/sys/devices/virtual/led-trigger/ directory that contains a sub-directoriy
for each LED trigger device. The name of the sub-directory matches the LED
trigger name.

We can find all available LED triggers by listing this directory contents.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 .../ABI/testing/sysfs-devices-virtual-led-trigger  |  8 +++
 drivers/leds/led-triggers.c                        | 57 ++++++++++++++++++++++
 include/linux/leds.h                               |  3 ++
 3 files changed, 68 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-virtual-led-trigger

diff --git a/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger b/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger
new file mode 100644
index 0000000..b8eb8f3
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger
@@ -0,0 +1,8 @@
+What:		/sys/devices/virtual/leds-trigger/
+Date:		September 2019
+KernelVersion:	5.5
+Contact:	linux-leds@vger.kernel.org
+Description:
+		This directory contains a sub-directoriy for each LED trigger
+		device. The name of the sub-directory matches the LED trigger
+		name.
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 79e30d2..0b810cf 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -267,21 +267,76 @@ void led_trigger_rename_static(const char *name, struct led_trigger *trig)
 }
 EXPORT_SYMBOL_GPL(led_trigger_rename_static);
 
+struct ledtrig_device {
+	struct device dev;
+};
+
+static void ledtrig_device_release(struct device *dev)
+{
+	struct ledtrig_device *trig_dev =
+		container_of(dev, struct ledtrig_device, dev);
+
+	kfree(trig_dev);
+}
+
+static struct bus_type led_trigger_subsys = {
+	.name = "led-trigger",
+};
+
+static int led_trigger_subsys_init(void)
+{
+	static DEFINE_MUTEX(init_mutex);
+	static bool init_done;
+	int ret = 0;
+
+	mutex_lock(&init_mutex);
+	if (!init_done) {
+		ret = subsys_virtual_register(&led_trigger_subsys, NULL);
+		if (!ret)
+			init_done = true;
+	}
+	mutex_unlock(&init_mutex);
+
+	return ret;
+}
+
 /* LED Trigger Interface */
 
 int led_trigger_register(struct led_trigger *trig)
 {
 	struct led_classdev *led_cdev;
 	struct led_trigger *_trig;
+	struct ledtrig_device *trig_dev;
+	int ret;
 
 	rwlock_init(&trig->leddev_list_lock);
 	INIT_LIST_HEAD(&trig->led_cdevs);
 
+	ret = led_trigger_subsys_init();
+	if (ret)
+		return ret;
+	trig_dev = kzalloc(sizeof(*trig_dev), GFP_KERNEL);
+	if (!trig_dev)
+		return -ENOMEM;
+
+	trig_dev->dev.bus = &led_trigger_subsys;
+	trig_dev->dev.release = ledtrig_device_release;
+	dev_set_name(&trig_dev->dev, "%s", trig->name);
+
+	ret = device_register(&trig_dev->dev);
+	if (ret) {
+		put_device(&trig_dev->dev);
+		return ret;
+	}
+
+	trig->trig_dev = trig_dev;
+
 	down_write(&triggers_list_lock);
 	/* Make sure the trigger's name isn't already in use */
 	list_for_each_entry(_trig, &trigger_list, next_trig) {
 		if (!strcmp(_trig->name, trig->name)) {
 			up_write(&triggers_list_lock);
+			device_unregister(&trig_dev->dev);
 			return -EEXIST;
 		}
 	}
@@ -327,6 +382,8 @@ void led_trigger_unregister(struct led_trigger *trig)
 		up_write(&led_cdev->trigger_lock);
 	}
 	up_read(&leds_list_lock);
+
+	device_unregister(&trig->trig_dev->dev);
 }
 EXPORT_SYMBOL_GPL(led_trigger_unregister);
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index da78b27..d63c8e7 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -336,6 +336,8 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
 
 #define TRIG_NAME_MAX 50
 
+struct ledtrig_device;
+
 struct led_trigger {
 	/* Trigger Properties */
 	const char	 *name;
@@ -350,6 +352,7 @@ struct led_trigger {
 	struct list_head  next_trig;
 
 	const struct attribute_group **groups;
+	struct ledtrig_device *trig_dev;
 };
 
 /*
-- 
2.7.4

Re: [PATCH -next 1/2] leds: add /sys/devices/virtual/led-trigger/

[Greg Kroah-Hartman]

On Thu, Oct 03, 2019 at 12:13:00AM +0900, Akinobu Mita wrote:
> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, this violates the "one value per file" rule of sysfs.
> 
> This makes led_triggers "real" devices and provides an
> /sys/devices/virtual/led-trigger/ directory that contains a sub-directoriy
> for each LED trigger device. The name of the sub-directory matches the LED
> trigger name.
> 
> We can find all available LED triggers by listing this directory contents.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  .../ABI/testing/sysfs-devices-virtual-led-trigger  |  8 +++
>  drivers/leds/led-triggers.c                        | 57 ++++++++++++++++++++++
>  include/linux/leds.h                               |  3 ++
>  3 files changed, 68 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-virtual-led-trigger
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger b/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger
> new file mode 100644
> index 0000000..b8eb8f3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger
> @@ -0,0 +1,8 @@
> +What:		/sys/devices/virtual/leds-trigger/
> +Date:		September 2019
> +KernelVersion:	5.5
> +Contact:	linux-leds@vger.kernel.org
> +Description:
> +		This directory contains a sub-directoriy for each LED trigger
> +		device. The name of the sub-directory matches the LED trigger
> +		name.
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 79e30d2..0b810cf 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -267,21 +267,76 @@ void led_trigger_rename_static(const char *name, struct led_trigger *trig)
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_rename_static);
>  
> +struct ledtrig_device {
> +	struct device dev;
> +};
> +
> +static void ledtrig_device_release(struct device *dev)
> +{
> +	struct ledtrig_device *trig_dev =
> +		container_of(dev, struct ledtrig_device, dev);
> +
> +	kfree(trig_dev);
> +}
> +
> +static struct bus_type led_trigger_subsys = {
> +	.name = "led-trigger",
> +};
> +
> +static int led_trigger_subsys_init(void)
> +{
> +	static DEFINE_MUTEX(init_mutex);
> +	static bool init_done;
> +	int ret = 0;
> +
> +	mutex_lock(&init_mutex);
> +	if (!init_done) {

a test and set of an atomic would solve the issue of having both a mutex
and a boolean, right?

thanks,

greg k-h

Re: [PATCH -next 1/2] leds: add /sys/devices/virtual/led-trigger/

[Greg Kroah-Hartman]

On Thu, Oct 03, 2019 at 12:13:00AM +0900, Akinobu Mita wrote:
> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, this violates the "one value per file" rule of sysfs.
> 
> This makes led_triggers "real" devices and provides an
> /sys/devices/virtual/led-trigger/ directory that contains a sub-directoriy
> for each LED trigger device. The name of the sub-directory matches the LED
> trigger name.
> 
> We can find all available LED triggers by listing this directory contents.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  .../ABI/testing/sysfs-devices-virtual-led-trigger  |  8 +++
>  drivers/leds/led-triggers.c                        | 57 ++++++++++++++++++++++
>  include/linux/leds.h                               |  3 ++
>  3 files changed, 68 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-virtual-led-trigger
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger b/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger
> new file mode 100644
> index 0000000..b8eb8f3
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-virtual-led-trigger
> @@ -0,0 +1,8 @@
> +What:		/sys/devices/virtual/leds-trigger/
> +Date:		September 2019
> +KernelVersion:	5.5
> +Contact:	linux-leds@vger.kernel.org
> +Description:
> +		This directory contains a sub-directoriy for each LED trigger

"directoriy"?

> +		device. The name of the sub-directory matches the LED trigger
> +		name.

You are just creating directories here, and doing nothing with them,
why?  That seems kind of pointless.

thanks,

greg k-h

[PATCH -next 2/2] leds: add /sys/class/leds/<led>/current-trigger

[Akinobu Mita]

Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
However, this violates the "one value per file" rule of sysfs.

This provides /sys/class/leds/<led>/current-trigger which is almost
identical to /sys/class/leds/<led>/trigger.  The only difference is that
'current-trigger' only shows the current trigger name.

This new file follows the "one value per file" rule of sysfs.
We can find all available LED triggers by listing the
/sys/devices/virtual/led-trigger/ directory.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 Documentation/ABI/testing/sysfs-class-led | 13 +++++++++++
 drivers/leds/led-class.c                  | 10 ++++++++
 drivers/leds/led-triggers.c               | 38 +++++++++++++++++++++++++++----
 drivers/leds/leds.h                       |  5 ++++
 4 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..fdfed3f 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,16 @@ Description:
 		gpio and backlight triggers. In case of the backlight trigger,
 		it is useful when driving a LED which is intended to indicate
 		a device in a standby like state.
+
+What:		/sys/class/leds/<led>/current-trigger
+Date:		September 2019
+KernelVersion:	5.5
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Set the trigger for this LED. A trigger is a kernel based source
+		of LED events.
+		Writing the trigger name to this file will change the current
+		trigger. Trigger specific parameters can appear in
+		/sys/class/leds/<led> once a given trigger is selected. For
+		their documentation see sysfs-class-led-trigger-*.
+		Reading this file will return the current LED trigger name.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3f04334..3cb0d8a 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,12 +74,22 @@ static ssize_t max_brightness_show(struct device *dev,
 static DEVICE_ATTR_RO(max_brightness);
 
 #ifdef CONFIG_LEDS_TRIGGERS
+
+static DEVICE_ATTR(current_trigger, 0644, led_current_trigger_show,
+		   led_current_trigger_store);
+
+static struct attribute *led_current_trigger_attrs[] = {
+	&dev_attr_current_trigger.attr,
+	NULL,
+};
+
 static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write, 0);
 static struct bin_attribute *led_trigger_bin_attrs[] = {
 	&bin_attr_trigger,
 	NULL,
 };
 static const struct attribute_group led_trigger_group = {
+	.attrs = led_current_trigger_attrs,
 	.bin_attrs = led_trigger_bin_attrs,
 };
 #endif
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 0b810cf..a2ef674 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -27,11 +27,9 @@ LIST_HEAD(trigger_list);
 
  /* Used by LED Class */
 
-ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
-			  struct bin_attribute *bin_attr, char *buf,
-			  loff_t pos, size_t count)
+static ssize_t led_trigger_store(struct device *dev, const char *buf,
+				 size_t count)
 {
-	struct device *dev = kobj_to_dev(kobj);
 	struct led_classdev *led_cdev = dev_get_drvdata(dev);
 	struct led_trigger *trig;
 	int ret = count;
@@ -67,8 +65,25 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 	mutex_unlock(&led_cdev->led_access);
 	return ret;
 }
+
+ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
+			  struct bin_attribute *bin_attr, char *buf,
+			  loff_t pos, size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+
+	return led_trigger_store(dev, buf, count);
+}
 EXPORT_SYMBOL_GPL(led_trigger_write);
 
+ssize_t led_current_trigger_store(struct device *dev,
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	return led_trigger_store(dev, buf, count);
+}
+EXPORT_SYMBOL_GPL(led_current_trigger_store);
+
 __printf(3, 4)
 static int led_trigger_snprintf(char *buf, ssize_t size, const char *fmt, ...)
 {
@@ -144,6 +159,21 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
 }
 EXPORT_SYMBOL_GPL(led_trigger_read);
 
+ssize_t led_current_trigger_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	int len;
+
+	down_read(&led_cdev->trigger_lock);
+	len = scnprintf(buf, PAGE_SIZE, "%s\n", led_cdev->trigger ?
+			led_cdev->trigger->name : "none");
+	up_read(&led_cdev->trigger_lock);
+
+	return len;
+}
+EXPORT_SYMBOL_GPL(led_current_trigger_show);
+
 /* Caller must ensure led_cdev->trigger_lock held */
 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 {
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 2d9eb48..c581690 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -29,6 +29,11 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
 ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
 			struct bin_attribute *bin_attr, char *buf,
 			loff_t pos, size_t count);
+ssize_t led_current_trigger_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count);
+ssize_t led_current_trigger_show(struct device *dev,
+				struct device_attribute *attr, char *buf);
 
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
-- 
2.7.4

Re: [PATCH -next 2/2] leds: add /sys/class/leds/<led>/current-trigger

[Greg Kroah-Hartman]

On Thu, Oct 03, 2019 at 12:13:01AM +0900, Akinobu Mita wrote:
> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, this violates the "one value per file" rule of sysfs.
> 
> This provides /sys/class/leds/<led>/current-trigger which is almost
> identical to /sys/class/leds/<led>/trigger.  The only difference is that
> 'current-trigger' only shows the current trigger name.
> 
> This new file follows the "one value per file" rule of sysfs.
> We can find all available LED triggers by listing the
> /sys/devices/virtual/led-trigger/ directory.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-class-led | 13 +++++++++++
>  drivers/leds/led-class.c                  | 10 ++++++++
>  drivers/leds/led-triggers.c               | 38 +++++++++++++++++++++++++++----
>  drivers/leds/leds.h                       |  5 ++++
>  4 files changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..fdfed3f 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,16 @@ Description:
>  		gpio and backlight triggers. In case of the backlight trigger,
>  		it is useful when driving a LED which is intended to indicate
>  		a device in a standby like state.
> +
> +What:		/sys/class/leds/<led>/current-trigger
> +Date:		September 2019
> +KernelVersion:	5.5
> +Contact:	linux-leds@vger.kernel.org
> +Description:
> +		Set the trigger for this LED. A trigger is a kernel based source
> +		of LED events.
> +		Writing the trigger name to this file will change the current
> +		trigger. Trigger specific parameters can appear in
> +		/sys/class/leds/<led> once a given trigger is selected. For
> +		their documentation see sysfs-class-led-trigger-*.
> +		Reading this file will return the current LED trigger name.
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 3f04334..3cb0d8a 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -74,12 +74,22 @@ static ssize_t max_brightness_show(struct device *dev,
>  static DEVICE_ATTR_RO(max_brightness);
>  
>  #ifdef CONFIG_LEDS_TRIGGERS
> +
> +static DEVICE_ATTR(current_trigger, 0644, led_current_trigger_show,
> +		   led_current_trigger_store);

DEVICE_ATTR_RW() please.

> +static struct attribute *led_current_trigger_attrs[] = {
> +	&dev_attr_current_trigger.attr,
> +	NULL,
> +};
> +
>  static BIN_ATTR(trigger, 0644, led_trigger_read, led_trigger_write, 0);
>  static struct bin_attribute *led_trigger_bin_attrs[] = {
>  	&bin_attr_trigger,
>  	NULL,
>  };
>  static const struct attribute_group led_trigger_group = {
> +	.attrs = led_current_trigger_attrs,
>  	.bin_attrs = led_trigger_bin_attrs,
>  };
>  #endif
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 0b810cf..a2ef674 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -27,11 +27,9 @@ LIST_HEAD(trigger_list);
>  
>   /* Used by LED Class */
>  
> -ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> -			  struct bin_attribute *bin_attr, char *buf,
> -			  loff_t pos, size_t count)
> +static ssize_t led_trigger_store(struct device *dev, const char *buf,
> +				 size_t count)
>  {
> -	struct device *dev = kobj_to_dev(kobj);
>  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>  	struct led_trigger *trig;
>  	int ret = count;
> @@ -67,8 +65,25 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
>  	mutex_unlock(&led_cdev->led_access);
>  	return ret;
>  }
> +
> +ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> +			  struct bin_attribute *bin_attr, char *buf,
> +			  loff_t pos, size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +
> +	return led_trigger_store(dev, buf, count);
> +}
>  EXPORT_SYMBOL_GPL(led_trigger_write);
>  
> +ssize_t led_current_trigger_store(struct device *dev,
> +				struct device_attribute *attr, const char *buf,
> +				size_t count)
> +{
> +	return led_trigger_store(dev, buf, count);
> +}
> +EXPORT_SYMBOL_GPL(led_current_trigger_store);
> +
>  __printf(3, 4)
>  static int led_trigger_snprintf(char *buf, ssize_t size, const char *fmt, ...)
>  {
> @@ -144,6 +159,21 @@ ssize_t led_trigger_read(struct file *filp, struct kobject *kobj,
>  }
>  EXPORT_SYMBOL_GPL(led_trigger_read);
>  
> +ssize_t led_current_trigger_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	int len;
> +
> +	down_read(&led_cdev->trigger_lock);
> +	len = scnprintf(buf, PAGE_SIZE, "%s\n", led_cdev->trigger ?

No need for "scnprintf() when writing a single number to a buffer.  Just
use sprintf() please.

> +			led_cdev->trigger->name : "none");

Can you have a trigger named "none"? :)

thanks,

greg k-h

Re: [PATCH -next 2/2] leds: add /sys/class/leds/<led>/current-trigger

[Dan Murphy]

Akinobu

On 10/2/19 10:13 AM, Akinobu Mita wrote:
> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, this violates the "one value per file" rule of sysfs.
>
> This provides /sys/class/leds/<led>/current-trigger which is almost
> identical to /sys/class/leds/<led>/trigger.  The only difference is that
> 'current-trigger' only shows the current trigger name.
>
> This new file follows the "one value per file" rule of sysfs.
> We can find all available LED triggers by listing the
> /sys/devices/virtual/led-trigger/ directory.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>   Documentation/ABI/testing/sysfs-class-led | 13 +++++++++++
>   drivers/leds/led-class.c                  | 10 ++++++++
>   drivers/leds/led-triggers.c               | 38 +++++++++++++++++++++++++++----
>   drivers/leds/leds.h                       |  5 ++++
>   4 files changed, 62 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> index 5f67f7a..fdfed3f 100644
> --- a/Documentation/ABI/testing/sysfs-class-led
> +++ b/Documentation/ABI/testing/sysfs-class-led
> @@ -61,3 +61,16 @@ Description:
>   		gpio and backlight triggers. In case of the backlight trigger,
>   		it is useful when driving a LED which is intended to indicate
>   		a device in a standby like state.
> +
> +What:		/sys/class/leds/<led>/current-trigger
> +Date:		September 2019
> +KernelVersion:	5.5
> +Contact:	linux-leds@vger.kernel.org
> +Description:
> +		Set the trigger for this LED. A trigger is a kernel based source
> +		of LED events.
> +		Writing the trigger name to this file will change the current
> +		trigger. Trigger specific parameters can appear in
> +		/sys/class/leds/<led> once a given trigger is selected. For
> +		their documentation see sysfs-class-led-trigger-*.
> +		Reading this file will return the current LED trigger name.

Why do we need this new file can't we just update the current trigger 
file implementation?

I don't see any documentation that states that the read of the trigger 
file will print a list of known triggers.

And writing to the trigger file still works so I would think the _show 
just needs to be fixed.

Besides this patch does not fix the issue in the commit message that the 
trigger file still violates the one value per file rule.

Dan

Re: [PATCH -next 2/2] leds: add /sys/class/leds/<led>/current-trigger

[Jacek Anaszewski]

Dan,

On 10/2/19 5:47 PM, Dan Murphy wrote:
> Akinobu
> 
> On 10/2/19 10:13 AM, Akinobu Mita wrote:
>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
>> However, this violates the "one value per file" rule of sysfs.
>>
>> This provides /sys/class/leds/<led>/current-trigger which is almost
>> identical to /sys/class/leds/<led>/trigger.  The only difference is that
>> 'current-trigger' only shows the current trigger name.
>>
>> This new file follows the "one value per file" rule of sysfs.
>> We can find all available LED triggers by listing the
>> /sys/devices/virtual/led-trigger/ directory.
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>> Cc: Pavel Machek <pavel@ucw.cz>
>> Cc: Dan Murphy <dmurphy@ti.com>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> ---
>>   Documentation/ABI/testing/sysfs-class-led | 13 +++++++++++
>>   drivers/leds/led-class.c                  | 10 ++++++++
>>   drivers/leds/led-triggers.c               | 38
>> +++++++++++++++++++++++++++----
>>   drivers/leds/leds.h                       |  5 ++++
>>   4 files changed, 62 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>> b/Documentation/ABI/testing/sysfs-class-led
>> index 5f67f7a..fdfed3f 100644
>> --- a/Documentation/ABI/testing/sysfs-class-led
>> +++ b/Documentation/ABI/testing/sysfs-class-led
>> @@ -61,3 +61,16 @@ Description:
>>           gpio and backlight triggers. In case of the backlight trigger,
>>           it is useful when driving a LED which is intended to indicate
>>           a device in a standby like state.
>> +
>> +What:        /sys/class/leds/<led>/current-trigger
>> +Date:        September 2019
>> +KernelVersion:    5.5
>> +Contact:    linux-leds@vger.kernel.org
>> +Description:
>> +        Set the trigger for this LED. A trigger is a kernel based source
>> +        of LED events.
>> +        Writing the trigger name to this file will change the current
>> +        trigger. Trigger specific parameters can appear in
>> +        /sys/class/leds/<led> once a given trigger is selected. For
>> +        their documentation see sysfs-class-led-trigger-*.
>> +        Reading this file will return the current LED trigger name.
> 
> Why do we need this new file can't we just update the current trigger
> file implementation?

We can't change existing ABI. It doesn't matter if it is documented
or not - it's in place for very long time and you can't guarantee there
are no users relying on triggers file show format.

> I don't see any documentation that states that the read of the trigger
> file will print a list of known triggers.
> 
> And writing to the trigger file still works so I would think the _show
> just needs to be fixed.
> 
> Besides this patch does not fix the issue in the commit message that the
> trigger file still violates the one value per file rule.
> 
> Dan
> 
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH -next 2/2] leds: add /sys/class/leds/<led>/current-trigger

[Dan Murphy]

Jacek

On 10/2/19 12:46 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 10/2/19 5:47 PM, Dan Murphy wrote:
>> Akinobu
>>
>> On 10/2/19 10:13 AM, Akinobu Mita wrote:
>>> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
>>> However, this violates the "one value per file" rule of sysfs.
>>>
>>> This provides /sys/class/leds/<led>/current-trigger which is almost
>>> identical to /sys/class/leds/<led>/trigger.  The only difference is that
>>> 'current-trigger' only shows the current trigger name.
>>>
>>> This new file follows the "one value per file" rule of sysfs.
>>> We can find all available LED triggers by listing the
>>> /sys/devices/virtual/led-trigger/ directory.
>>>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
>>> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
>>> Cc: Pavel Machek <pavel@ucw.cz>
>>> Cc: Dan Murphy <dmurphy@ti.com>
>>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>>> ---
>>>    Documentation/ABI/testing/sysfs-class-led | 13 +++++++++++
>>>    drivers/leds/led-class.c                  | 10 ++++++++
>>>    drivers/leds/led-triggers.c               | 38
>>> +++++++++++++++++++++++++++----
>>>    drivers/leds/leds.h                       |  5 ++++
>>>    4 files changed, 62 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led
>>> b/Documentation/ABI/testing/sysfs-class-led
>>> index 5f67f7a..fdfed3f 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-led
>>> +++ b/Documentation/ABI/testing/sysfs-class-led
>>> @@ -61,3 +61,16 @@ Description:
>>>            gpio and backlight triggers. In case of the backlight trigger,
>>>            it is useful when driving a LED which is intended to indicate
>>>            a device in a standby like state.
>>> +
>>> +What:        /sys/class/leds/<led>/current-trigger
>>> +Date:        September 2019
>>> +KernelVersion:    5.5
>>> +Contact:    linux-leds@vger.kernel.org
>>> +Description:
>>> +        Set the trigger for this LED. A trigger is a kernel based source
>>> +        of LED events.
>>> +        Writing the trigger name to this file will change the current
>>> +        trigger. Trigger specific parameters can appear in
>>> +        /sys/class/leds/<led> once a given trigger is selected. For
>>> +        their documentation see sysfs-class-led-trigger-*.
>>> +        Reading this file will return the current LED trigger name.
>> Why do we need this new file can't we just update the current trigger
>> file implementation?
> We can't change existing ABI. It doesn't matter if it is documented
> or not - it's in place for very long time and you can't guarantee there
> are no users relying on triggers file show format.

So if it has been in place for a very long time why do we need another 
ABI that does sorta the same thing?

This seems to be a bit confusing and extra.

Maybe this ABI should be RO where a user can read the current-trigger as 
a single value per file but writing the trigger still

is done through the old ABI.

Dan


>
>> I don't see any documentation that states that the read of the trigger
>> file will print a list of known triggers.
>>
>> And writing to the trigger file still works so I would think the _show
>> just needs to be fixed.
>>
>> Besides this patch does not fix the issue in the commit message that the
>> trigger file still violates the one value per file rule.
>>
>> Dan
>>
>>

Re: [PATCH -next 2/2] leds: add /sys/class/leds/<led>/current-trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

Hi!

> >>>diff --git a/Documentation/ABI/testing/sysfs-class-led
> >>>b/Documentation/ABI/testing/sysfs-class-led
> >>>index 5f67f7a..fdfed3f 100644
> >>>--- a/Documentation/ABI/testing/sysfs-class-led
> >>>+++ b/Documentation/ABI/testing/sysfs-class-led
> >>>@@ -61,3 +61,16 @@ Description:
> >>>           gpio and backlight triggers. In case of the backlight trigger,
> >>>           it is useful when driving a LED which is intended to indicate
> >>>           a device in a standby like state.
> >>>+
> >>>+What:        /sys/class/leds/<led>/current-trigger
> >>>+Date:        September 2019
> >>>+KernelVersion:    5.5
> >>>+Contact:    linux-leds@vger.kernel.org
> >>>+Description:
> >>>+        Set the trigger for this LED. A trigger is a kernel based source
> >>>+        of LED events.
> >>>+        Writing the trigger name to this file will change the current
> >>>+        trigger. Trigger specific parameters can appear in
> >>>+        /sys/class/leds/<led> once a given trigger is selected. For
> >>>+        their documentation see sysfs-class-led-trigger-*.
> >>>+        Reading this file will return the current LED trigger name.
> >>Why do we need this new file can't we just update the current trigger
> >>file implementation?
> >We can't change existing ABI. It doesn't matter if it is documented
> >or not - it's in place for very long time and you can't guarantee there
> >are no users relying on triggers file show format.
> 
> So if it has been in place for a very long time why do we need another ABI
> that does sorta the same thing?
> 
> This seems to be a bit confusing and extra.

Agreed. Lets simply keep the existing ABI.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH -next 0/2] leds: add substitutes for /sys/class/leds/<led>/trigger

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

Hi!

> Reading /sys/class/leds/<led>/trigger returns all available LED triggers.
> However, this violates the "one value per file" rule of sysfs.
> 
> This series provides a new /sys/devices/virtual/led-trigger/ directory and
> /sys/class/leds/<led>/current-trigger. The new api follows the "one value
> per file" rule of sysfs.

Lets not do this. We'll have to maintain the old interface, anyway, so
it does not really help.

Thanks,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]