Re: [PATCH 1/2] leds: ncp5623: Add device tree binding documentation
From: Jacek Anaszewski
Date: Thu Jun 23 2016 - 09:53:55 EST
On 06/23/2016 02:01 PM, Florian Vaussard wrote:
On 06/23/2016 01:21 PM, Jacek Anaszewski wrote:
On 06/23/2016 10:32 AM, Florian Vaussard wrote:
Hi Jacek,
On 06/23/2016 09:23 AM, Jacek Anaszewski wrote:
On 06/22/2016 04:25 PM, Florian Vaussard wrote:
Hi Jacek,
Le 22. 06. 16 à 10:51, Jacek Anaszewski a écrit :
Hi Florian,
On 06/22/2016 08:08 AM, Florian Vaussard wrote:
Hi Jacek,
Le 21. 06. 16 à 17:28, Jacek Anaszewski a écrit :
Hi Florian,
Thanks for the patch. I have two remarks below.
On 06/21/2016 09:29 AM, Florian Vaussard wrote:
Add device tree binding documentation for On Semiconductor NCP5623 I2C
LED driver. The driver can independently control the PWM of the 3
channels with 32 levels of intensity.
The current delivered by the current source can be controlled using the
led-max-microamp property. In order to control this value, it is also
necessary to know the current on the Iref pin, hence the
onnn,led-iref-microamp property. It is usually set using an external
bias resistor, following Iref = Vref/Rbias with Vref=0.6V.
Signed-off-by: Florian Vaussard <florian.vaussard@xxxxxxxxxx>
---
.../devicetree/bindings/leds/leds-ncp5623.txt | 44
++++++++++++++++++++++
1 file changed, 44 insertions(+)
create mode 100644
Documentation/devicetree/bindings/leds/leds-ncp5623.txt
diff --git a/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
new file mode 100644
index 0000000..0dc8345
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-ncp5623.txt
@@ -0,0 +1,44 @@
+* ON Semiconductor - NCP5623 3-Channel LED Driver
+
+The NCP5623 is a 3-channel I2C LED driver. The brightness of each
+channel can be independently set using 32 levels. Each LED is represented
+as a sub-node of the device.
+
+Required properties:
+ - compatible: Should be "onnn,ncp5623"
+ - reg: I2C slave address (fixed to 0x38)
+ - #address-cells: must be 1
+ - #size-cells: must be 0
+ - onnn,led-iref-microamp: Current on the Iref pin in microampere
I think that you don't need this property. Just provide the formula for
calculating led-max-microamp value, similarly as you're doing that in
the commit message.
I am not completely sure to understand your suggestion. So at the end, I
have to
compute the value of the register (let call it 'ILED') that I need to send to
chip to configure the current source. The formula is:
ILED = 31 - 2400*Iref/led-max-microamp
led-max-microamp is the maximum current value for given LED.
According to the documentation it can be calculated as follows:
ILEDmax = Iref * 2400 / (31 - n)
Since this is global setting for all LEDs, then I'd always set n to 30,
and calculate max_brightness value for each LED separately, basing on
led-max-microamp property value. Effectively, I'm revoking my previous
statement about setting max_brightness to fixed level.
Ok your proposal simplifies a bit the handling. Thus ILEDmax of the current
source would be always equal to Iref * 2400 and we use the PWM to limit the
current inside the LED. The only downside of this approach is a reduced number
of possible PWM steps, thus a limited number of RGB colors.
Yes, but by max_brightness being always 31, lowering led-max-microamp
results in decreasing the amount of current per brightness level.
Effectively, a human ability to notice perceived brightness level
change also decreases then.
In the approach I proposed this limitation is reflected in reduced
amount of available brightness levels.
Regarding the DT binding, this would mean something like this:
ncp5623@38 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "onnn,ncp5623";
reg = <0x38>;
led-max-microamp = <30000>;
Please drop it from here. It doesn't need to be configurable.
You can hard code this in the driver.
It is not user configurable, but it is a hardware configuration imposed by the
bias resistor on the Iref pin (ILEDmax = 2400*Iref = 2400*0.6V/Rbias). So I
cannot hard code it as it can change from one design to another. And I need this
piece of information to compute the maximum allowable PWM ratio.
OK, please keep here the property you initially introduced for that:
onnn,led-iref-microamp
Ok.
ledr@0 {
label = "ncp:power:red";
reg = <0>;
linux,default-trigger = "default-on";
led-max-microamp = <5000>;
Is 5mA the maximum allowed current value for the LEDs on the board
you're using? Is brightness level change easily noticeable by max
current set to 5mA and max_brightness set to 31? It would be good
to empirically check this configuration.
No the maximum is 20mA on our board, but limiting to 5mA is safer to avoid
blinding the user :) This RGB led is quite powerful...
Some experiments:
1) When setting the current source at 5mA, the PWM steps are easily noticeable
at low brightness (below 50%). Above the eye is not sensitive enough. Thus on
the 32768 possible colours, I agree that not all will be distinguishable.
2) When setting the current source at 20mA, the PWM steps are even more visible
at low brightness. As I have to keep the PWM ratio below 25% to satisfy the 5mA
limit, all the 7 steps (brightness = [0; 7]) are clearly noticeable. This also
means only 512 different colours. For sure in this case they are all
distinguishable :)
With your proposal, the hardware fix is probably to decrease Iref by increasing
the bias resistor. This way the PWM steps would be smaller and less noticeable.
But a hardware fix is not always possible.
It would be nice to have all 31 levels available per LED, but is it
feasible having that ILED register is global for all LEDs? It seems that
we couldn't set different maximum current for each LED then.
Am I right or I am missing something?
If led-max-microamp is different for each LED, we can select the highest one and
use it to compute the ILED register. Then we limit the PWM ratio for the LEDs
that have a lowest led-max-microamp. I guess that this is the best compromise.
Sounds good. Please proceed accordingly.
--
Best regards,
Jacek Anaszewski