[8/8] dt-bindings: display: Add SSD132x OLED controllers

  Add a Device Tree binding schema for the OLED panels based on the Solomon
SSD132x family of controllers.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 .../bindings/display/solomon,ssd132x.yaml     | 116 ++++++++++++++++++
 1 file changed, 116 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/solomon,ssd132x.yaml
  

Hey,

On Mon, Oct 09, 2023 at 08:34:22PM +0200, Javier Martinez Canillas wrote:
> Add a Device Tree binding schema for the OLED panels based on the Solomon
> SSD132x family of controllers.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  .../bindings/display/solomon,ssd132x.yaml     | 116 ++++++++++++++++++
>  1 file changed, 116 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/solomon,ssd132x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/solomon,ssd132x.yaml b/Documentation/devicetree/bindings/display/solomon,ssd132x.yaml
> new file mode 100644
> index 000000000000..b64904703a3a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/solomon,ssd132x.yaml
> @@ -0,0 +1,116 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/solomon,ssd132x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Solomon SSD132x OLED Controllers
> +
> +maintainers:
> +  - Javier Martinez Canillas <javierm@redhat.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - solomon,ssd1322
> +          - solomon,ssd1325
> +          - solomon,ssd1327

You don't need the oneOf here here as there is only the enum as a
possible item.
I didn't get anything else in the series, I have to ask - are these
controllers not compatible with eachother?

> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  # Only required for SPI
> +  dc-gpios:
> +    description:
> +      GPIO connected to the controller's D/C# (Data/Command) pin,
> +      that is needed for 4-wire SPI to tell the controller if the
> +      data sent is for a command register or the display data RAM
> +    maxItems: 1
> +
> +  solomon,height:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Height in pixel of the screen driven by the controller.
> +      The default value is controller-dependent.

You probably know better than me, operating in drm stuff, but are there
really no generic properties for the weidth/height of a display?

> +
> +  solomon,width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Width in pixel of the screen driven by the controller.
> +      The default value is controller-dependent.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: solomon,ssd1322
> +    then:
> +      properties:
> +        width:
> +          default: 480
> +        height:
> +          default: 128
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: solomon,ssd1325
> +    then:
> +      properties:
> +        width:
> +          default: 128
> +        height:
> +          default: 80
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: solomon,ssd1327
> +    then:
> +      properties:
> +        width:
> +          default: 128
> +        height:
> +          default: 128

Unless you did it like this for clarity, 2 of these have the same
default width and 2 have the same default height. You could cut this
down to a pair of if/then/else on that basis AFAICT.
:wq

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            ssd1327_i2c: oled@3c {

This label is unused as far as I can tell. Ditto below.

Cheers,
Conor.

> +                    compatible = "solomon,ssd1327";
> +                    reg = <0x3c>;
> +                    reset-gpios = <&gpio2 7>;
> +            };
> +
> +    };
> +  - |
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            ssd1327_spi: oled@0 {
> +                    compatible = "solomon,ssd1327";
> +                    reg = <0x0>;
> +                    reset-gpios = <&gpio2 7>;
> +                    dc-gpios = <&gpio2 8>;
> +                    spi-max-frequency = <10000000>;
> +            };
> +    };
> -- 
> 2.41.0
> 
>
  

On Mon, Oct 09, 2023 at 08:34:22PM +0200, Javier Martinez Canillas wrote:
> Add a Device Tree binding schema for the OLED panels based on the Solomon
> SSD132x family of controllers.

Looks like the same binding as solomon,ssd1307fb.yaml. Why a different 
binding? Why does that binding need a slew of custom properties and here 
you don't?

> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  .../bindings/display/solomon,ssd132x.yaml     | 116 ++++++++++++++++++
>  1 file changed, 116 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/solomon,ssd132x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/solomon,ssd132x.yaml b/Documentation/devicetree/bindings/display/solomon,ssd132x.yaml
> new file mode 100644
> index 000000000000..b64904703a3a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/solomon,ssd132x.yaml
> @@ -0,0 +1,116 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/solomon,ssd132x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Solomon SSD132x OLED Controllers
> +
> +maintainers:
> +  - Javier Martinez Canillas <javierm@redhat.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - solomon,ssd1322
> +          - solomon,ssd1325
> +          - solomon,ssd1327
> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  # Only required for SPI
> +  dc-gpios:
> +    description:
> +      GPIO connected to the controller's D/C# (Data/Command) pin,
> +      that is needed for 4-wire SPI to tell the controller if the
> +      data sent is for a command register or the display data RAM
> +    maxItems: 1
> +
> +  solomon,height:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Height in pixel of the screen driven by the controller.
> +      The default value is controller-dependent.
> +
> +  solomon,width:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Width in pixel of the screen driven by the controller.
> +      The default value is controller-dependent.

Don't define the same properties twice. Either share the binding or 
split out the common properties into their own schema file.

Rob
  

Conor Dooley <conor@kernel.org> writes:

Hello Conor,

Thanks a lot for your feedback.

> Hey,
>
> On Mon, Oct 09, 2023 at 08:34:22PM +0200, Javier Martinez Canillas wrote:

[...]

>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - enum:
>> +          - solomon,ssd1322
>> +          - solomon,ssd1325
>> +          - solomon,ssd1327
>
> You don't need the oneOf here here as there is only the enum as a
> possible item.

Indeed. I'll fix that in v2.

> I didn't get anything else in the series, I have to ask - are these
> controllers not compatible with eachother?
>

They are not, basically the difference is in the default width and height
for each controller. That's why the width and height fields are optional.

