Re: [PATCH v7 3/6] drm/tidss: Add support for AM625 DSS

[Tomi Valkeinen]

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).

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.

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.

 Tomi