"LED / flash API integration" related improvements

DISCUSSION Jacek Anaszewski 2 participants 4 messages May 9, 2014 – Jul 7, 2014

Subsystems (2)

Media (V4L/DVB) LED

lore

Share & tools

lore

Jacek Anaszewski

author

2014-05-09 14:28 UTC #0 lore raw

During review of "LED / flash API integration" patch sets two issues requiring modifications in the LED subsystem core emerged. I would like to consult possible ways of solving them:

1.
==================================================================

Some LED devices allow to control multiple LEDs independently. Currently there is no direct support for this in the LED subsystem and existing drivers register separate devices for each LED.

LED / flash API integration effort is a good opportunity to provide support for exposing multiple LEDs by a single LED class device.

I would like to add following API:

/**
* led_get_sub_leds_number - get the number of exposed LEDs
* @led_cdev: the LED to query
* @num_leds: number of exposed leds
*
* Get the number of leds exposed by the device.
*
* Returns: 0 on success or negative error value on failure */
int led_get_sub_leds_number(struct led_classdev *led_cdev,

                        int *num_leds);

/**
* led_select_sub_led - select sub led to control
* @led_cdev: the LED to set
* @led_id: id of the sub led to control
*
* Set the sub led to be the target of the LED class API calls.
* Maximum led_id equals num_leds - 1.
*
* Returns: 0 on success or negative error value on failure */
int led_select_sub_led(struct led_classdev *led_cdev,

                    int led_id);

/**
* led_get_sub_led - get currently selected sub led
* @led_cdev: the LED to set
* @led_id: id of currently selected sub led
*
* Get id of the sub led chosen as the target of LED class
* API calls. Maximum led_id equals num_leds - 1. *
* Returns: 0 on success or negative error value on failure */
int led_get_target_led(struct led_classdev *led_cdev,

                        int* led_id);

The functions functions would be mapped on the sysfs attributes:
- available_leds - RO
- sub_led_id - RW

The attributes would be created only if the related callbacks are registered by the driver.

2.
==================================================================

The second issue, refers to the work queues being used in the brightness_set callbacks of the LED subsystem drivers. It interferes with the way how V4L2 Flash controls work, which expect that setting flash brightness has immediate effect.

Proposed solutions:
- move work queues out from the drivers to the LED core.
- add brightness_set_now callback to be registered by the LED drivers and intended for call by v4l2-flash driver; it wouldn't schedule a work but do the job immediately

==================================================================

I'd be glad to hear any comments on this.

Thanks,
Jacek Anaszewski

Sakari Ailus

2014-06-16 08:53 UTC #1 lore raw

Hi Jacek and others,

Comments from the LED API folks would be highly appreciated.

(Cc linux-media as well.)

My comments below.

Quoted 39 lines · Jacek Anaszewski

During review of "LED / flash API integration" patch sets two issues requiring modifications in the LED subsystem core emerged. I would like to consult possible ways of solving them:
1.
==================================================================
Some LED devices allow to control multiple LEDs independently. Currently there is no direct support for this in the LED subsystem and existing drivers register separate devices for each LED.
LED / flash API integration effort is a good opportunity to provide support for exposing multiple LEDs by a single LED class device.
I would like to add following API:
/**
* led_get_sub_leds_number - get the number of exposed LEDs
* @led_cdev: the LED to query
* @num_leds: number of exposed leds
*
* Get the number of leds exposed by the device.
*
* Returns: 0 on success or negative error value on failure */
int led_get_sub_leds_number(struct led_classdev *led_cdev,
                        int *num_leds);
/**
* led_select_sub_led - select sub led to control
* @led_cdev: the LED to set
* @led_id: id of the sub led to control
*
* Set the sub led to be the target of the LED class API calls.
* Maximum led_id equals num_leds - 1.
*
* Returns: 0 on success or negative error value on failure */
int led_select_sub_led(struct led_classdev *led_cdev,

Quoted 1 line

                    int led_id);

Instead of this, how about using an array indexed by LED for the other functions?

The problem with this is that often the registers to control LED specific properties contain the same configuration for another LED. The driver should thus cache the information until it needs to be applied, and I don't think configuring everything for every LED separately makes sense for either the user nor the driver.

Caching the configuration before applying it should still be done since this way extra register accesses can be avoided: registers often share different kind of configurations such as timeout and current as well. The configuration should be applied once an apply fonction is called.

Quoted 11 lines

/**
* led_get_sub_led - get currently selected sub led
* @led_cdev: the LED to set
* @led_id: id of currently selected sub led
*
* Get id of the sub led chosen as the target of LED class
* API calls. Maximum led_id equals num_leds - 1. *
* Returns: 0 on success or negative error value on failure */
int led_get_target_led(struct led_classdev *led_cdev,

Quoted 5 lines

                        int* led_id);
The functions functions would be mapped on the sysfs attributes:
- available_leds - RO
- sub_led_id - RW

This kind of interface isn't going to be compatible with existing applications. If you want to keep the compatibility, the LEDs would probably need to be exposed as separate devices. If that's not a requirement, I'd use an array here as well.

The V4L2 driver would still expose the device as a single sub-device. Controls that apply to individual LEDs would be array controls.

Quoted 11 lines

The attributes would be created only if the related callbacks are registered by the driver.
2.
==================================================================
The second issue, refers to the work queues being used in the brightness_set callbacks of the LED subsystem drivers. It interferes with the way how V4L2 Flash controls work, which expect that setting flash brightness has immediate effect.

Quoted 5 lines

Proposed solutions:
- move work queues out from the drivers to the LED core.
- add brightness_set_now callback to be registered by the LED drivers and intended for call by v4l2-flash driver; it wouldn't schedule a work but do the job immediately

I favour the former, but the latter could be used to leave the existing callback as a compatibility means until all drivers are converted. This would probably be more work in total though than doing everything in a single patchset.

Signature · 4 lines

Kind regards,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]

Jacek Anaszewski

author

2014-06-16 11:34 UTC #2 lore raw

Hi Sakari,

Quoted 50 lines · Sakari Ailus

Hi Jacek and others,
Comments from the LED API folks would be highly appreciated.
(Cc linux-media as well.)
My comments below.
Quoted 39 lines · Jacek Anaszewski
During review of "LED / flash API integration" patch sets two issues requiring modifications in the LED subsystem core emerged. I would like to consult possible ways of solving them:
1.
==================================================================
Some LED devices allow to control multiple LEDs independently. Currently there is no direct support for this in the LED subsystem and existing drivers register separate devices for each LED.
LED / flash API integration effort is a good opportunity to provide support for exposing multiple LEDs by a single LED class device.
I would like to add following API:
/**
* led_get_sub_leds_number - get the number of exposed LEDs
* @led_cdev: the LED to query
* @num_leds: number of exposed leds
*
* Get the number of leds exposed by the device. *
* Returns: 0 on success or negative error value on failure */
int led_get_sub_leds_number(struct led_classdev *led_cdev,
                         int *num_leds);
/**
* led_select_sub_led - select sub led to control
* @led_cdev: the LED to set
* @led_id: id of the sub led to control
*
* Set the sub led to be the target of the LED class API calls.
* Maximum led_id equals num_leds - 1.
*
* Returns: 0 on success or negative error value on failure */
int led_select_sub_led(struct led_classdev *led_cdev,
Quoted 1 line
                     int led_id);

Quoted 8 lines

Instead of this, how about using an array indexed by LED for the other functions?
The problem with this is that often the registers to control LED specific properties contain the same configuration for another LED. The driver should thus cache the information until it needs to be applied, and I don't think configuring everything for every LED separately makes sense for either the user nor the driver.

The max77693-flash is an example of such a design. It has one register for setting flash timeout and it affects timeout for both leds. The register would have to be written right before strobing the flash, in case the cached timeout value is different from the one in the register (cached on last write operation).
The device has also common flash status register. I don't have good idea how to proceed in this case. After strobing the flash for any of the sub-leds the remaining ones will also report that they are strobing at the moment.

Quoted 18 lines

Caching the configuration before applying it should still be done since this way extra register accesses can be avoided: registers often share different kind of configurations such as timeout and current as well. The configuration should be applied once an apply fonction is called.
Quoted 13 lines
/**
* led_get_sub_led - get currently selected sub led
* @led_cdev: the LED to set
* @led_id: id of currently selected sub led
*
* Get id of the sub led chosen as the target of LED class
* API calls. Maximum led_id equals num_leds - 1. *
* Returns: 0 on success or negative error value on failure */
int led_get_target_led(struct led_classdev *led_cdev,
                         int* led_id);

Quoted 8 lines

Quoted 3 lines
The functions functions would be mapped on the sysfs attributes:
- available_leds - RO
- sub_led_id - RW
This kind of interface isn't going to be compatible with existing applications. If you want to keep the compatibility, the LEDs would probably need to be exposed as separate devices. If that's not a requirement, I'd use an array here as well.

I started to implement the interface I proposed but encountered several issues related to led-triggers and led_blink feature. They would require significant modifications, I'd rather avoid. Instead I decided to register separate led class device in the driver for the max77693-flash device, similarly as it is accomplished in the led drivers of the other compound led devices.

Quoted 3 lines

The V4L2 driver would still expose the device as a single sub-device. Controls that apply to individual LEDs would be array controls.

Have those array controls been already added? If so, could you spot the place where they are implemented, or maybe there are some examples of usage or documentation?

Quoted 17 lines

Quoted 11 lines
The attributes would be created only if the related callbacks are registered by the driver.
2.
==================================================================
The second issue, refers to the work queues being used in the brightness_set callbacks of the LED subsystem drivers. It interferes with the way how V4L2 Flash controls work, which expect that setting flash brightness has immediate effect.
Quoted 5 lines
Proposed solutions:
- move work queues out from the drivers to the LED core.
- add brightness_set_now callback to be registered by the LED drivers and intended for call by v4l2-flash driver; it wouldn't schedule a work but do the job immediately

Quoted 5 lines

I favour the former, but the latter could be used to leave the existing callback as a compatibility means until all drivers are converted. This would probably be more work in total though than doing everything in a single patchset.

Actually I am wondering whether avoiding work queue in the led flash class driver really poses a problem. If we register the driver with led_classdev_flash_register, then we explicitly declare that the device is to be used as a flash led, and not as e.g. a HD LED, and thus the torch brightness should be set immediately in the brightness_set callback.

Best Regards,
Jacek Anaszewski

Sakari Ailus

2014-07-07 16:03 UTC #3 lore raw

Hi Jacek,

I'll promise to reply sooner the next time. Just back from holidays. :-)

Quoted 60 lines · Jacek Anaszewski

Quoted 50 lines · Sakari Ailus
Hi Jacek and others,
Comments from the LED API folks would be highly appreciated.
(Cc linux-media as well.)
My comments below.
Quoted 39 lines · Jacek Anaszewski
During review of "LED / flash API integration" patch sets two issues requiring modifications in the LED subsystem core emerged. I would like to consult possible ways of solving them:
1.
==================================================================
Some LED devices allow to control multiple LEDs independently. Currently there is no direct support for this in the LED subsystem and existing drivers register separate devices for each LED.
LED / flash API integration effort is a good opportunity to provide support for exposing multiple LEDs by a single LED class device.
I would like to add following API:
/**
* led_get_sub_leds_number - get the number of exposed LEDs
* @led_cdev: the LED to query
* @num_leds: number of exposed leds
*
* Get the number of leds exposed by the device.
*
* Returns: 0 on success or negative error value on failure */
int led_get_sub_leds_number(struct led_classdev *led_cdev,
                        int *num_leds);
/**
* led_select_sub_led - select sub led to control
* @led_cdev: the LED to set
* @led_id: id of the sub led to control
*
* Set the sub led to be the target of the LED class API calls.
* Maximum led_id equals num_leds - 1.
*
* Returns: 0 on success or negative error value on failure */
int led_select_sub_led(struct led_classdev *led_cdev,
Quoted 1 line
                    int led_id);
