Re: [PATCH] dt-bindings: display: atmel,lcdc: convert to dtschema

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 23/02/2024 10:14, Dharma Balasubiramani wrote:
> Convert the atmel,lcdc bindings to DT schema.
> Changes during conversion: add missing clocks and clock-names properties.
> 
> Signed-off-by: Dharma Balasubiramani <dharma.b@xxxxxxxxxxxxx>
> ---
>  .../devicetree/bindings/display/atmel,lcdc.txt     |  87 --------------
>  .../devicetree/bindings/display/atmel,lcdc.yaml    | 133 +++++++++++++++++++++
>  2 files changed, 133 insertions(+), 87 deletions(-)

You have several patch errors... check your git repo (git show). You
will easily spot them. Or just use decent text editor to clean it up.
Run checkpatch...

...

> diff --git a/Documentation/devicetree/bindings/display/atmel,lcdc.yaml b/Documentation/devicetree/bindings/display/atmel,lcdc.yaml
> new file mode 100644
> index 000000000000..4a1de5a8d64b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/atmel,lcdc.yaml
> @@ -0,0 +1,133 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/atmel,lcdc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip's LCDC Framebuffer
> +
> +maintainers:
> +  - Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
> +  - Dharma Balasubiramani <dharma.b@xxxxxxxxxxxxx>
> +
> +description:
> +  The LCDC works with a framebuffer, which is a section of memory that contains
> +  a complete frame of data representing pixel values for the display. The LCDC
> +  reads the pixel data from the framebuffer and sends it to the LCD panel to
> +  render the image.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - atmel,at91sam9261-lcdc 
> +      - atmel,at91sam9263-lcdc
> +      - atmel,at91sam9g10-lcdc
> +      - atmel,at91sam9g45-lcdc
> +      - atmel,at91sam9g45es-lcdc
> +      - atmel,at91sam9rl-lcdc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks: 
> +    maxItems: 2
> + 
> +  clock-names:
> +    items:
> +      - const: hclk
> +      - const: lcdc_clk
> +
> +  display:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: A phandle pointing to the display node.

Phandle does not have properties. Didn't you want object?

This cannot work - just test it. Change the properties in the example,
remove or add something. Do you see errors? No, because it does not work
at all.

I don't know what's this exactly, but if embedded display then maybe
could be part of this device node. If some other display, then maybe you
need another schema, with compatible? But first I would check how others
are doing this.


> +
> +    properties:
> +      atmel,dmacon:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: dma controller configuration
> +
> +      atmel,lcdcon2:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: lcd controller configuration
> +
> +      atmel,guard-time:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: lcd guard time (Delay in frame periods)
> +
> +      bits-per-pixel:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: lcd panel bit-depth.
> +
> +      atmel,lcdcon-backlight: 
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: enable backlight
> +
> +      atmel,lcdcon-backlight-inverted:
> +        $ref: /schemas/types.yaml#/definitions/flag
> +        description: invert backlight PWM polarity
> +
> +      atmel,lcd-wiring-mode:
> +        $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +        description: lcd wiring mode "RGB" or "BRG"
> +
> +      atmel,power-control-gpio:
> +        description: gpio to power on or off the LCD (as many as needed)
> +
> +    required:
> +      - atmel,dmacon
> +      - atmel,lcdcon2
> +      - atmel,guard-time
> +      - bits-per-pixel 
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - display
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/at91.h>
> +    fb@500000 {
> +      compatible = "atmel,at91sam9g45-lcdc";
> +      reg = <0x00500000 0x1000>;
> +      interrupts = <23 3 0>;

Aren't here some standard interrupt flags?

> +      pinctrl-names = "default";
> +      pinctrl-0 = <&pinctrl_fb>;
> +      clocks = <&pmc PMC_TYPE_PERIPHERAL 23>, <&pmc PMC_TYPE_PERIPHERAL 23>;
> +      clock-names = "hclk", "lcdc_clk";
> +      display = <&display0>;
> +    };
> +


Best regards,
Krzysztof