On 08/01/2012 12:05 AM, Fabio Baltieri wrote:
> This patch implements the functions to add two LED triggers, named
> <ifname>-tx and <ifname>-rx, to a canbus device driver.
>
> Triggers are called from specific handlers by each CAN device driver and
> can be disabled altogether with a Kconfig option.
>
> The implementation keeps the LED on when the interface is UP and blinks
> the LED on network activity at a configurable rate.
>
> This only supports can-dev based drivers, as it uses some support field
> in the can_priv structure.
>
> Supported drivers should call can_led_init() and can_led_event() as
> needed.
>
> Cleanup is handled automatically by devres, so no *_exit function is
> needed.
>
> Supported events are:
> - CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
> - CAN_LED_EVENT_STOP: turn off tx/rx LEDs
> - CAN_LED_EVENT_TX: trigger tx LED blink
> - CAN_LED_EVENT_RX: trigger tx LED blink
>
> Cc: Oliver Hartkopp <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Fabio Baltieri <[email protected]>
> ---
>
> Hi!
>
> So, here is the v4.
>
> Changes against v3:
> - add a netdev_err message in kasprintf error path
> - implement a devres_alloc release function to de-allocate strings and trigger
> - can_led_exit is now unneeded and gone
>
> Is that what you had in mind Marc?
Yes, that's it. Can you add a devm_ prefix to can_led_init() please.
> I like the devres implementation, as callers don't have to care about exit
> function anymore... still it looks like a bit of an hack to me.
No :)
> Also, I made an initial version with the four pointers in a separate can_led
> struct to be allocated as a payload of devres_alloc - but I think this one
> looks cleaner and is more cache-friendly.
If you allocate the 4 pointers in a separate struct you only save 3
points, as you need the pointer to the struct anyways.
> What you think about this?
Looks good. See comment inline,
Marc
> drivers/net/can/Kconfig | 12 ++++++
> drivers/net/can/Makefile | 2 +
> drivers/net/can/led.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/can/dev.h | 8 ++++
> include/linux/can/led.h | 34 ++++++++++++++++
> 5 files changed, 158 insertions(+)
> create mode 100644 drivers/net/can/led.c
> create mode 100644 include/linux/can/led.h
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index bb709fd..19dec19 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -54,6 +54,18 @@ config CAN_CALC_BITTIMING
> arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
> If unsure, say Y.
>
> +config CAN_LEDS
> + bool "Enable LED triggers for Netlink based drivers"
> + depends on CAN_DEV
> + depends on LEDS_CLASS
> + select LEDS_TRIGGERS
> + ---help---
> + This option adds two LED triggers for packet receive and transmit
> + events on each supported CAN device.
> +
> + Say Y here if you are working on a system with led-class supported
> + LEDs and you want to use them as canbus activity indicators.
> +
> config CAN_AT91
> tristate "Atmel AT91 onchip CAN controller"
> depends on CAN_DEV && (ARCH_AT91SAM9263 || ARCH_AT91SAM9X5)
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 938be37..24ee98b 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -8,6 +8,8 @@ obj-$(CONFIG_CAN_SLCAN) += slcan.o
> obj-$(CONFIG_CAN_DEV) += can-dev.o
> can-dev-y := dev.o
>
> +can-dev-$(CONFIG_CAN_LEDS) += led.o
> +
> obj-y += usb/
> obj-y += softing/
>
> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
> new file mode 100644
> index 0000000..4fa8a47
> --- /dev/null
> +++ b/drivers/net/can/led.c
> @@ -0,0 +1,102 @@
> +/*
> + * Copyright 2012, Fabio Baltieri <[email protected]>
> + *
> + * 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/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/netdevice.h>
> +#include <linux/can/dev.h>
> +
> +#include <linux/can/led.h>
> +
> +static unsigned long led_delay = 50;
> +module_param(led_delay, ulong, 0644);
> +MODULE_PARM_DESC(led_delay,
> + "blink delay time for activity leds (msecs, default: 50).");
> +
> +/*
> + * Trigger a LED event in response to a CAN device event
> + */
> +void can_led_event(struct net_device *netdev, enum can_led_event event)
> +{
> + struct can_priv *priv = netdev_priv(netdev);
> +
> + switch (event) {
> + case CAN_LED_EVENT_OPEN:
> + led_trigger_event(priv->tx_led_trig, LED_FULL);
> + led_trigger_event(priv->rx_led_trig, LED_FULL);
> + break;
> + case CAN_LED_EVENT_STOP:
> + led_trigger_event(priv->tx_led_trig, LED_OFF);
> + led_trigger_event(priv->rx_led_trig, LED_OFF);
> + break;
> + case CAN_LED_EVENT_TX:
> + if (led_delay)
> + led_trigger_blink_oneshot(priv->tx_led_trig,
> + &led_delay, &led_delay, 1);
> + break;
> + case CAN_LED_EVENT_RX:
> + if (led_delay)
> + led_trigger_blink_oneshot(priv->rx_led_trig,
> + &led_delay, &led_delay, 1);
> + break;
> + }
> +}
> +EXPORT_SYMBOL_GPL(can_led_event);
> +
> +static void can_led_release(struct device *gendev, void *res)
> +{
> + struct can_priv *priv = netdev_priv(to_net_dev(gendev));
> +
> + led_trigger_unregister_simple(priv->tx_led_trig);
> + led_trigger_unregister_simple(priv->rx_led_trig);
> +
> + kfree(priv->tx_led_trig_name);
> + kfree(priv->rx_led_trig_name);
> +}
> +
> +/*
> + * Register CAN LED triggers for a CAN device
> + *
> + * This is normally called from a driver's probe function
> + */
> +void can_led_init(struct net_device *netdev)
> +{
> + struct can_priv *priv = netdev_priv(netdev);
> + void *res;
> +
> + res = devres_alloc(can_led_release, 0, GFP_KERNEL);
^
I'm not really sure if this is working. For example, pinctrl [1]
allocates a double pointer here. The res pointer here and in
can_led_release simply points to invalid memory. But as long as you
don't dereference it, it should work.
[1] http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L862
> + if (!res)
> + goto err_out;
> +
> + priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
> + if (!priv->tx_led_trig_name)
> + goto err_out;
> +
> + priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
> + if (!priv->rx_led_trig_name)
> + goto err_out_rx;
> +
> + led_trigger_register_simple(priv->tx_led_trig_name,
> + &priv->tx_led_trig);
> + led_trigger_register_simple(priv->rx_led_trig_name,
> + &priv->rx_led_trig);
> +
> + devres_add(&netdev->dev, res);
> +
> + return;
> +
> +err_out_rx:
> + kfree(priv->tx_led_trig_name);
> + priv->tx_led_trig_name = NULL;
> +err_out:
> + netdev_err(netdev, "cannot register LED triggers\n");
> + devres_free(res);
> +}
> +EXPORT_SYMBOL_GPL(can_led_init);
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On 08/01/2012 11:36 AM, Marc Kleine-Budde wrote:
[...]
>> +/*
>> + * Register CAN LED triggers for a CAN device
>> + *
>> + * This is normally called from a driver's probe function
>> + */
>> +void can_led_init(struct net_device *netdev)
>> +{
>> + struct can_priv *priv = netdev_priv(netdev);
>> + void *res;
>> +
>> + res = devres_alloc(can_led_release, 0, GFP_KERNEL);
> ^
> I'm not really sure if this is working. For example, pinctrl [1]
> allocates a double pointer here. The res pointer here and in
> can_led_release simply points to invalid memory. But as long as you
> don't dereference it, it should work.
>
> [1] http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L862
The pinctrl usecase if different, pinctrl needs that extra memory
because they cannot get a reference to their pinctrl they have to put.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On Wed, Aug 01, 2012 at 11:36:51AM +0200, Marc Kleine-Budde wrote:
> On 08/01/2012 12:05 AM, Fabio Baltieri wrote:
> > This patch implements the functions to add two LED triggers, named
> > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> >
> > Triggers are called from specific handlers by each CAN device driver and
> > can be disabled altogether with a Kconfig option.
> >
> > The implementation keeps the LED on when the interface is UP and blinks
> > the LED on network activity at a configurable rate.
> >
> > This only supports can-dev based drivers, as it uses some support field
> > in the can_priv structure.
> >
> > Supported drivers should call can_led_init() and can_led_event() as
> > needed.
> >
> > Cleanup is handled automatically by devres, so no *_exit function is
> > needed.
> >
> > Supported events are:
> > - CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
> > - CAN_LED_EVENT_STOP: turn off tx/rx LEDs
> > - CAN_LED_EVENT_TX: trigger tx LED blink
> > - CAN_LED_EVENT_RX: trigger tx LED blink
> >
> > Cc: Oliver Hartkopp <[email protected]>
> > Cc: Wolfgang Grandegger <[email protected]>
> > Cc: Marc Kleine-Budde <[email protected]>
> > Signed-off-by: Fabio Baltieri <[email protected]>
> > ---
> >
> > Hi!
> >
> > So, here is the v4.
> >
> > Changes against v3:
> > - add a netdev_err message in kasprintf error path
> > - implement a devres_alloc release function to de-allocate strings and trigger
> > - can_led_exit is now unneeded and gone
> >
> > Is that what you had in mind Marc?
>
> Yes, that's it. Can you add a devm_ prefix to can_led_init() please.
Okay.
>
> > I like the devres implementation, as callers don't have to care about exit
> > function anymore... still it looks like a bit of an hack to me.
>
> No :)
Fair enough :)
> > Also, I made an initial version with the four pointers in a separate can_led
> > struct to be allocated as a payload of devres_alloc - but I think this one
> > looks cleaner and is more cache-friendly.
>
> If you allocate the 4 pointers in a separate struct you only save 3
> points, as you need the pointer to the struct anyways.
>
> > What you think about this?
>
> Looks good. See comment inline,
>
> Marc
>
> > +/*
> > + * Register CAN LED triggers for a CAN device
> > + *
> > + * This is normally called from a driver's probe function
> > + */
> > +void can_led_init(struct net_device *netdev)
> > +{
> > + struct can_priv *priv = netdev_priv(netdev);
> > + void *res;
> > +
> > + res = devres_alloc(can_led_release, 0, GFP_KERNEL);
> ^
> I'm not really sure if this is working. For example, pinctrl [1]
> allocates a double pointer here. The res pointer here and in
> can_led_release simply points to invalid memory. But as long as you
> don't dereference it, it should work.
>
> [1] http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L862
Actually that's also used by libata-core (not in lxr yet) and dma-mapping:
http://lxr.free-electrons.com/source/drivers/base/dma-mapping.c#L193
actually, res should point at the end of some internal devres structure,
and is only used as return value in this case.
Of course, in this case the release function can only use the struct
device pointer.
I've run some fail test and ftraced the whole thing and it seems to work
pretty good! I'll send a v5 with the rename later.
Thanks,
Fabio
On 08/01/2012 12:30 PM, Fabio Baltieri wrote:
[...]
>>> +void can_led_init(struct net_device *netdev)
>>> +{
>>> + struct can_priv *priv = netdev_priv(netdev);
>>> + void *res;
>>> +
>>> + res = devres_alloc(can_led_release, 0, GFP_KERNEL);
>> ^
>> I'm not really sure if this is working. For example, pinctrl [1]
>> allocates a double pointer here. The res pointer here and in
>> can_led_release simply points to invalid memory. But as long as you
>> don't dereference it, it should work.
>>
>> [1] http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L862
>
> Actually that's also used by libata-core (not in lxr yet) and dma-mapping:
>
> http://lxr.free-electrons.com/source/drivers/base/dma-mapping.c#L193
>
> actually, res should point at the end of some internal devres structure,
> and is only used as return value in this case.
>
> Of course, in this case the release function can only use the struct
> device pointer.
>
> I've run some fail test and ftraced the whole thing and it seems to work
> pretty good! I'll send a v5 with the rename later.
Thanks for checking. I just noticed, the pinctrl usecase is a bit different.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
This patch implements the functions to add two LED triggers, named
<ifname>-tx and <ifname>-rx, to a canbus device driver.
Triggers are called from specific handlers by each CAN device driver and
can be disabled altogether with a Kconfig option.
The implementation keeps the LED on when the interface is UP and blinks
the LED on network activity at a configurable rate.
This only supports can-dev based drivers, as it uses some support field
in the can_priv structure.
Supported drivers should call devm_can_led_init() and can_led_event() as
needed.
Cleanup is handled automatically by devres, so no *_exit function is
needed.
Supported events are:
- CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
- CAN_LED_EVENT_STOP: turn off tx/rx LEDs
- CAN_LED_EVENT_TX: trigger tx LED blink
- CAN_LED_EVENT_RX: trigger tx LED blink
Cc: Oliver Hartkopp <[email protected]>
Cc: Wolfgang Grandegger <[email protected]>
Cc: Marc Kleine-Budde <[email protected]>
Signed-off-by: Fabio Baltieri <[email protected]>
---
Hello,
so, v5, renamed can_led_init to devm_can_led_init like other devres based
functions.
Also, I'm reposting implementation into flexcan in 2/2... I have a small
patchset for others embedded can drivers to be tested which I can post in
another thread if this patch is accepted.
Fabio
drivers/net/can/Kconfig | 12 ++++++
drivers/net/can/Makefile | 2 +
drivers/net/can/led.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
include/linux/can/dev.h | 8 ++++
include/linux/can/led.h | 34 ++++++++++++++++
5 files changed, 158 insertions(+)
create mode 100644 drivers/net/can/led.c
create mode 100644 include/linux/can/led.h
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index bb709fd..19dec19 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -54,6 +54,18 @@ config CAN_CALC_BITTIMING
arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
If unsure, say Y.
+config CAN_LEDS
+ bool "Enable LED triggers for Netlink based drivers"
+ depends on CAN_DEV
+ depends on LEDS_CLASS
+ select LEDS_TRIGGERS
+ ---help---
+ This option adds two LED triggers for packet receive and transmit
+ events on each supported CAN device.
+
+ Say Y here if you are working on a system with led-class supported
+ LEDs and you want to use them as canbus activity indicators.
+
config CAN_AT91
tristate "Atmel AT91 onchip CAN controller"
depends on CAN_DEV && (ARCH_AT91SAM9263 || ARCH_AT91SAM9X5)
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 938be37..24ee98b 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -8,6 +8,8 @@ obj-$(CONFIG_CAN_SLCAN) += slcan.o
obj-$(CONFIG_CAN_DEV) += can-dev.o
can-dev-y := dev.o
+can-dev-$(CONFIG_CAN_LEDS) += led.o
+
obj-y += usb/
obj-y += softing/
diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
new file mode 100644
index 0000000..8b4f6a9
--- /dev/null
+++ b/drivers/net/can/led.c
@@ -0,0 +1,102 @@
+/*
+ * Copyright 2012, Fabio Baltieri <[email protected]>
+ *
+ * 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/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/netdevice.h>
+#include <linux/can/dev.h>
+
+#include <linux/can/led.h>
+
+static unsigned long led_delay = 50;
+module_param(led_delay, ulong, 0644);
+MODULE_PARM_DESC(led_delay,
+ "blink delay time for activity leds (msecs, default: 50).");
+
+/*
+ * Trigger a LED event in response to a CAN device event
+ */
+void can_led_event(struct net_device *netdev, enum can_led_event event)
+{
+ struct can_priv *priv = netdev_priv(netdev);
+
+ switch (event) {
+ case CAN_LED_EVENT_OPEN:
+ led_trigger_event(priv->tx_led_trig, LED_FULL);
+ led_trigger_event(priv->rx_led_trig, LED_FULL);
+ break;
+ case CAN_LED_EVENT_STOP:
+ led_trigger_event(priv->tx_led_trig, LED_OFF);
+ led_trigger_event(priv->rx_led_trig, LED_OFF);
+ break;
+ case CAN_LED_EVENT_TX:
+ if (led_delay)
+ led_trigger_blink_oneshot(priv->tx_led_trig,
+ &led_delay, &led_delay, 1);
+ break;
+ case CAN_LED_EVENT_RX:
+ if (led_delay)
+ led_trigger_blink_oneshot(priv->rx_led_trig,
+ &led_delay, &led_delay, 1);
+ break;
+ }
+}
+EXPORT_SYMBOL_GPL(can_led_event);
+
+static void can_led_release(struct device *gendev, void *res)
+{
+ struct can_priv *priv = netdev_priv(to_net_dev(gendev));
+
+ led_trigger_unregister_simple(priv->tx_led_trig);
+ led_trigger_unregister_simple(priv->rx_led_trig);
+
+ kfree(priv->tx_led_trig_name);
+ kfree(priv->rx_led_trig_name);
+}
+
+/*
+ * Register CAN LED triggers for a CAN device
+ *
+ * This is normally called from a driver's probe function
+ */
+void devm_can_led_init(struct net_device *netdev)
+{
+ struct can_priv *priv = netdev_priv(netdev);
+ void *res;
+
+ res = devres_alloc(can_led_release, 0, GFP_KERNEL);
+ if (!res)
+ goto err_out;
+
+ priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
+ if (!priv->tx_led_trig_name)
+ goto err_out;
+
+ priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
+ if (!priv->rx_led_trig_name)
+ goto err_out_rx;
+
+ led_trigger_register_simple(priv->tx_led_trig_name,
+ &priv->tx_led_trig);
+ led_trigger_register_simple(priv->rx_led_trig_name,
+ &priv->rx_led_trig);
+
+ devres_add(&netdev->dev, res);
+
+ return;
+
+err_out_rx:
+ kfree(priv->tx_led_trig_name);
+ priv->tx_led_trig_name = NULL;
+err_out:
+ netdev_err(netdev, "cannot register LED triggers\n");
+ devres_free(res);
+}
+EXPORT_SYMBOL_GPL(devm_can_led_init);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 2b2fc34..167b04a 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -16,6 +16,7 @@
#include <linux/can.h>
#include <linux/can/netlink.h>
#include <linux/can/error.h>
+#include <linux/can/led.h>
/*
* CAN mode
@@ -52,6 +53,13 @@ struct can_priv {
unsigned int echo_skb_max;
struct sk_buff **echo_skb;
+
+#ifdef CONFIG_CAN_LEDS
+ struct led_trigger *tx_led_trig;
+ char *tx_led_trig_name;
+ struct led_trigger *rx_led_trig;
+ char *rx_led_trig_name;
+#endif
};
/*
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
new file mode 100644
index 0000000..946059c
--- /dev/null
+++ b/include/linux/can/led.h
@@ -0,0 +1,34 @@
+/*
+ * Copyright 2012, Fabio Baltieri <[email protected]>
+ *
+ * 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.
+ */
+
+#ifndef CAN_LED_H
+#define CAN_LED_H
+
+#include <linux/leds.h>
+
+enum can_led_event {
+ CAN_LED_EVENT_OPEN,
+ CAN_LED_EVENT_STOP,
+ CAN_LED_EVENT_TX,
+ CAN_LED_EVENT_RX,
+};
+
+#ifdef CONFIG_CAN_LEDS
+void can_led_event(struct net_device *netdev, enum can_led_event event);
+void devm_can_led_init(struct net_device *netdev);
+#else
+static inline void can_led_event(struct net_device *netdev,
+ enum can_led_event event)
+{
+}
+static inline void devm_can_led_init(struct net_device *netdev)
+{
+}
+#endif
+
+#endif
--
1.7.11.rc1.9.gf623ca1.dirty
Add support for canbus activity led indicators on flexcan devices by
calling appropriate can_led_* functions.
These are only enabled when CONFIG_CAN_LEDS is Y, becomes no-op
otherwise.
Cc: Oliver Hartkopp <[email protected]>
Cc: Wolfgang Grandegger <[email protected]>
Cc: Marc Kleine-Budde <[email protected]>
Signed-off-by: Fabio Baltieri <[email protected]>
---
drivers/net/can/flexcan.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index c5f1431..6467fc1 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -23,6 +23,7 @@
#include <linux/can.h>
#include <linux/can/dev.h>
#include <linux/can/error.h>
+#include <linux/can/led.h>
#include <linux/can/platform/flexcan.h>
#include <linux/clk.h>
#include <linux/delay.h>
@@ -547,6 +548,8 @@ static int flexcan_read_frame(struct net_device *dev)
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+ can_led_event(dev, CAN_LED_EVENT_RX);
+
return 1;
}
@@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
stats->tx_bytes += can_get_echo_skb(dev, 0);
stats->tx_packets++;
+ can_led_event(dev, CAN_LED_EVENT_TX);
flexcan_write((1 << FLEXCAN_TX_BUF_ID), ®s->iflag1);
netif_wake_queue(dev);
}
@@ -844,6 +848,9 @@ static int flexcan_open(struct net_device *dev)
err = flexcan_chip_start(dev);
if (err)
goto out_close;
+
+ can_led_event(dev, CAN_LED_EVENT_OPEN);
+
napi_enable(&priv->napi);
netif_start_queue(dev);
@@ -872,6 +879,8 @@ static int flexcan_close(struct net_device *dev)
close_candev(dev);
+ can_led_event(dev, CAN_LED_EVENT_STOP);
+
return 0;
}
@@ -1068,6 +1077,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
goto failed_register;
}
+ devm_can_led_init(dev);
+
dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
priv->base, dev->irq);
--
1.7.11.rc1.9.gf623ca1.dirty
On 08/01/2012 01:49 PM, Fabio Baltieri wrote:
> Add support for canbus activity led indicators on flexcan devices by
> calling appropriate can_led_* functions.
>
> These are only enabled when CONFIG_CAN_LEDS is Y, becomes no-op
> otherwise.
>
> Cc: Oliver Hartkopp <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Fabio Baltieri <[email protected]>
> ---
> drivers/net/can/flexcan.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index c5f1431..6467fc1 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -23,6 +23,7 @@
> #include <linux/can.h>
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> +#include <linux/can/led.h>
> #include <linux/can/platform/flexcan.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> @@ -547,6 +548,8 @@ static int flexcan_read_frame(struct net_device *dev)
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
>
> + can_led_event(dev, CAN_LED_EVENT_RX);
> +
> return 1;
> }
>
> @@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
> stats->tx_bytes += can_get_echo_skb(dev, 0);
> stats->tx_packets++;
> + can_led_event(dev, CAN_LED_EVENT_TX);
Should the led blink on TX or TX completion interrupt?
> flexcan_write((1 << FLEXCAN_TX_BUF_ID), ®s->iflag1);
> netif_wake_queue(dev);
> }
> @@ -844,6 +848,9 @@ static int flexcan_open(struct net_device *dev)
> err = flexcan_chip_start(dev);
> if (err)
> goto out_close;
> +
> + can_led_event(dev, CAN_LED_EVENT_OPEN);
> +
> napi_enable(&priv->napi);
> netif_start_queue(dev);
>
> @@ -872,6 +879,8 @@ static int flexcan_close(struct net_device *dev)
>
> close_candev(dev);
>
> + can_led_event(dev, CAN_LED_EVENT_STOP);
> +
> return 0;
> }
>
> @@ -1068,6 +1077,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
> goto failed_register;
> }
>
> + devm_can_led_init(dev);
> +
> dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
> priv->base, dev->irq);
>
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On 08/01/2012 01:49 PM, Fabio Baltieri wrote:
> This patch implements the functions to add two LED triggers, named
> <ifname>-tx and <ifname>-rx, to a canbus device driver.
>
> Triggers are called from specific handlers by each CAN device driver and
> can be disabled altogether with a Kconfig option.
>
> The implementation keeps the LED on when the interface is UP and blinks
> the LED on network activity at a configurable rate.
>
> This only supports can-dev based drivers, as it uses some support field
> in the can_priv structure.
>
> Supported drivers should call devm_can_led_init() and can_led_event() as
> needed.
>
> Cleanup is handled automatically by devres, so no *_exit function is
> needed.
>
> Supported events are:
> - CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
> - CAN_LED_EVENT_STOP: turn off tx/rx LEDs
> - CAN_LED_EVENT_TX: trigger tx LED blink
> - CAN_LED_EVENT_RX: trigger tx LED blink
>
> Cc: Oliver Hartkopp <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Fabio Baltieri <[email protected]>
Applied to can-next/master.
But as net-next is still closed, feel free to send Acked-by or
Tested-by. Or even code improvements.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Sorry for this potentially mangled mail from my webmail access ...
Fabio Baltieri <[email protected]> hat am 1. August 2012 um 13:49
geschrieben:
> +void devm_can_led_init(struct net_device *netdev)
> +{
> + struct can_priv *priv = netdev_priv(netdev);
> + void *res;
> +
> + res = devres_alloc(can_led_release, 0, GFP_KERNEL);
> + if (!res)
> + goto err_out;
> +
> + priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
IMO putting a string with 8 or 9 bytes into a separate kmalloc memory sniplet is
pretty oversized.
Ok - these functions provide to hide the complexitiy for allocating and storing
strings, which is
definitely fine for path names and these kind of strings that are not known in
length and probably
more than 100 bytes long.
But in this case i would suggest to allocate a fixed space in can_priv, as we
know the string length
very good (IFNAMSZ + strlen("-tx")) and there's no reason to get all the
overhead from three kmallocs
instead of one for that small memory allocations.
So i would suggest:
> @@ -52,6 +53,13 @@ struct can_priv {
>
> unsigned int echo_skb_max;
> struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> + struct led_trigger *tx_led_trig;
> + char *tx_led_trig_name;
char tx_led_trig_name[IFNAMSZ+4];
> + struct led_trigger *rx_led_trig;
> + char *rx_led_trig_name;
char rx_led_trig_name[IFNAMSZ+4];
> +#endif
> };
>
Just my two cents as i was in CC here :-)
Thanks for the cool LED support & best regards
Oliver
On 08/01/2012 02:06 PM, Oliver Hartkopp wrote:
> Sorry for this potentially mangled mail from my webmail access ...
>
> Fabio Baltieri <[email protected]> hat am 1. August 2012 um 13:49
> geschrieben:
>
>> +void devm_can_led_init(struct net_device *netdev)
>> +{
>> + struct can_priv *priv = netdev_priv(netdev);
>> + void *res;
>> +
>> + res = devres_alloc(can_led_release, 0, GFP_KERNEL);
>> + if (!res)
>> + goto err_out;
>> +
>> + priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
>
> IMO putting a string with 8 or 9 bytes into a separate kmalloc memory sniplet is
> pretty oversized.
> Ok - these functions provide to hide the complexitiy for allocating and storing
> strings, which is
> definitely fine for path names and these kind of strings that are not known in
> length and probably
> more than 100 bytes long.
>
> But in this case i would suggest to allocate a fixed space in can_priv, as we
> know the string length
> very good (IFNAMSZ + strlen("-tx")) and there's no reason to get all the
> overhead from three kmallocs
> instead of one for that small memory allocations.
>
> So i would suggest:
>
>> @@ -52,6 +53,13 @@ struct can_priv {
>>
>> unsigned int echo_skb_max;
>> struct sk_buff **echo_skb;
>> +
>> +#ifdef CONFIG_CAN_LEDS
>> + struct led_trigger *tx_led_trig;
>> + char *tx_led_trig_name;
>
> char tx_led_trig_name[IFNAMSZ+4];
>
>> + struct led_trigger *rx_led_trig;
>> + char *rx_led_trig_name;
>
> char rx_led_trig_name[IFNAMSZ+4];
>
>> +#endif
>> };
Sounds reasonable. Please Introduce a #define for "IFNAMSZ + 4" and
document where the 4 comes from. Send a v6 patch, I'll force update the
git repo, after you have Oliver's Acked-by.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On Wed, Aug 01, 2012 at 01:53:22PM +0200, Marc Kleine-Budde wrote:
[...]
> > @@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> > if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
> > stats->tx_bytes += can_get_echo_skb(dev, 0);
> > stats->tx_packets++;
> > + can_led_event(dev, CAN_LED_EVENT_TX);
>
> Should the led blink on TX or TX completion interrupt?
I'd say on complention interrupt, together with can_get_echo_skb().
That was briefly discussed with Oliver in my first patch:
http://article.gmane.org/gmane.linux.can/1007
Fabio
On 08/01/2012 02:24 PM, Fabio Baltieri wrote:
> On Wed, Aug 01, 2012 at 01:53:22PM +0200, Marc Kleine-Budde wrote:
> [...]
>>> @@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>> if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
>>> stats->tx_bytes += can_get_echo_skb(dev, 0);
>>> stats->tx_packets++;
>>> + can_led_event(dev, CAN_LED_EVENT_TX);
>>
>> Should the led blink on TX or TX completion interrupt?
>
> I'd say on complention interrupt, together with can_get_echo_skb().
Yes, good that we already agreed on this :D
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
This patch implements the functions to add two LED triggers, named
<ifname>-tx and <ifname>-rx, to a canbus device driver.
Triggers are called from specific handlers by each CAN device driver and
can be disabled altogether with a Kconfig option.
The implementation keeps the LED on when the interface is UP and blinks
the LED on network activity at a configurable rate.
This only supports can-dev based drivers, as it uses some support field
in the can_priv structure.
Supported drivers should call devm_can_led_init() and can_led_event() as
needed.
Cleanup is handled automatically by devres, so no *_exit function is
needed.
Supported events are:
- CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
- CAN_LED_EVENT_STOP: turn off tx/rx LEDs
- CAN_LED_EVENT_TX: trigger tx LED blink
- CAN_LED_EVENT_RX: trigger tx LED blink
Cc: Oliver Hartkopp <[email protected]>
Cc: Wolfgang Grandegger <[email protected]>
Cc: Marc Kleine-Budde <[email protected]>
Signed-off-by: Fabio Baltieri <[email protected]>
---
Hi all,
so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ + 4)
and removed kasprintf as suggested by Oliver (thanks!).
This also has the side effect of reducing the error path to just one check to
devres_alloc return value... nice!
I've put CAN_LED_NAME_SZ definition with the active function declaration,
but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
code, as it looks cleaner to me.
I'm not reposting the flexcan patch as it's not affected by the change.
Fabio
drivers/net/can/Kconfig | 12 +++++++
drivers/net/can/Makefile | 2 ++
drivers/net/can/led.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/can/dev.h | 8 +++++
include/linux/can/led.h | 42 +++++++++++++++++++++++
5 files changed, 153 insertions(+)
create mode 100644 drivers/net/can/led.c
create mode 100644 include/linux/can/led.h
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index bb709fd..19dec19 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -54,6 +54,18 @@ config CAN_CALC_BITTIMING
arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
If unsure, say Y.
+config CAN_LEDS
+ bool "Enable LED triggers for Netlink based drivers"
+ depends on CAN_DEV
+ depends on LEDS_CLASS
+ select LEDS_TRIGGERS
+ ---help---
+ This option adds two LED triggers for packet receive and transmit
+ events on each supported CAN device.
+
+ Say Y here if you are working on a system with led-class supported
+ LEDs and you want to use them as canbus activity indicators.
+
config CAN_AT91
tristate "Atmel AT91 onchip CAN controller"
depends on CAN_DEV && (ARCH_AT91SAM9263 || ARCH_AT91SAM9X5)
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 938be37..24ee98b 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -8,6 +8,8 @@ obj-$(CONFIG_CAN_SLCAN) += slcan.o
obj-$(CONFIG_CAN_DEV) += can-dev.o
can-dev-y := dev.o
+can-dev-$(CONFIG_CAN_LEDS) += led.o
+
obj-y += usb/
obj-y += softing/
diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
new file mode 100644
index 0000000..eaa14ac
--- /dev/null
+++ b/drivers/net/can/led.c
@@ -0,0 +1,89 @@
+/*
+ * Copyright 2012, Fabio Baltieri <[email protected]>
+ *
+ * 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/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/netdevice.h>
+#include <linux/can/dev.h>
+
+#include <linux/can/led.h>
+
+static unsigned long led_delay = 50;
+module_param(led_delay, ulong, 0644);
+MODULE_PARM_DESC(led_delay,
+ "blink delay time for activity leds (msecs, default: 50).");
+
+/*
+ * Trigger a LED event in response to a CAN device event
+ */
+void can_led_event(struct net_device *netdev, enum can_led_event event)
+{
+ struct can_priv *priv = netdev_priv(netdev);
+
+ switch (event) {
+ case CAN_LED_EVENT_OPEN:
+ led_trigger_event(priv->tx_led_trig, LED_FULL);
+ led_trigger_event(priv->rx_led_trig, LED_FULL);
+ break;
+ case CAN_LED_EVENT_STOP:
+ led_trigger_event(priv->tx_led_trig, LED_OFF);
+ led_trigger_event(priv->rx_led_trig, LED_OFF);
+ break;
+ case CAN_LED_EVENT_TX:
+ if (led_delay)
+ led_trigger_blink_oneshot(priv->tx_led_trig,
+ &led_delay, &led_delay, 1);
+ break;
+ case CAN_LED_EVENT_RX:
+ if (led_delay)
+ led_trigger_blink_oneshot(priv->rx_led_trig,
+ &led_delay, &led_delay, 1);
+ break;
+ }
+}
+EXPORT_SYMBOL_GPL(can_led_event);
+
+static void can_led_release(struct device *gendev, void *res)
+{
+ struct can_priv *priv = netdev_priv(to_net_dev(gendev));
+
+ led_trigger_unregister_simple(priv->tx_led_trig);
+ led_trigger_unregister_simple(priv->rx_led_trig);
+}
+
+/*
+ * Register CAN LED triggers for a CAN device
+ *
+ * This is normally called from a driver's probe function
+ */
+void devm_can_led_init(struct net_device *netdev)
+{
+ struct can_priv *priv = netdev_priv(netdev);
+ void *res;
+
+ res = devres_alloc(can_led_release, 0, GFP_KERNEL);
+ if (!res) {
+ netdev_err(netdev, "cannot register LED triggers\n");
+ return;
+ }
+
+ snprintf(priv->tx_led_trig_name, sizeof(priv->tx_led_trig_name),
+ "%s-tx", netdev->name);
+ snprintf(priv->rx_led_trig_name, sizeof(priv->rx_led_trig_name),
+ "%s-rx", netdev->name);
+
+ led_trigger_register_simple(priv->tx_led_trig_name,
+ &priv->tx_led_trig);
+ led_trigger_register_simple(priv->rx_led_trig_name,
+ &priv->rx_led_trig);
+
+ devres_add(&netdev->dev, res);
+}
+EXPORT_SYMBOL_GPL(devm_can_led_init);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 2b2fc34..7747d9b 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -16,6 +16,7 @@
#include <linux/can.h>
#include <linux/can/netlink.h>
#include <linux/can/error.h>
+#include <linux/can/led.h>
/*
* CAN mode
@@ -52,6 +53,13 @@ struct can_priv {
unsigned int echo_skb_max;
struct sk_buff **echo_skb;
+
+#ifdef CONFIG_CAN_LEDS
+ struct led_trigger *tx_led_trig;
+ char tx_led_trig_name[CAN_LED_NAME_SZ];
+ struct led_trigger *rx_led_trig;
+ char rx_led_trig_name[CAN_LED_NAME_SZ];
+#endif
};
/*
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
new file mode 100644
index 0000000..12d5549
--- /dev/null
+++ b/include/linux/can/led.h
@@ -0,0 +1,42 @@
+/*
+ * Copyright 2012, Fabio Baltieri <[email protected]>
+ *
+ * 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.
+ */
+
+#ifndef CAN_LED_H
+#define CAN_LED_H
+
+#include <linux/if.h>
+#include <linux/leds.h>
+
+enum can_led_event {
+ CAN_LED_EVENT_OPEN,
+ CAN_LED_EVENT_STOP,
+ CAN_LED_EVENT_TX,
+ CAN_LED_EVENT_RX,
+};
+
+#ifdef CONFIG_CAN_LEDS
+
+/* keep space for interface name + "-tx"/"-rx" suffix and null terminator */
+#define CAN_LED_NAME_SZ (IFNAMSIZ + 4)
+
+void can_led_event(struct net_device *netdev, enum can_led_event event);
+void devm_can_led_init(struct net_device *netdev);
+
+#else
+
+static inline void can_led_event(struct net_device *netdev,
+ enum can_led_event event)
+{
+}
+static inline void devm_can_led_init(struct net_device *netdev)
+{
+}
+
+#endif
+
+#endif
--
1.7.11.rc1.9.gf623ca1.dirty
On 08/01/2012 08:21 PM, Fabio Baltieri wrote:
> This patch implements the functions to add two LED triggers, named
> <ifname>-tx and <ifname>-rx, to a canbus device driver.
>
> Triggers are called from specific handlers by each CAN device driver and
> can be disabled altogether with a Kconfig option.
>
> The implementation keeps the LED on when the interface is UP and blinks
> the LED on network activity at a configurable rate.
>
> This only supports can-dev based drivers, as it uses some support field
> in the can_priv structure.
>
> Supported drivers should call devm_can_led_init() and can_led_event() as
> needed.
>
> Cleanup is handled automatically by devres, so no *_exit function is
> needed.
>
> Supported events are:
> - CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
> - CAN_LED_EVENT_STOP: turn off tx/rx LEDs
> - CAN_LED_EVENT_TX: trigger tx LED blink
> - CAN_LED_EVENT_RX: trigger tx LED blink
>
> Cc: Oliver Hartkopp <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Fabio Baltieri <[email protected]>
> ---
>
> Hi all,
>
> so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ + 4)
> and removed kasprintf as suggested by Oliver (thanks!).
>
> This also has the side effect of reducing the error path to just one check to
> devres_alloc return value... nice!
>
> I've put CAN_LED_NAME_SZ definition with the active function declaration,
> but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
> code, as it looks cleaner to me.
>
> I'm not reposting the flexcan patch as it's not affected by the change.
Pushed to can-next/master, it even compiles now, as David has included
some upstream branches.
I'm still taking Tested- and Acked-by for these patches.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On 08/01/2012 01:49 PM, Fabio Baltieri wrote:
> Add support for canbus activity led indicators on flexcan devices by
> calling appropriate can_led_* functions.
>
> These are only enabled when CONFIG_CAN_LEDS is Y, becomes no-op
> otherwise.
>
> Cc: Oliver Hartkopp <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Fabio Baltieri <[email protected]>
> ---
Applied to can-next/master
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
Fabio Baltieri <[email protected]> hat am 1. August 2012 um 20:21
geschrieben:
> so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ +
> 4)
> and removed kasprintf as suggested by Oliver (thanks!).
>
> This also has the side effect of reducing the error path to just one check to
> devres_alloc return value... nice!
>
> I've put CAN_LED_NAME_SZ definition with the active function declaration,
> but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
> code, as it looks cleaner to me.
Yes. Nice improvement.
Thanks Fabio!
Acked-by: Oliver Hartkopp <[email protected]>
On Wed, Aug 01, 2012 at 11:00:04PM +0200, Marc Kleine-Budde wrote:
> On 08/01/2012 08:21 PM, Fabio Baltieri wrote:
> > This patch implements the functions to add two LED triggers, named
> > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> >
> > Triggers are called from specific handlers by each CAN device driver and
> > can be disabled altogether with a Kconfig option.
> >
> > The implementation keeps the LED on when the interface is UP and blinks
> > the LED on network activity at a configurable rate.
> >
> > This only supports can-dev based drivers, as it uses some support field
> > in the can_priv structure.
> >
> > Supported drivers should call devm_can_led_init() and can_led_event() as
> > needed.
> >
> > Cleanup is handled automatically by devres, so no *_exit function is
> > needed.
> >
> > Supported events are:
> > - CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
> > - CAN_LED_EVENT_STOP: turn off tx/rx LEDs
> > - CAN_LED_EVENT_TX: trigger tx LED blink
> > - CAN_LED_EVENT_RX: trigger tx LED blink
> >
> > Cc: Oliver Hartkopp <[email protected]>
> > Cc: Wolfgang Grandegger <[email protected]>
> > Cc: Marc Kleine-Budde <[email protected]>
> > Signed-off-by: Fabio Baltieri <[email protected]>
> > ---
> >
> > Hi all,
> >
> > so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ + 4)
> > and removed kasprintf as suggested by Oliver (thanks!).
> >
> > This also has the side effect of reducing the error path to just one check to
> > devres_alloc return value... nice!
> >
> > I've put CAN_LED_NAME_SZ definition with the active function declaration,
> > but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
> > code, as it looks cleaner to me.
> >
> > I'm not reposting the flexcan patch as it's not affected by the change.
>
> Pushed to can-next/master, it even compiles now, as David has included
> some upstream branches.
>
> I'm still taking Tested- and Acked-by for these patches.
Nice! So, I'll start preparing some patch for other embedded CAN
controllers for test/review by developers who have access to the
actual hardware.
In the meantime, thanks to everyone on the list for reviews and ideas!
Cheers!
Fabio
Hello,
I find the CAN led triggers an interesting thing.
And then, this scenario fell crossed my mind:
Imagine I do:
[insert CAN device: can0]
$ ip link set can0 name helga
[insert another CAN device: again 'can0']
Registering 'can0-tx' led trigger will fail for the second CAN device,
since that led trigger name is already reserved for CAN device 'helga'.
I'm not sure how to fix such.
If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
Just wild thinking ...
Kind regards,
Kurt
On Wed, Aug 01, 2012 at 08:21:38PM +0200, Fabio Baltieri wrote:
> This patch implements the functions to add two LED triggers, named
> <ifname>-tx and <ifname>-rx, to a canbus device driver.
>
> Triggers are called from specific handlers by each CAN device driver and
> can be disabled altogether with a Kconfig option.
>
> The implementation keeps the LED on when the interface is UP and blinks
> the LED on network activity at a configurable rate.
>
> This only supports can-dev based drivers, as it uses some support field
> in the can_priv structure.
>
> Supported drivers should call devm_can_led_init() and can_led_event() as
> needed.
>
> Cleanup is handled automatically by devres, so no *_exit function is
> needed.
>
> Supported events are:
> - CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
> - CAN_LED_EVENT_STOP: turn off tx/rx LEDs
> - CAN_LED_EVENT_TX: trigger tx LED blink
> - CAN_LED_EVENT_RX: trigger tx LED blink
>
> Cc: Oliver Hartkopp <[email protected]>
> Cc: Wolfgang Grandegger <[email protected]>
> Cc: Marc Kleine-Budde <[email protected]>
> Signed-off-by: Fabio Baltieri <[email protected]>
> ---
>
> Hi all,
>
> so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ + 4)
> and removed kasprintf as suggested by Oliver (thanks!).
>
> This also has the side effect of reducing the error path to just one check to
> devres_alloc return value... nice!
>
> I've put CAN_LED_NAME_SZ definition with the active function declaration,
> but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
> code, as it looks cleaner to me.
>
> I'm not reposting the flexcan patch as it's not affected by the change.
>
> Fabio
>
> drivers/net/can/Kconfig | 12 +++++++
> drivers/net/can/Makefile | 2 ++
> drivers/net/can/led.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/can/dev.h | 8 +++++
> include/linux/can/led.h | 42 +++++++++++++++++++++++
> 5 files changed, 153 insertions(+)
> create mode 100644 drivers/net/can/led.c
> create mode 100644 include/linux/can/led.h
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index bb709fd..19dec19 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -54,6 +54,18 @@ config CAN_CALC_BITTIMING
> arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
> If unsure, say Y.
>
> +config CAN_LEDS
> + bool "Enable LED triggers for Netlink based drivers"
> + depends on CAN_DEV
> + depends on LEDS_CLASS
> + select LEDS_TRIGGERS
> + ---help---
> + This option adds two LED triggers for packet receive and transmit
> + events on each supported CAN device.
> +
> + Say Y here if you are working on a system with led-class supported
> + LEDs and you want to use them as canbus activity indicators.
> +
> config CAN_AT91
> tristate "Atmel AT91 onchip CAN controller"
> depends on CAN_DEV && (ARCH_AT91SAM9263 || ARCH_AT91SAM9X5)
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 938be37..24ee98b 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -8,6 +8,8 @@ obj-$(CONFIG_CAN_SLCAN) += slcan.o
> obj-$(CONFIG_CAN_DEV) += can-dev.o
> can-dev-y := dev.o
>
> +can-dev-$(CONFIG_CAN_LEDS) += led.o
> +
> obj-y += usb/
> obj-y += softing/
>
> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
> new file mode 100644
> index 0000000..eaa14ac
> --- /dev/null
> +++ b/drivers/net/can/led.c
> @@ -0,0 +1,89 @@
> +/*
> + * Copyright 2012, Fabio Baltieri <[email protected]>
> + *
> + * 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/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/netdevice.h>
> +#include <linux/can/dev.h>
> +
> +#include <linux/can/led.h>
> +
> +static unsigned long led_delay = 50;
> +module_param(led_delay, ulong, 0644);
> +MODULE_PARM_DESC(led_delay,
> + "blink delay time for activity leds (msecs, default: 50).");
> +
> +/*
> + * Trigger a LED event in response to a CAN device event
> + */
> +void can_led_event(struct net_device *netdev, enum can_led_event event)
> +{
> + struct can_priv *priv = netdev_priv(netdev);
> +
> + switch (event) {
> + case CAN_LED_EVENT_OPEN:
> + led_trigger_event(priv->tx_led_trig, LED_FULL);
> + led_trigger_event(priv->rx_led_trig, LED_FULL);
> + break;
> + case CAN_LED_EVENT_STOP:
> + led_trigger_event(priv->tx_led_trig, LED_OFF);
> + led_trigger_event(priv->rx_led_trig, LED_OFF);
> + break;
> + case CAN_LED_EVENT_TX:
> + if (led_delay)
> + led_trigger_blink_oneshot(priv->tx_led_trig,
> + &led_delay, &led_delay, 1);
> + break;
> + case CAN_LED_EVENT_RX:
> + if (led_delay)
> + led_trigger_blink_oneshot(priv->rx_led_trig,
> + &led_delay, &led_delay, 1);
> + break;
> + }
> +}
> +EXPORT_SYMBOL_GPL(can_led_event);
> +
> +static void can_led_release(struct device *gendev, void *res)
> +{
> + struct can_priv *priv = netdev_priv(to_net_dev(gendev));
> +
> + led_trigger_unregister_simple(priv->tx_led_trig);
> + led_trigger_unregister_simple(priv->rx_led_trig);
> +}
> +
> +/*
> + * Register CAN LED triggers for a CAN device
> + *
> + * This is normally called from a driver's probe function
> + */
> +void devm_can_led_init(struct net_device *netdev)
> +{
> + struct can_priv *priv = netdev_priv(netdev);
> + void *res;
> +
> + res = devres_alloc(can_led_release, 0, GFP_KERNEL);
> + if (!res) {
> + netdev_err(netdev, "cannot register LED triggers\n");
> + return;
> + }
> +
> + snprintf(priv->tx_led_trig_name, sizeof(priv->tx_led_trig_name),
> + "%s-tx", netdev->name);
> + snprintf(priv->rx_led_trig_name, sizeof(priv->rx_led_trig_name),
> + "%s-rx", netdev->name);
> +
> + led_trigger_register_simple(priv->tx_led_trig_name,
> + &priv->tx_led_trig);
> + led_trigger_register_simple(priv->rx_led_trig_name,
> + &priv->rx_led_trig);
> +
> + devres_add(&netdev->dev, res);
> +}
> +EXPORT_SYMBOL_GPL(devm_can_led_init);
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 2b2fc34..7747d9b 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -16,6 +16,7 @@
> #include <linux/can.h>
> #include <linux/can/netlink.h>
> #include <linux/can/error.h>
> +#include <linux/can/led.h>
>
> /*
> * CAN mode
> @@ -52,6 +53,13 @@ struct can_priv {
>
> unsigned int echo_skb_max;
> struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> + struct led_trigger *tx_led_trig;
> + char tx_led_trig_name[CAN_LED_NAME_SZ];
> + struct led_trigger *rx_led_trig;
> + char rx_led_trig_name[CAN_LED_NAME_SZ];
> +#endif
> };
>
> /*
> diff --git a/include/linux/can/led.h b/include/linux/can/led.h
> new file mode 100644
> index 0000000..12d5549
> --- /dev/null
> +++ b/include/linux/can/led.h
> @@ -0,0 +1,42 @@
> +/*
> + * Copyright 2012, Fabio Baltieri <[email protected]>
> + *
> + * 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.
> + */
> +
> +#ifndef CAN_LED_H
> +#define CAN_LED_H
> +
> +#include <linux/if.h>
> +#include <linux/leds.h>
> +
> +enum can_led_event {
> + CAN_LED_EVENT_OPEN,
> + CAN_LED_EVENT_STOP,
> + CAN_LED_EVENT_TX,
> + CAN_LED_EVENT_RX,
> +};
> +
> +#ifdef CONFIG_CAN_LEDS
> +
> +/* keep space for interface name + "-tx"/"-rx" suffix and null terminator */
> +#define CAN_LED_NAME_SZ (IFNAMSIZ + 4)
> +
> +void can_led_event(struct net_device *netdev, enum can_led_event event);
> +void devm_can_led_init(struct net_device *netdev);
> +
> +#else
> +
> +static inline void can_led_event(struct net_device *netdev,
> + enum can_led_event event)
> +{
> +}
> +static inline void devm_can_led_init(struct net_device *netdev)
> +{
> +}
> +
> +#endif
> +
> +#endif
> --
> 1.7.11.rc1.9.gf623ca1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Kurt Van Dijck
GRAMMER EiA ELECTRONICS
http://www.eia.be
[email protected]
+32-38708534
On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> Hello,
>
> I find the CAN led triggers an interesting thing.
>
> And then, this scenario fell crossed my mind:
> Imagine I do:
> [insert CAN device: can0]
> $ ip link set can0 name helga
> [insert another CAN device: again 'can0']
>
> Registering 'can0-tx' led trigger will fail for the second CAN device,
> since that led trigger name is already reserved for CAN device 'helga'.
Good point.
> I'm not sure how to fix such.
> If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> Just wild thinking ...
I think the device's name (not netdev) is unique in the system and
cannot be changed.
On my device tree enabled mx28 I'm talking about the "80032000.can" in:
> dmesg |grep flexcan
> flexcan 80032000.can: device registered (reg_base=f5032000, irq=8)
> flexcan 80034000.can: device registered (reg_base=f5034000, irq=9)
> $ ls /sys/bus/platform/devices/*can* -d
> /sys/bus/platform/devices/80032000.can
> /sys/bus/platform/devices/80034000.can
regards, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
> On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> > Hello,
> >
> > I find the CAN led triggers an interesting thing.
> >
> > And then, this scenario fell crossed my mind:
> > Imagine I do:
> > [insert CAN device: can0]
> > $ ip link set can0 name helga
> > [insert another CAN device: again 'can0']
> >
> > Registering 'can0-tx' led trigger will fail for the second CAN device,
> > since that led trigger name is already reserved for CAN device 'helga'.
>
> Good point.
>
> > I'm not sure how to fix such.
> > If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> > Just wild thinking ...
>
> I think the device's name (not netdev) is unique in the system and
> cannot be changed.
but may contain several netdev's ...
>
> On my device tree enabled mx28 I'm talking about the "80032000.can" in:
You idea triggered another thougt: since control is put in device drivers,
why putting the name in the generic can_dev struct?
A more flexible approach to assign names is the key to success here.
The correct 'works in all conditions' approach is not yet in my sight :-(
Kurt
>
> regards, Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
--
Kurt Van Dijck
GRAMMER EiA ELECTRONICS
http://www.eia.be
[email protected]
+32-38708534
Hello Kurt,
On Fri, Aug 24, 2012 at 02:42:48PM +0200, Kurt Van Dijck wrote:
> On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
> > On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> > > Hello,
> > >
> > > I find the CAN led triggers an interesting thing.
> > >
> > > And then, this scenario fell crossed my mind:
> > > Imagine I do:
> > > [insert CAN device: can0]
> > > $ ip link set can0 name helga
> > > [insert another CAN device: again 'can0']
> > >
> > > Registering 'can0-tx' led trigger will fail for the second CAN device,
> > > since that led trigger name is already reserved for CAN device 'helga'.
> > Good point.
Yep, thanks for pointing that out!
Interface renaming was something I considered when I first wrote the
code and I had the mac80211-led driver in mind, as that driver uses the
phy name and not the netdev one for its triggers.
The reason why I did not care that much in the end is that on SoC based
systems trigger-led association is made at probe time, based on data
either from platform_data or devicetree, so I imagined that once the
kernel is ported to the board and default triggers are set correctly at
boot time, the userspace is free to rename CAN interfaces and nobody
should notice... :^)
The thing I did not consider are hot-plug interfaces mixed with
renaming, such as in the case you pointed out - it's probably not really
common but still possible.
> > > I'm not sure how to fix such.
> > > If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> > > Just wild thinking ...
> >
> > I think the device's name (not netdev) is unique in the system and
> > cannot be changed.
>
> but may contain several netdev's ...
Ouch.
>
> >
> > On my device tree enabled mx28 I'm talking about the "80032000.can" in:
>
> You idea triggered another thougt: since control is put in device drivers,
> why putting the name in the generic can_dev struct?
Why not? That makes the API easy.
> A more flexible approach to assign names is the key to success here.
> The correct 'works in all conditions' approach is not yet in my sight :-(
Agreed.
What about using a combination of device name + an optional port index
specified in devm_can_led_init()? (something like to platform_device names)
Of course that would require changing the API for libraries like
register_sja1000dev(), to add a port index.
Fabio
On Sat, Aug 25, 2012 at 12:01:42AM +0200, Fabio Baltieri wrote:
> Hello Kurt,
>
> On Fri, Aug 24, 2012 at 02:42:48PM +0200, Kurt Van Dijck wrote:
> > On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
> > > On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> > > > Hello,
> > > >
> > > > I find the CAN led triggers an interesting thing.
> > > >
> > > > And then, this scenario fell crossed my mind:
> > > > Imagine I do:
> > > > [insert CAN device: can0]
> > > > $ ip link set can0 name helga
> > > > [insert another CAN device: again 'can0']
> > > >
> > > > Registering 'can0-tx' led trigger will fail for the second CAN device,
> > > > since that led trigger name is already reserved for CAN device 'helga'.
> > > Good point.
>
> Yep, thanks for pointing that out!
>
> Interface renaming was something I considered when I first wrote the
> code and I had the mac80211-led driver in mind, as that driver uses the
> phy name and not the netdev one for its triggers.
>
> The reason why I did not care that much in the end is that on SoC based
> systems trigger-led association is made at probe time, based on data
> either from platform_data or devicetree, so I imagined that once the
> kernel is ported to the board and default triggers are set correctly at
> boot time, the userspace is free to rename CAN interfaces and nobody
> should notice... :^)
>
> The thing I did not consider are hot-plug interfaces mixed with
> renaming, such as in the case you pointed out - it's probably not really
> common but still possible.
>
> > > > I'm not sure how to fix such.
> > > > If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> > > > Just wild thinking ...
> > >
> > > I think the device's name (not netdev) is unique in the system and
> > > cannot be changed.
> >
> > but may contain several netdev's ...
>
> Ouch.
>
> >
> > >
> > > On my device tree enabled mx28 I'm talking about the "80032000.can" in:
> >
> > You idea triggered another thougt: since control is put in device drivers,
> > why putting the name in the generic can_dev struct?
>
> Why not? That makes the API easy.
The code is not next to the data?
Anyway, I said the above because I think the led names need a per-device approach,
but the data is in common parts. That's where things start being complicated.
I understand the common part now because you mainly addressed the SoC based drivers.
>
> > A more flexible approach to assign names is the key to success here.
> > The correct 'works in all conditions' approach is not yet in my sight :-(
>
> Agreed.
>
> What about using a combination of device name + an optional port index
> specified in devm_can_led_init()? (something like to platform_device names)
> Of course that would require changing the API for libraries like
> register_sja1000dev(), to add a port index.
>
> Fabio
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Kurt Van Dijck
GRAMMER EiA ELECTRONICS
http://www.eia.be
[email protected]
+32-38708534