Re: [PATCH 1/2] dt-bindings: display: Add Sharp Memory LCD bindings

[Alex Lanzano]

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@xxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxx>
> > +
> > +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