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

Tomasz Figa
2013-12-07T12:14:19Z

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