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

[Krzysztof Kozlowski]

On 04/04/2024 10:07, Shreeya Patel wrote:
> 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);
> 	}

That's a driver....

> shreeya@shreeya:~/collabora/rd/rockchip/linux$ make dtbs
>   DTC     arch/arm64/boot/dts/rockchip/rk3588-coolpi-cm5-evb.dtb

and that's a DTS.

> 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

They are not related anyhow. Look above which Makefile target produced
error. Which file failed to build. This is a expressed in make[3] line.
Directory is expressed in other places.

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

What dependency?

It seems you did not understand anything from Heiko's message, so please
reach to your colleagues for explanation where is the dependency.
> 
> 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.

They are not dependent.

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

Sorry, that's some third party gitlab... I don't know what does it prove.

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

There were guidelines - presented in the conferences, mailing list and
even SoC maintainer profile explains how patches eventually end up. I
agree that it still might be confusing, but these are the basics of
submitting patches to anything touching SoC. Anyone working with SoC
will need to know them, so how about Collabora creates some internal
guideline explaining this, so such confusions could be avoided?

I know that such guidelines exist in other companies...

Best regards,
Krzysztof