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@xxxxxxx> > > > --- > > > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
