| Message ID | 20250402-st7571-v1-1-351d6b9eeb4a@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Sitronix ST7571 LCD controller | expand |
On Wed, Apr 02, 2025 at 08:12:10AM +0200, Marcus Folkesson wrote: > Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. > The controller has a SPI, I2C and 8bit parallel interface, this is for > the I2C interface only. > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> > --- > .../bindings/display/sitronix,st7571-i2c.yaml | 71 ++++++++++++++++++++++ > 1 file changed, 71 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml b/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..6e5e0994a98db646a37bb17c4289332546c9266e > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml Drop i2c > @@ -0,0 +1,71 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/sitronix,st7571-i2c.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Sitronix ST7571 Display Panels > + > +maintainers: > + - Marcus Folkesson <marcus.folkesson@gmail.com> > + > +properties: > + compatible: > + const: sitronix,st7571-i2c Drop i2c > + > + reg: > + description: I2C address of the LCD controller Drop description > + maxItems: 1 > + > + sitronix,panel-width-mm: > + description: physical panel width [mm] > + > + sitronix,panel-height-mm: > + description: physical panel height [mm] No, use standard properties. > + > + sitronix,panel-nlines: > + description: Number of lines in the panel > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 1 > + maximum: 128 > + default: 128 Ditto > + > + sitronix,panel-start-line: > + description: Start line of the panel > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 127 > + default: 0 Ditto > + > + reset-gpios: > + description: GPIO connected to the RST_N signal. > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - reset-gpios Keep same order as in properties. > + - sitronix,panel-width-mm > + - sitronix,panel-height-mm > + Missing ref to panels schema. > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + # Drop > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + display@3f { Look how this is called in other bindings... The binding and example are not following existing code. Why? Why doing something entirely different? Best regards, Krzysztof
On Wed, Apr 02, 2025 at 08:12:10AM +0200, Marcus Folkesson wrote: > + display@3f { > + compatible = "sitronix,st7571-i2c"; > + reg = <0x3f>; > + > + reset-gpios = <&gpio0 3 GPIO_ACTIVE_HIGH>; so RST or RST_N? Best regards, Krzysztof
On 02/04/2025 08:12, Marcus Folkesson wrote: > Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. > The controller has a SPI, I2C and 8bit parallel interface, this is for > the I2C interface only. > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> I spotted one more thing: Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. For bindings, the preferred subjects are explained here: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters (missing display parts) A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 Best regards, Krzysztof
Thank you Krzysztof, I will fix the issues you pointed out, just a few comments below. On Wed, Apr 02, 2025 at 10:27:53AM +0200, Krzysztof Kozlowski wrote: > On Wed, Apr 02, 2025 at 08:12:10AM +0200, Marcus Folkesson wrote: > > Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. > > The controller has a SPI, I2C and 8bit parallel interface, this is for > > the I2C interface only. > > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> > > --- > > .../bindings/display/sitronix,st7571-i2c.yaml | 71 ++++++++++++++++++++++ > > 1 file changed, 71 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml b/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml > > new file mode 100644 > > index 0000000000000000000000000000000000000000..6e5e0994a98db646a37bb17c4289332546c9266e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml > [...] > > > + maxItems: 1 > > + > > + sitronix,panel-width-mm: > > + description: physical panel width [mm] > > + > > + sitronix,panel-height-mm: > > + description: physical panel height [mm] > > No, use standard properties. I will use width-mm and height-mm from panels.yaml from panel-common.yaml instead > > > + > > + sitronix,panel-nlines: > > + description: Number of lines in the panel > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 1 > > + maximum: 128 > > + default: 128 > > Ditto I will use vactive in panel-timing instead. Do I need to specify those properties or should I just list them as required? Some bindings set e.g. reg: true reset-gpios: true and others just list them as required. > > > + > > + sitronix,panel-start-line: > > + description: Start line of the panel > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + minimum: 0 > > + maximum: 127 > > + default: 0 > > Ditto I will use vfront-porch in panel-timing instead. [...] > > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + display@3f { > > Look how this is called in other bindings... The binding and example are > not following existing code. Why? Why doing something entirely > different? Sorry, I'm not sure what you mean here. > > Best regards, > Krzysztof Best regards, Marcus Folkesson
On 03/04/2025 12:31, Marcus Folkesson wrote: >>> + i2c { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + display@3f { >> >> Look how this is called in other bindings... The binding and example are >> not following existing code. Why? Why doing something entirely >> different? > > Sorry, I'm not sure what you mean here. You added code entirely different than existing code. Why doing something entirely different? Open any other panel and look how it is called. Best regards, Krzysztof
On Thu, Apr 03, 2025 at 04:28:22PM +0200, Krzysztof Kozlowski wrote: > On 03/04/2025 12:31, Marcus Folkesson wrote: > >>> + i2c { > >>> + #address-cells = <1>; > >>> + #size-cells = <0>; > >>> + > >>> + display@3f { > >> > >> Look how this is called in other bindings... The binding and example are > >> not following existing code. Why? Why doing something entirely > >> different? > > > > Sorry, I'm not sure what you mean here. > You added code entirely different than existing code. Why doing > something entirely different? Open any other panel and look how it is > called. This is still unclear to me. I assume you are referring to the display@3f? I can see many other panels use display@<address>, e.g. elgin,jg10309-01.yaml and sitronix,st7735r.yaml Those are using address 0, but that is because they are SPI devices, this is a I2C device and address 0 is not valid. There are plenty of examples of I2C devices using the real addresses in the node name. Or do you want me to call it panel@3f ? I can go with that if it is preferred. > > Best regards, > Krzysztof Best regards, Marcus Folkesson
diff --git a/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml b/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml new file mode 100644 index 0000000000000000000000000000000000000000..6e5e0994a98db646a37bb17c4289332546c9266e --- /dev/null +++ b/Documentation/devicetree/bindings/display/sitronix,st7571-i2c.yaml @@ -0,0 +1,71 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/sitronix,st7571-i2c.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sitronix ST7571 Display Panels + +maintainers: + - Marcus Folkesson <marcus.folkesson@gmail.com> + +properties: + compatible: + const: sitronix,st7571-i2c + + reg: + description: I2C address of the LCD controller + maxItems: 1 + + sitronix,panel-width-mm: + description: physical panel width [mm] + + sitronix,panel-height-mm: + description: physical panel height [mm] + + sitronix,panel-nlines: + description: Number of lines in the panel + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 1 + maximum: 128 + default: 128 + + sitronix,panel-start-line: + description: Start line of the panel + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 127 + default: 0 + + reset-gpios: + description: GPIO connected to the RST_N signal. + maxItems: 1 + +required: + - compatible + - reg + - reset-gpios + - sitronix,panel-width-mm + - sitronix,panel-height-mm + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + # + i2c { + #address-cells = <1>; + #size-cells = <0>; + + display@3f { + compatible = "sitronix,st7571-i2c"; + reg = <0x3f>; + + reset-gpios = <&gpio0 3 GPIO_ACTIVE_HIGH>; + sitronix,panel-width-mm = <37>; + sitronix,panel-height-mm = <27>; + sitronix,panel-nlines = <96>; + sitronix,panel-start-line = <0>; + }; + };
Sitronix ST7571 is a 4bit gray scale dot matrix LCD controller. The controller has a SPI, I2C and 8bit parallel interface, this is for the I2C interface only. Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> --- .../bindings/display/sitronix,st7571-i2c.yaml | 71 ++++++++++++++++++++++ 1 file changed, 71 insertions(+)