| Message ID | 20230527112854.2366-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | leds: introduce new LED hw control APIs | expand |
On Sat, May 27, 2023 at 01:28:44PM +0200, Christian Marangi wrote: > + - hw_control_set: > + activate hw control. LED driver will use the provided > + flags passed from the supported trigger, parse them to > + a set of mode and setup the LED to be driven by hardware > + following the requested modes. > + > + Set LED_OFF via the brightness_set to deactivate hw control. > + > + Return 0 on success, a negative error number on flags apply > + fail. "... on failing to apply flags." > + - hw_control_get_device: > + return the device associated with the LED driver in > + hw control. A trigger might use this to match the > + returned device from this function with a configured > + device for the trigger as the source for blinking > + events and correctly enable hw control. > + (example a netdev trigger configured to blink for a > + particular dev match the returned dev from get_device > + to set hw control) > + > + Return a device or NULL if nothing is currently attached. Returns a device name? > + > +LED driver can activate additional modes by default to workaround the > +impossibility of supporting each different mode on the supported trigger. > +Example are hardcoding the blink speed to a set interval, enable special "Examples are hardcoding ..." Thanks.
On Mon, May 29, 2023 at 08:12:42AM -0600, Jonathan Corbet wrote: > Bagas Sanjaya <bagasdotme@gmail.com> writes: > > >> + - hw_control_get_device: > >> + return the device associated with the LED driver in > >> + hw control. A trigger might use this to match the > >> + returned device from this function with a configured > >> + device for the trigger as the source for blinking > >> + events and correctly enable hw control. > >> + (example a netdev trigger configured to blink for a > >> + particular dev match the returned dev from get_device > >> + to set hw control) > >> + > >> + Return a device or NULL if nothing is currently attached. > > Returns a device name? > > The return type of this function is struct device * - how would you > expect it to return a name? > Just to clarify, a device name can't be returned. Not every device have a name and such name can be changed. An example is network device where you can change the name of the interface. Using the device prevents all of this problem.
Bagas Sanjaya <bagasdotme@gmail.com> writes: >> + - hw_control_get_device: >> + return the device associated with the LED driver in >> + hw control. A trigger might use this to match the >> + returned device from this function with a configured >> + device for the trigger as the source for blinking >> + events and correctly enable hw control. >> + (example a netdev trigger configured to blink for a >> + particular dev match the returned dev from get_device >> + to set hw control) >> + >> + Return a device or NULL if nothing is currently attached. > Returns a device name? The return type of this function is struct device * - how would you expect it to return a name? jon
On Mon, May 29, 2023 at 03:10:15PM +0700, Bagas Sanjaya wrote: > On Sat, May 27, 2023 at 01:28:44PM +0200, Christian Marangi wrote: > > + - hw_control_set: > > + activate hw control. LED driver will use the provided > > + flags passed from the supported trigger, parse them to > > + a set of mode and setup the LED to be driven by hardware > > + following the requested modes. > > + > > + Set LED_OFF via the brightness_set to deactivate hw control. > > + > > + Return 0 on success, a negative error number on flags apply > > + fail. > "... on failing to apply flags." > > > + - hw_control_get_device: > > + return the device associated with the LED driver in > > + hw control. A trigger might use this to match the > > + returned device from this function with a configured > > + device for the trigger as the source for blinking > > + events and correctly enable hw control. > > + (example a netdev trigger configured to blink for a > > + particular dev match the returned dev from get_device > > + to set hw control) > > + > > + Return a device or NULL if nothing is currently attached. > Returns a device name? How about "Returns a pointer to a struct device ..." Andrew
On Sat, May 27, 2023 at 01:28:42PM +0200, Christian Marangi wrote: > Add an option to permit LED driver to declare support for a specific > trigger to use hw control and setup the LED to blink based on specific > provided modes. > > Add APIs for LEDs hw control. These functions will be used to activate > hardware control where a LED will use the provided flags, from an > unique defined supported trigger, to setup the LED to be driven by > hardware. > > Add hw_control_is_supported() to ask the LED driver if the requested > mode by the trigger are supported and the LED can be setup to follow > the requested modes. > > Deactivate hardware blink control by setting brightness to LED_OFF via > the brightness_set() callback. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Sat, May 27, 2023 at 01:28:48PM +0200, Christian Marangi wrote: > Reject interval store with hw_control enabled. It's are currently not > supported and MUST be set to the default value with hw control enabled. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 5/29/23 21:09, Christian Marangi wrote: > Just to clarify, a device name can't be returned. Not every device have > a name and such name can be changed. An example is network device where > you can change the name of the interface. > > Using the device prevents all of this problem. > Oh, I guess it was /dev/something.
On Tue, May 30, 2023 at 10:09:28AM +0700, Bagas Sanjaya wrote: > On 5/29/23 21:09, Christian Marangi wrote: > > Just to clarify, a device name can't be returned. Not every device have > > a name and such name can be changed. An example is network device where > > you can change the name of the interface. > > > > Using the device prevents all of this problem. > > > > Oh, I guess it was /dev/something. Network devices don't appear in /dev. At least not in Linux. Some other Unix implementations do, i think SunOS used to have an entry in /dev, but i could be remembering wrongly. But within the kernel, you generally don't refer to a device by its /dev/foo name. That is a user space abstraction. In the kernel, each device in the system has a struct device representing it. Andrew
Since this series is cross subsystem between LED and netdev, a stable branch was created to facilitate merging process. This is based on top of branch ib-leds-netdev-v6.5 present here [1] and rebased on top of net-next since the LED stable branch got merged. This is a continue of [2]. It was decided to take a more gradual approach to implement LEDs support for switch and phy starting with basic support and then implementing the hw control part when we have all the prereq done. This is the main part of the series, the one that actually implement the hw control API. Some history about this feature and why ======================================= This proposal is highly requested by the entire net community but the API is not strictly designed for net usage but for a more generic usage. Initial version were very flexible and designed to try to support every aspect of the LED driver with many complex function that served multiple purpose. There was an idea to have sw only and hw only LEDs and sw only and hw only LEDs. With some heads up from Andrew from the net mailing list, it was suggested to implement a more basic yet easy to implement system. These API strictly work with a designated trigger to offload their function. This may be confused with hw blink offload but LED may have an even more advanced configuration where the entire aspect of the trigger is offloaded and completely handled by the hardware. An example of this usage are PHY or switch port LEDs. Almost every of these kind of device have multiple LED attached and provide info of the current port state. Currently we lack any support of them but these device always provide a way to configure them, from basic feature like turning the LED off or no (implemented in previous series related to this feature) or even entirely driven by the hw and power on/off/blink based on some events, like tx/rx traffic, ethernet cable attached, link speed of 10mbps, 100mbps, 1000mbps or more. They can also support multiple logic like blink with traffic only if a particular link speed is attached. (an example of this is when a LED is designated to be turned on only with 100mbps link speed and configured to blink on traffic and a secondary LED of a different color is present to serve the same function but only when the link speed is 1000mbps) These case are very common for a PHY or a switch but they were never standardized so OEM support all kind of variant and configuration. Again with Andrew we compared some feature and we reached a common set of modes that are for sure present in every kind of devices. And this concludes history and why. What is present in this series ============================== This patch contain the required API to support this feature, I decided on the name of hw control to quickly describe this feature. I documented each require API in the related Documentation for leds-class so I think it might me redundant to expose them here. Feel free to tell me how to improve it if anything is not clear. On an abstract idea, this feature require this: - The trigger needs to make use of it, this is currently implemented for the netdev trigger but other trigger can be expanded if the device expose these function. An idea might be a anything that handle a storage disk and have the LED configurable to blink when there is any activity to the disk. - The LED driver needs to expose and implement these new API. Currently a LED driver supports only a trigger. The trigger should use the related helper to check if the LED can be driven hy hardware. The different modes a trigger support are exposed in the kernel include leds.h header and are used by the LED driver to understand what to do.