| Message ID | 20240725004734.644986-2-lanzano.alex@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add driver for Sharp Memory LCD | expand |
On 25/07/2024 02:47, Alex Lanzano wrote: > Add device tree bindings for the monochrome Sharp Memory LCD > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > --- > .../bindings/display/sharp,sharp-memory.yaml | 97 +++++++++++++++++++ > include/dt-bindings/display/sharp-memory.h | 9 ++ > 2 files changed, 106 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml > create mode 100644 include/dt-bindings/display/sharp-memory.h > > diff --git a/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml > new file mode 100644 > index 000000000000..a79edd97c857 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml Filename based on compatible, so e.g. sharp,ls010b7dh04.yaml. > @@ -0,0 +1,97 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/sharp,sharp-memory.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Sharp Memory LCD panels > + > +maintainers: > + - Alex Lanzano <lanzano.alex@gmail.com> > + > +description: > + This binding is for the Sharp Memory LCD monochrome displays. Do not say that binding is a binding... instead describe hardware. > + > +properties: > + compatible: > + enum: > + - sharp,ls010b7dh04 > + - sharp,ls011b7dh03 > + - sharp,ls012b7dd01 > + - sharp,ls013b7dh03 > + - sharp,ls013b7dh05 > + - sharp,ls018b7dh02 > + - sharp,ls027b7dh01 > + - sharp,ls027b7dh01a > + - sharp,ls032b7dd02 > + - sharp,ls044q7dh01 > + > + reg: > + maxItems: 1 > + > + spi-cs-high: true You can drop it. > + > + spi-max-frequency: > + maximum: 2000000 > + > + vcom-mode: Missing vendor prefix. > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + VCOM is a signal that prevents DC bias from being built up in > + the panel resulting in pixels being forever stuck in one state. > + vcom-mode can be set to one of three modes > + > + SHARP_MEMORY_SOFTWARE_VCOM - This method uses a kthread to periodically send a > + "maintain display" message to the display, toggling the vcom > + bit on and off with each message You described Linux, this is not suitable for bindings. > + > + SHARP_MEMORY_EXTERNAL_VCOM - This method relies on an external clock to generate > + the signal on the EXTCOMM pin > + > + SHARP_MEMORY_PWM_VCOM - This method uses a pwm device to generate the signal > + on the EXTCOMM pin I don't see why do you even need this property. Just choose the best option based on available connections/pins. > + > + enum: [ 0, 1, 2 ] Here 0/1/2 but above something entirely else. Just use strings. > + > + enable-gpios: > + maxItems: 1 > + description: Enables display Drop description and maxItems. :true is enough > + > + pwms: > + maxItems: 1 > + description: External VCOM signal > + > +required: > + - compatible > + - reg > + - spi-cs-high > + - vcom-mode > + allOf: and missing ref to spi peripheral props > +if: > + properties: > + vcom-mode: > + # SHARP_MEMORY_PWM_VCOM > + enum: [2] > +then: > + required: > + - pwms > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/display/sharp-memory.h> > + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; Mess indentation. Use 4 spaces for example indentation. > + > + display@0{ > + compatible = "sharp,ls013b7dh03"; > + reg = <0>; > + spi-cs-high; > + spi-max-frequency = <1000000>; > + vcom-mode = <SHARP_MEMORY_SOFTWARE_VCOM>; > + }; > + }; > +... > diff --git a/include/dt-bindings/display/sharp-memory.h b/include/dt-bindings/display/sharp-memory.h > new file mode 100644 > index 000000000000..dea14c7bd7ec > --- /dev/null > +++ b/include/dt-bindings/display/sharp-memory.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > +#ifndef _DT_BINDINGS_SHARP_MEMORY > +#define _DT_BINDINGS_SHARP_MEMORY > + > +#define SHARP_MEMORY_SOFTWARE_VCOM 0 > +#define SHARP_MEMORY_EXTERNAL_VCOM 1 > +#define SHARP_MEMORY_PWM_VCOM 2 Nope, drop the binding. Best regards, Krzysztof
On 25/07/2024 02:47, Alex Lanzano wrote: > Add device tree bindings for the monochrome Sharp Memory LCD > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > --- > .../bindings/display/sharp,sharp-memory.yaml | 97 +++++++++++++++++++ > include/dt-bindings/display/sharp-memory.h | 9 ++ > 2 files changed, 106 insertions(+) BTW, look at your original binding: https://lore.kernel.org/dri-devel/2a7c51b6e619c02ec175a5c219b0a0fd7a24499d.1701267411.git.mehdi.djait@bootlin.com/ Why did you decide to introduce mistakes? Eg. dropping all $refs? And finally look at the comment: https://lore.kernel.org/dri-devel/5dbdf7bd-cfa3-492b-a0a3-fdc323cf11f8@linaro.org/ You just ignored it completely.... Best regards, Krzysztof
Thank you for the review! I will address these comments in V2 On Thu, Jul 25, 2024 at 08:17:01AM GMT, Krzysztof Kozlowski wrote: > On 25/07/2024 02:47, Alex Lanzano wrote: > > Add device tree bindings for the monochrome Sharp Memory LCD > > > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > > --- > > .../bindings/display/sharp,sharp-memory.yaml | 97 +++++++++++++++++++ > > include/dt-bindings/display/sharp-memory.h | 9 ++ > > 2 files changed, 106 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml > > create mode 100644 include/dt-bindings/display/sharp-memory.h > > > > diff --git a/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml > > new file mode 100644 > > index 000000000000..a79edd97c857 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml > > Filename based on compatible, so e.g. sharp,ls010b7dh04.yaml. > > > @@ -0,0 +1,97 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/sharp,sharp-memory.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Sharp Memory LCD panels > > + > > +maintainers: > > + - Alex Lanzano <lanzano.alex@gmail.com> > > + > > +description: > > + This binding is for the Sharp Memory LCD monochrome displays. > > Do not say that binding is a binding... instead describe hardware. > > > + > > +properties: > > + compatible: > > + enum: > > + - sharp,ls010b7dh04 > > + - sharp,ls011b7dh03 > > + - sharp,ls012b7dd01 > > + - sharp,ls013b7dh03 > > + - sharp,ls013b7dh05 > > + - sharp,ls018b7dh02 > > + - sharp,ls027b7dh01 > > + - sharp,ls027b7dh01a > > + - sharp,ls032b7dd02 > > + - sharp,ls044q7dh01 > > + > > + reg: > > + maxItems: 1 > > + > > + spi-cs-high: true > > You can drop it. > > > + > > + spi-max-frequency: > > + maximum: 2000000 > > + > > + vcom-mode: > > Missing vendor prefix. > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: | > > + VCOM is a signal that prevents DC bias from being built up in > > + the panel resulting in pixels being forever stuck in one state. > > + vcom-mode can be set to one of three modes > > + > > + SHARP_MEMORY_SOFTWARE_VCOM - This method uses a kthread to periodically send a > > + "maintain display" message to the display, toggling the vcom > > + bit on and off with each message > > You described Linux, this is not suitable for bindings. > > > + > > + SHARP_MEMORY_EXTERNAL_VCOM - This method relies on an external clock to generate > > + the signal on the EXTCOMM pin > > + > > + SHARP_MEMORY_PWM_VCOM - This method uses a pwm device to generate the signal > > + on the EXTCOMM pin > > I don't see why do you even need this property. Just choose the best > option based on available connections/pins. > I wanted to cover most of the hardware configurations I've seen with these displays. This property simplifies the driver implementation and allows the user to explicitly state how the VCOM signal will be generated on their platform. > > + > > + enum: [ 0, 1, 2 ] > > Here 0/1/2 but above something entirely else. Just use strings. > > > + > > + enable-gpios: > > + maxItems: 1 > > + description: Enables display > > Drop description and maxItems. :true is enough > > > + > > + pwms: > > + maxItems: 1 > > + description: External VCOM signal > > + > > +required: > > + - compatible > > + - reg > > + - spi-cs-high > > + - vcom-mode > > + > > allOf: > > and missing ref to spi peripheral props > > > +if: > > + properties: > > + vcom-mode: > > + # SHARP_MEMORY_PWM_VCOM > > + enum: [2] > > +then: > > + required: > > + - pwms > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/display/sharp-memory.h> > > + > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > Mess indentation. > > Use 4 spaces for example indentation. > > > + > > + display@0{ > > + compatible = "sharp,ls013b7dh03"; > > + reg = <0>; > > + spi-cs-high; > > + spi-max-frequency = <1000000>; > > + vcom-mode = <SHARP_MEMORY_SOFTWARE_VCOM>; > > + }; > > + }; > > +... > > diff --git a/include/dt-bindings/display/sharp-memory.h b/include/dt-bindings/display/sharp-memory.h > > new file mode 100644 > > index 000000000000..dea14c7bd7ec > > --- /dev/null > > +++ b/include/dt-bindings/display/sharp-memory.h > > @@ -0,0 +1,9 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > > +#ifndef _DT_BINDINGS_SHARP_MEMORY > > +#define _DT_BINDINGS_SHARP_MEMORY > > + > > +#define SHARP_MEMORY_SOFTWARE_VCOM 0 > > +#define SHARP_MEMORY_EXTERNAL_VCOM 1 > > +#define SHARP_MEMORY_PWM_VCOM 2 > > Nope, drop the binding. > > > Best regards, > Krzysztof > Best regards, Alex
On Thu, Jul 25, 2024 at 08:18:44AM GMT, Krzysztof Kozlowski wrote: > On 25/07/2024 02:47, Alex Lanzano wrote: > > Add device tree bindings for the monochrome Sharp Memory LCD > > > > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> > > --- > > .../bindings/display/sharp,sharp-memory.yaml | 97 +++++++++++++++++++ > > include/dt-bindings/display/sharp-memory.h | 9 ++ > > 2 files changed, 106 insertions(+) > > BTW, look at your original binding: > https://lore.kernel.org/dri-devel/2a7c51b6e619c02ec175a5c219b0a0fd7a24499d.1701267411.git.mehdi.djait@bootlin.com/ > > Why did you decide to introduce mistakes? Eg. dropping all $refs? > > And finally look at the comment: > https://lore.kernel.org/dri-devel/5dbdf7bd-cfa3-492b-a0a3-fdc323cf11f8@linaro.org/ > You just ignored it completely.... > > Best regards, > Krzysztof > Will fix in V2. Best regards, Alex
On Thu, Jul 25, 2024 at 08:17:01AM GMT, Krzysztof Kozlowski wrote: > On 25/07/2024 02:47, Alex Lanzano wrote: > > + > > +properties: > > + compatible: > > + enum: > > + - sharp,ls010b7dh04 > > + - sharp,ls011b7dh03 > > + - sharp,ls012b7dd01 > > + - sharp,ls013b7dh03 > > + - sharp,ls013b7dh05 > > + - sharp,ls018b7dh02 > > + - sharp,ls027b7dh01 > > + - sharp,ls027b7dh01a > > + - sharp,ls032b7dd02 > > + - sharp,ls044q7dh01 > > + > > + reg: > > + maxItems: 1 > > + > > + spi-cs-high: true > > You can drop it. > This is a required property in order for the display to function correctly. But I have no issues removing it if there's a better place to document it. Best regards, Alex
On 27/07/2024 18:30, Alex Lanzano wrote: > On Thu, Jul 25, 2024 at 08:17:01AM GMT, Krzysztof Kozlowski wrote: >> On 25/07/2024 02:47, Alex Lanzano wrote: >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - sharp,ls010b7dh04 >>> + - sharp,ls011b7dh03 >>> + - sharp,ls012b7dd01 >>> + - sharp,ls013b7dh03 >>> + - sharp,ls013b7dh05 >>> + - sharp,ls018b7dh02 >>> + - sharp,ls027b7dh01 >>> + - sharp,ls027b7dh01a >>> + - sharp,ls032b7dd02 >>> + - sharp,ls044q7dh01 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + spi-cs-high: true >> >> You can drop it. >> > > This is a required property in order for the display to function correctly. > But I have no issues removing it if there's a better place to document it. The sharp LCD device or the board (e.g. via some inversion)? Best regards, Krzysztof
On Sun, Jul 28, 2024 at 10:40:23AM GMT, Krzysztof Kozlowski wrote: > On 27/07/2024 18:30, Alex Lanzano wrote: > > On Thu, Jul 25, 2024 at 08:17:01AM GMT, Krzysztof Kozlowski wrote: > >> On 25/07/2024 02:47, Alex Lanzano wrote: > >>> + > >>> +properties: > >>> + compatible: > >>> + enum: > >>> + - sharp,ls010b7dh04 > >>> + - sharp,ls011b7dh03 > >>> + - sharp,ls012b7dd01 > >>> + - sharp,ls013b7dh03 > >>> + - sharp,ls013b7dh05 > >>> + - sharp,ls018b7dh02 > >>> + - sharp,ls027b7dh01 > >>> + - sharp,ls027b7dh01a > >>> + - sharp,ls032b7dd02 > >>> + - sharp,ls044q7dh01 > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + spi-cs-high: true > >> > >> You can drop it. > >> > > > > This is a required property in order for the display to function correctly. > > But I have no issues removing it if there's a better place to document it. > > The sharp LCD device or the board (e.g. via some inversion)? > The sharp LCD device itself. https://cdn.sparkfun.com/assets/d/e/8/9/7/LS013B7DH03_datasheet.pdf Page 16 of the PDF below shows the timing chart for it if interested
On 28/07/2024 14:56, Alex Lanzano wrote: >>>>> + >>>>> + spi-cs-high: true >>>> >>>> You can drop it. >>>> >>> >>> This is a required property in order for the display to function correctly. >>> But I have no issues removing it if there's a better place to document it. >> >> The sharp LCD device or the board (e.g. via some inversion)? >> > > The sharp LCD device itself. > > https://cdn.sparkfun.com/assets/d/e/8/9/7/LS013B7DH03_datasheet.pdf > Page 16 of the PDF below shows the timing chart for it if interested OK, so you enforce chip select as always high and then someone comes with a board which has a inversion and expect the SoC to drive it low, as usually? In such system this would not work, would it? IOW, I think this is the first case where you require specific CS high and this should make you think why... Best regards, Krzysztof
On Sun, Jul 28, 2024 at 04:43:42PM GMT, Krzysztof Kozlowski wrote: > On 28/07/2024 14:56, Alex Lanzano wrote: > >>>>> + > >>>>> + spi-cs-high: true > >>>> > >>>> You can drop it. > >>>> > >>> > >>> This is a required property in order for the display to function correctly. > >>> But I have no issues removing it if there's a better place to document it. > >> > >> The sharp LCD device or the board (e.g. via some inversion)? > >> > > > > The sharp LCD device itself. > > > > https://cdn.sparkfun.com/assets/d/e/8/9/7/LS013B7DH03_datasheet.pdf > > Page 16 of the PDF below shows the timing chart for it if interested > > OK, so you enforce chip select as always high and then someone comes > with a board which has a inversion and expect the SoC to drive it low, > as usually? In such system this would not work, would it? > > IOW, I think this is the first case where you require specific CS high > and this should make you think why... Ahh I see what you mean! I'll remove it. Best regard, Alex
diff --git a/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml new file mode 100644 index 000000000000..a79edd97c857 --- /dev/null +++ b/Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml @@ -0,0 +1,97 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/sharp,sharp-memory.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Sharp Memory LCD panels + +maintainers: + - Alex Lanzano <lanzano.alex@gmail.com> + +description: + This binding is for the Sharp Memory LCD monochrome displays. + +properties: + compatible: + enum: + - sharp,ls010b7dh04 + - sharp,ls011b7dh03 + - sharp,ls012b7dd01 + - sharp,ls013b7dh03 + - sharp,ls013b7dh05 + - sharp,ls018b7dh02 + - sharp,ls027b7dh01 + - sharp,ls027b7dh01a + - sharp,ls032b7dd02 + - sharp,ls044q7dh01 + + reg: + maxItems: 1 + + spi-cs-high: true + + spi-max-frequency: + maximum: 2000000 + + vcom-mode: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + VCOM is a signal that prevents DC bias from being built up in + the panel resulting in pixels being forever stuck in one state. + vcom-mode can be set to one of three modes + + SHARP_MEMORY_SOFTWARE_VCOM - This method uses a kthread to periodically send a + "maintain display" message to the display, toggling the vcom + bit on and off with each message + + SHARP_MEMORY_EXTERNAL_VCOM - This method relies on an external clock to generate + the signal on the EXTCOMM pin + + SHARP_MEMORY_PWM_VCOM - This method uses a pwm device to generate the signal + on the EXTCOMM pin + + enum: [ 0, 1, 2 ] + + enable-gpios: + maxItems: 1 + description: Enables display + + pwms: + maxItems: 1 + description: External VCOM signal + +required: + - compatible + - reg + - spi-cs-high + - vcom-mode + +if: + properties: + vcom-mode: + # SHARP_MEMORY_PWM_VCOM + enum: [2] +then: + required: + - pwms + +additionalProperties: false + +examples: + - | + #include <dt-bindings/display/sharp-memory.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + + display@0{ + compatible = "sharp,ls013b7dh03"; + reg = <0>; + spi-cs-high; + spi-max-frequency = <1000000>; + vcom-mode = <SHARP_MEMORY_SOFTWARE_VCOM>; + }; + }; +... diff --git a/include/dt-bindings/display/sharp-memory.h b/include/dt-bindings/display/sharp-memory.h new file mode 100644 index 000000000000..dea14c7bd7ec --- /dev/null +++ b/include/dt-bindings/display/sharp-memory.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ +#ifndef _DT_BINDINGS_SHARP_MEMORY +#define _DT_BINDINGS_SHARP_MEMORY + +#define SHARP_MEMORY_SOFTWARE_VCOM 0 +#define SHARP_MEMORY_EXTERNAL_VCOM 1 +#define SHARP_MEMORY_PWM_VCOM 2 + +#endif
Add device tree bindings for the monochrome Sharp Memory LCD Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com> --- .../bindings/display/sharp,sharp-memory.yaml | 97 +++++++++++++++++++ include/dt-bindings/display/sharp-memory.h | 9 ++ 2 files changed, 106 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/sharp,sharp-memory.yaml create mode 100644 include/dt-bindings/display/sharp-memory.h