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

[Shreeya Patel]

On Thursday, April 04, 2024 11:45 IST, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:

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

For some context, when I was testing the downstream driver against the
device tree, I saw some failures because of the missing reset ( which I am trying
to add in the first two patches for this series )

..
	hdmirx_dev->rst_biu = devm_reset_control_get(hdmirx_dev->dev, "rst_biu");
	if (IS_ERR(hdmirx_dev->rst_biu)) {
		dev_err(dev, "failed to get rst_biu control\n");
		return PTR_ERR(hdmirx_dev->rst_biu);
	}

shreeya@shreeya:~/collabora/rd/rockchip/linux$ make dtbs
  DTC     arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb
Error: arch/arm64/boot/dts/rockchip/rk3588.dtsi:187.23-24 syntax error
FATAL ERROR: Unable to parse input tree
make[3]: *** [scripts/Makefile.lib:419: arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb] Error 1
make[2]: *** [scripts/Makefile.build:481: arch/arm64/boot/dts/rockchip] Error 2
make[1]: *** [/home/shreeya/collabora/rd/rockchip/linux/Makefile:1392: dtbs] Error 2
make: *** [Makefile:240: __sub-make] Error 2

Sorry for referring this as a driver build failure but I am sure you would 
also have not been okay with the above issues.
Ofcourse I can simply remove this dependency from the driver but maybe
that will affect the functionality and I didn't want to send a buggy patch series.

My intention here was just like Heiko said, to keep all the dependent patches
together. I didn't know that would be a trouble for Maintainers.

FWIW, HDMIRX patch series was reviewed multiple times and that is why you
see a Reviewed-by tag from a Collabora Engineer. Sometimes the things that look
problematic to one might not look the same to others and that is why I asked you
to provide some more details about the problem that you were seeing.

https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/21
https://gitlab.collabora.com/hardware-enablement/rockchip-3588/linux/-/merge_requests/17


> > 
> > 
> > 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.
> 
> Other schools are indeed saying that seeing DTS is good and
> recommendation is to post it separate and provide a link. That's way you
> avoid DTS being pulled by Greg, media or networking.
> 

This was my mistake and I simply forgot to remove the DTS when I was
testing the driver for the last time before sending the v3 upstream.
Adding it in the middle was incorrect, I should have added it as a separate
patch but honestly this has always been very confusing and the expectation
differs from maintainers to maintainers.

> > 
> > On the other hand I also remember seeing requests for "the full picture"
> > and individual maintainers then just picking and applying the patches
> > meant for their subsystem.
> > 
> > The series as it stands right now is nice in that it allows (random)
> > developers to just pick it up, apply it to a tree and test the actual driver
> > without needing to hunt for multiple dependant series.
> > 
> > 
> > Of course you're right, the "arm64: dts:" patch should be the last in the
> > series and not be in the middle of it.
> 

I will make sure to correct this in my v4.

Thanks,
Shreeya Patel
> 
> Best regards,
> Krzysztof
>