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.
