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 <w@xxxxxx>
> > + * 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 <j.anaszewski@xxxxxxxxxxx>
> 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 <bo.zhang@xxxxxxx>
> 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
