Re: [PATCH 3/3] leds: leds-mc13783: Add devicetree support

[Tomasz Figa <tomasz.figa@gmail.com>]

On Saturday 07 of December 2013 16:08:41 Alexander Shiyan wrote:
> > Hi Alexander,
> > 
> > Please see my comments inline.
> > 
> > On Saturday 07 of December 2013 10:24:24 Alexander Shiyan wrote:
> > > This patch adds devicetree support for the MC13XXX LED driver.
> > > 
> > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > ---
> > >  Documentation/devicetree/bindings/mfd/mc13xxx.txt |  38 +++++
> > >  drivers/leds/leds-mc13783.c                       | 161 ++++++++++++++--------
> > >  2 files changed, 143 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/mc13xxx.txt b/Documentation/devicetree/bindings/mfd/mc13xxx.txt
> > > index abd9e3c..bdf31e8 100644
> > > --- a/Documentation/devicetree/bindings/mfd/mc13xxx.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/mc13xxx.txt
> > > @@ -10,9 +10,37 @@ Optional properties:
> > >  - fsl,mc13xxx-uses-touch : Indicate the touchscreen controller is being used
> > >  
> > >  Sub-nodes:
> > > +- leds : Contain the led nodes and initial register values in property
> > > +  "led_control". Number of register depends of used IC, for MC13783 is 6,
> > > +  for MC13892 is 4. See datasheet for bits definitions of these register.
> > 
> > Can't the driver somehow derive correct register values from LEDs listed
> > as subnodes of this node? If not, I'd say that more fine grained
> > properties should be used instead. What kind of configuration do these
> > register control?
> 
> Registers contain the basic state for LED (high current mode, current,
> duty cycle, ramp etc.). Presence and bit position for each LED is individually.
> So I decided that the use of a simple initial setup is better, especially since
> these will not be changed during operation.
> 
> > > +  Each led node should contain "reg", which used as LED ID (described below).
> > > +  Optional properties "label" and "linux,default-trigger" is described in
> > > +  Documentation/devicetree/bindings/leds/common.txt.
> > >  - regulators : Contain the regulator nodes. The regulators are bound using
> > >    their names as listed below with their registers and bits for enabling.
> > >  
> > > +MC13783 LED IDs:
> > > +    0  : Main display
> > > +    1  : AUX display
> > > +    2  : Keypad
> > > +    3  : Red 1
> > > +    4  : Green 1
> > > +    5  : Blue 1
> > > +    6  : Red 2
> > > +    7  : Green 2
> > > +    8  : Blue 2
> > > +    9  : Red 3
> > > +    10 : Green 3
> > > +    11 : Blue 3
> > > +
> > > +MC13892 LED IDs:
> > > +    12 : Main display
> > > +    13 : AUX display
> > > +    14 : Keypad
> > > +    15 : Red
> > > +    16 : Green
> > > +    17 : Blue
> > 
> > This looks a bit strange. Does the numbering relate to hardware design
> > in any way? From what I can see, those are just enums values from Linux
> > driver, which doesn't seem the right namespace to export to DT.
> 
> Which a better solution here? Make names as well as for regulators?
> Thanks.

I don't have anything against using simple numbers, but they should be
somehow related to hardware layout. If such relation does not exist, then
using the <0, number_of_leds_in_the_chip - 1> namespace should be fine.

Best regards,
Tomasz