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

[Dharma.B]

On 05/03/24 3:31 am, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Mar 04, 2024 at 08:00:03PM +0530, 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>
>> ---
>> This patch converts the existing lcdc display text binding to JSON schema.
>> The binding is split into two namely
>> lcdc.yaml
>> - Holds the frame buffer properties
>> lcdc-display.yaml
>> - Holds the display panel properties which is a phandle to the display
>> property in lcdc fb node.
>>
>> These bindings are tested using the following command.
>> 'make DT_CHECKER_FLAGS=-m dt_binding_check'
>> ---
>> Changes in v3:
>> - Remove the generic property "bits-per-pixel"
>> - Link to v2: https://lore.kernel.org/r/20240304-lcdc-fb-v2-1-a14b463c157a@xxxxxxxxxxxxx
>>
>> Changes in v2:
>> - Run checkpatch and remove whitespace errors.
>> - Add the standard interrupt flags.
>> - Split the binding into two, namely lcdc.yaml and lcdc-display.yaml.
>> - Link to v1: https://lore.kernel.org/r/20240223-lcdc-fb-v1-1-4c64cb6277df@xxxxxxxxxxxxx
>> ---
>>   .../bindings/display/atmel,lcdc-display.yaml       | 97 ++++++++++++++++++++++
>>   .../devicetree/bindings/display/atmel,lcdc.txt     | 87 -------------------
>>   .../devicetree/bindings/display/atmel,lcdc.yaml    | 70 ++++++++++++++++
>>   3 files changed, 167 insertions(+), 87 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/atmel,lcdc-display.yaml b/Documentation/devicetree/bindings/display/atmel,lcdc-display.yaml
>> new file mode 100644
>> index 000000000000..5e0b706d695d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/atmel,lcdc-display.yaml
>> @@ -0,0 +1,97 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/atmel,lcdc-display.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Microchip's LCDC Display
>> +
>> +maintainers:
>> +  - Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>
>> +  - Dharma Balasubiramani <dharma.b@xxxxxxxxxxxxx>
>> +
>> +description:
>> +  The LCD Controller (LCDC) consists of logic for transferring LCD image data
>> +  from an external display buffer to a TFT LCD panel. The LCDC has one display
>> +  input buffer per layer that fetches pixels through the single bus host
>> +  interface and a look-up table to allow palletized display configurations. The
>> +  LCDC is programmable on a per layer basis, and supports different LCD
>> +  resolutions, window sizes, image formats and pixel depths.
>> +
>> +# We need a select here since this schema is applicable only for nodes with the
>> +# following properties
>> +
>> +select:
>> +  anyOf:
>> +    - required: [ 'atmel,dmacon' ]
>> +    - required: [ 'atmel,lcdcon2' ]
>> +    - required: [ 'atmel,guard-time' ]
>> +
>> +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)
> 
> Is there a maximum?

The datasheet (https://www.keil.com/dd/chip/4210.htm) for both 
AT91SAM9261 and AT91SAM9263 specifies the GUARD_TIME field within the 
PWRCON register. This field is 7 bits wide (bits 1-7) and acts as a mask 
to define the guard time delay.

Hence the maximum value for the guard time for both AT91SAM9261 and 
AT91SAM9263 SoCs is 127 when using the GUARD_TIME field in the PWRCON 
register.

The datasheet doesn't specify the duration of each "frame period." So, 
this value only represents the maximum number of frame periods that can 
be set for the guard time, not the actual time delay in seconds or 
milliseconds.

> 
>> +
>> +  bits-per-pixel:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: lcd panel bit-depth.
> 
> Constraints?

The bits-per-pixel value can be 1, 2, 4, 8, 16 or 24.

> 
>> +
>> +  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
> 
> Isn't this just a single string rather than an array?
> 
>> +    description: lcd wiring mode "RGB" or "BRG"
> 
> enum:
>    - RGB
>    - BRG
> 
> No BGR?

In the current driver implementation, we have interpreted the wiring
mode represented by ATMEL_LCDC_WIRING_BGR as 'BRG' in the array
atmel_lcdfb_wiring_modes. Considering conventional color representation,
would it be appropriate to consider modifying the existing driver to use
the 'BGR' string instead of 'BRG' for better alignment with standard
naming conventions?

static const char *atmel_lcdfb_wiring_modes[] = {
         [ATMEL_LCDC_WIRING_BGR] = "BRG",
         [ATMEL_LCDC_WIRING_RGB] = "RGB",
};


> 
> But wait, the example shows the value is '1'. That should fail testing.
> It didn't, but I've now fixed that.

It seems correctly configured in our dts files but didn't noticed the 
same in the bindings example, thanks for letting me know, I will correct 
it in the next revision.

> 
>> +
>> +  atmel,power-control-gpio:
>> +    description: gpio to power on or off the LCD (as many as needed)
> 
> maxItems: 1

Thanks, I will add it.
> 
>> +
>> +  display-timings:
>> +    $ref: panel/display-timings.yaml#
>> +
>> +required:
>> +  - atmel,dmacon
>> +  - atmel,lcdcon2
>> +  - atmel,guard-time
>> +  - bits-per-pixel
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    display: panel {
>> +      bits-per-pixel = <32>;
>> +      atmel,lcdcon-backlight;
>> +      atmel,dmacon = <0x1>;
>> +      atmel,lcdcon2 = <0x80008002>;
>> +      atmel,guard-time = <9>;
>> +      atmel,lcd-wiring-mode = <1>;
>> +
>> +      display-timings {
>> +        native-mode = <&timing0>;
>> +        timing0: timing0 {
>> +          clock-frequency = <9000000>;
>> +          hactive = <480>;
>> +          vactive = <272>;
>> +          hback-porch = <1>;
>> +          hfront-porch = <1>;
>> +          vback-porch = <40>;
>> +          vfront-porch = <1>;
>> +          hsync-len = <45>;
>> +          vsync-len = <1>;
>> +        };
>> +      };
>> +    };

-- 
With Best Regards,
Dharma B.