lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <561528DE.4070507@aksignal.cz> Date: Wed, 7 Oct 2015 16:14:54 +0200 From: Jiří Prchal <jiri.prchal@...ignal.cz> To: Jacek Anaszewski <j.anaszewski@...sung.com> Cc: rpurdie@...ys.net, linux-leds@...r.kernel.org, cooloney@...il.com, kyungmin.park@...sung.com, linux-kernel@...r.kernel.org Subject: Re: [PATCH] leds: triggers: add invert to heartbeat Hi Jacek, comments below... On 7.10.2015 15:32, Jacek Anaszewski wrote: > Hi Jiri, > > Thanks for the patch. It's nice in general, but please see my > comments below. > > On 10/07/2015 11:31 AM, Jiri Prchal wrote: >> This patcht adds possibility to invert heartbeat blinking. The inverted LED is >> more time ON then OFF. >> It's because it looks better when the heartbeat LED is next to other LED which >> is most time ON. >> The invert value is exported same way via sysfs in file invert like oneshot. I >> get inspiration from this trigger. > > Please adjust commit message so as it wouldn't exceed 75 characters line > length limit. scripts/checkpatch.pl is useful for catching this kind of > problems. > >> Signed-off-by: Jiri Prchal <jiri.prchal@...ignal.cz> >> --- >> drivers/leds/trigger/ledtrig-heartbeat.c | 49 ++++++++++++++++++++++++++++++-- >> 1 file changed, 47 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/leds/trigger/ledtrig-heartbeat.c b/drivers/leds/trigger/ledtrig-heartbeat.c >> index fea6871..c09c30b 100644 >> --- a/drivers/leds/trigger/ledtrig-heartbeat.c >> +++ b/drivers/leds/trigger/ledtrig-heartbeat.c >> @@ -27,6 +27,7 @@ struct heartbeat_trig_data { >> unsigned int phase; >> unsigned int period; >> struct timer_list timer; >> + unsigned int invert; >> }; >> >> static void led_heartbeat_function(unsigned long data) >> @@ -56,21 +57,27 @@ static void led_heartbeat_function(unsigned long data) >> msecs_to_jiffies(heartbeat_data->period); >> delay = msecs_to_jiffies(70); >> heartbeat_data->phase++; >> - brightness = led_cdev->max_brightness; >> + if (!heartbeat_data->invert) >> + brightness = led_cdev->max_brightness; >> break; >> case 1: >> delay = heartbeat_data->period / 4 - msecs_to_jiffies(70); >> heartbeat_data->phase++; >> + if (heartbeat_data->invert) >> + brightness = led_cdev->max_brightness; > > As you are at it, could you change, in a separate patch, > led_cdev->max_brightness to led_cdev->brightness? I think there is no > reason for which we shouldn't allow other values than max. I changed it but it doesn't blink at all. > >> break; >> case 2: >> delay = msecs_to_jiffies(70); >> heartbeat_data->phase++; >> - brightness = led_cdev->max_brightness; >> + if (!heartbeat_data->invert) >> + brightness = led_cdev->max_brightness; >> break; >> default: >> delay = heartbeat_data->period - heartbeat_data->period / 4 - >> msecs_to_jiffies(70); >> heartbeat_data->phase = 0; >> + if (heartbeat_data->invert) >> + brightness = led_cdev->max_brightness; >> break; >> } >> >> @@ -78,15 +85,50 @@ static void led_heartbeat_function(unsigned long data) >> mod_timer(&heartbeat_data->timer, jiffies + delay); >> } >> >> +static ssize_t led_invert_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct led_classdev *led_cdev = dev_get_drvdata(dev); >> + struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data; >> + >> + return sprintf(buf, "%u\n", heartbeat_data->invert); >> +} >> + >> +static ssize_t led_invert_store(struct device *dev, >> + struct device_attribute *attr, const char *buf, size_t size) >> +{ >> + struct led_classdev *led_cdev = dev_get_drvdata(dev); >> + struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data; >> + unsigned long state; >> + int ret; >> + >> + ret = kstrtoul(buf, 0, &state); >> + if (ret) >> + return ret; >> + >> + heartbeat_data->invert = !!state; >> + >> + return size; >> +} >> + >> +static DEVICE_ATTR(invert, 0644, led_invert_show, led_invert_store); >> + >> static void heartbeat_trig_activate(struct led_classdev *led_cdev) >> { >> struct heartbeat_trig_data *heartbeat_data; >> + int rc; >> >> heartbeat_data = kzalloc(sizeof(*heartbeat_data), GFP_KERNEL); >> if (!heartbeat_data) >> return; >> >> led_cdev->trigger_data = heartbeat_data; >> + rc = device_create_file(led_cdev->dev, &dev_attr_invert); >> + if (rc) { >> + kfree(led_cdev->trigger_data); >> + return; >> + } >> + >> setup_timer(&heartbeat_data->timer, >> led_heartbeat_function, (unsigned long) led_cdev); >> heartbeat_data->phase = 0; >> @@ -100,9 +142,12 @@ static void heartbeat_trig_deactivate(struct led_classdev *led_cdev) >> >> if (led_cdev->activated) { >> del_timer_sync(&heartbeat_data->timer); >> + device_remove_file(led_cdev->dev, &dev_attr_invert); >> kfree(heartbeat_data); >> led_cdev->activated = false; >> } >> + >> + led_set_brightness(led_cdev, LED_OFF); > > I believe this is a fix. Could you split it into a separate patch > and explain its merit? I'm not sure of necessity of it, I copied it from oneshot. Without it could leave LED in ON state. But never happens to me. Should I produce separate patch to discuss it in separate thread? > >> } >> >> static struct led_trigger heartbeat_led_trigger = { >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists