Messages in this thread | | | Date | Tue, 18 Nov 2014 17:02:14 +0100 | From | Jacek Anaszewski <> | Subject | Re: [RFC] adp1653: Add device tree bindings for LED controller |
| |
Hi Pavel,
On 11/18/2014 02:21 PM, Pavel Machek wrote: > Hi! > >>>>> @@ -19,5 +30,10 @@ Examples: >>>>> system-status { >>>>> label = "Status"; >>>>> linux,default-trigger = "heartbeat"; >>>>> + iout-torch = <500 500>; >>>>> + iout-flash = <1000 1000>; >>>>> + iout-indicator = <100 100>; >>>>> + flash-timeout = <1000>; >>>>> + >>>>> ... >>>>> }; >>>>> >>>>> I don't get it; system-status describes single LED, why are iout-torch >>>>> (and friends) arrays of two? >>>> >>>> Some devices can control more than one led. The array is for such >>>> purposes. The system-status should be probably renamed to >>>> something more generic for both common leds and flash leds, >>>> e.g. system-led. >>> >>> No, sorry. The Documentation/devicetree/bindings/leds/common.txt >>> describes binding for _one LED_. Yes, your device can have two leds, >>> so your devices should have two such blocks in the device tree... Each >>> led should have its own label and default trigger, for example. And I >>> guess flash-timeout be per-LED, too. >> >> I think that a device tree binding describes a single physical device. >> No matter how many sub-leds a device controls, it is still one >> piece of hardware. > > You got this wrong, sorry. > > In my case, there are three physical devices: > > adp1653 > white LED > red LED
You've mentioned that your white led is torch/flash and indicator is the red led. They are probably connected to the HPLED and ILED pins of the ADP1653 device respectively. The device is just a regulator, that delivers electric current to the leds connected to it. Kernel cannot directly activate leds, but has to talk to the device through I2C bus. One I2C device can have only one related device tree binding.
> Each LED should have an label, and probably default trigger -- default > trigger for red one should be "we are recording video" and for white > should be "this is flash for default camera".
default-trigger is not mandatory, the device doesn't have to have associated led-trigger. I think that you should look at Documentation/leds/leds-class.txt and drivers/leds/triggers for more detailed information. In a nutshell triggers are kernel sources of led events. You can set e.g. "heartbeat", "timer" trigger etc. As for now the driver belongs to the V4L2 subsystem it doesn't support triggers. Moreover your event "we are recording a video" should be activated by setting V4L2_CID_FLASH_INDICATOR_INTENSITY v4l2 control followed by V4L2_FLASH_LED_MODE_TORCH. Your event "this is flash for default camera" seems to be flash strobe, that can be activated by setting V4L2_CID_FLASH_STROBE control. The driver by default sets the indicator current for both actions to the value previously set with V4L2_CID_FLASH_INDICATOR_INTENSITY.
> If the hardware LED changes with one that needs different current, the > block for the adp1653 stays the same, but white LED block should be > updated with different value.
I think that you are talking about sub nodes. Indeed I am leaning towards this type of design.
>> default-trigger property should also be an array of strings. > > That is not how it currently works.
OK, I agree.
> >> I agree that flash-timeout should be per led - an array, similarly >> as in case of iout's. > > Agreed about per-led, disagreed about the array. As all the fields > would need arrays, and as LED system currently does not use arrays for > label and linux,default-trigger, I believe we should follow existing > design and model it as three devices. (It _is_ physically three devices.)
Right, I missed that the leds/common.txt describes child node.
I propose following modifications to the binding:
Optional properties for child nodes: - iout-mode-led : maximum intensity in microamperes of the LED (torch LED for flash devices) - iout-mode-flash : initial intensity in microamperes of the flash LED; it is required to enable support for the flash led - iout-mode-indicator : initial intensity in microamperes of the indicator LED; it is required to enable support for the indicator led - max-iout-mode-led : maximum intensity in microamperes of the LED (torch LED for flash devices) - max-iout-mode-flash : maximum intensity in microamperes of the flash LED - max-iout-mode-indicator : maximum intensity in microamperes of the indicator LED - flash-timeout : timeout in microseconds after which flash led is turned off
system-status { label = "max77693_1"; iout-mode-led = <500>; max-iout-mode-led = <500>; ... };
camera-flash1 { label = "max77693_2"; iout-mode-led = <500>; iout-mode-flash = <1000>; iout-mode-indicator = <100>; max-iout-mode-led = <500>; max-iout-mode-flash = <1000>; max-iout-mode-indicator = <100>; flash-timeout = <1000>; ... };
I propose to avoid name "torch", as for ordinary leds it would be misleading.
Regards, Jacek
| |