leds: add device trigger

The patch adds LED triggers for indicating an activity on a selected device. The drivers that intend to use triggers need to register respective devices using ledtrig_dev_add(). Triggers are generated by explicitly calling ledtrig_dev_activity().

Signed-off-by Maciek Borzecki <[email protected]>

---

drivers/leds/trigger/ledtrig-device.c new file +4 -0

+struct ledtrig_dev_data {
+	char name[MAX_NAME_LEN];
+	dev_t dev;
+	struct led_trigger *trig;

drivers/leds/trigger/ledtrig-device.c new file +8 -0

+{
+	struct ledtrig_dev_data *dev_trig;
+
+	if (!down_read_trylock(&devs_list_lock))
+		return;
+
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		if (dev_trig->dev == dev) {

Hello Maciek-

Some architectural questions below:

Quoted 7 lines · Maciek Borzecki

The patch adds LED triggers for indicating an activity on a selected device. The drivers that intend to use triggers need to register respective devices using ledtrig_dev_add(). Triggers are generated by explicitly calling ledtrig_dev_activity().

Signed-off-by Maciek Borzecki <[email protected]>

---

[..]

Quoted 4 lines

drivers/leds/trigger/ledtrig-device.c new file +4 -0

+struct ledtrig_dev_data {
+	char name[MAX_NAME_LEN];
+	dev_t dev;
+	struct led_trigger *trig;

drivers/leds/trigger/ledtrig-device.c line 41

Quoted 10 lines

drivers/leds/trigger/ledtrig-device.c new file +10 -0

+	struct list_head node;
+};
+
+/**
+ * ledtrig_dev_activity - signal activity on device
+ * @dev: device
+ *
+ * Fires a trigger assigned to @dev device.
+ */
+void ledtrig_dev_activity(dev_t dev)

It seems a bit strange to me to associate a device LED trigger with dev_t. Some devices don't expose a dev node, some devices expose multiple dev nodes...

Is there a reason why you are not tying to the device model?

Quoted 12 lines

drivers/leds/trigger/ledtrig-device.c new file +12 -0

+{
+	struct ledtrig_dev_data *dev_trig;
+
+	if (!down_read_trylock(&devs_list_lock))
+		return;
+
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		if (dev_trig->dev == dev) {
+			led_trigger_blink_oneshot(dev_trig->trig,
+						  &blink_delay,
+						  &blink_delay,
+						  0);

drivers/leds/trigger/ledtrig-device.c line 59

Quoted 6 lines

drivers/leds/trigger/ledtrig-device.c new file +6 -0

+			break;
+		}
+	}
+	up_read(&devs_list_lock);
+}
+EXPORT_SYMBOL(ledtrig_dev_activity);

Not _GPL?

Quoted 28 lines · Josh Cartwright

Quoted 7 lines · Maciek Borzecki

The patch adds LED triggers for indicating an activity on a selected device. The drivers that intend to use triggers need to register respective devices using ledtrig_dev_add(). Triggers are generated by explicitly calling ledtrig_dev_activity().

Signed-off-by Maciek Borzecki <[email protected]>

---

[..]

Quoted 4 lines

drivers/leds/trigger/ledtrig-device.c new file +4 -0

+struct ledtrig_dev_data {
+	char name[MAX_NAME_LEN];
+	dev_t dev;
+	struct led_trigger *trig;

drivers/leds/trigger/ledtrig-device.c line 41

Quoted 10 lines

drivers/leds/trigger/ledtrig-device.c new file +10 -0

+	struct list_head node;
+};
+
+/**
+ * ledtrig_dev_activity - signal activity on device
+ * @dev: device
+ *
+ * Fires a trigger assigned to @dev device.
+ */
+void ledtrig_dev_activity(dev_t dev)

It seems a bit strange to me to associate a device LED trigger with dev_t. Some devices don't expose a dev node, some devices expose multiple dev nodes...

Quoted 2 lines

Is there a reason why you are not tying to the device model?

Thanks for the comments.

The first proof of concept used sturct device as parameter in all API calls, but then there's a problem of naming of a trigger in a sane way. The trigger name followed the same approach as __dev_printk, and the naming was done in this fashion:

   sprintf(..., "dev-%s-%s", dev_driver_string(dev), dev_name(dev));

Then for instance on wandboard, /dev/ttymxc0 and /dev/ttymxc2 would appear as dev-serial-2020000 and dev-serial-21ec000. In my opinion this was unnecessarily complicated.

Also, if I'm not mistaken, using this approach the partitions on MMC card or SATA drive would end up with the same trigger name, as it is a single device.

Multiple dev nodes will already have different minor numbers, so their dev_t is different anyway.

As for devices that do not have a dev_t assigned to them one can still pass a custom tag in ledtrig_dev_add(). It's just a number so as long as there's no collision in numbering things should be fine.

Hmm, maybe we're bikeshedding at this point, but LEDs with those names seem much more straightfoward to me than a "dev-<maj>:<min>" name, for devices which have done dynamic dev_t allocation.

Quoted 3 lines

Also, if I'm not mistaken, using this approach the partitions on MMC card or SATA drive would end up with the same trigger name, as it is a single device.

This would only be true if you used _just_ the struct device. I was imagining that you'd specify a (struct device, unsigned index) pair. Better, you could do a (struct device, const char *) pair.

Also, from a lifetime management perspective, it starts to feel like something that might integrate better as a managed resource (devm_*).

[..]

Quoted 2 lines

Multiple dev nodes will already have different minor numbers, so their dev_t is different anyway.

Okay, backing up I don't really see what this API really buys the consumer. The dev_t -> struct led_trigger mapping just seems like a total waste. Why not just make your ledtrig_dev_add() function return the struct led_trigger * that the consumer keeps track of?

Maybe seeing an example consumer would provide some clarification.

Quoted 3 lines

As for devices that do not have a dev_t assigned to them one can still pass a custom tag in ledtrig_dev_add(). It's just a number so as long as there's no collision in numbering things should be fine.

Ensuring no collision will be difficult, especially given that it's most common that the dynamic allocator is used. In order to guarantee no collisions, a user who doesn't expose any device nodes would need to do their own dev_t allocation...to use this interface. And that seems silly to me.

A series of patches that add yet another LED trigger, a device activity trigger.

The motivation is to have a LED trigger that is associated with a device and can be fired from cetrain points in the code that have been chosen by the developer. With this this device activity trigger it is possible for instance to easily hook up a tty driver for a console to blink one LED, yet another serial port to blink a second LED and writes to a block device to trigger a third LED.

The patches have been tested on Wandboard Quad.

The first patch adds the actual trigger. Each device wishing to use the trigger has to be explicitly registered by calling ledtrig_dev_add(), and passing it's dev_t. The intention is that the trigger will be used in scenarios that are impossible to foresee at this moment, and are likely to be approach in a case by case manner anyway.

The second patch adds couple of debugfs helpers.

The third patch adds documentation and notes on debugfs interface.

Example hooks into tty driver can be seen here:
https://github.com/bboozzoo/linux/commit/d8a875673e37b27d9c9066febe7633382f97d8af

Changes since v1:
- fixed debugfs user address space access
- added unregister debugfs attribute
- documentation update

Maciek Borzecki (3):
leds: add device activity LED triggers
leds: add debugfs to device trigger
Documentation: leds: document ledtrig-device trigger

  Documentation/leds/00-INDEX           |   3 +
  Documentation/leds/ledtrig-device.txt |  35 ++++
  drivers/leds/trigger/Kconfig          |   8 +
  drivers/leds/trigger/Makefile         |   1 +
  drivers/leds/trigger/ledtrig-device.c | 326 ++++++++++++++++++++++++++++++++++
  include/linux/leds.h                  |  10 ++
  6 files changed, 383 insertions(+)

create mode 100644 Documentation/leds/ledtrig-device.txt create mode 100644 drivers/leds/trigger/ledtrig-device.c

Hi Maciek,

I tested your trigger, and it works fine, but I wonder if it really improves the things.

Basically, similarly as Josh, I have doubts related to associating triggers with dev_t. It requires from user to figure out how they can obtain valid dev_t. For example, in case of v4l2 jpeg codec, I had to spend some time looking for the location of struct device containing dev_t related to the exposed encoder and decoder nodes, whereas addition of debugging instruction should be easy and intuitive.

It is much simpler to register a trigger with human readable names. What is more, use of dev_t makes the user thinking that there is some mechanism involved, that automatically associates device with trigger, and after some time of code investigation one gets disillusioned and realizes that dev_t is used only to uniquely identify registered triggers.

I tried to add trigger to the JPEG codec interrupt handler using ledtrig-oneshot, and below is the result:

drivers/media/platform/s5p-jpeg/jpeg-core.c +8 -0

index 9690f9d..af826d3 100644
@@ -28,6 +28,7 @@
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-core.h>
 #include <media/videobuf2-dma-contig.h>
+#include <linux/leds.h>
 
 #include "jpeg-core.h"
 #include "jpeg-hw-s5p.h"

@@ -35,6 +36,9 @@
 #include "jpeg-hw-exynos3250.h"
 #include "jpeg-regs.h"
 
+static unsigned long blink_delay = 30;
+struct led_trigger *jpeg_led_trig;
+
 static struct s5p_jpeg_fmt sjpeg_formats[] = {
        {
                .name           = "JPEG JFIF",

@@ -2318,6 +2322,7 @@ static irqreturn_t s5p_jpeg_irq(int irq, void *dev_id)
        return IRQ_HANDLED;
 }
 
+
 static irqreturn_t exynos4_jpeg_irq(int irq, void *priv)
 {
        unsigned int int_status;

@@ -2375,7 +2380,10 @@ static irqreturn_t exynos4_jpeg_irq(int irq, void
 *priv)
        v4l2_m2m_job_finish(jpeg->m2m_dev, curr_ctx->fh.m2m_ctx);
        curr_ctx->subsampling = exynos4_jpeg_get_frame_fmt(jpeg->regs);
 
+       led_trigger_blink_oneshot(jpeg_led_trig, &blink_delay, &blink_delay,
 0);
+
        spin_unlock(&jpeg->slock);
+
        return IRQ_HANDLED;

}

drivers/media/platform/s5p-jpeg/jpeg-core.c +2 -0

@@ -2589,6 +2597,8 @@ static int s5p_jpeg_probe(struct platform_device
 *pdev)
 
        v4l2_info(&jpeg->v4l2_dev, "Samsung S5P JPEG codec\n");
 
+       led_trigger_register_simple("jpeg-trig", &jpeg_led_trig);
+
        return 0;
 

enc_vdev_register_rollback:

drivers/media/platform/s5p-jpeg/jpeg-core.c +2 -0

@@ -2633,6 +2643,8 @@ static int s5p_jpeg_remove(struct platform_device
 *pdev)
        if (!IS_ERR(jpeg->sclk))
                clk_put(jpeg->sclk);
 
+       led_trigger_unregister_simple(jpeg_led_trig);
+
        return 0;
 }

It needs addition of 6 lines of code versus 2 lines in case of ledtrig-device. Not a big deal, taking into account that we don't have to spend additional time looking for the suitable struct device.

Please note that in case of drivers that expose many dev nodes, there are cases like interrupt handlers, where struct device can't be automatically retrieved, but it needs additional analysis to find out which dev node given call to ISR referes to. In this case we would have to check current mode (encoding/decoding) and call either

	ledtrig_dev_activity(jpeg->vfd_encoder->dev.devt)
or
	ledtrig_dev_activity(jpeg->vfd_decoder->dev.devt).

When comparing the steps required from user space side to activate the triggers, it looks as follows:

ledtrig-oneshot (we have one trigger for encoding and decoding mode):
=================
#cd /sys/class/leds/aat1290
#cat triggers
#[none] jpeg-trig
#echo "jpeg-trig" > trigger

ledtrig-dev approach (we have to define trigger per dev_t, we choose encoder):
=====================
#grep . /sys/class/video4linux/video*/name #/sys/class/video4linux/video7/name:s5p-jpeg-enc #/sys/class/video4linux/video8/name:s5p-jpeg-dec
#ls -l /dev/video7
#crw-rw---T 1 root video 81, 8 Jan 1 03:32 /dev/video8 #echo "81:7" > /sys/kernel/debug/ledtrig-dev/register
#cd /sys/class/leds/aat1290
#cat triggers
#[none] dev-81:7
#echo "dev-81:7" > trigger

It seems that ledtrig-dev is more troublesome in use.

Please let me know if I missed something.