| Message ID | 20190401103034.21062-3-masneyb@onstation.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | backlight: lm3630a: bug fix and device tree support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
On Mon 2019-04-01 06:30:33, Brian Masney wrote: > Add new backlight bindings for the TI LM3630A dual-string white LED. > > Signed-off-by: Brian Masney <masneyb@onstation.org> > --- > .../leds/backlight/lm3630a-backlight.yaml | 112 ++++++++++++++++++ What is that? Is it future of all the bindings? Up to device tree people, I guess, but... Pavel > 1 file changed, 112 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml > > diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml > new file mode 100644 > index 000000000000..42a8c59d237a > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml > @@ -0,0 +1,112 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/backlight/lm3630a-backlight.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: TI LM3630A High-Efficiency Dual-String White LED > + > +maintainers: > + - Lee Jones <lee.jones@linaro.org> > + - Daniel Thompson <daniel.thompson@linaro.org> > + - Jingoo Han <jingoohan1@gmail.com> > + > +description: | > + The LM3630A is a current-mode boost converter which supplies the power and > + controls the current in up to two strings of 10 LEDs per string. > + https://www.ti.com/product/LM3630A > + > +properties: > + compatible: > + const: ti,lm3630a > + > + reg: > + maxItems: 1 > + > + ti,linear-mapping-mode: > + description: | > + Enable linear mapping mode. If disabled, then it will use exponential > + mapping mode in which the ramp up/down appears to have a more uniform > + tranisiton to the human eye. > + type: boolean > + > +required: > + - compatible > + - reg > + > +patternProperties: > + "^led*$": > + type: object > + description: | > + Properties for a string of connected LEDs. > + > + properties: > + label: > + description: | > + The label for this LED. If omitted, the label is taken from the node > + name (excluding the unit address). It has to uniquely identify a > + device, i.e. no other LED class device can be assigned the same label. > + > + led-sources: > + description: | > + List of device current outputs the LED is connected to. > + allOf: > + - $ref: /schemas/types.yaml#/definitions/uint32-array > + - minItems: 1 > + maxItems: 2 > + items: > + minimum: 0 > + maximum: 1 > + > + default-brightness: > + description: Default brightness level on boot. > + minimum: 0 > + maximum: 255 > + > + max-brightness: > + description: Maximum brightness level on boot. > + minimum: 0 > + maximum: 255 > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + lm3630a_bl@38 { > + compatible = "ti,lm3630a"; > + status = "ok"; > + reg = <0x38>; > + > + led { > + label = "main-lcd"; > + led-sources = <0 1>; > + default-brightness = <200>; > + max-brightness = <255>; > + }; > + }; > + }; > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + lm3630a_bl@38 { > + compatible = "ti,lm3630a"; > + status = "ok"; > + reg = <0x38>; > + > + led-bank-a { > + led-sources = <0>; > + default-brightness = <150>; > + ti,linear-mapping-mode; > + }; > + > + led-bank-b { > + led-sources = <1>; > + default-brightness = <225>; > + ti,linear-mapping-mode; > + }; > + }; > + };
On Mon, Apr 01, 2019 at 11:39:26PM +0200, Pavel Machek wrote: > On Mon 2019-04-01 06:30:33, Brian Masney wrote: > > Add new backlight bindings for the TI LM3630A dual-string white LED. > > > > Signed-off-by: Brian Masney <masneyb@onstation.org> > > --- > > .../leds/backlight/lm3630a-backlight.yaml | 112 > ++++++++++++++++++ > > What is that? Is it future of all the bindings? > > Up to device tree people, I guess, but... Yes, this is the future format of the bindings so that automated validation can be performed. See Documentation/devicetree/writing-schema.md. Brian
Hello On 4/1/19 5:30 AM, Brian Masney wrote: > Add new backlight bindings for the TI LM3630A dual-string white LED. > > Signed-off-by: Brian Masney <masneyb@onstation.org> > --- > .../leds/backlight/lm3630a-backlight.yaml | 112 ++++++++++++++++++ > 1 file changed, 112 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml > > diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml > new file mode 100644 > index 000000000000..42a8c59d237a > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml > @@ -0,0 +1,112 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/backlight/lm3630a-backlight.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: TI LM3630A High-Efficiency Dual-String White LED > + > +maintainers: > + - Lee Jones <lee.jones@linaro.org> > + - Daniel Thompson <daniel.thompson@linaro.org> > + - Jingoo Han <jingoohan1@gmail.com> > + > +description: | > + The LM3630A is a current-mode boost converter which supplies the power and > + controls the current in up to two strings of 10 LEDs per string. > + https://www.ti.com/product/LM3630A > + > +properties: > + compatible: > + const: ti,lm3630a > + > + reg: > + maxItems: 1 > + No description on the reg here. Assumed to be the i2c address? > + ti,linear-mapping-mode: > + description: | > + Enable linear mapping mode. If disabled, then it will use exponential > + mapping mode in which the ramp up/down appears to have a more uniform > + tranisiton to the human eye. s/tranisiton/transition > + type: boolean > + > +required: > + - compatible > + - reg > + > +patternProperties: > + "^led*$": > + type: object > + description: | > + Properties for a string of connected LEDs. > + I have written a few of these drivers that have control bank support to control. We have been defining the reg property as the control bank id reg 0 - Control bank A reg 1 - Control bank B Then use the led-sources to define what outputs are associated with which control bank. This way the hardware can just have CB B enabled without CB A. something like below would allow just control bank B to be enabled. led@1 { reg = <1>; label = "main-lcd"; default-brightness = <200>; max-brightness = <255>; }; This would connect control bank B to control bank A. Or just use a flag to denote to connect them and not use led-sources. But led-sources is the property of choice. led@0 { reg = <0>; led-sources = < 0 1 >; label = "main-lcd"; default-brightness = <200>; max-brightness = <255>; }; And just the presence of 2 child nodes would indicate individual bank control led@0 { reg = <0>; label = "main-lcd"; default-brightness = <200>; max-brightness = <255>; }; led@1 { reg = <1>; label = "main-keypad"; default-brightness = <200>; max-brightness = <255>; }; > + properties: > + label: > + description: | > + The label for this LED. If omitted, the label is taken from the node > + name (excluding the unit address). It has to uniquely identify a > + device, i.e. no other LED class device can be assigned the same label. > + > + led-sources: > + description: | > + List of device current outputs the LED is connected to. > + allOf: > + - $ref: /schemas/types.yaml#/definitions/uint32-array > + - minItems: 1 > + maxItems: 2 > + items: > + minimum: 0 > + maximum: 1 > + label and led-sources are already defined in the common.txt no need to redefine them here. > + default-brightness: > + description: Default brightness level on boot. > + minimum: 0 > + maximum: 255 > + > + max-brightness: > + description: Maximum brightness level on boot. This is not the max brightness on boot. This is the max brightness that is allowed during runtime. Dan > + minimum: 0 > + maximum: 255 > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + lm3630a_bl@38 { > + compatible = "ti,lm3630a"; > + status = "ok"; > + reg = <0x38>; > + > + led { > + label = "main-lcd"; > + led-sources = <0 1>; > + default-brightness = <200>; > + max-brightness = <255>; > + }; > + }; > + }; > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + lm3630a_bl@38 { > + compatible = "ti,lm3630a"; > + status = "ok"; > + reg = <0x38>; > + > + led-bank-a { > + led-sources = <0>; > + default-brightness = <150>; > + ti,linear-mapping-mode; > + }; > + > + led-bank-b { > + led-sources = <1>; > + default-brightness = <225>; > + ti,linear-mapping-mode; > + }; > + }; > + }; >
On Tue, Apr 02, 2019 at 07:56:55AM -0500, Dan Murphy wrote: > This would connect control bank B to control bank A. Or just use a flag to denote to connect them > and not use led-sources. But led-sources is the property of choice. > > led@0 { > reg = <0>; > led-sources = < 0 1 >; > label = "main-lcd"; > default-brightness = <200>; > max-brightness = <255>; > }; OK, I see. I wondered how we could do that in device tree. > > + properties: > > + label: > > + description: | > > + The label for this LED. If omitted, the label is taken from the node > > + name (excluding the unit address). It has to uniquely identify a > > + device, i.e. no other LED class device can be assigned the same label. > > + > > + led-sources: > > + description: | > > + List of device current outputs the LED is connected to. > > + allOf: > > + - $ref: /schemas/types.yaml#/definitions/uint32-array > > + - minItems: 1 > > + maxItems: 2 > > + items: > > + minimum: 0 > > + maximum: 1 > > + > > label and led-sources are already defined in the common.txt no need to redefine them here. If I'm going to use the new-style bindings, then I'll need to convert common.txt over to use the new format as well so that the automated schema validations will work. I'm willing to do that work if there is interest from the LED / backlight maintainers. The main issue is that there are 62 references to the file common.txt in the directory Documentation/devicetree/bindings/leds/. Would the maintainers prefer: - Once common.txt is converted to common.yaml, make common.txt only have a line stating that the common bindings were moved into common.yaml. We can remove this file once all of the other bindings are converted to the new-style format. - Update all references to common.txt to common.yaml. (1 patch or 62 patches?) - Or, just go with the older-style binding format for now. Thanks Dan for your other comments. They make sense and I'll incorporate those changes into my next version. Brian
Brian On 4/2/19 8:24 AM, Brian Masney wrote: > On Tue, Apr 02, 2019 at 07:56:55AM -0500, Dan Murphy wrote: >> This would connect control bank B to control bank A. Or just use a flag to denote to connect them >> and not use led-sources. But led-sources is the property of choice. >> >> led@0 { >> reg = <0>; >> led-sources = < 0 1 >; >> label = "main-lcd"; >> default-brightness = <200>; >> max-brightness = <255>; >> }; > > OK, I see. I wondered how we could do that in device tree. > >>> + properties: >>> + label: >>> + description: | >>> + The label for this LED. If omitted, the label is taken from the node >>> + name (excluding the unit address). It has to uniquely identify a >>> + device, i.e. no other LED class device can be assigned the same label. >>> + >>> + led-sources: >>> + description: | >>> + List of device current outputs the LED is connected to. >>> + allOf: >>> + - $ref: /schemas/types.yaml#/definitions/uint32-array >>> + - minItems: 1 >>> + maxItems: 2 >>> + items: >>> + minimum: 0 >>> + maximum: 1 >>> + >> >> label and led-sources are already defined in the common.txt no need to redefine them here. > > If I'm going to use the new-style bindings, then I'll need to convert > common.txt over to use the new format as well so that the automated > schema validations will work. I'm willing to do that work if there is > interest from the LED / backlight maintainers. The main issue is that > there are 62 references to the file common.txt in the directory > Documentation/devicetree/bindings/leds/. Would the maintainers prefer: > > - Once common.txt is converted to common.yaml, make common.txt only > have a line stating that the common bindings were moved into > common.yaml. We can remove this file once all of the other bindings > are converted to the new-style format. > > - Update all references to common.txt to common.yaml. (1 patch or 62 > patches?) > > - Or, just go with the older-style binding format for now. > > Thanks Dan for your other comments. They make sense and I'll incorporate > those changes into my next version. > That is up to the maintainers. Also one other comment I noticed when reviewing the code that there is no definition to which child led properties are optional and which are required? Dan > Brian >
On Tue, Apr 2, 2019 at 8:24 AM Brian Masney <masneyb@onstation.org> wrote: > > On Tue, Apr 02, 2019 at 07:56:55AM -0500, Dan Murphy wrote: > > This would connect control bank B to control bank A. Or just use a flag to denote to connect them > > and not use led-sources. But led-sources is the property of choice. > > > > led@0 { > > reg = <0>; > > led-sources = < 0 1 >; > > label = "main-lcd"; > > default-brightness = <200>; > > max-brightness = <255>; > > }; > > OK, I see. I wondered how we could do that in device tree. > > > > + properties: > > > + label: > > > + description: | > > > + The label for this LED. If omitted, the label is taken from the node > > > + name (excluding the unit address). It has to uniquely identify a > > > + device, i.e. no other LED class device can be assigned the same label. > > > + > > > + led-sources: > > > + description: | > > > + List of device current outputs the LED is connected to. > > > + allOf: > > > + - $ref: /schemas/types.yaml#/definitions/uint32-array > > > + - minItems: 1 > > > + maxItems: 2 > > > + items: > > > + minimum: 0 > > > + maximum: 1 > > > + > > > > label and led-sources are already defined in the common.txt no need to redefine them here. > > If I'm going to use the new-style bindings, then I'll need to convert > common.txt over to use the new format as well so that the automated > schema validations will work. I'm willing to do that work if there is > interest from the LED / backlight maintainers. The main issue is that > there are 62 references to the file common.txt in the directory > Documentation/devicetree/bindings/leds/. Would the maintainers prefer: That would be awesome. > - Once common.txt is converted to common.yaml, make common.txt only > have a line stating that the common bindings were moved into > common.yaml. We can remove this file once all of the other bindings > are converted to the new-style format. > > - Update all references to common.txt to common.yaml. (1 patch or 62 > patches?) I'd probably go with the former if I had to pick. The real complication here is there's some other changes in flight. > > - Or, just go with the older-style binding format for now. I'm okay if you write it with the assumption that the common properties have a schema even if they don't yet. All that means is you don't need to define the type, but only the additional constraints. There doesn't have to be a reference back to the common property (those references tend to get broken as we can see here). With everything as structured data, we could even generate references or have some tool that can lookup the common definitions. Rob
Hi Dan, On Tue, Apr 02, 2019 at 08:44:22AM -0500, Dan Murphy wrote: > Also one other comment I noticed when reviewing the code that there is no definition to > which child led properties are optional and which are required? With the new YAML bindings, there is a separate toplevel 'required' tag in the schema. Here is a snippet from my last submission that illustrates this: properties: compatible: const: ti,lm3630a reg: maxItems: 1 ti,linear-mapping-mode: description: | Enable linear mapping mode. If disabled, then it will use exponential mapping mode in which the ramp up/down appears to have a more uniform transition to the human eye. type: boolean required: - compatible - reg So 'ti,linear-mapping-mode' is optional in this example. > > + led-sources: > > + description: | > > + List of device current outputs the LED is connected to. > > + allOf: > > + - $ref: /schemas/types.yaml#/definitions/uint32-array > > + - minItems: 1 > > + maxItems: 2 > > + items: > > + minimum: 0 > > + maximum: 1 > > + > > label and led-sources are already defined in the common.txt no need to redefine them here. We'll still need to define the led-sources and label in this binding with the new format even though it will also be in the LED common bindings. We won't have to put the common things like the description since that information will come from the common binding. We only need to specify the additional constraints like the min/max number of items and the min/max value for each item for this particular example. Brian
diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml new file mode 100644 index 000000000000..42a8c59d237a --- /dev/null +++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml @@ -0,0 +1,112 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/backlight/lm3630a-backlight.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI LM3630A High-Efficiency Dual-String White LED + +maintainers: + - Lee Jones <lee.jones@linaro.org> + - Daniel Thompson <daniel.thompson@linaro.org> + - Jingoo Han <jingoohan1@gmail.com> + +description: | + The LM3630A is a current-mode boost converter which supplies the power and + controls the current in up to two strings of 10 LEDs per string. + https://www.ti.com/product/LM3630A + +properties: + compatible: + const: ti,lm3630a + + reg: + maxItems: 1 + + ti,linear-mapping-mode: + description: | + Enable linear mapping mode. If disabled, then it will use exponential + mapping mode in which the ramp up/down appears to have a more uniform + tranisiton to the human eye. + type: boolean + +required: + - compatible + - reg + +patternProperties: + "^led*$": + type: object + description: | + Properties for a string of connected LEDs. + + properties: + label: + description: | + The label for this LED. If omitted, the label is taken from the node + name (excluding the unit address). It has to uniquely identify a + device, i.e. no other LED class device can be assigned the same label. + + led-sources: + description: | + List of device current outputs the LED is connected to. + allOf: + - $ref: /schemas/types.yaml#/definitions/uint32-array + - minItems: 1 + maxItems: 2 + items: + minimum: 0 + maximum: 1 + + default-brightness: + description: Default brightness level on boot. + minimum: 0 + maximum: 255 + + max-brightness: + description: Maximum brightness level on boot. + minimum: 0 + maximum: 255 + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lm3630a_bl@38 { + compatible = "ti,lm3630a"; + status = "ok"; + reg = <0x38>; + + led { + label = "main-lcd"; + led-sources = <0 1>; + default-brightness = <200>; + max-brightness = <255>; + }; + }; + }; + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + lm3630a_bl@38 { + compatible = "ti,lm3630a"; + status = "ok"; + reg = <0x38>; + + led-bank-a { + led-sources = <0>; + default-brightness = <150>; + ti,linear-mapping-mode; + }; + + led-bank-b { + led-sources = <1>; + default-brightness = <225>; + ti,linear-mapping-mode; + }; + }; + };
Add new backlight bindings for the TI LM3630A dual-string white LED. Signed-off-by: Brian Masney <masneyb@onstation.org> --- .../leds/backlight/lm3630a-backlight.yaml | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml