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

[<Dharma.B@microchip.com>]

Hi Krzysztof,

On 27/02/24 5:33 pm, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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@microchip.com>
>> ---
>>   .../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...
> 

There seems to be an issue with my git hooks, Thanks for letting me know 
these errors, I will fix it.

> ...
> 
>> 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@microchip.com>
>> +  - Dharma Balasubiramani <dharma.b@microchip.com>
>> +
>> +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.

Yes Indeed, I thought its working as expected as I had all required 
property.

> 
> 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.

Okay, then I think the driver also needs to be modified, currently the 
driver parses the phandle and looks for these properties. Also the 
corresponding dts files.

> 
> 
>> +
>> +    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?

I will update the flags as well in the next revision.

> 
>> +      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
> 

-- 
With Best Regards,
Dharma B.