Quoted 8 lines
Instead of this, how about using an array indexed by LED for the other functions?
The problem with this is that often the registers to control LED specific properties contain the same configuration for another LED. The driver should thus cache the information until it needs to be applied, and I don't think configuring everything for every LED separately makes sense for either the user nor the driver.

Quoted 9 lines

The max77693-flash is an example of such a design. It has one register for setting flash timeout and it affects timeout for both leds. The register would have to be written right before strobing the flash, in case the cached timeout value is different from the one in the register (cached on last write operation).
The device has also common flash status register. I don't have good idea how to proceed in this case. After strobing the flash for any of the sub-leds the remaining ones will also report that they are strobing at the moment.

That's a good point; it's in the end hardware dependent whether a particular property is specific to a controller or a led. It should be possible to find that out by using the interface. If we keep things simple, this would mean three functions per parameters, set, get and get_n to return the number of properties (in this very case probably either 1 or the number of LEDs).

Alternatively, as all the interface functions either set or get an integer, you could access the properties (timeout, intensity etc.) by an ID. You'd need three interface functions in total, not per property.

Quoted 34 lines

Quoted 18 lines
Caching the configuration before applying it should still be done since this way extra register accesses can be avoided: registers often share different kind of configurations such as timeout and current as well. The configuration should be applied once an apply fonction is called.
Quoted 13 lines
/**
* led_get_sub_led - get currently selected sub led
* @led_cdev: the LED to set
* @led_id: id of currently selected sub led
*
* Get id of the sub led chosen as the target of LED class
* API calls. Maximum led_id equals num_leds - 1. *
* Returns: 0 on success or negative error value on failure */
int led_get_target_led(struct led_classdev *led_cdev,
                        int* led_id);
Quoted 8 lines
Quoted 3 lines
The functions functions would be mapped on the sysfs attributes:
- available_leds - RO
- sub_led_id - RW
This kind of interface isn't going to be compatible with existing applications. If you want to keep the compatibility, the LEDs would probably need to be exposed as separate devices. If that's not a requirement, I'd use an array here as well.
I started to implement the interface I proposed but encountered several issues related to led-triggers and led_blink feature. They would require significant modifications, I'd rather avoid. Instead I decided to register separate led class device in the driver for the max77693-flash device, similarly as it is accomplished in the led drivers of the other compound led devices.
Quoted 1 line

Quoted 6 lines

Quoted 2 lines
The V4L2 driver would still expose the device as a single sub-device. Controls that apply to individual LEDs would be array controls.
Have those array controls been already added? If so, could you spot the place where they are implemented, or maybe there are some examples of usage or documentation?

Hans has updated the compound controls patchset and sent a pull request:

<URL:https://patchwork.linuxtv.org/patch/24546/>

Mauro mentioned he'll take a look at the patches tomorrow. Independently of when the patchset goes in (I don't think it'll take too long) and in whichever form, getting a sound user space interface takes priority over getting things in fast IMHO.

Quoted 23 lines

Quoted 17 lines
Quoted 11 lines
The attributes would be created only if the related callbacks are registered by the driver.
2.
==================================================================
The second issue, refers to the work queues being used in the brightness_set callbacks of the LED subsystem drivers. It interferes with the way how V4L2 Flash controls work, which expect that setting flash brightness has immediate effect.
Quoted 5 lines
Proposed solutions:
- move work queues out from the drivers to the LED core.
- add brightness_set_now callback to be registered by the LED drivers and intended for call by v4l2-flash driver; it wouldn't schedule a work but do the job immediately
Quoted 5 lines
I favour the former, but the latter could be used to leave the existing callback as a compatibility means until all drivers are converted. This would probably be more work in total though than doing everything in a single patchset.

Quoted 6 lines

Actually I am wondering whether avoiding work queue in the led flash class driver really poses a problem. If we register the driver with led_classdev_flash_register, then we explicitly declare that the device is to be used as a flash led, and not as e.g. a HD LED, and thus the torch brightness should be set immediately in the brightness_set callback.

I don't have an issue with that, however it sounds like to me that moving the work queue handling to the framework is a worthwhile change independently of the flash support changes.

Signature · 4 lines

Kind regards,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]