Re: [PATCH 3/3] leds: triggers: heartbeat: add RGB version of the trigger

[Jacek Anaszewski]

On 03/17/2016 09:04 PM, Heiner Kallweit wrote:
> Am 17.03.2016 um 14:41 schrieb Jacek Anaszewski:
> > Hi Heiner,
> >
> > Thanks for the patch. I have a few questions:
> >
> > 1. Why do you want to register another trigger in
> >     a ledtrig-heartbeat.c?
>       The existing trigger uses the heartbeat and can be used with RGB
>       and non-RGB LED's. The new trigger uses color to visualize the load
>       and can be used with RGB LED's only.
>
> > 2. heartbeat trigger implements LED blinking timing that
> >     resembles human heart beat rhythm, whereas heartbeat_led_rgb_trigger
> >     seems not to reuse led_heartbeat_function code, but instead
> >     led_heartbeat_rgb_function() sets timer interval to 500ms, after
> >     doing some color magic in led_trigger_range_event(). Where is the
> >     heartbeat rhythm implemented here?
>       It uses a color to display the load instead of a rhythm.
>       I have to admit that this trigger might not perfectly fit here as it
>       doesn't actually implement a heartbeat. However it visualizes the same
>       parameter (system load). If preferrable I can make it a separate trigger.

Having it in the ledtrig-heartbeat.c would make sense if we you applied
also the same blinking pattern. Is there some specific reason for which
you didn't reuse heartbeat blinking pattern, but instead used fixed
period?

> > 3. Why heartbeat_range.end is somehow related to num_online_cpus()?
>       Because e.g. 5 is a high load on a single CPU system but more or less
>       idle on a 64 CPU system.

I see, thanks for the explanation.

I have also few comments below.

