Hi Jacek, On Sun, Aug 27, 2017 at 06:44:05PM +0200, Jacek Anaszewski wrote: > Hi Willy, > > Thanks for the updated patch. > > One formal note: please send the patches with git send-email instead > of attaching them to the message.
Yep, I hesitated and wanted to reply. Will do it the other way next time, sorry for the hassle. > > diff --git a/drivers/leds/trigger/ledtrig-activity.c > > b/drivers/leds/trigger/ledtrig-activity.c > > new file mode 100644 > > index 0000000..6f00235 > > --- /dev/null > > +++ b/drivers/leds/trigger/ledtrig-activity.c > > @@ -0,0 +1,297 @@ > > +/* > > + * Activity LED trigger > > + * > > + * Copyright (C) 2017 Willy Tarreau <[email protected]> > > + * Partially based on Atsushi Nemoto's ledtrig-heartbeat.c. > > + * > > + * 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/kernel.h> > > +#include <linux/kernel_stat.h> > > +#include <linux/init.h> > > +#include <linux/slab.h> > > +#include <linux/timer.h> > > +#include <linux/sched.h> > > +#include <linux/leds.h> > > +#include <linux/reboot.h> > > +#include <linux/suspend.h> > > Please sort the includes alphabetically. I'm amazed I did this, I suspect I inherited it from the original file because I'm also used to annoy people for the same thing! Shame on me! > > + activity_data->time_left -= 100; > > + if (activity_data->time_left <= 0) { > > + activity_data->time_left = 0; > > + activity_data->state = !activity_data->state; > > + led_set_brightness_nosleep(led_cdev, > > + (activity_data->state ^ activity_data->invert) ? > > + led_cdev->max_brightness : LED_OFF); > > Have you considered making the top brightness adjustable? I'd make it > possible especially that we have a similar solution in the > ledtrig-heartbeat.c already - see the following patch in 4.12: > > commit fb3d769173d26268d7bf068094a599bb28b2ac63 > Author: Jacek Anaszewski <[email protected]> > Date: Wed Nov 9 11:43:46 2016 +0100 (...) I never thought about it and it makes a lot of sense actually. I'll check this commit, thanks for the pointer. > > + switch (pm_event) { > > + case PM_SUSPEND_PREPARE: > > + case PM_HIBERNATION_PREPARE: > > + case PM_RESTORE_PREPARE: > > + led_trigger_unregister(&activity_led_trigger); > > + break; > > + case PM_POST_SUSPEND: > > + case PM_POST_HIBERNATION: > > + case PM_POST_RESTORE: > > + rc = led_trigger_register(&activity_led_trigger); > > + if (rc) > > + pr_err("could not re-register activity trigger\n"); > > + break; > > + default: > > + break; > > + } > > + return NOTIFY_DONE; > > +} > > It turned out to cause problems in ledtrig-heartbeat.c and was reverted. > Please don't register pm notifier and remove related facilities from the > patch according to the following revert patch: > > commit 436c4c45b5b9562b59cedbb51b7343ab4a6dd8cc > Author: Zhang Bo <[email protected]> > Date: Tue Jun 13 10:39:20 2017 +0800 OK fine for me. I thought it was mandatory to properly handle pm eventhough I was not particularly interested in this for this specific purpose. I'll send you an updated patch ASAP. Thanks very much for your review, Willy
