Message-ID: <b7efc616-cf01-77a7-5eb4-9076ae40e2f8@ti.com>
Date:   Wed, 26 Apr 2023 11:43:07 +0530
From:   Aradhya Bhatia <a-bhatia1@...com>
To:     Andrew Davis <afd@...com>, Nishanth Menon <nm@...com>,
        Vignesh Raghavendra <vigneshr@...com>,
        Tero Kristo <kristo@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>
CC:     Devicetree List <devicetree@...r.kernel.org>,
        Linux Kernel List <linux-kernel@...r.kernel.org>,
        Linux ARM Kernel List <linux-arm-kernel@...ts.infradead.org>,
        Jyri Sarha <jyri.sarha@....fi>,
        Tomi Valkeinen <tomba@...nel.org>,
        Praneeth Bajjuri <praneeth@...com>,
        Rahul T R <r-ravikumar@...com>,
        Devarsh Thakkar <devarsht@...com>,
        Jai Luthra <j-luthra@...com>,
        Jayesh Choudhary <j-choudhary@...com>
Subject: Re: [PATCH 1/2] arm64: dts: ti: Add overlay for OLDI-LCD1EVM Display
 and touch screen

Hi Andrew,

On 25-Apr-23 23:14, Andrew Davis wrote:
> On 4/25/23 12:00 PM, Aradhya Bhatia wrote:
>> Hi Andrew,
>>
>> On 25-Apr-23 20:39, Andrew Davis wrote:
>>> On 4/25/23 12:12 AM, Aradhya Bhatia wrote:
>>>> From: Jyri Sarha <jsarha@...com>
>>>>
>>>> The OLDI-LCD1EVM add on board has Rocktech RK101II01D-CT panel with
>>>> integrated touch screen. The integrated touch screen is Goodix GT928.
>>>> Add DT nodes for these and connect the endpoint nodes with DSS.
>>>>
>>>> This patch was picked from TI's public tree based on 5.10 LTS kernel.
>>>>
>>>> Signed-off-by: Jyri Sarha <jsarha@...com>
>>>> Signed-off-by: Nikhil Devshatwar <nikhil.nd@...com>
>>>> [abhatia1@...com: Make syntax changes to support 6.1 DTSO format]
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@...com>
>>>> ---
>>>>    arch/arm64/boot/dts/ti/Makefile               |  2 +
>>>>    .../dts/ti/k3-am654-evm-oldi-lcd1evm.dtso     | 70
>>>> +++++++++++++++++++
>>>>    2 files changed, 72 insertions(+)
>>>>    create mode 100644
>>>> arch/arm64/boot/dts/ti/k3-am654-evm-oldi-lcd1evm.dtso
>>>>
>>>> diff --git a/arch/arm64/boot/dts/ti/Makefile
>>>> b/arch/arm64/boot/dts/ti/Makefile
>>>> index 6acd12409d59..8956b19e587a 100644
>>>> --- a/arch/arm64/boot/dts/ti/Makefile
>>>> +++ b/arch/arm64/boot/dts/ti/Makefile
>>>> @@ -26,6 +26,7 @@ dtb-$(CONFIG_ARCH_K3) +=
>>>> k3-am6548-iot2050-advanced.dtb
>>>>    dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced-m2.dtb
>>>>    dtb-$(CONFIG_ARCH_K3) += k3-am6548-iot2050-advanced-pg2.dtb
>>>>    dtb-$(CONFIG_ARCH_K3) += k3-am654-base-board.dtb
>>>> +dtb-$(CONFIG_ARCH_K3) += k3-am654-evm-oldi-lcd1evm.dtbo
>>>
>>> This name is a bit odd, why "evm" twice? Looks like the first instance
>>> is the redundant one as most of the documents on this LCD board call it
>>> the "LCD1EVM". How about:
>>>
>>> k3-am654-lcd1evm.dtbo
>>
>> I didn't think I could change the name of the overlay picking the patch
>> from our tree, but if we are going to do it, can we take up another
>> approach, where it would be easier to add panels for AM62x family and
>> ensure uniformity throughout.
>>
> 
> Yes, we can change what we want when upstreaming. How we did it in our
> evil vendor tree should in no way prevent us from doing things better
> in upstream.
> 
Okay.