> > On 03/13/2016 06:18 PM, Heiner Kallweit wrote:
> > > Add a RGB version of the heartbeat trigger (heartbeat-rgb). This version of
> > > the trigger can only be used with LED devices supporting the RGB extension.
> > >
> > > It maps the load [0 .. num_online_cpus] to a color between green and red.
> > >
> > > Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> > > ---
> > >    drivers/leds/trigger/ledtrig-heartbeat.c | 64 +++++++++++++++++++++++++++++++-
> > >    1 file changed, 63 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c
> > > index 410c39c..510a3c9 100644
> > > --- a/drivers/leds/trigger/ledtrig-heartbeat.c
> > > +++ b/drivers/leds/trigger/ledtrig-heartbeat.c
> > > @@ -22,6 +22,7 @@
> > >    #include "../leds.h"
> > >
> > >    static int panic_heartbeats;
> > > +static struct led_trigger heartbeat_led_rgb_trigger;
> > >
> > >    struct heartbeat_trig_data {
> > >        unsigned int phase;
> > > @@ -30,6 +31,14 @@ struct heartbeat_trig_data {
> > >        unsigned int invert;
> > >    };
> > >
> > > +static struct led_trigger_range led_heartbeat_range = {
> > > +    .start = 0,
> > > +    /* default, overridden by module init function */
> > > +    .end = 4 * FIXED_1,
> > > +    .color_start = 0x54ffff, /* GREEN */
> > > +    .color_end = 0x00ffff, /* RED */
> > > +};
> > > +
> > >    static void led_heartbeat_function(unsigned long data)
> > >    {
> > >        struct led_classdev *led_cdev = (struct led_classdev *) data;
> > > @@ -85,6 +94,15 @@ static void led_heartbeat_function(unsigned long data)
> > >        mod_timer(&heartbeat_data->timer, jiffies + delay);
> > >    }
> > >
> > > +static void led_heartbeat_rgb_function(unsigned long data)
> > > +{
> > > +    struct led_classdev *led_cdev = (struct led_classdev *) data;
> > > +    struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
> > > +
> > > +    led_trigger_range_event(&heartbeat_led_rgb_trigger, avenrun[0]);
> > > +    mod_timer(&heartbeat_data->timer, jiffies + msecs_to_jiffies(500));
> > > +}

This function could reuse the code from led_heartbeat_function(), that
calculates timer delay. That code could be made reusable by
both monochrome and rgb heartbeat trigger.

> > >    static ssize_t led_invert_show(struct device *dev,
> > >            struct device_attribute *attr, char *buf)
> > >    {
> > > @@ -136,6 +154,22 @@ static void heartbeat_trig_activate(struct led_classdev *led_cdev)
> > >        led_cdev->activated = true;
> > >    }
> > >
> > > +static void heartbeat_trig_rgb_activate(struct led_classdev *led_cdev)
> > > +{
> > > +    struct heartbeat_trig_data *heartbeat_data;
> > > +
> > > +    heartbeat_data = kzalloc(sizeof(*heartbeat_data), GFP_KERNEL);
> > > +    if (!heartbeat_data)
> > > +        return;
> > > +
> > > +    led_cdev->trigger_data = heartbeat_data;

How about adding struct rgb_heartbeat_trig_data comprising
two properties of types struct heartbeat_trig_data and
struct led_trigger_range respectively?
It would be used by the new led_heartbeat_rgb_function to
store blinking and color related data.

> > > +    setup_timer(&heartbeat_data->timer,
> > > +            led_heartbeat_rgb_function, (unsigned long) led_cdev);
> > > +    led_heartbeat_rgb_function(heartbeat_data->timer.data);
> > > +    led_cdev->activated = true;
> > > +}
> > > +
> > >    static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)
> > >    {
> > >        struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
> > > @@ -148,12 +182,31 @@ static void heartbeat_trig_deactivate(struct led_classdev *led_cdev)
> > >        }
> > >    }
> > >
> > > +static void heartbeat_trig_rgb_deactivate(struct led_classdev *led_cdev)
> > > +{
> > > +    struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
> > > +
> > > +    if (led_cdev->activated) {
> > > +        del_timer_sync(&heartbeat_data->timer);
> > > +        kfree(heartbeat_data);
> > > +        led_cdev->activated = false;
> > > +    }
> > > +}
> > > +
> > >    static struct led_trigger heartbeat_led_trigger = {
> > >        .name     = "heartbeat",
> > >        .activate = heartbeat_trig_activate,
> > >        .deactivate = heartbeat_trig_deactivate,
> > >    };
> > >
> > > +static struct led_trigger heartbeat_led_rgb_trigger = {
> > > +    .flags    = LED_TRIG_CAP_RGB,
> > > +    .name     = "heartbeat-rgb",
> > > +    .range    = &led_heartbeat_range,
> > > +    .activate = heartbeat_trig_rgb_activate,
> > > +    .deactivate = heartbeat_trig_rgb_deactivate,
> > > +};
> > > +
> > >    static int heartbeat_reboot_notifier(struct notifier_block *nb,
> > >                         unsigned long code, void *unused)
> > >    {
> > > @@ -178,9 +231,17 @@ static struct notifier_block heartbeat_panic_nb = {
> > >
> > >    static int __init heartbeat_trig_init(void)
> > >    {
> > > -    int rc = led_trigger_register(&heartbeat_led_trigger);
> > > +    int rc;
> > > +
> > > +    led_heartbeat_range.end = num_online_cpus() * FIXED_1;
> > > +
> > > +    rc = led_trigger_register(&heartbeat_led_rgb_trigger);
> > > +    if (rc)
> > > +        return rc;

With inherited blinking pattern rgb version will also have to
register the same notifiers as the monochrome one.

> > > +    rc = led_trigger_register(&heartbeat_led_trigger);
> > >        if (!rc) {
> > > +        led_trigger_unregister(&heartbeat_led_rgb_trigger);
> > >            atomic_notifier_chain_register(&panic_notifier_list,
> > >                               &heartbeat_panic_nb);
> > >            register_reboot_notifier(&heartbeat_reboot_nb);
> > > @@ -194,6 +255,7 @@ static void __exit heartbeat_trig_exit(void)
> > >        atomic_notifier_chain_unregister(&panic_notifier_list,
> > >                         &heartbeat_panic_nb);
> > >        led_trigger_unregister(&heartbeat_led_trigger);
> > > +    led_trigger_unregister(&heartbeat_led_rgb_trigger);
> > >    }
> > >
> > >    module_init(heartbeat_trig_init);
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
Best regards,
Jacek Anaszewski