Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1237437 > unrolled thread

[PATCH v2 1/3] leds: add device activity LED triggers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/3] leds: add device activity LED triggers Maciek Borzecki <maciek.borzecki@gmail.com> - 2015-10-01 16:10 +0200
    Re: [PATCH v2 1/3] leds: add device activity LED triggers Josh Cartwright <joshc@ni.com> - 2015-10-01 16:50 +0200
      Re: [PATCH v2 1/3] leds: add device activity LED triggers Maciek Borzecki <maciek.borzecki@gmail.com> - 2015-10-02 09:50 +0200
        Re: [PATCH v2 1/3] leds: add device activity LED triggers Josh Cartwright <joshc@ni.com> - 2015-10-02 19:10 +0200
          Re: [PATCH v2 1/3] leds: add device activity LED triggers Maciek Borzecki <maciek.borzecki@gmail.com> - 2015-10-02 21:10 +0200

#1237437 — [PATCH v2 1/3] leds: add device activity LED triggers

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 <maciek.borzecki@gmail.com>
---
 drivers/leds/trigger/Kconfig          |   8 ++
 drivers/leds/trigger/Makefile         |   1 +
 drivers/leds/trigger/ledtrig-device.c | 185 ++++++++++++++++++++++++++++++++++
 include/linux/leds.h                  |  10 ++
 4 files changed, 204 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-device.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 5bda6a9b56bbd90b4a3749f87bc0c6fda8dd5034..c6ecfbd7c0876f21688140aa9afc7eb5b9fec3a2 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -108,4 +108,12 @@ config LEDS_TRIGGER_CAMERA
 	  This enables direct flash/torch on/off by the driver, kernel space.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_DEVICE
+	bool "LED Device Activity Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows LEDs to be triggered by an actvity on a selected
+          device.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 1abf48dacf7ebfcfb8208f7ae7bdf29d7c11ba32..86beeacd5403afc163873d7c3f817ee082b64e04 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CPU)		+= ledtrig-cpu.o
 obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
+obj-$(CONFIG_LEDS_TRIGGER_DEVICE)	+= ledtrig-device.o
diff --git a/drivers/leds/trigger/ledtrig-device.c b/drivers/leds/trigger/ledtrig-device.c
new file mode 100644
index 0000000000000000000000000000000000000000..dbb8d7d2b4a0258149c581a040c416d412d9ceeb
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-device.c
@@ -0,0 +1,185 @@
+/*
+ * LED Device Activity Trigger
+ *
+ * Copyright 2015 Maciej Borzecki <maciek.borzecki@gmail.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.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/rwsem.h>
+#include <linux/kdev_t.h>
+
+#define BLINK_DELAY 30
+static unsigned long blink_delay = BLINK_DELAY;
+
+static DECLARE_RWSEM(devs_list_lock);
+static LIST_HEAD(devs_list);
+
+#define MAX_NAME_LEN 20
+
+struct ledtrig_dev_data {
+	char name[MAX_NAME_LEN];
+	dev_t dev;
+	struct led_trigger *trig;
+	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)
+{
+	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);
+			break;
+		}
+	}
+	up_read(&devs_list_lock);
+}
+EXPORT_SYMBOL(ledtrig_dev_activity);
+
+static struct ledtrig_dev_data *ledtrig_dev_new(dev_t dev)
+{
+	struct ledtrig_dev_data *dev_trig;
+
+	dev_trig = kzalloc(sizeof(*dev_trig), GFP_KERNEL);
+	if (!dev_trig)
+		return NULL;
+
+	INIT_LIST_HEAD(&dev_trig->node);
+	dev_trig->dev = dev;
+	snprintf(dev_trig->name, sizeof(dev_trig->name),
+		 "dev-%u:%u", MAJOR(dev), MINOR(dev));
+
+	return dev_trig;
+}
+
+static void ledtrig_dev_release(struct ledtrig_dev_data *dev_trig)
+{
+	led_trigger_unregister_simple(dev_trig->trig);
+
+	kfree(dev_trig);
+}
+
+/**
+ * ledtrig_dev_add - add a trigger for device
+ * @dev: device for which the trigger is to be added
+ *
+ * Create and register a new trigger for device @dev. The trigger will
+ * show up as dev-<major>:<minor> in the list of avaialble LED
+ * triggers.
+ */
+void ledtrig_dev_add(dev_t dev)
+{
+	int found = 0;
+	struct ledtrig_dev_data *new_dev_trig;
+	struct ledtrig_dev_data *dev_trig;
+
+	new_dev_trig = ledtrig_dev_new(dev);
+	if (!new_dev_trig)
+		return;
+
+	down_write(&devs_list_lock);
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		if (dev_trig->dev == dev) {
+			found = 1;
+			break;
+		}
+	}
+	if (!found)
+		list_add(&new_dev_trig->node, &devs_list);
+	up_write(&devs_list_lock);
+
+	if (!found)
+		/* register with led triggers */
+		led_trigger_register_simple(new_dev_trig->name,
+					    &new_dev_trig->trig);
+	else
+		kfree(new_dev_trig);
+}
+EXPORT_SYMBOL(ledtrig_dev_add);
+
+/**
+ * ledtrig_dev_del - delete a trigger
+ * @dev: device for which to delete a trigger
+ */
+void ledtrig_dev_del(dev_t dev)
+{
+
+	struct ledtrig_dev_data *dev_trig;
+
+	down_write(&devs_list_lock);
+	list_for_each_entry(dev_trig, &devs_list, node) {
+		if (dev_trig->dev == dev) {
+			/* remove from devs list */
+			list_del(&dev_trig->node);
+
+			/* unregister & release data */
+			ledtrig_dev_release(dev_trig);
+			break;
+		}
+	}
+	up_write(&devs_list_lock);
+
+}
+EXPORT_SYMBOL(ledtrig_dev_del);
+
+static void ledtrig_dev_remove_all(void)
+{
+	struct list_head *en;
+
+	down_write(&devs_list_lock);
+	list_for_each(en, &devs_list) {
+		struct list_head *prev = en->prev;
+		struct ledtrig_dev_data *dev_trig;
+
+		dev_trig = list_entry(en, struct ledtrig_dev_data,
+				      node);
+		/* remove from list */
+		list_del(en);
+
+		/* unregister & release data */
+		ledtrig_dev_release(dev_trig);
+
+		/* and go back */
+		en = prev;
+	}
+	up_write(&devs_list_lock);
+}
+
+static int __init ledtrig_dev_init(void)
+{
+	return 0;
+}
+
+static void __exit ledtrig_dev_exit(void)
+{
+	ledtrig_dev_remove_all();
+}
+
+module_init(ledtrig_dev_init);
+module_exit(ledtrig_dev_exit);
+
+MODULE_AUTHOR("Maciej Borzecki <maciek.borzecki@gmail.com>");
+MODULE_DESCRIPTION("LED Device Activity Trigger");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc17b8a8b1a1852dc1c420ecf0f8d2..e487d5b2ac556bdb2f1525d8b5e84df0c245d4c9 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -369,4 +369,14 @@ static inline void ledtrig_cpu(enum cpu_led_event evt)
 }
 #endif
 