>> We have 2 different panels from Vendor A, and another one from Vendor B.
>> Vendor B panel connects to AM625-SK via an adapter board.
>>
>> Vendor-A/Panel-1 only says the name, 'SK-LCD1' on its circuit board.
>> Vendor-A/Panel-2 doesn't have any name yet. We only have development
>> units.
>> Vendor-B/Panel-2 mentions '$(LCD_model) to AM62x SoC adapter board'.
>>
>> Since, there are too many manufacturers, it is difficult to maintain
>> uniformity with the names of panel-boards. So, I have this approach in
>> mind (which I have used for our tree for AM62x), but would like your
>> comments.
>>
>> k3-$soc-$board-$(panel_vendor)-$(brief_compatible).dtso
>>
>> So, for AM625-SKs,
>> k3-am625-sk-$(vendor_name)-$(brief_compatible).dtso
>>
> 
> Looks reasonable to me.
Great! I tweaked this to add the keyword "panel" to also make it reader
friendly in v2.

k3-$soc-$board-$(panel_vendor)-$(brief_compatible)-panel.dtso

> 
>> and for the current panel Rocktech RK101II01D-CT, which applies on AM654
>> base-board,
>>
>> k3-am654-base-board-rocktech-rk101.dtso.
>>
>> This does become rather long, but also is distinguishable.
>>
> 
> No limit to file names here, being clear and distinguishable is more
> important than short names.
> 
>> Let me know what you think.
>>
>>>
>>> I would like the overlay names to give some hint to what base DTB they
>>> apply to,
>>
>> Agreed. That is indeed how it should be.
>>
>>> or better yet, apply them here in the build which will check
>>> that they apply cleanly. Plus you can drop the silly "+= -@" below.
>>>
>>
>> The above approach will give a hint of the base EVM where a combined
>> build is not possible simply because there is no 'official' name for a
>> particular combination of panel and EVM.
>>
> 
> We do not need to name each possible combination (and we shouldn't, there
> would be a combinatorial explosion, avoiding that is the whole point
> of using overlays vs .dtsi includes).
> 
> I do think we should name at least the combinations that we ship together.
> So as below for AM654 that would be the GPEVM and the IDK. Those are the
> two out-of-box combinations available for purchase as a kit. Folks can
> still
> buy additional add-on cards, and/or mix and match from those two sets.
> 
> As long as we have at least one named combination, then the base-board dtb
> file gets symbols automatically and we can drop the "+= -@" line.
> 

Understood! I have incorporated these suggestions, as well as the ones
from Tomi and Nishanth, in v2 and posted them.

Regards
Aradhya

