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