+#ifdef CONFIG_LEDS_TRIGGER_DEVICE
+extern void ledtrig_dev_add(dev_t dev);
+extern void ledtrig_dev_del(dev_t dev);
+extern void ledtrig_dev_activity(dev_t dev);
+#else
+static inline void ledtrig_dev_add(dev_t dev) {}
+static inline void ledtrig_dev_del(dev_t dev) {}
+static inline void ledtrig_dev_activity(dev_t dev) {}
+#endif
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
2.6.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1237472

[Multipart message — attachments visible in raw view] — view raw

Hello Maciek-

Some architectural questions below:

On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> 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 <maciek.borzecki@gmail.com>
> ---
[..]
> +struct ledtrig_dev_data {
> +	char name[MAX_NAME_LEN];
> +	dev_t dev;
> +	struct led_trigger *trig;
> +	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?

> +{
> +	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);
> +			break;
> +		}
> +	}
> +	up_read(&devs_list_lock);
> +}
> +EXPORT_SYMBOL(ledtrig_dev_activity);

Not _GPL?

  Josh

[toc] | [prev] | [next] | [standalone]

#1238012

On 10/01 09:47, Josh Cartwright wrote:
> Hello Maciek-
>
> Some architectural questions below:
>
> On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> > 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 <maciek.borzecki@gmail.com>
> > ---
> [..]
> > +struct ledtrig_dev_data {
> > +	char name[MAX_NAME_LEN];
> > +	dev_t dev;
> > +	struct led_trigger *trig;
> > +	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?
>
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. However the the
major:minor numbers assigned to respective partitions are different, and
you'd still be able to say trigger the LEDS on writes to a particular
partition.

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.

Hopefull this clears up the things a little.

> > +{
> > +	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);
> > +			break;
> > +		}
> > +	}
> > +	up_read(&devs_list_lock);
> > +}
> > +EXPORT_SYMBOL(ledtrig_dev_activity);
>
> Not _GPL?

I'm ok with EXPORT_SYMBOL_GPL() if that's a policy for new code. Though,
I've looked at other triggers that are called from kernel code, and it
seems that ledtrig-camera is the only one using _GPL.

--
Maciek Borzecki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1238420

[Multipart message — attachments visible in raw view] — view raw

On Fri, Oct 02, 2015 at 09:45:37AM +0200, Maciek Borzecki wrote:
> On 10/01 09:47, Josh Cartwright wrote:
> > On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> > > 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 <maciek.borzecki@gmail.com>
> > > ---
> > [..]
> > > +struct ledtrig_dev_data {
> > > +	char name[MAX_NAME_LEN];
> > > +	dev_t dev;
> > > +	struct led_trigger *trig;
> > > +	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?
> >
> 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.

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.

> 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_*).

[..]
> 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.

> 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.

  Josh

[toc] | [prev] | [next] | [standalone]

#1238538

On 10/02 12:08, Josh Cartwright wrote:
<snip>
>
> 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.
>
> > 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_*).
>
> [..]
> > 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.
>
> > 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.

Thanks, I really appreciate your feedback.

--
Maciek Borzecki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web