[v2,1/2] dt-bindings: backlight: document new property default-brightness-level

From: Yannick Fertre <yannick.fertre@foss.st.com>

Add documentation for new default-brightness-level property.

Signed-off-by: Yannick Fertre <yannick.fertre@foss.st.com>
Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
---

Link to original patch:
  https://github.com/STMicroelectronics/linux/commit/c4067d7bd883c6fa14ffd49892c4ce663cdafe98

Changelog v1 -> v2:
* https://lore.kernel.org/dri-devel/20230519200520.10657-2-alex@shruggie.ro/
* removed 'brightness-levels' reference
* updated doc-text for 'default-brightness-level'
* updated doc-text for 'default-on'
* added 'minimum' & 'maximum' to 'default-brightness-level' property
* removed 'Reviewed-by: Philippe CORNU <philippe.cornu@foss.st.com>' as
  requested
* patch is first in series of 2 patches (was second patch)

 .../bindings/leds/backlight/gpio-backlight.yaml    | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

On Thu, Jun 22, 2023 at 12:54:56AM +0300, Alexandru Ardelean wrote:
> From: Yannick Fertre <yannick.fertre@foss.st.com>
> 
> Add documentation for new default-brightness-level property.

Why?

> 
> Signed-off-by: Yannick Fertre <yannick.fertre@foss.st.com>
> Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> ---
> 
> Link to original patch:
>   https://github.com/STMicroelectronics/linux/commit/c4067d7bd883c6fa14ffd49892c4ce663cdafe98
> 
> Changelog v1 -> v2:
> * https://lore.kernel.org/dri-devel/20230519200520.10657-2-alex@shruggie.ro/
> * removed 'brightness-levels' reference
> * updated doc-text for 'default-brightness-level'
> * updated doc-text for 'default-on'
> * added 'minimum' & 'maximum' to 'default-brightness-level' property
> * removed 'Reviewed-by: Philippe CORNU <philippe.cornu@foss.st.com>' as
>   requested
> * patch is first in series of 2 patches (was second patch)
> 
>  .../bindings/leds/backlight/gpio-backlight.yaml    | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> index 584030b6b0b9..2da6552a207c 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> +++ b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> @@ -20,9 +20,21 @@ properties:
>      maxItems: 1
>  
>    default-on:
> -    description: enable the backlight at boot.
> +    description:
> +      The default power state of the backlight at boot.
>      type: boolean
>  
> +  default-brightness-level:
> +    description:
> +      The default brightness level on device init. The value can be 0 or 1.
> +      If omitted, the value is 1. In the context of the "gpio-backlight" driver
> +      the effect of this setting will be that the backlight is on/off.
> +      The difference between this setting and "default-on" is that this handles
> +      brightness, while "default-on" handles the power setting of the device.

What power setting? You only have 1 GPIO to control here which is 2 
states. I fail to see why you need 6 possible states with all the 
combinations of 2 properties.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 1
> +
>  required:
>    - compatible
>    - gpios
> -- 
> 2.40.1
>

On Thu, Jun 22, 2023 at 5:13 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jun 22, 2023 at 12:54:56AM +0300, Alexandru Ardelean wrote:
> > From: Yannick Fertre <yannick.fertre@foss.st.com>
> >
> > Add documentation for new default-brightness-level property.
>
> Why?

I'll admit, I liked the fact that the "default-brightness-level" is
more uniform in the space of backlights.
The "default-on" property is more specific to the gpio-backlight driver.
And then there's gpio hogging that could also work.

