[v2,1/2] dt-bindings: display: Add Sharp Memory LCD bindings

Add device tree bindings for the monochrome Sharp Memory LCD

Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
---
 .../bindings/display/sharp,ls010b7dh04.yaml   | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml

On 26/07/2024 21:44, Alex Lanzano wrote:
> Add device tree bindings for the monochrome Sharp Memory LCD
> 
> Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
> Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>

The order of tags is incorrect. Who developed it first? Please read at
Submitting patches - it explained this case quite precisely.

> ---
>  .../bindings/display/sharp,ls010b7dh04.yaml   | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> new file mode 100644
> index 000000000000..79bde7bf0d7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/sharp,ls010b7dh04.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sharp Memory LCD panels
> +
> +maintainers:
> +  - Alex Lanzano <lanzano.alex@gmail.com>
> +
> +description:
> +  Sharp Memory LCDs are a series of monochrome displays that operate over
> +  a SPI bus when the chip select is high. The displays require a signal (VCOM)
> +  to be generated to prevent DC bias build up resulting in pixels being
> +  unable to change. Three modes can be used to provide the VCOM signal
> +  ("software", "external", "pwm").
> +
> +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

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

> +
> +  spi-max-frequency:
> +    maximum: 2000000
> +
> +  sharp,vcom-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: |
> +      software - This mode relies on a software operation to send a
> +      "maintain display" message to the display, toggling the vcom
> +      bit on and off with each message
> +
> +      external - This mode relies on an external clock to generate
> +      the signal on the EXTCOMM pin

External clock? Then you might be missing clocks property.
> +
> +      pwm - This mode relies on a pwm device to generate the signal
> +      on the EXTCOMM pin

That's an enum. Otherwise why "pony" would be a correct vcom-mode?

> +
> +  enable-gpios: true
> +
> +  pwms:
> +    maxItems: 1
> +    description: External VCOM signal
> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-cs-high
> +  - sharp,vcom-mode
> +
> +allOf:
> +  - $ref: panel/panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  - if:
> +      properties:
> +        sharp,vcom-mode:
> +          const: pwm
> +    then:
> +      required:
> +        - pwms
> +
> +additionalProperties: false

Instead:
unevaluatedProperties: false

> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        display@0{

Missing space (see DTS coding style or any DTS)

> +            compatible = "sharp,ls013b7dh03";
> +            reg = <0>;
> +            spi-cs-high;
> +            spi-max-frequency = <1000000>;
> +            sharp,vcom-mode = "software";
> +        };
> +    };
> +...

Best regards,
Krzysztof

On Sat, Jul 27, 2024 at 11:06:05AM GMT, Krzysztof Kozlowski wrote:
> On 26/07/2024 21:44, Alex Lanzano wrote:
> > Add device tree bindings for the monochrome Sharp Memory LCD
> > 
> > Signed-off-by: Alex Lanzano <lanzano.alex@gmail.com>
> > Co-developed-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> 
> The order of tags is incorrect. Who developed it first? Please read at
> Submitting patches - it explained this case quite precisely.
> 

Will fix!

> > ---
> >  .../bindings/display/sharp,ls010b7dh04.yaml   | 94 +++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> > new file mode 100644
> > index 000000000000..79bde7bf0d7d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/sharp,ls010b7dh04.yaml
> > @@ -0,0 +1,94 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/sharp,ls010b7dh04.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sharp Memory LCD panels
> > +
> > +maintainers:
> > +  - Alex Lanzano <lanzano.alex@gmail.com>
> > +
> > +description:
> > +  Sharp Memory LCDs are a series of monochrome displays that operate over
> > +  a SPI bus when the chip select is high. The displays require a signal (VCOM)
> > +  to be generated to prevent DC bias build up resulting in pixels being
> > +  unable to change. Three modes can be used to provide the VCOM signal
> > +  ("software", "external", "pwm").
> > +
> > +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
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
> 
> Thank you.
> </form letter>
> 

My apologies! I must've missed the comment on this line. I replied to this
on the v1 thread

> > +
> > +  spi-max-frequency:
> > +    maximum: 2000000
> > +
> > +  sharp,vcom-mode:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    description: |
> > +      software - This mode relies on a software operation to send a
> > +      "maintain display" message to the display, toggling the vcom
> > +      bit on and off with each message
> > +
> > +      external - This mode relies on an external clock to generate
> > +      the signal on the EXTCOMM pin
> 
> External clock? Then you might be missing clocks property.

This is to handle the case where a clock IC or other signal generator IC is provided
on the display module itself. So I don't believe the clocks property would be 
needed.

> > +
> > +      pwm - This mode relies on a pwm device to generate the signal
> > +      on the EXTCOMM pin
> 
> That's an enum. Otherwise why "pony" would be a correct vcom-mode?
> 

Will fix!

> > +
> > +  enable-gpios: true
> > +
> > +  pwms:
> > +    maxItems: 1
> > +    description: External VCOM signal
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - spi-cs-high
> > +  - sharp,vcom-mode
> > +
> > +allOf:
> > +  - $ref: panel/panel-common.yaml#
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +  - if:
> > +      properties:
> > +        sharp,vcom-mode:
> > +          const: pwm
> > +    then:
> > +      required:
> > +        - pwms
> > +
> > +additionalProperties: false
> 
> Instead:
> unevaluatedProperties: false
> 

Will fix!

> > +
> > +examples:
> > +  - |
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        display@0{
> 
> Missing space (see DTS coding style or any DTS)
> 

Will Fix!

> > +            compatible = "sharp,ls013b7dh03";
> > +            reg = <0>;
> > +            spi-cs-high;
> > +            spi-max-frequency = <1000000>;
> > +            sharp,vcom-mode = "software";
> > +        };
> > +    };
> > +...
> 
> Best regards,
> Krzysztof
> 

Again, thank you for taking the time to review.

Best regards,
Alex