| Message ID | 20200720203506.3883129-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2,v1] dt-bindings: backlight: Add Kinetic KTD253 bindings | expand |
Hi Linus. On Mon, Jul 20, 2020 at 10:35:05PM +0200, Linus Walleij wrote: > This adds device tree bindings for the Kinetic KTD253 > white LED backlight driver. > > Cc: devicetree@vger.kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> See a few comments in the following. Sam > --- > .../leds/backlight/kinetic,ktd253.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml > > diff --git a/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml > new file mode 100644 > index 000000000000..610bf9a0e270 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/backlight/kinetic,ktd253.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Kinetic Technologies KTD253 one-wire backlight > + > +maintainers: > + - Linus Walleij <linus.walleij@linaro.org> > + > +description: | > + The Kinetic Technologies KTD253 is a white LED backlight that is > + controlled by a single GPIO line. If you just turn on the backlight > + it goes to maximum backlight then you can set the level of backlight > + using pulses on the enable wire. No $ref for common.yaml? > + > +properties: > + compatible: > + const: kinetic,ktd253 > + > + gpios: A less generic and more descriptive name would be good. > + description: GPIO to use to enable/disable and dim the backlight. > + maxItems: 1 > + > + default-brightness: > + description: Default brightness level on boot. 0 is off. > + minimum: 0 > + maximum: 255 > + > + max-brightness: > + description: Maximum brightness that is allowed during runtime. > + minimum: 0 > + maximum: 255 Both looks like candidates for common.yaml - they are used by other bindings. > + > +required: > + - compatible > + - gpios It would make senste that maximum-brighness was mandatory too. addtionalProperties: false?? > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + backlight { > + compatible = "kinetic,ktd253"; > + gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; > + default-on; default-on is not documented - and not part of common.yaml. > + default-brightness = <160>; > + }; > -- > 2.26.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Jul 20, 2020 at 10:35:05PM +0200, Linus Walleij wrote: > This adds device tree bindings for the Kinetic KTD253 > white LED backlight driver. > > Cc: devicetree@vger.kernel.org > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > .../leds/backlight/kinetic,ktd253.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml > > diff --git a/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml > new file mode 100644 > index 000000000000..610bf9a0e270 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/leds/backlight/kinetic,ktd253.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Kinetic Technologies KTD253 one-wire backlight > + > +maintainers: > + - Linus Walleij <linus.walleij@linaro.org> > + > +description: | > + The Kinetic Technologies KTD253 is a white LED backlight that is > + controlled by a single GPIO line. If you just turn on the backlight > + it goes to maximum backlight then you can set the level of backlight > + using pulses on the enable wire. > + > +properties: > + compatible: > + const: kinetic,ktd253 > + > + gpios: > + description: GPIO to use to enable/disable and dim the backlight. > + maxItems: 1 > + > + default-brightness: > + description: Default brightness level on boot. 0 is off. > + minimum: 0 > + maximum: 255 > + > + max-brightness: > + description: Maximum brightness that is allowed during runtime. > + minimum: 0 > + maximum: 255 [I ended up dropping this into this thread... but it applies to both patches] I'm a bit sceptical of having a max-brightness in the DT and a driver defined lookup table in the driver itself. That doesn't make a whole lot of sense to me since the maximum brightness here is basically relies on knowing what scale the Linux driver has opted to implement in its tables. I think there are two options here. 1. Throw away the brightness table in the driver and expose the hardware steps directly (maybe using allowing properties such as max-brightness = 24 if the top 8 values cannot be distinguished visually). 2. Implement a brightness table in the DT if there really is a need to linearize the feel of the slider. In that case max-brightness can be inferred from the maximum value in the table. Note that #2 is absolutely *not* the same as the tables in pwm_bl.c (which are used to map a very wide linear scale on the hardware into a smaller logarithmic interface for software to use). For this driver the driver's lookup table is used to present an oversized scale to software and quantizing it in the driver (using variably sized quantums) to create a hardware value. This can be useful if the hardware's perceptual response feels *really* lumpy but often results in sliders with dead zones (because they do not "snap" to the hardware intervals). Looking at the gaps in the driver I'm suspect the table is not worth the effort (the difference in the deltas is pretty modest) but I'm happy to contradicted by someone with access to the hardware! Daniel.
On Tue, Jul 21, 2020 at 10:32 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > +description: | > > + The Kinetic Technologies KTD253 is a white LED backlight that is > > + controlled by a single GPIO line. If you just turn on the backlight > > + it goes to maximum backlight then you can set the level of backlight > > + using pulses on the enable wire. > > No $ref for common.yaml? Since this is a backlight, and we do not have common bindings for, backlight I first looked into using the LED bindings in ../common.yaml, but that has several problems, it cannot really be used for backlight. Backlight doesn't have "triggers", patterns, flash properties, the function is also pretty much evident. So I will look into creating a new common for backlight. Yours, Linus Walleij
On Tue, Jul 21, 2020 at 1:28 PM Daniel Thompson <daniel.thompson@linaro.org> wrote: > I'm a bit sceptical of having a max-brightness in the DT and a driver > defined lookup table in the driver itself. That doesn't make a whole lot > of sense to me since the maximum brightness here is basically relies on > knowing what scale the Linux driver has opted to implement in its tables. That's a good point. > I think there are two options here. > > 1. Throw away the brightness table in the driver and expose the hardware > steps directly (maybe using allowing properties such as > max-brightness = 24 if the top 8 values cannot be distinguished > visually). I think I will opt for this. It makes most sense given how we use the device tree to restrict maximum brightness, and that is definitely related to the hardware max brightness. Thanks Daniel! Linus Walleij
Hi Linus. On Wed, Aug 12, 2020 at 08:48:42AM +0200, Linus Walleij wrote: > On Tue, Jul 21, 2020 at 10:32 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > +description: | > > > + The Kinetic Technologies KTD253 is a white LED backlight that is > > > + controlled by a single GPIO line. If you just turn on the backlight > > > + it goes to maximum backlight then you can set the level of backlight > > > + using pulses on the enable wire. > > > > No $ref for common.yaml? > > Since this is a backlight, and we do not have common bindings for, > backlight I first looked into using the LED bindings in > ../common.yaml, but that has several problems, it cannot really > be used for backlight. Backlight doesn't have "triggers", > patterns, flash properties, the function is also pretty much > evident. > > So I will look into creating a new common for backlight. Hmm, I think I looked at leds/ when I wrote that comment about common.yaml. Please consider Rob's comment in commit: 44e1655a444fe7a1bd81994d34c6bbb5245b9e60 ("dt-bindings: backlight: Convert common backlight bindings to DT schema") Rob did not see the need for a common binding - but that may change as we add more backlight bindings. Sam
On Wed, Aug 12, 2020 at 9:34 AM Sam Ravnborg <sam@ravnborg.org> wrote: > Hmm, I think I looked at leds/ when I wrote that comment about > common.yaml. > > Please consider Rob's comment in commit: 44e1655a444fe7a1bd81994d34c6bbb5245b9e60 > ("dt-bindings: backlight: Convert common backlight bindings to DT > schema") > > Rob did not see the need for a common binding - but that may change as > we add more backlight bindings. It can't hurt. The proposal is out there, there are some drivers in backlight that can readily be converted to use it if it is favored. Thanks! Linus Walleij
diff --git a/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml new file mode 100644 index 000000000000..610bf9a0e270 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/backlight/kinetic,ktd253.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Kinetic Technologies KTD253 one-wire backlight + +maintainers: + - Linus Walleij <linus.walleij@linaro.org> + +description: | + The Kinetic Technologies KTD253 is a white LED backlight that is + controlled by a single GPIO line. If you just turn on the backlight + it goes to maximum backlight then you can set the level of backlight + using pulses on the enable wire. + +properties: + compatible: + const: kinetic,ktd253 + + gpios: + description: GPIO to use to enable/disable and dim the backlight. + maxItems: 1 + + default-brightness: + description: Default brightness level on boot. 0 is off. + minimum: 0 + maximum: 255 + + max-brightness: + description: Maximum brightness that is allowed during runtime. + minimum: 0 + maximum: 255 + +required: + - compatible + - gpios + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + backlight { + compatible = "kinetic,ktd253"; + gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; + default-on; + default-brightness = <160>; + };
This adds device tree bindings for the Kinetic KTD253 white LED backlight driver. Cc: devicetree@vger.kernel.org Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- .../leds/backlight/kinetic,ktd253.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/backlight/kinetic,ktd253.yaml -- 2.26.2