On Mon, Nov 08, 2021 at 03:04:23PM +0100, Andrew Lunn wrote:

quoted

+static inline int led_trigger_offload(struct led_classdev *led_cdev)
+{
+	int ret;
+
+	if (!led_cdev->trigger_offload)
+		return -EOPNOTSUPP;
+
+	ret = led_cdev->trigger_offload(led_cdev, true);
+	led_cdev->offloaded = !ret;
+
+	return ret;
+}
+
+static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
+{
+	if (!led_cdev->trigger_offload)
+		return;
+
+	if (led_cdev->offloaded) {
+		led_cdev->trigger_offload(led_cdev, false);
+		led_cdev->offloaded = false;
+	}
+}
+#endif

I think there should be two calls into the cdev driver, not this
true/false parameter. trigger_offload_start() and
trigger_offload_stop().

To not add too much function to the struct, can we introduce one
function that both enable and disable the hw mode?

There are also a number of PHYs which don't allow software blinking of
the LED. So for them, trigger_offload_stop() is going to return
-EOPNOTSUPP. And you need to handle that correctly.

So we have PHYs that can only work in offload or off. Correct?

It would be go to also document the expectations of
trigger_offload_stop(). Should it leave the LED in whatever state it
was, or force it off? 

I think it should be put off. Do you agree? (also the brightness should
be set to 0 in this case)

     Andrew

-- 
	Ansuel