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 11:39:39 +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: <20200622103939.goqysclrsxps344i@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:
> Good to have these converted. A few comments in the following. One
> comment is for the backlight people as you copied the original text.

... and I've sliced out everything except that in this reply.

> On Thu, Jun 18, 2020 at 04:44:13PM -0600, Rob Herring wrote:
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/led-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/led-backlight.yaml
> > new file mode 100644
> > index 000000000000..ae50945d2798
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/backlight/led-backlight.yaml
> > @@ -0,0 +1,58 @@
> > ...
> > +
> > +  default-brightness-level:
> > +    description: The default brightness level (index into the array defined
> > +      by the "brightness-levels" property).
>
> This description does not match my understading.
> The description says "index into", but in reality this is a value that
> matches somewhere in the range specified by brightness-levels.
> So it is not an index.
> 
> Maybe I just read it wrong and the description is fine. But when I read
> index the when it says 6 I would look for brightness-levels[6] equals
> 128 in the example below.

When the brightness-levels array is not present then backlight
brightness and led brightness have a 1:1 mapping. In this case the
meaning of "default brightness level" is (hopefully) obvious and the
parenthetic text does not apply.

When the array is present then we have two different scales of
brightness and it is important to describe which scale we use for the
default brightness. The language about "index into" was adopted to avoid
having to introduce extra terminology whilst making it clear that
setting the default brightness-level to 128 is invalid (because it is
not an acceptable index) and that a value of 6 will result in the LED
brightness being set to 128.


> And this is not how it is coded.

That had me worried for a moment but I think the code and bindings are
in agreement.

There is some code to map the LED scale (128) to the backlight scale (6)
but this used to ensure we supply the correct brightness level to user
space when an active backlight is handed over from bootloader to kernel.

If a default-brightness-level is provided then the above logic is
overridden and the value read from the DT gets placed directly into
props.brightness where it will act as in index into the brightness
table.


Daniel.