| Message ID | 20241206170717.1090206-2-alexander.sverdlin@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | leds: TI LP8864/LP8866 support | expand |
On 12/6/24 11:14 AM, Conor Dooley wrote: > On Fri, Dec 06, 2024 at 06:07:12PM +0100, A. Sverdlin wrote: >> From: Alexander Sverdlin <alexander.sverdlin@siemens.com> >> >> Add Texas Instruments' LP8864/LP8866 bindings into LP8860 converting them >> into YAML format simultaneously. While here, drop the index of the "led" >> subnode, this one is neither used nor mandated by the drivers. All the >> *-cells properties are therefore not required. > The index isn't needed, but we should still allow for multiple child LED nodes. The usual way to do this is with node names led-0, led-1, etc.. See here for the usual patternProperties for that: https://www.kernel.org/doc/Documentation/devicetree/bindings/leds/leds-pwm.yaml > Are you sure this is a correct thing to do? The lp8860-q1 product link > cites it as being a 4-channel device. Even if the kernel only ever > supports it as a single-channel device, the binding should reflect what > it is capable of doing. > Agree, the driver today just checks the first child node, but it could be extended for all 4 supported LED channels, and we shouldn't have to change the binding for that new support. Andrew > Cheers, > Conor. > >> >> Move the file into backlight directory because all of the LP886x drivers >> are special backlight products. >> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> >> --- >> .../bindings/leds/backlight/ti,lp8860.yaml | 86 +++++++++++++++++++ >> .../devicetree/bindings/leds/leds-lp8860.txt | 50 ----------- >> 2 files changed, 86 insertions(+), 50 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti,lp8860.yaml >> delete mode 100644 Documentation/devicetree/bindings/leds/leds-lp8860.txt >> >> diff --git a/Documentation/devicetree/bindings/leds/backlight/ti,lp8860.yaml b/Documentation/devicetree/bindings/leds/backlight/ti,lp8860.yaml >> new file mode 100644 >> index 0000000000000..3ece2f6fc3f02 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/leds/backlight/ti,lp8860.yaml >> @@ -0,0 +1,86 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/leds/backlight/ti,lp8860.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Texas Instruments - LP886x 4/6-Channel LED Driver family >> + >> +maintainers: >> + - Andrew Davis <afd@ti.com> >> + - Alexander Sverdlin <alexander.sverdlin@siemens.com> >> + >> +description: | >> + The LP8860-Q1 is an high-efficiency LED driver with boost controller. >> + It has 4 high-precision current sinks that can be controlled by a PWM input >> + signal, a SPI/I2C master, or both. >> + >> + LP8866-Q1, LP8866S-Q1, LP8864-Q1, LP8864S-Q1 are newer products offering >> + similar functionality with 4/6 channels. >> + >> + For more product information please see the links below: >> + https://www.ti.com/product/lp8860-q1 >> + https://www.ti.com/product/LP8864-Q1 >> + https://www.ti.com/product/LP8864S-Q1 >> + https://www.ti.com/product/LP8866-Q1 >> + https://www.ti.com/product/LP8866S-Q1 >> + >> +properties: >> + compatible: >> + enum: >> + - ti,lp8860 >> + - ti,lp8864 >> + >> + reg: >> + maxItems: 1 >> + description: I2C slave address >> + >> + enable-gpios: >> + maxItems: 1 >> + description: GPIO pin to enable (active high) / disable the device >> + >> + vled-supply: >> + description: LED supply >> + >> + led: >> + type: object >> + $ref: common.yaml# >> + properties: >> + function: true >> + color: true >> + label: true >> + linux,default-trigger: true >> + >> + additionalProperties: false >> + >> +required: >> + - compatible >> + - reg >> + - led >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/gpio/gpio.h> >> + #include <dt-bindings/leds/common.h> >> + >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + led-controller@2d { >> + compatible = "ti,lp8860"; >> + reg = <0x2d>; >> + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; >> + vled-supply = <&vbatt>; >> + >> + led { >> + function = LED_FUNCTION_BACKLIGHT; >> + color = <LED_COLOR_ID_WHITE>; >> + linux,default-trigger = "backlight"; >> + }; >> + }; >> + }; >> + >> +... >> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt >> deleted file mode 100644 >> index 8bb25749a3da3..0000000000000 >> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt >> +++ /dev/null >> @@ -1,50 +0,0 @@ >> -* Texas Instruments - lp8860 4-Channel LED Driver >> - >> -The LP8860-Q1 is an high-efficiency LED >> -driver with boost controller. It has 4 high-precision >> -current sinks that can be controlled by a PWM input >> -signal, a SPI/I2C master, or both. >> - >> -Required properties: >> - - compatible : >> - "ti,lp8860" >> - - reg : I2C slave address >> - - #address-cells : 1 >> - - #size-cells : 0 >> - >> -Optional properties: >> - - enable-gpios : gpio pin to enable (active high)/disable the device. >> - - vled-supply : LED supply >> - >> -Required child properties: >> - - reg : 0 >> - >> -Optional child properties: >> - - function : see Documentation/devicetree/bindings/leds/common.txt >> - - color : see Documentation/devicetree/bindings/leds/common.txt >> - - label : see Documentation/devicetree/bindings/leds/common.txt (deprecated) >> - - linux,default-trigger : >> - see Documentation/devicetree/bindings/leds/common.txt >> - >> -Example: >> - >> -#include <dt-bindings/leds/common.h> >> - >> -led-controller@2d { >> - compatible = "ti,lp8860"; >> - #address-cells = <1>; >> - #size-cells = <0>; >> - reg = <0x2d>; >> - enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; >> - vled-supply = <&vbatt>; >> - >> - led@0 { >> - reg = <0>; >> - function = LED_FUNCTION_BACKLIGHT; >> - color = <LED_COLOR_ID_WHITE>; >> - linux,default-trigger = "backlight"; >> - }; >> -} >> - >> -For more product information please see the link below: >> -https://www.ti.com/product/lp8860-q1 >> -- >> 2.47.1 >>
Hello Conor, On Fri, 2024-12-06 at 17:14 +0000, Conor Dooley wrote: > > Add Texas Instruments' LP8864/LP8866 bindings into LP8860 converting them > > into YAML format simultaneously. While here, drop the index of the "led" > > subnode, this one is neither used nor mandated by the drivers. All the > > *-cells properties are therefore not required. > > Are you sure this is a correct thing to do? The lp8860-q1 product link > cites it as being a 4-channel device. Even if the kernel only ever > supports it as a single-channel device, the binding should reflect what > it is capable of doing. my understanding is: - The whole family is multi-channel (4 or 6), but this is rather used internally in the chip for power balancing; separate diagnostics is provided. From the user perspective one has only one brightness per chip. - The lp8860 driver didn't attempt to do anything with the index. I'm glad Andrew Davis had a time for a pre-public review of the new binding and actually suggested this simplification.
Hi Andrew, On Fri, 2024-12-06 at 11:43 -0600, Andrew Davis wrote: > > Are you sure this is a correct thing to do? The lp8860-q1 product link > > cites it as being a 4-channel device. Even if the kernel only ever > > supports it as a single-channel device, the binding should reflect what > > it is capable of doing. > > > > Agree, the driver today just checks the first child node, but it could > be extended for all 4 supported LED channels, and we shouldn't have > to change the binding for that new support. but the channels are (in my understanding) for power-balancing only, there are no separate controls for them. What do I miss?
On 12/6/24 11:46 AM, Sverdlin, Alexander wrote: > Hi Andrew, > > On Fri, 2024-12-06 at 11:43 -0600, Andrew Davis wrote: >>> Are you sure this is a correct thing to do? The lp8860-q1 product link >>> cites it as being a 4-channel device. Even if the kernel only ever >>> supports it as a single-channel device, the binding should reflect what >>> it is capable of doing. >>> >> >> Agree, the driver today just checks the first child node, but it could >> be extended for all 4 supported LED channels, and we shouldn't have >> to change the binding for that new support. > > but the channels are (in my understanding) for power-balancing only, there > are no separate controls for them. What do I miss? > I'm not very familiar with this part either, but looking at the datasheet I see: > Supports Display Mode (Global Dimming) and > Cluster Mode (Independent Dimming) > In Cluster mode LED strings have independent control but fewer features enabled than in Display Mode. And one channel controlling the others is only in this "Display Mode", but the currents to the others can be independently controlled in a different mode (seems these modes have less features which is probably why the driver doesn't make use of that today). Andrew
Hi Andrew, On Fri, 2024-12-06 at 12:02 -0600, Andrew Davis wrote: > On 12/6/24 11:46 AM, Sverdlin, Alexander wrote: > > Hi Andrew, > > > > On Fri, 2024-12-06 at 11:43 -0600, Andrew Davis wrote: > > > > Are you sure this is a correct thing to do? The lp8860-q1 product link > > > > cites it as being a 4-channel device. Even if the kernel only ever > > > > supports it as a single-channel device, the binding should reflect what > > > > it is capable of doing. > > > > > > > > > > Agree, the driver today just checks the first child node, but it could > > > be extended for all 4 supported LED channels, and we shouldn't have > > > to change the binding for that new support. > > > > but the channels are (in my understanding) for power-balancing only, there > > are no separate controls for them. What do I miss? > > > > I'm not very familiar with this part either, but looking at the datasheet > I see: > > > Supports Display Mode (Global Dimming) and > > Cluster Mode (Independent Dimming) > > > In Cluster mode LED strings have independent control but fewer features enabled than in Display Mode. thanks for looking into this! > And one channel controlling the others is only in this "Display Mode", > but the currents to the others can be independently controlled in a > different mode (seems these modes have less features which is probably > why the driver doesn't make use of that today). You are right! This seems to be the feature of the legacy lp8860. Shall I then leave its binding alone and re-submit new YAML binding as-is for the newer LP8864/LP8866 family? Seems that they don't have the cluster mode any more.
On 12/6/24 12:20 PM, Sverdlin, Alexander wrote: > Hi Andrew, > > On Fri, 2024-12-06 at 12:02 -0600, Andrew Davis wrote: >> On 12/6/24 11:46 AM, Sverdlin, Alexander wrote: >>> Hi Andrew, >>> >>> On Fri, 2024-12-06 at 11:43 -0600, Andrew Davis wrote: >>>>> Are you sure this is a correct thing to do? The lp8860-q1 product link >>>>> cites it as being a 4-channel device. Even if the kernel only ever >>>>> supports it as a single-channel device, the binding should reflect what >>>>> it is capable of doing. >>>>> >>>> >>>> Agree, the driver today just checks the first child node, but it could >>>> be extended for all 4 supported LED channels, and we shouldn't have >>>> to change the binding for that new support. >>> >>> but the channels are (in my understanding) for power-balancing only, there >>> are no separate controls for them. What do I miss? >>> >> >> I'm not very familiar with this part either, but looking at the datasheet >> I see: >> >>> Supports Display Mode (Global Dimming) and >>> Cluster Mode (Independent Dimming) >> >>> In Cluster mode LED strings have independent control but fewer features enabled than in Display Mode. > > thanks for looking into this! > >> And one channel controlling the others is only in this "Display Mode", >> but the currents to the others can be independently controlled in a >> different mode (seems these modes have less features which is probably >> why the driver doesn't make use of that today). > > You are right! This seems to be the feature of the legacy lp8860. > Shall I then leave its binding alone and re-submit new YAML binding as-is > for the newer LP8864/LP8866 family? Seems that they don't have the cluster mode > any more. > Well the txt to yaml binding conversion looks good other than the patternProperties for multiple LEDs part. But if you don't plan on reusing the binding then you don't need it as part of this series. (still good to send it by itself since you already did the work) A new binding doc for these new parts might be the way to go then. Andrew
On Fri, Dec 06, 2024 at 05:44:15PM +0000, Sverdlin, Alexander wrote: > Hello Conor, > > On Fri, 2024-12-06 at 17:14 +0000, Conor Dooley wrote: > > > Add Texas Instruments' LP8864/LP8866 bindings into LP8860 converting them > > > into YAML format simultaneously. While here, drop the index of the "led" > > > subnode, this one is neither used nor mandated by the drivers. All the > > > *-cells properties are therefore not required. > > > > Are you sure this is a correct thing to do? The lp8860-q1 product link > > cites it as being a 4-channel device. Even if the kernel only ever > > supports it as a single-channel device, the binding should reflect what > > it is capable of doing. > > my understanding is: > - The whole family is multi-channel (4 or 6), but this is rather used internally > in the chip for power balancing; separate diagnostics is provided. From the user > perspective one has only one brightness per chip. One brightness perhaps, but what do you do where several LEDs of different colours are connected? Or if one was to be active-low? I don't see the benefit of changing the binding in a way that makes it less able to describe the hardware. > - The lp8860 driver didn't attempt to do anything with the index. I don't see this as being relevant, the bindings need only address what the hardware is able to do. The driver may only implement a subset of that, and that is perfectly okay. > I'm glad Andrew Davis had a time for a pre-public review of the new binding > and actually suggested this simplification. Okay. Thanks, Conor.
diff --git a/Documentation/devicetree/bindings/leds/backlight/ti,lp8860.yaml b/Documentation/devicetree/bindings/leds/backlight/ti,lp8860.yaml new file mode 100644 index 0000000000000..3ece2f6fc3f02 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/backlight/ti,lp8860.yaml @@ -0,0 +1,86 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/backlight/ti,lp8860.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Texas Instruments - LP886x 4/6-Channel LED Driver family + +maintainers: + - Andrew Davis <afd@ti.com> + - Alexander Sverdlin <alexander.sverdlin@siemens.com> + +description: | + The LP8860-Q1 is an high-efficiency LED driver with boost controller. + It has 4 high-precision current sinks that can be controlled by a PWM input + signal, a SPI/I2C master, or both. + + LP8866-Q1, LP8866S-Q1, LP8864-Q1, LP8864S-Q1 are newer products offering + similar functionality with 4/6 channels. + + For more product information please see the links below: + https://www.ti.com/product/lp8860-q1 + https://www.ti.com/product/LP8864-Q1 + https://www.ti.com/product/LP8864S-Q1 + https://www.ti.com/product/LP8866-Q1 + https://www.ti.com/product/LP8866S-Q1 + +properties: + compatible: + enum: + - ti,lp8860 + - ti,lp8864 + + reg: + maxItems: 1 + description: I2C slave address + + enable-gpios: + maxItems: 1 + description: GPIO pin to enable (active high) / disable the device + + vled-supply: + description: LED supply + + led: + type: object + $ref: common.yaml# + properties: + function: true + color: true + label: true + linux,default-trigger: true + + additionalProperties: false + +required: + - compatible + - reg + - led + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/leds/common.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + led-controller@2d { + compatible = "ti,lp8860"; + reg = <0x2d>; + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; + vled-supply = <&vbatt>; + + led { + function = LED_FUNCTION_BACKLIGHT; + color = <LED_COLOR_ID_WHITE>; + linux,default-trigger = "backlight"; + }; + }; + }; + +... diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt deleted file mode 100644 index 8bb25749a3da3..0000000000000 --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt +++ /dev/null @@ -1,50 +0,0 @@ -* Texas Instruments - lp8860 4-Channel LED Driver - -The LP8860-Q1 is an high-efficiency LED -driver with boost controller. It has 4 high-precision -current sinks that can be controlled by a PWM input -signal, a SPI/I2C master, or both. - -Required properties: - - compatible : - "ti,lp8860" - - reg : I2C slave address - - #address-cells : 1 - - #size-cells : 0 - -Optional properties: - - enable-gpios : gpio pin to enable (active high)/disable the device. - - vled-supply : LED supply - -Required child properties: - - reg : 0 - -Optional child properties: - - function : see Documentation/devicetree/bindings/leds/common.txt - - color : see Documentation/devicetree/bindings/leds/common.txt - - label : see Documentation/devicetree/bindings/leds/common.txt (deprecated) - - linux,default-trigger : - see Documentation/devicetree/bindings/leds/common.txt - -Example: - -#include <dt-bindings/leds/common.h> - -led-controller@2d { - compatible = "ti,lp8860"; - #address-cells = <1>; - #size-cells = <0>; - reg = <0x2d>; - enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; - vled-supply = <&vbatt>; - - led@0 { - reg = <0>; - function = LED_FUNCTION_BACKLIGHT; - color = <LED_COLOR_ID_WHITE>; - linux,default-trigger = "backlight"; - }; -} - -For more product information please see the link below: -https://www.ti.com/product/lp8860-q1