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

[Dharma.B]

On 06/03/24 10:16 pm, Nicolas Ferre - M43238 wrote:
> Dharma, all,
> 
> On 06/03/2024 at 15:35, Dharma B - I70843 wrote:
>>
>> On 05/03/24 3:31 am, Rob Herring wrote:
>>> 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
> 
> [..]
> 
>>>> +  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?
> 
> This "BRG" thing is definitively a typo. We never had such thing as a 
> BRG color wiring but did have BGR wiring mode.
> 
>> static const char *atmel_lcdfb_wiring_modes[] = {
>>           [ATMEL_LCDC_WIRING_BGR] = "BRG",
>>           [ATMEL_LCDC_WIRING_RGB] = "RGB",
> 
> The thing is that we have one DT using that:
> arch/arm/boot/dts/at91sam9261ek.dts
> 
> So, either I would leave it like that: it's only old product using it.

Thank you for your input, Nicolas. If Rob is in agreement with this
proposal as well, I plan to proceed and submit v4, incorporating
adjustments based on other review comments.

> Or just focus on first character in the string so that it works for both 
> "BRG" or "BGR", and maintains the backward compatibility.
> 
> Regards,
>    Nicolas
> 
> 
>> };
>>
>>
>>>
>>> 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.
> 

-- 
With Best Regards,
Dharma B.