On Mon, Jun 5, 2017 at 9:36 AM, Zhang Bo [off-list ref] wrote:

System cannot enter suspend mode because of heartbeat led trigger.

As mentioned this should not be a problem.

Heartbeat_pm_notifier is called when system prepare to enter suspend
mode, then call led_trigger_unregister to change the trigger of led device
to none. It will send uevent message and the wakeup source count changed.

Add suspend/resume ops to the struct led_trigger, implement these two ops
for ledtrig-heartbeat. Add led_trigger_suspend/led_trigger_resume funcitons
to iterate through all LED class devices registered on given trigger and
call their suspend/resume ops and disable/enable sysfs at the same time.

Call led_trigger_suspend{resuem} API in pm_notifier function to
suspend/resume all led devices listed in given trigger.

Signed-off-by: Zhang Bo <redacted>

The patch is nice because it fixes my sloppy approach of just adding/removing
the trigger made earlier.

It stops unsolicited I2C traffic and other nastines from happening during the
suspend/resume cycle.

I would even add:

Fixes: 5ab92a7cb82c ("leds: handle suspend/resume in heartbeat trigger")

(...)

+static void heartbeat_trig_resume(struct led_classdev *led_cdev)
+{
+       struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
+
+       if (led_cdev->suspended) {
+               setup_timer(&heartbeat_data->timer,
+                           led_heartbeat_function, (unsigned long) led_cdev);
+               heartbeat_data->phase = 0;
+               led_heartbeat_function(heartbeat_data->timer.data);
+               led_cdev->suspended = false;
+       }
+}
+
+static void heartbeat_trig_suspend(struct led_classdev *led_cdev)
+{
+       struct heartbeat_trig_data *heartbeat_data = led_cdev->trigger_data;
+
+       if (led_cdev->activated && !led_cdev->suspended) {
+               del_timer_sync(&heartbeat_data->timer);
+               led_cdev->suspended = true;
        }

Are we sure that the LED is *off* at this point so you don't suspend/resume
with a glowing LED?

Else insert a call to make sure that it's like so, and maybe even
a variable to hold the current state (off/on) across the suspend/resume
cycle.

(...)

+       void            (*suspend)(struct led_classdev *led_cdev);
+       void            (*resume)(struct led_classdev *led_cdev);

These names do not correspons to the actual PM calls
they are utilized for.

Name them
.pm_prepare_suspend()
.pm_post_suspend()

Instead, this indicates better the place when they get called.

Yours,
Linus Walleij