> Andrew
> 
>>
>> Regards
>> Aradhya
>>
>>> Let's see how this should be called, from the AM65x GP EVM doc[0] we
>>> get a nice picture on page 5 and the following:
>>>
>>> "The AM65x GP EVM consists of a common processor board, an LCD adapter,
>>> and a one-lane PCIe/USB3 personality card."
>>>
>>> So, this would translate to:
>>>
>>> k3-am654-gp-evm-dtbs := k3-am654-base-board.dtb k3-am654-lcd1evm.dtbo
>>> k3-am654-pcie-usb3.dtbo
>>> dtb-$(CONFIG_ARCH_K3) += k3-am654-gp-evm.dtb>
>>> Next, from the AM65x IDK doc[1] also with a nice image on page 5:
>>>
>>> "The AM65x IDK consists of a common processor board, IDK application
>>> board,
>>> and a two-lane PCIe personality card.:
>>>
>>> So:
>>>
>>> k3-am654-idk-dtbs := k3-am654-base-board.dtb k3-am654-idk.dtbo
>>> k3-am654-pcie-usb2.dtbo
>>> dtb-$(CONFIG_ARCH_K3) += k3-am654-idk.dtb
>>>
>>> Note that we do have all those missing dtso files in our evil vendor
>>> tree[2]
>>> and will be upstreaming them next, so this naming should all work out
>>> nicely.
>>>
>>> Andrew
>>>
>>> [0] https://www.ti.com/lit/ug/spruim7/spruim7.pdf
>>> [1] https://www.ti.com/lit/ug/spruim6a/spruim6a.pdf
>>> [2]
>>> https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/arch/arm64/boot/dts/ti?h=ti-linux-5.10.y
>>>
>>>>      # Boards with J7200 SoC
>>>>    dtb-$(CONFIG_ARCH_K3) += k3-j7200-common-proc-board.dtb
>>>> @@ -45,3 +46,4 @@ dtb-$(CONFIG_ARCH_K3) += k3-j784s4-evm.dtb
>>>>      # Enable support for device-tree overlays
>>>>    DTC_FLAGS_k3-am6548-iot2050-advanced-m2 += -@
>>>> +DTC_FLAGS_k3-am654-base-board += -@
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am654-evm-oldi-lcd1evm.dtso
>>>> b/arch/arm64/boot/dts/ti/k3-am654-evm-oldi-lcd1evm.dtso
>>>> new file mode 100644
>>>> index 000000000000..b2c790b314cf
>>>> --- /dev/null
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am654-evm-oldi-lcd1evm.dtso
>>>> @@ -0,0 +1,70 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/**
>>>> + * OLDI-LCD1EVM Rocktech integrated panel and touch DT overlay for
>>>> AM654-EVM.
>>>> + *
>>>> + * Copyright (C) 2023 Texas Instruments Incorporated -
>>>> http://www.ti.com/
>>>> + */
>>>> +
>>>> +/dts-v1/;
>>>> +/plugin/;
>>>> +
>>>> +#include <dt-bindings/pwm/pwm.h>
>>>> +#include <dt-bindings/gpio/gpio.h>
>>>> +#include <dt-bindings/interrupt-controller/irq.h>
>>>> +
>>>> +&{/} {
>>>> +    display0 {
>>>> +        compatible = "rocktech,rk101ii01d-ct";
>>>> +        backlight = <&lcd_bl>;
>>>> +        enable-gpios = <&pca9555 8 GPIO_ACTIVE_HIGH>;
>>>> +        port {
>>>> +            lcd_in0: endpoint {
>>>> +                remote-endpoint = <&oldi_out0>;
>>>> +            };
>>>> +        };
>>>> +    };
>>>> +
>>>> +    lcd_bl: backlight {
>>>> +        compatible = "pwm-backlight";
>>>> +        pwms = <&ecap0 0 50000 PWM_POLARITY_INVERTED>;
>>>> +        brightness-levels =
>>>> +            <0 32 64 96 128 160 192 224 255>;
>>>> +        default-brightness-level = <8>;
>>>> +    };
>>>> +};
>>>> +
>>>> +&dss {
>>>> +    status = "okay";
>>>> +};
>>>> +
>>>> +&dss_ports {
>>>> +    #address-cells = <1>;
>>>> +    #size-cells = <0>;
>>>> +
>>>> +    port@0 {
>>>> +        reg = <0>;
>>>> +
>>>> +        oldi_out0: endpoint {
>>>> +            remote-endpoint = <&lcd_in0>;
>>>> +        };
>>>> +    };
>>>> +};
>>>> +
>>>> +&main_i2c1 {
>>>> +    #address-cells = <1>;
>>>> +    #size-cells = <0>;
>>>> +
>>>> +    gt928: touchscreen@14 {
>>>> +        status = "okay";
>>>> +        compatible = "goodix,gt928";
>>>> +        reg = <0x14>;
>>>> +
>>>> +        interrupt-parent = <&pca9554>;
>>>> +        interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
>>>> +        touchscreen-size-x = <1280>;
>>>> +        touchscreen-size-y = <800>;
>>>> +
>>>> +        reset-gpios = <&pca9555 9 GPIO_ACTIVE_HIGH>;
>>>> +        irq-gpios = <&pca9554 3 GPIO_ACTIVE_HIGH>;
>>>> +    };
>>>> +};

Powered by blists - more mailing lists