Re: [PATCH 1/2 v1] dt-bindings: backlight: Add Kinetic KTD253 bindings

[Daniel Thompson <daniel.thompson@linaro.org>]

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.