From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH 3/3] leds: triggers: heartbeat: add RGB version of the trigger Date: Thu, 17 Mar 2016 14:41:30 +0100 Message-ID: <56EAB40A.3030004@samsung.com> References: <56E5A0DA.1000700@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mailout4.w1.samsung.com ([210.118.77.14]:56351 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932614AbcCQNlf (ORCPT ); Thu, 17 Mar 2016 09:41:35 -0400 Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O46000F5RD8J880@mailout4.w1.samsung.com> for linux-leds@vger.kernel.org; Thu, 17 Mar 2016 13:41:32 +0000 (GMT) In-reply-to: <56E5A0DA.1000700@gmail.com> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Heiner Kallweit Cc: "linux-leds@vger.kernel.org" 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? 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? 3. Why heartbeat_range.end is somehow related to num_online_cpus()? Best regards, Jacek Anaszewski 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 > --- > 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)); > +} > + > 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; > + > + 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; > > + 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); >