Message-ID: <d0a6be9e-1597-bd1a-84f3-390820e73a98@ti.com>
Date:   Mon, 6 Feb 2023 22:26:07 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v7 3/6] drm/tidss: Add support for AM625 DSS
Content-Language: en-US
To:     Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jyri Sarha <jyri.sarha@iki.fi>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>
CC:     DRI Development List <dri-devel@lists.freedesktop.org>,
        Devicetree List <devicetree@vger.kernel.org>,
        Linux Kernel List <linux-kernel@vger.kernel.org>,
        Nishanth Menon <nm@ti.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Rahul T R <r-ravikumar@ti.com>,
        Devarsh Thakkar <devarsht@ti.com>,
        Jai Luthra <j-luthra@ti.com>,
        Jayesh Choudhary <j-choudhary@ti.com>
References: <20230125113529.13952-1-a-bhatia1@ti.com>
 <20230125113529.13952-4-a-bhatia1@ti.com>
 <ab6f52bb-a3f5-afda-c037-f009153a0bb6@ideasonboard.com>
 <1662a593-8a5d-9214-8a3e-ef2699a35265@ti.com>
 <12ba1f03-d6dd-c9c5-abf0-e9298dc22f28@ideasonboard.com>
From:   Aradhya Bhatia <a-bhatia1@ti.com>
In-Reply-To: <12ba1f03-d6dd-c9c5-abf0-e9298dc22f28@ideasonboard.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Tomi,

On 06-Feb-23 16:28, Tomi Valkeinen wrote:
> On 05/02/2023 16:31, Aradhya Bhatia wrote:
>>
>>
>> On 03-Feb-23 21:03, Tomi Valkeinen wrote:
>>> On 25/01/2023 13:35, Aradhya Bhatia wrote:
>>>> Add support for the DSS controller on TI's new AM625 SoC in the tidss
>>>> driver.
>>>>
>>>> The first video port (VP0) in am625-dss can output OLDI signals through
>>>> 2 OLDI TXes. A 3rd output port has been added with "DISPC_PORT_OLDI" bus
>>>> type.
>>>
>>> Not a big thing here as you add support for a new SoC, but the ordering
>>> of the patches is not optimal. Here you add the AM625 DSS support, but
>>> then you continue actually adding the DSS support (well, mainly OLDI) in
>>> the following patches.
>>>
>>> I think patch 6 could be before this patch. Parts of patch 4 could also
>>> be before this patch. The AM65X renames from patch 5 could be before
>>> this patch.
>>
>> I can move whole of Patch 6 and even of Patch 4 before this one. I have
>> mentioned 'AM625-DSS' in a couple comments which I can make generic,
>> and the rest everything is SoC-agnostic.
>>
>> I haven't tried this, but my concern is if we break patch 5 into 2
>> separate patches,
>>
>> i. AM65X rename plus SoC based switch case, and
>> ii. Addition of AM625 SoC case
>>
>> then I might have to overwrite some changes implemented during (i) in
>> (ii). I don't suppose that would be okay, would it?
> 
> I'm not sure I follow here. Wouldn't (i) be a valid patch in its own?
> Nothing wrong in expanding that later (even if you end up changing a lot
> of it).
> 

(i) would be a valid patch, but implementing (ii) would over-write
certain changes done in (i), albeit small changes in terms of brackets
and indents. That didn't feel right initially and hence the question.

> That said, I don't think this is a very important topic. There are only
> a few commits in the history that might be problematic. A simple fix
> would be to add all the features first, and only last add the compatible
> string for am625.
> 
> Or do all the changes for am625 in a single patch, and try to implement
> all the generic restructuring work before that.
> 
> Here we do have to change the vp-to-output mapping management, so maybe
> the second option won't be simple enough, and it's better to do the
> am625 changes in pieces, as in the first option.
> 

Yeah, the first option does seem a little less complicated. Will try to
re-order this as much clearly as possible.

> So, it's really up to you. Just wanted to raise this possible issue so
> that you are aware of it and can do any easy fixes (if there are such).
> 
>> Also, is it important to keep the compatible-addition patches of
>> DT-binding and driver next to each other in the series? Or should
>> the DT-binding patches should be the first ones? Just curious! =)
> 
> I believe the convention is to have the DT-binding changes before you
> add the compatible string to the driver (if I recall right checkpatch or
> some other checking tool complains if you add a driver for a compatible
> that doesn't have a DT binding). Generic restructurings could be before
> the DT patch, of course, but usually I like to keep the DT binding
> changes at the very beginning of the series.
> 

Okay, I will keep the compatible-append in the binding as the first
patch in the series, before the other general structurings.

Thank you!


Regards
Aradhya