Re: [RFC PATCH v3 2/8] leds: add function to configure hardware controlled LED
From: Andrew Lunn <andrew@lunn.ch>
Date: 2021-11-09 20:49:46
Also in:
linux-devicetree, linux-doc, linux-leds, lkml
quoted
quoted
+#ifdef CONFIG_LEDS_HARDWARE_CONTROL +enum blink_mode_cmd { + BLINK_MODE_ENABLE, /* Enable the hardware blink mode */ + BLINK_MODE_DISABLE, /* Disable the hardware blink mode */ + BLINK_MODE_READ, /* Read the status of the hardware blink mode */ + BLINK_MODE_SUPPORTED, /* Ask the driver if the hardware blink mode is supported */ + BLINK_MODE_ZERO, /* Disable any hardware blink active */ +}; +#endifthis is a strange proposal for the API. Anyway, led_classdev already has the blink_set() method, which is documented as /* * Activate hardware accelerated blink, delays are in milliseconds * and if both are zero then a sensible default should be chosen. * The call should adjust the timings in that case and if it can't * match the values specified exactly. * Deactivate blinking again when the brightness is set to LED_OFF * via the brightness_set() callback. */ int (*blink_set)(struct led_classdev *led_cdev, unsigned long *delay_on, unsigned long *delay_off); So we already have a method to set hardware blkinking, we don't need another one. MarekBut that is about hardware blink, not a LED controlled by hardware based on some rules/modes. Doesn't really match the use for the hardware control. Blink_set makes the LED blink contantly at the declared delay. The blink_mode_cmd are used to request stuff to a LED in hardware mode. Doesn't seem correct to change/enhance the blink_set function with something that would do something completely different.
Humm. I can see merits for both. What i like about reusing blink_set() is that it is well understood. There is a defined sysfs API for it. ledtrig-oneshot.c also uses it, for a non-repeating blink. So i think that also fits the PHY LED use case. Andrew