[1/2] dt-bindings: display: Add SSD133x OLED controllers

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

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

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

On Sun, Dec 17, 2023 at 11:07:03AM +0100, Javier Martinez Canillas wrote:
> Add a Device Tree binding schema for the OLED panels based on the
> Solomon SSD133x family of controllers.
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>  .../bindings/display/solomon,ssd133x.yaml     | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/solomon,ssd133x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/solomon,ssd133x.yaml b/Documentation/devicetree/bindings/display/solomon,ssd133x.yaml
> new file mode 100644
> index 000000000000..eee8d8c9ca35
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/solomon,ssd133x.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/solomon,ssd133x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Solomon SSD133x OLED Display Controllers
> +
> +maintainers:
> +  - Javier Martinez Canillas <javierm@redhat.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - solomon,ssd1331
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - $ref: solomon,ssd-common.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: solomon,ssd1331
> +    then:
> +      properties:
> +        width:
> +          default: 96
> +        height:
> +          default: 64

Do you envisage a rake of devices that are going to end up in this
binding? Otherwise, why not unconditionally set the constraints?

Cheers,
Conor.
  

Conor Dooley <conor@kernel.org> writes:

Hello Connor,

> On Sun, Dec 17, 2023 at 11:07:03AM +0100, Javier Martinez Canillas wrote:

[...]

>> +properties:
>> +  compatible:
>> +    enum:
>> +      - solomon,ssd1331
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +allOf:
>> +  - $ref: solomon,ssd-common.yaml#
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: solomon,ssd1331
>> +    then:
>> +      properties:
>> +        width:
>> +          default: 96
>> +        height:
>> +          default: 64
>
> Do you envisage a rake of devices that are going to end up in this
> binding? Otherwise, why not unconditionally set the constraints?
>

Because these are only for the default width and height, there can be
panels using the same controller but that have a different resolution.

For example, there are panels using the SSD1306 controller that have
128x32 [0], 64x32 [1] or 128x64 [2] resolutions.

But answering your question, yes I think that more devices for this
SSD133x family are going to be added later. Looking at [3], there is
at least SSD1333 that has a different default resolutions (176x176).

I think that even the SSD135x family could be supported by the same
modsetting pipeline, but I need to get one to figure it out.

[0]: https://es.aliexpress.com/item/1005003648174074.html
[1]: https://www.buydisplay.com/white-0-49-inch-oled-display-64x32-iic-i2c-ssd1306-connector-fpc
[2]: https://es.aliexpress.com/item/1005001582340858.html?gatewayAdapt=glo2esp
[3]: https://www.solomon-systech.com/product-search/?technology=oled-display

> Cheers,
> Conor.
  

On Sun, Dec 17, 2023 at 10:33:24PM +0100, Javier Martinez Canillas wrote:
> Conor Dooley <conor@kernel.org> writes:
> 
> Hello Connor,
> 
> > On Sun, Dec 17, 2023 at 11:07:03AM +0100, Javier Martinez Canillas wrote:
> 
> [...]
> 
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - solomon,ssd1331
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +
> >> +allOf:
> >> +  - $ref: solomon,ssd-common.yaml#
> >> +
> >> +  - if:
> >> +      properties:
> >> +        compatible:
> >> +          contains:
> >> +            const: solomon,ssd1331
> >> +    then:
> >> +      properties:
> >> +        width:
> >> +          default: 96
> >> +        height:
> >> +          default: 64
> >
> > Do you envisage a rake of devices that are going to end up in this
> > binding? Otherwise, why not unconditionally set the constraints?
> >
> 
> Because these are only for the default width and height, there can be
> panels using the same controller but that have a different resolution.
> 
> For example, there are panels using the SSD1306 controller that have
> 128x32 [0], 64x32 [1] or 128x64 [2] resolutions.

This, as you know, does not matter here.

> But answering your question, yes I think that more devices for this
> SSD133x family are going to be added later. Looking at [3], there is
> at least SSD1333 that has a different default resolutions (176x176).

That's fair enough though. I'd probably err on the side of introducing
this complexity when the other users actually show up though.

> 
> I think that even the SSD135x family could be supported by the same
> modsetting pipeline, but I need to get one to figure it out.
> 
> [0]: https://es.aliexpress.com/item/1005003648174074.html
> [1]: https://www.buydisplay.com/white-0-49-inch-oled-display-64x32-iic-i2c-ssd1306-connector-fpc
> [2]: https://es.aliexpress.com/item/1005001582340858.html?gatewayAdapt=glo2esp
> [3]: https://www.solomon-systech.com/product-search/?technology=oled-display
> 
> > Cheers,
> > Conor.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
  

Conor Dooley <conor@kernel.org> writes:

Hello Conor,

> On Sun, Dec 17, 2023 at 10:33:24PM +0100, Javier Martinez Canillas wrote:

[...]

>> >> +    then:
>> >> +      properties:
>> >> +        width:
>> >> +          default: 96
>> >> +        height:
>> >> +          default: 64
>> >
>> > Do you envisage a rake of devices that are going to end up in this
>> > binding? Otherwise, why not unconditionally set the constraints?
>> >
>> 
>> Because these are only for the default width and height, there can be
>> panels using the same controller but that have a different resolution.
>> 
>> For example, there are panels using the SSD1306 controller that have
>> 128x32 [0], 64x32 [1] or 128x64 [2] resolutions.
>
> This, as you know, does not matter here.
>

Are you sure? What I tried to say is that the SSD133x are just OLED
controllers and manufacturers use those chips to attach a panel that
has a certain resolution.

While it makes sense to use all the supported width and height, some
manufacturers chose to have a smaller panel instead (I used SSD1306
as an example because I knew about these but the same might be true
for let's say SSD1331).

Or saying another way, are you sure that every manufacturer selling
RGB OLED panels using the SSD1331 chip will use the default resolution
and users won't have to set a custom width and height ?

I have already chosen to make the DT binding as simple as possible to
prevent what happened with the SSD1306 "solomon,page-offset" property
that has a broken default [0] but I think that not allowing to set the
resolution is already too restrictive and would make it unusable for
any panel that doesn't have the default width and height.

[0]: https://lists.freedesktop.org/archives/dri-devel/2023-November/431150.html

>> But answering your question, yes I think that more devices for this
>> SSD133x family are going to be added later. Looking at [3], there is
>> at least SSD1333 that has a different default resolutions (176x176).
>
> That's fair enough though. I'd probably err on the side of introducing
> this complexity when the other users actually show up though.
>

Agree and the reason why I did not include entries for the SSD1332 and
SSD1333. I was planning to add those only if there were users since it
seems that the SSD1331 is the most popular controller from this family.

But as explained, even for the SSD1331 it may be needed to set a width
and height that is different than the default of this panel controller.
  

On Sun, Dec 17, 2023 at 11:00:07PM +0100, Javier Martinez Canillas wrote:
> Conor Dooley <conor@kernel.org> writes:
> 
> Hello Conor,
> 
> > On Sun, Dec 17, 2023 at 10:33:24PM +0100, Javier Martinez Canillas wrote:
> 
> [...]
> 
> >> >> +    then:
> >> >> +      properties:
> >> >> +        width:
> >> >> +          default: 96
> >> >> +        height:
> >> >> +          default: 64
> >> >
> >> > Do you envisage a rake of devices that are going to end up in this
> >> > binding? Otherwise, why not unconditionally set the constraints?
> >> >
> >> 
> >> Because these are only for the default width and height, there can be
> >> panels using the same controller but that have a different resolution.
> >> 
> >> For example, there are panels using the SSD1306 controller that have
> >> 128x32 [0], 64x32 [1] or 128x64 [2] resolutions.
> >
> > This, as you know, does not matter here.
> >
> 
> Are you sure? What I tried to say is that the SSD133x are just OLED
> controllers and manufacturers use those chips to attach a panel that
> has a certain resolution.
> 
> While it makes sense to use all the supported width and height, some
> manufacturers chose to have a smaller panel instead (I used SSD1306
> as an example because I knew about these but the same might be true
> for let's say SSD1331).
> 
> Or saying another way, are you sure that every manufacturer selling
> RGB OLED panels using the SSD1331 chip will use the default resolution
> and users won't have to set a custom width and height ?

That's not at all what I was saying. I just meant unconditionally set
the constraints on the property (in this case the default) since you
only have one compatible. Not unconditionally set the height and width.

Apologies if if that was unclear.

Thanks,
Conor.

> I have already chosen to make the DT binding as simple as possible to
> prevent what happened with the SSD1306 "solomon,page-offset" property
> that has a broken default [0] but I think that not allowing to set the
> resolution is already too restrictive and would make it unusable for
> any panel that doesn't have the default width and height.
> 
> [0]: https://lists.freedesktop.org/archives/dri-devel/2023-November/431150.html
> 
> >> But answering your question, yes I think that more devices for this
> >> SSD133x family are going to be added later. Looking at [3], there is
> >> at least SSD1333 that has a different default resolutions (176x176).
> >
> > That's fair enough though. I'd probably err on the side of introducing
> > this complexity when the other users actually show up though.
> >
> 
> Agree and the reason why I did not include entries for the SSD1332 and
> SSD1333. I was planning to add those only if there were users since it
> seems that the SSD1331 is the most popular controller from this family.
> 
> But as explained, even for the SSD1331 it may be needed to set a width
> and height that is different than the default of this panel controller.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>
  

Conor Dooley <conor@kernel.org> writes:

Hello Connor,

> On Sun, Dec 17, 2023 at 11:00:07PM +0100, Javier Martinez Canillas wrote:
>> Conor Dooley <conor@kernel.org> writes:
>> 
>> Hello Conor,
>> 
>> > On Sun, Dec 17, 2023 at 10:33:24PM +0100, Javier Martinez Canillas wrote:
>> 
>> [...]
>> 
>> >> >> +    then:
>> >> >> +      properties:
>> >> >> +        width:
>> >> >> +          default: 96
>> >> >> +        height:
>> >> >> +          default: 64
>> >> >
>> >> > Do you envisage a rake of devices that are going to end up in this
>> >> > binding? Otherwise, why not unconditionally set the constraints?
>> >> >
>> >> 
>> >> Because these are only for the default width and height, there can be
>> >> panels using the same controller but that have a different resolution.
>> >> 
>> >> For example, there are panels using the SSD1306 controller that have
>> >> 128x32 [0], 64x32 [1] or 128x64 [2] resolutions.
>> >
>> > This, as you know, does not matter here.
>> >
>> 
>> Are you sure? What I tried to say is that the SSD133x are just OLED
>> controllers and manufacturers use those chips to attach a panel that
>> has a certain resolution.
>> 
>> While it makes sense to use all the supported width and height, some
>> manufacturers chose to have a smaller panel instead (I used SSD1306
>> as an example because I knew about these but the same might be true
>> for let's say SSD1331).
>> 
>> Or saying another way, are you sure that every manufacturer selling
>> RGB OLED panels using the SSD1331 chip will use the default resolution
>> and users won't have to set a custom width and height ?
>
> That's not at all what I was saying. I just meant unconditionally set
> the constraints on the property (in this case the default) since you
> only have one compatible. Not unconditionally set the height and width.
>
> Apologies if if that was unclear.
>

Oh, I see that you meant now. Sorry for my confusion!

And yes, I agree with you that doesn't make sense to make it conditional
if there's only a single compatible. I'll drop that if condition on v2.

Thanks a lot for your feedback.

> Thanks,
> Conor.
>
  

On 17/12/2023 11:07, Javier Martinez Canillas wrote:
>> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: solomon,ssd1331
> +    then:
> +      properties:
> +        width:
> +          default: 96
> +        height:
> +          default: 64
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +

Use 4 spaces for example indentation.

Best regards,
Krzysztof
  

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

Hello Krzysztof,

> On 17/12/2023 11:07, Javier Martinez Canillas wrote:
>>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: solomon,ssd1331
>> +    then:
>> +      properties:
>> +        width:
>> +          default: 96
>> +        height:
>> +          default: 64
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>
> Use 4 spaces for example indentation.
>

Sure, I'll change it in v2.

> Best regards,
> Krzysztof
>