>
> >
> > Signed-off-by: Yannick Fertre <yannick.fertre@foss.st.com>
> > Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
> > ---
> >
> > Link to original patch:
> >   https://github.com/STMicroelectronics/linux/commit/c4067d7bd883c6fa14ffd49892c4ce663cdafe98
> >
> > Changelog v1 -> v2:
> > * https://lore.kernel.org/dri-devel/20230519200520.10657-2-alex@shruggie.ro/
> > * removed 'brightness-levels' reference
> > * updated doc-text for 'default-brightness-level'
> > * updated doc-text for 'default-on'
> > * added 'minimum' & 'maximum' to 'default-brightness-level' property
> > * removed 'Reviewed-by: Philippe CORNU <philippe.cornu@foss.st.com>' as
> >   requested
> > * patch is first in series of 2 patches (was second patch)
> >
> >  .../bindings/leds/backlight/gpio-backlight.yaml    | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> > index 584030b6b0b9..2da6552a207c 100644
> > --- a/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> > +++ b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> > @@ -20,9 +20,21 @@ properties:
> >      maxItems: 1
> >
> >    default-on:
> > -    description: enable the backlight at boot.
> > +    description:
> > +      The default power state of the backlight at boot.
> >      type: boolean
> >
> > +  default-brightness-level:
> > +    description:
> > +      The default brightness level on device init. The value can be 0 or 1.
> > +      If omitted, the value is 1. In the context of the "gpio-backlight" driver
> > +      the effect of this setting will be that the backlight is on/off.
> > +      The difference between this setting and "default-on" is that this handles
> > +      brightness, while "default-on" handles the power setting of the device.
>
> What power setting? You only have 1 GPIO to control here which is 2
> states. I fail to see why you need 6 possible states with all the
> combinations of 2 properties.

So, the "default-on" bool gets converted to backlight power settings,
which eventually gets converted back to GPIO values (at some point).
Which sounds quirky (when saying/writing it).

But, yeah.
That's one thing that also made me a bit undecided to send this.
On the one hand I like the uniformity it brings.
On the other hand, because there is the legacy behavior (the
"default-on" property, and the fact that we can use the GPIO DT
settings to control this) just explodes complexity/quirks.

We can probably just drop this.
I'll also admit that my doc-writing skills aren't too great.

Thanks
Alex

>
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 1
> > +
> >  required:
> >    - compatible
> >    - gpios
> > --
> > 2.40.1
> >

On Fri, Jun 23, 2023 at 10:19:57AM +0300, Alexandru Ardelean wrote:
> On Thu, Jun 22, 2023 at 5:13 AM Rob Herring <robh@kernel.org> wrote:
> > > +++ b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
> > >    default-on:
> > > -    description: enable the backlight at boot.
> > > +    description:
> > > +      The default power state of the backlight at boot.
> > >      type: boolean
> > >
> > > +  default-brightness-level:
> > > +    description:
> > > +      The default brightness level on device init. The value can be 0 or 1.
> > > +      If omitted, the value is 1. In the context of the "gpio-backlight" driver
> > > +      the effect of this setting will be that the backlight is on/off.
> > > +      The difference between this setting and "default-on" is that this handles
> > > +      brightness, while "default-on" handles the power setting of the device.
> >
> > What power setting? You only have 1 GPIO to control here which is 2
> > states.

There are at least three states: On/Off/HiZ .

Currently the DT description isn't acually rich enough to allow drivers
to safely use the HiZ state so that is not why this change is potentially
useful today (but does illustrate why it is not "wrong" to put it on the
h/ware description).


> > I fail to see why you need 6 possible states with all the
> > combinations of 2 properties.
>
> So, the "default-on" bool gets converted to backlight power settings,
> which eventually gets converted back to GPIO values (at some point).
> Which sounds quirky (when saying/writing it).

Modern DT practice is to for the display to link to backlight. This
gives display control over power state (so backlight automatically
follows the display power state). On such systems the backlight will
be turned "on" when the display hardware comes up (regardless of whether
or not default-on is set).

Thus this control covers the case where we have a display that is
readable when the GPIO is off (e.g. transflexive LCD or epaper).
A display that is readable with the GPIO off means the default
brightness brightness at boot can meaningfully be zero. In this
case the backlight is nominally on but the GPIO is off.

In short, this becomes part of the hardware description, rather than
merely being a driver feature, due to the effect of linking display
to backlight in the DT.

Note also that most backlights do expose on/off via DT for the same
reasons (when the off and zero states both result in the backlight
output pin doing physically the same thing).


> But, yeah.
> That's one thing that also made me a bit undecided to send this.
> On the one hand I like the uniformity it brings.
> On the other hand, because there is the legacy behavior (the
> "default-on" property, and the fact that we can use the GPIO DT
> settings to control this) just explodes complexity/quirks.
>
> We can probably just drop this.
> I'll also admit that my doc-writing skills aren't too great.

It may be potentially useful for people building kit with sunlight
readable displays and trivial backlights as a backup in the dark.

Of course if the pin the backlight is connected to is PWM capable
then the PWM backlight is probably a better bet ;-) .


Daniel.