But other than the default resolution, yes the controllers are very much
the same.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  # Only required for SPI
>> +  dc-gpios:
>> +    description:
>> +      GPIO connected to the controller's D/C# (Data/Command) pin,
>> +      that is needed for 4-wire SPI to tell the controller if the
>> +      data sent is for a command register or the display data RAM
>> +    maxItems: 1
>> +
>> +  solomon,height:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Height in pixel of the screen driven by the controller.
>> +      The default value is controller-dependent.
>
> You probably know better than me, operating in drm stuff, but are there
> really no generic properties for the weidth/height of a display?
>

There are some common properties, such as the width-mm and height-mm for
the panel-common:

Documentation/devicetree/bindings/display/panel/panel-common.yaml

But those are to describe the physical area expressed in millimeters and
the Solomon drivers (the old ssd1307fb fbdev driver and the new ssd130x
DRM driver for backward compatibility with existing DTB) express the width
and height in pixels.

That's why are Solomon controller specific properties "solomon,width" and
"solomon,height".

[...]

>> +    then:
>> +      properties:
>> +        width:
>> +          default: 128
>> +        height:
>> +          default: 128
>
> Unless you did it like this for clarity, 2 of these have the same
> default width and 2 have the same default height. You could cut this
> down to a pair of if/then/else on that basis AFAICT.
> :wq
>

Yes, this was done like that for clarity. Because is easier for someone
reading the DT binding schema to reason about resolution (width,height)
for a given SSD132x controller, rather than following the if/else logic.

>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            ssd1327_i2c: oled@3c {
>
> This label is unused as far as I can tell. Ditto below.
>

Right, I'll drop those too.

> Cheers,
> Conor.
>
  

Rob Herring <robh@kernel.org> writes:

Hello Rob,

Thanks a lot for your feedback.

> On Mon, Oct 09, 2023 at 08:34:22PM +0200, Javier Martinez Canillas wrote:
>> Add a Device Tree binding schema for the OLED panels based on the Solomon
>> SSD132x family of controllers.
>
> Looks like the same binding as solomon,ssd1307fb.yaml. Why a different 
> binding? Why does that binding need a slew of custom properties and here 
> you don't?
>

It's a sub-set of it. Because only the minimum required properties are
defined. But also, is a clean slate schema because the old ssd1307fb fbdev
driver only supports the Solomon SSD130x family, so there is no need to
follow the existing solomon,ssd1307fb.yaml nor the need for backward compat.

[...]

>> +  solomon,height:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Height in pixel of the screen driven by the controller.
>> +      The default value is controller-dependent.
>> +
>> +  solomon,width:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Width in pixel of the screen driven by the controller.
>> +      The default value is controller-dependent.
>
> Don't define the same properties twice. Either share the binding or 
> split out the common properties into their own schema file.
>

Agreed. I'll do that in v2.

> Rob
>
  

Javier Martinez Canillas <javierm@redhat.com> writes:

> Rob Herring <robh@kernel.org> writes:
>
> Hello Rob,
>
> Thanks a lot for your feedback.
>
>> On Mon, Oct 09, 2023 at 08:34:22PM +0200, Javier Martinez Canillas wrote:
>>> Add a Device Tree binding schema for the OLED panels based on the Solomon
>>> SSD132x family of controllers.
>>
>> Looks like the same binding as solomon,ssd1307fb.yaml. Why a different 
>> binding? Why does that binding need a slew of custom properties and here 
>> you don't?
>>
>
> It's a sub-set of it. Because only the minimum required properties are
> defined. But also, is a clean slate schema because the old ssd1307fb fbdev
> driver only supports the Solomon SSD130x family, so there is no need to
> follow the existing solomon,ssd1307fb.yaml nor the need for backward compat.
>

I think this answer was a little sparse, let me elaborate a bit. The Solomon
display controllers are quite flexible, so that could be used with different
OLED panels.

And the ssd1307fb binding schema defines a set of properties that are almost
a 1:1 mapping from properties to the configuration registers. This makes the
driver to support most SSD130x controller + panel configurations but at the
expense of making the binding more complicated (a slew of custom propertie
as you pointed out).

Now, as mentioned this is support for greyscale Solomon controllers (the old
fbdev driver only supports monochrome controllers) so we don't care about DT
backward compatibility.

I decided for now to keep the binding at a minimum and be more opinionated in
the driver with having what I think are sane defaults for most of the config
registers.

If there is a need to expose any of this configuration as DT properties, then
we can later added it share some of the existing solomon,ssd1307fb.yaml custom
properties and move them to a common schema.

We can always change the driver in the future anyways, but we can't do the same
with the DT binding schema since that is considered an ABI.
  

On Wed, Oct 11, 2023 at 08:34:29AM +0200, Javier Martinez Canillas wrote:
> Conor Dooley <conor@kernel.org> writes:
> >> +  # Only required for SPI
> >> +  dc-gpios:
> >> +    description:
> >> +      GPIO connected to the controller's D/C# (Data/Command) pin,
> >> +      that is needed for 4-wire SPI to tell the controller if the
> >> +      data sent is for a command register or the display data RAM
> >> +    maxItems: 1
> >> +
> >> +  solomon,height:
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    description:
> >> +      Height in pixel of the screen driven by the controller.
> >> +      The default value is controller-dependent.
> >
> > You probably know better than me, operating in drm stuff, but are there
> > really no generic properties for the weidth/height of a display?
> >
> 
> There are some common properties, such as the width-mm and height-mm for
> the panel-common:
> 
> Documentation/devicetree/bindings/display/panel/panel-common.yaml
> 
> But those are to describe the physical area expressed in millimeters and
> the Solomon drivers (the old ssd1307fb fbdev driver and the new ssd130x
> DRM driver for backward compatibility with existing DTB) express the width
> and height in pixels.
> 
> That's why are Solomon controller specific properties "solomon,width" and
> "solomon,height".

Okay. Thanks for the explanation.