| Message ID | 1466494154-3786-2-git-send-email-florian.vaussard@heig-vd.ch |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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@heig-vd.ch> > --- > .../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. > + - led-max-microamp: Desired maximum current for each LED in microampere > + (maximum 30000uA) Please add instead of (maximum ...): Valid values: min - max, step by N (rounded {up|down}) E.g. Valid values: 10000 - 30000, step by 1000 (rounded down) > + > +LED sub-node properties: > + - reg : LED channel number (0..2) > + - For other LED properties see: > + Documentation/devicetree/bindings/leds/common.txt > + > +Example: > + > +led1: ncp5623@38 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "onnn,ncp5623"; > + reg = <0x38>; > + > + led1_r@0 { > + label = "ncp:power:red"; > + reg = <0>; > + linux,default-trigger = "default-on"; > + }; > + > + led1_b@1 { > + label = "ncp:power:blue"; > + reg = <1>; > + }; > + > + led1_g@2 { > + label = "ncp:power:green"; > + reg = <2>; > + }; > +}; >
On Tue, Jun 21, 2016 at 09:29:13AM +0200, 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@heig-vd.ch> > --- > .../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 > + - led-max-microamp: Desired maximum current for each LED in microampere > + (maximum 30000uA) > + > +LED sub-node properties: > + - reg : LED channel number (0..2) > + - For other LED properties see: > + Documentation/devicetree/bindings/leds/common.txt > + > +Example: > + > +led1: ncp5623@38 { > + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "onnn,ncp5623"; > + reg = <0x38>; > + > + led1_r@0 { Don't use underscores in node names. > + label = "ncp:power:red"; > + reg = <0>; > + linux,default-trigger = "default-on"; > + }; > + > + led1_b@1 { > + label = "ncp:power:blue"; > + reg = <1>; > + }; > + > + led1_g@2 { > + label = "ncp:power:green"; > + reg = <2>; > + }; > +}; > -- > 2.5.0 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@heig-vd.ch> >> --- >> .../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 I need two pieces of information that depends on the hardware setup; the current on the Iref pin (which is setup by a bias resistor Iref = 0.6V/Rbias), and the desired output current (usually limited due to the chosen LED or the required brightness). On the other hand, I could also simply ask people to put the right value of 'ILED' (between 0 - 31) directly in the device tree, but this is less user-friendly. >> + - led-max-microamp: Desired maximum current for each LED in microampere >> + (maximum 30000uA) > > Please add instead of (maximum ...): > > Valid values: min - max, step by N (rounded {up|down}) > > E.g. > > Valid values: 10000 - 30000, step by 1000 (rounded down) > This is unfortunately not a linear relationship, led-max-microamp = 2400*Iref/(31-ILED) thus steps are not constant. This can be seen on figure 5 (p.9) of the datasheet [1]. Thanks for your review, Florian [1] http://www.onsemi.com/pub/Collateral/NCP5623-D.PDF -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, Le 21. 06. 16 à 23:52, Rob Herring a écrit : > On Tue, Jun 21, 2016 at 09:29:13AM +0200, 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@heig-vd.ch> >> --- >> .../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 >> + - led-max-microamp: Desired maximum current for each LED in microampere >> + (maximum 30000uA) >> + >> +LED sub-node properties: >> + - reg : LED channel number (0..2) >> + - For other LED properties see: >> + Documentation/devicetree/bindings/leds/common.txt >> + >> +Example: >> + >> +led1: ncp5623@38 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "onnn,ncp5623"; >> + reg = <0x38>; >> + >> + led1_r@0 { > > Don't use underscores in node names. > Sorry for the mistake, I will fix. Thanks for the review, Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@heig-vd.ch> >>> --- >>> .../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. You can compare drivers/leds/leds-aat1290.c and its bindings, as it uses similar approach. > > I need two pieces of information that depends on the hardware setup; the current > on the Iref pin (which is setup by a bias resistor Iref = 0.6V/Rbias), and the > desired output current (usually limited due to the chosen LED or the required > brightness). > > On the other hand, I could also simply ask people to put the right value of > 'ILED' (between 0 - 31) directly in the device tree, but this is less user-friendly. > >>> + - led-max-microamp: Desired maximum current for each LED in microampere >>> + (maximum 30000uA) >> >> Please add instead of (maximum ...): >> >> Valid values: min - max, step by N (rounded {up|down}) >> >> E.g. >> >> Valid values: 10000 - 30000, step by 1000 (rounded down) >> > > This is unfortunately not a linear relationship, OK, please define only max and min value then. > led-max-microamp = 2400*Iref/(31-ILED) > > thus steps are not constant. This can be seen on figure 5 (p.9) of the datasheet > [1]. > > Thanks for your review, > Florian > > [1] http://www.onsemi.com/pub/Collateral/NCP5623-D.PDF > >
On 06/21/2016 05:28 PM, Jacek Anaszewski wrote: > 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@heig-vd.ch> >> --- >> .../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. > >> + - led-max-microamp: Desired maximum current for each LED in >> microampere >> + (maximum 30000uA) > > Please add instead of (maximum ...): > > Valid values: min - max, step by N (rounded {up|down}) > > E.g. > > Valid values: 10000 - 30000, step by 1000 (rounded down) > >> + >> +LED sub-node properties: >> + - reg : LED channel number (0..2) >> + - For other LED properties see: >> + Documentation/devicetree/bindings/leds/common.txt >> + >> +Example: >> + >> +led1: ncp5623@38 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + compatible = "onnn,ncp5623"; >> + reg = <0x38>; >> + >> + led1_r@0 { >> + label = "ncp:power:red"; >> + reg = <0>; >> + linux,default-trigger = "default-on"; Please add also led-max-microamp to all child nodes in this example. >> + }; >> + >> + led1_b@1 { >> + label = "ncp:power:blue"; >> + reg = <1>; >> + }; >> + >> + led1_g@2 { >> + label = "ncp:power:green"; >> + reg = <2>; >> + }; >> +}; >> > >
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@heig-vd.ch> >>>> --- >>>> .../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. 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>; ledr@0 { label = "ncp:power:red"; reg = <0>; linux,default-trigger = "default-on"; led-max-microamp = <5000>; }; ledb@1 { label = "ncp:power:blue"; reg = <1>; led-max-microamp = <5000>; }; ledg@2 { label = "ncp:power:green"; reg = <2>; led-max-microamp = <5000>; }; }; The led-max-microamp property of the root node is used to infer Iref, and the led-max-microamp property inside each LED node is used to compute the maximum allowed PWM ratio (thus max_brightness). Would it be fine like this? > You can compare drivers/leds/leds-aat1290.c and its bindings, as it > uses similar approach. > Thanks for the pointer, interesting reading. In this case the flash-max-microamp property is implicitly used to get the value of Rset, and led-max-microamp is used to compute the flash/movie-mode ratio. Indeed similar but not exactly the same, as the NCP5623 allows a finer control on the current using one register to configure the current source and one register for the PWM. Regards, Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@heig-vd.ch> >>>>> --- >>>>> .../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. > > 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. > }; > > ledb@1 { > label = "ncp:power:blue"; > reg = <1>; > led-max-microamp = <5000>; > }; > > ledg@2 { > label = "ncp:power:green"; > reg = <2>; > led-max-microamp = <5000>; > }; > }; > > The led-max-microamp property of the root node is used to infer Iref, and the > led-max-microamp property inside each LED node is used to compute the maximum > allowed PWM ratio (thus max_brightness). > > Would it be fine like this? > >> You can compare drivers/leds/leds-aat1290.c and its bindings, as it >> uses similar approach. >> > > Thanks for the pointer, interesting reading. In this case the flash-max-microamp > property is implicitly used to get the value of Rset, and led-max-microamp is > used to compute the flash/movie-mode ratio. Indeed similar but not exactly the > same, as the NCP5623 allows a finer control on the current using one register to > configure the current source and one register for the PWM. Right, but it shows how led-max-microamp can be used to infer max_brightness level. This is quite new DT property with not too many users, because previously LED class drivers had been defining max_brightness directly in a Device Tree. Nonetheless brightness level was eventually considered not suitable unit for describing hardware property.
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@heig-vd.ch> >>>>>> --- >>>>>> .../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. >> >> 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. >> }; >> >> ledb@1 { >> label = "ncp:power:blue"; >> reg = <1>; >> led-max-microamp = <5000>; >> }; >> >> ledg@2 { >> label = "ncp:power:green"; >> reg = <2>; >> led-max-microamp = <5000>; >> }; >> }; >> >> The led-max-microamp property of the root node is used to infer Iref, and the >> led-max-microamp property inside each LED node is used to compute the maximum >> allowed PWM ratio (thus max_brightness). >> >> Would it be fine like this? >> >>> You can compare drivers/leds/leds-aat1290.c and its bindings, as it >>> uses similar approach. >>> >> >> Thanks for the pointer, interesting reading. In this case the flash-max-microamp >> property is implicitly used to get the value of Rset, and led-max-microamp is >> used to compute the flash/movie-mode ratio. Indeed similar but not exactly the >> same, as the NCP5623 allows a finer control on the current using one register to >> configure the current source and one register for the PWM. > > Right, but it shows how led-max-microamp can be used to infer > max_brightness level. This is quite new DT property with not too many > users, because previously LED class drivers had been defining > max_brightness directly in a Device Tree. Nonetheless brightness level > was eventually considered not suitable unit for describing hardware > property. > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@heig-vd.ch> >>>>>>> --- >>>>>>> .../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 > >>> >>> 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? >>> }; >>> >>> ledb@1 { >>> label = "ncp:power:blue"; >>> reg = <1>; >>> led-max-microamp = <5000>; >>> }; >>> >>> ledg@2 { >>> label = "ncp:power:green"; >>> reg = <2>; >>> led-max-microamp = <5000>; >>> }; >>> }; >>> >>> The led-max-microamp property of the root node is used to infer Iref, and the >>> led-max-microamp property inside each LED node is used to compute the maximum >>> allowed PWM ratio (thus max_brightness). >>> >>> Would it be fine like this? >>> >>>> You can compare drivers/leds/leds-aat1290.c and its bindings, as it >>>> uses similar approach. >>>> >>> >>> Thanks for the pointer, interesting reading. In this case the flash-max-microamp >>> property is implicitly used to get the value of Rset, and led-max-microamp is >>> used to compute the flash/movie-mode ratio. Indeed similar but not exactly the >>> same, as the NCP5623 allows a finer control on the current using one register to >>> configure the current source and one register for the PWM. >> >> Right, but it shows how led-max-microamp can be used to infer >> max_brightness level. This is quite new DT property with not too many >> users, because previously LED class drivers had been defining >> max_brightness directly in a Device Tree. Nonetheless brightness level >> was eventually considered not suitable unit for describing hardware >> property. >> > >
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@heig-vd.ch> >>>>>>>> --- >>>>>>>> .../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. Regards, Florian >>>> }; >>>> >>>> ledb@1 { >>>> label = "ncp:power:blue"; >>>> reg = <1>; >>>> led-max-microamp = <5000>; >>>> }; >>>> >>>> ledg@2 { >>>> label = "ncp:power:green"; >>>> reg = <2>; >>>> led-max-microamp = <5000>; >>>> }; >>>> }; >>>> >>>> The led-max-microamp property of the root node is used to infer Iref, and the >>>> led-max-microamp property inside each LED node is used to compute the maximum >>>> allowed PWM ratio (thus max_brightness). >>>> >>>> Would it be fine like this? >>>> >>>>> You can compare drivers/leds/leds-aat1290.c and its bindings, as it >>>>> uses similar approach. >>>>> >>>> >>>> Thanks for the pointer, interesting reading. In this case the >>>> flash-max-microamp >>>> property is implicitly used to get the value of Rset, and led-max-microamp is >>>> used to compute the flash/movie-mode ratio. Indeed similar but not exactly the >>>> same, as the NCP5623 allows a finer control on the current using one >>>> register to >>>> configure the current source and one register for the PWM. >>> >>> Right, but it shows how led-max-microamp can be used to infer >>> max_brightness level. This is quite new DT property with not too many >>> users, because previously LED class drivers had been defining >>> max_brightness directly in a Device Tree. Nonetheless brightness level >>> was eventually considered not suitable unit for describing hardware >>> property. >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@heig-vd.ch> >>>>>>>>> --- >>>>>>>>> .../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.
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 + - led-max-microamp: Desired maximum current for each LED in microampere + (maximum 30000uA) + +LED sub-node properties: + - reg : LED channel number (0..2) + - For other LED properties see: + Documentation/devicetree/bindings/leds/common.txt + +Example: + +led1: ncp5623@38 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "onnn,ncp5623"; + reg = <0x38>; + + led1_r@0 { + label = "ncp:power:red"; + reg = <0>; + linux,default-trigger = "default-on"; + }; + + led1_b@1 { + label = "ncp:power:blue"; + reg = <1>; + }; + + led1_g@2 { + label = "ncp:power:green"; + reg = <2>; + }; +};
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@heig-vd.ch> --- .../devicetree/bindings/leds/leds-ncp5623.txt | 44 ++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-ncp5623.txt