Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 22 Jun 2020 17:57:30 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sam Ravnborg <sam@ravnborg.org>
Cc:     Rob Herring <robh@kernel.org>, devicetree@vger.kernel.org,
        dri-devel@lists.freedesktop.org, Jingoo Han <jingoohan1@gmail.com>,
        Lee Jones <lee.jones@linaro.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] dt-bindings: backlight: Convert common backlight
 bindings to DT schema
Message-ID: <20200622165730.pnx7fzbq5e6q5h4l@holly.lan>
References: <20200618224413.1115849-1-robh@kernel.org>
 <20200619215341.GA6857@ravnborg.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200619215341.GA6857@ravnborg.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jun 19, 2020 at 11:53:41PM +0200, Sam Ravnborg wrote:
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.yaml
> > new file mode 100644
> > index 000000000000..7e1f109a38a4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.yaml
> > @@ -0,0 +1,98 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/backlight/pwm-backlight.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: pwm-backlight bindings
> > +
> > +maintainers:
> > +  - Lee Jones <lee.jones@linaro.org>
> > +  - Daniel Thompson <daniel.thompson@linaro.org>
> > +  - Jingoo Han <jingoohan1@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: pwm-backlight
> > +
> > +  pwms:
> > +    maxItems: 1
> > +
> > +  pwm-names: true
> > +
> > +  power-supply:
> > +    description: regulator for supply voltage
> > +
> > +  enable-gpios:
> > +    description: Contains a single GPIO specifier for the GPIO which enables
> > +      and disables the backlight
> > +    maxItems: 1
> > +
> > +  post-pwm-on-delay-ms:
> > +    description: Delay in ms between setting an initial (non-zero) PWM and
> > +      enabling the backlight using GPIO.
> > +
> > +  pwm-off-delay-ms:
> > +    description: Delay in ms between disabling the backlight using GPIO
> > +      and setting PWM value to 0.
> > +
> > +  brightness-levels:
> > +    description: Array of distinct brightness levels. Typically these are
> > +      in the range from 0 to 255, but any range starting at 0 will do. The
> > +      actual brightness level (PWM duty cycle) will be interpolated from
> > +      these values. 0 means a 0% duty cycle (darkest/off), while the last
> > +      value in the array represents a 100% duty cycle (brightest).
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +
> > +  default-brightness-level:
> > +    description: The default brightness level (index into the array defined
> > +      by the "brightness-levels" property).
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> Same comment as before...

Sorry the "ditto" meant I didn't thing about PWM as much as I should
have.

The situation for PWM is a little different to LED. That's mostly
because we decided not to clutter the LED code with
"num-interpolated-steps".

The PWM code implements the default-brightness-level as an index into
the brightness array *after* it has been expanded using interpolation.
In other words today Linux treats the default-brightness-level more
like[1].

    description: The default brightness level. When
      num-interpolated-steps is not set this is simply an index into
      the array defined by the "brightness-levels" property. If
      num-interpolated-steps is set the brightness array will be
      expanded by interpolation before we index to get a default
      level.

This is the best I have come up with so far... but I concede it still
lacks elegance.


Daniel.


[1] I don't know my way round the BSD code bases to be sure what they
    do... I did a couple of web searches but didn't pull up anything
    definitive.


> 
> > +
> > +  num-interpolated-steps:
> > +    description: Number of interpolated steps between each value of brightness-levels
> > +      table. This way a high resolution pwm duty cycle can be used without
> > +      having to list out every possible value in the brightness-level array.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +dependencies:
> > +  default-brightness-level: [brightness-levels]
> > +  num-interpolated-steps: [brightness-levels]
> > +
> > +required:
> > +  - compatible
> > +  - pwms
> > +  - power-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    backlight {
> > +        compatible = "pwm-backlight";
> > +        pwms = <&pwm 0 5000000>;
> > +
> > +        brightness-levels = <0 4 8 16 32 64 128 255>;
> > +        default-brightness-level = <6>;
> > +
> > +        power-supply = <&vdd_bl_reg>;
> > +        enable-gpios = <&gpio 58 0>;
> > +        post-pwm-on-delay-ms = <10>;
> > +        pwm-off-delay-ms = <10>;
> > +    };
> > +
> > +  - |
> > +    // Example using num-interpolation-steps:
> > +    backlight {
> > +        compatible = "pwm-backlight";
> > +        pwms = <&pwm 0 5000000>;
> > +
> > +        brightness-levels = <0 2048 4096 8192 16384 65535>;
> > +        num-interpolated-steps = <2048>;
> > +        default-brightness-level = <4096>;
> > +
> > +        power-supply = <&vdd_bl_reg>;
> > +        enable-gpios = <&gpio 58 0>;
> > +    };
> > +
> > +...
> > -- 
> > 2.25.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel