| Message ID | 20231009183522.543918-9-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/solomon: Add support for the SSD132x controller family | expand |
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.
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. + +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 + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ssd1327_i2c: oled@3c { + 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>; + }; + };
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