Received-SPF: pass (google.com: domain of linux-kernel+bounces-94290-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <e6537742-d9be-4745-b457-b36e60ccaa9b@microchip.com>
Date: Wed, 6 Mar 2024 17:46:19 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] dt-bindings: display: atmel,lcdc: convert to dtschema
Content-Language: en-US, fr-FR
To: Dharma B - I70843 <Dharma.B@microchip.com>, Rob Herring <robh@kernel.org>
CC: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard
	<mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, David Airlie
	<airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>, Claudiu Beznea
	<claudiu.beznea@tuxon.dev>, "dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>, "devicetree@vger.kernel.org"
	<devicetree@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
References: <20240304-lcdc-fb-v3-1-8b616fbb0199@microchip.com>
 <20240304220154.GA1115739-robh@kernel.org>
 <d3ae3196-4e8a-4e73-bc36-f53541598ab2@microchip.com>
From: Nicolas Ferre <nicolas.ferre@microchip.com>
Organization: microchip
In-Reply-To: <d3ae3196-4e8a-4e73-bc36-f53541598ab2@microchip.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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@microchip.com>
>>> ---
>>> 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@microchip.com
>>>
>>> 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@microchip.com
>>> ---
>>>    .../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.
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.