Re: [PATCH v3 0/6] Add Synopsys DesignWare HDMI RX Controller

[Heiko Stübner]

Am Donnerstag, 4. April 2024, 08:15:50 CEST schrieb Krzysztof Kozlowski:
> On 04/04/2024 00:48, Heiko Stübner wrote:
> > Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski:
> >> On 03/04/2024 13:20, Shreeya Patel wrote:
> >>> On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>>
> >>>> On 03/04/2024 11:24, Shreeya Patel wrote:
> >>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx> wrote:
> >>>>>
> >>>>>> This series implements support for the Synopsys DesignWare
> >>>>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
> >>>>>> and HDMI 2.0.
> >>>>>>
> >>>>>
> >>>>> Hi Mauro and Hans,
> >>>>>
> >>>>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
> >>>>
> >>>> Why did you put clk changes here? These go via different subsystem. That
> >>>> might be one of obstacles for your patchset.
> >>>>
> >>>
> >>> I added clock changes in this patch series because HDMIRX driver depends on it.
> >>> I thought it is wrong to send the driver patches which don't even compile?
> >>
> >> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
> >> Please get it reviewed internally first.
> > 
> > For the change in question, the clock controller on the soc also handles
> > the reset controls (hence its name CRU, clock-and-reset-unit) .
> > 
> > There are at least 660 reset lines in the unit and it seems the hdmi-rx one
> > was overlooked on the initial submission, hence patches 1+2 add the
> > reset-line.
> > 
> > Of course, here only the "arm64: dts:" patch depends on the clock
> > change, is it references the new reset-id.
> 
> Wait, that's expected, but it is not what was written. Claim was HDMIRX
> driver depends *build time* ("don't even compile").

Trying to do a full build (kernel + dts) will fail, because the the
device-tree patch references the newly added reset-id .

So you end up with a dtc error. Same with the binding.


I think in the past to uncouple this we did reference the id by number
first:

+      resets = <&cru SRST_A_HDMIRX>, <&cru SRST_P_HDMIRX>,
+               <&cru SRST_HDMIRX_REF>, <&cru 660>;

Had the id-addition separately and then a later series for kernel x+1
to convert from 660 to SRST_A_HDMIRX_BIU .


> > Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski:
> >> Please do not engage multiple subsystems in one patchset, if not
> >> necessary. Especially do not mix DTS into media or USB subsystems. And
> >> do not put DTS in the middle!
> > 
> > picking up your reply from patch 4/6, there seem to be different "schools
> > of thought" for this. Some maintainers might want to really only see
> > patches that are explicitly for their subsystem - I guess networking
> > might be a prime example for that, who will essentially apply whole series'
> > if nobody protests in time (including dts patches)
> 
> There is no school saying DTS is allowed to be in the middle.


I think I wrote exactly that in my original reply :-)

Am Donnerstag, 4. April 2024, 00:48:38 CEST schrieb Heiko Stübner:
> Of course you're right, the "arm64: dts:" patch should be the last in the
> series and not be in the middle of it.


Heiko