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

From: "Shreeya Patel" <shreeya.patel@collabora.com>
To: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>
Cc: "Heiko Stübner" <heiko@sntech.de>,
	mchehab@kernel.org, hverkuil@xs4all.nl, hverkuil-cisco@xs4all.nl,
	robh@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	conor+dt@kernel.org, mturquette@baylibre.com, sboyd@kernel.org,
	p.zabel@pengutronix.de, shawn.wen@rock-chips.com,
	kernel@collabora.com, linux-kernel@vger.kernel.org,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm@lists.infradead.org
Subject: Re: [PATCH v3 0/6] Add Synopsys DesignWare HDMI RX Controller
Date: Thu, 04 Apr 2024 09:07:47 +0100	[thread overview]
Message-ID: <36bd27-660e6000-3-6c1c1e00@12777057> (raw)
In-Reply-To: <c790c8ba-a9bd-4820-8084-1294e5e523d9@linaro.org>

On Thursday, April 04, 2024 11:45 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> wrote:
> >>>
> >>>> On 03/04/2024 11:24, Shreeya Patel wrote:
> >>>>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> 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
>