Re: [PATCH v3 3/7] dt-bindings: phy: rockchip-inno-csi-dphy: add rk3588 variant

From: Michael Riesch <michael.riesch@collabora.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>, Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Kever Yang <kever.yang@rock-chips.com>,
	Jagan Teki <jagan@amarulasolutions.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	Diederik de Haas <didi.debian@cknow.org>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Collabora Kernel Team <kernel@collabora.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-phy@lists.infradead.org
Subject: Re: [PATCH v3 3/7] dt-bindings: phy: rockchip-inno-csi-dphy: add rk3588 variant
Date: Tue, 2 Sep 2025 12:14:57 +0200	[thread overview]
Message-ID: <b0cad1e8-8b02-4039-b1d2-b9056fd51318@collabora.com> (raw)
In-Reply-To: <20250902-piquant-secret-moose-06c4b6@kuoka>

Hi Krzysztof,

Thanks for your comments.

On 9/2/25 09:55, Krzysztof Kozlowski wrote:
> On Mon, Sep 01, 2025 at 10:47:44PM +0200, Michael Riesch wrote:
>> The Rockchip RK3588 variant of the CSI-2 DPHY features two reset lines.
>> Add the variant and allow for the additional reset.
>>
>> Signed-off-by: Michael Riesch <michael.riesch@collabora.com>
>> ---
>>  .../bindings/phy/rockchip-inno-csi-dphy.yaml       | 50 +++++++++++++++++++++-
>>  1 file changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/rockchip-inno-csi-dphy.yaml b/Documentation/devicetree/bindings/phy/rockchip-inno-csi-dphy.yaml
>> index 9ad72518e6da..e37c9fd74788 100644
>> --- a/Documentation/devicetree/bindings/phy/rockchip-inno-csi-dphy.yaml
>> +++ b/Documentation/devicetree/bindings/phy/rockchip-inno-csi-dphy.yaml
>> @@ -21,6 +21,7 @@ properties:
>>        - rockchip,rk3326-csi-dphy
>>        - rockchip,rk3368-csi-dphy
>>        - rockchip,rk3568-csi-dphy
>> +      - rockchip,rk3588-csi-dphy
>>  
>>    reg:
>>      maxItems: 1
>> @@ -40,11 +41,15 @@ properties:
>>  
>>    resets:
>>      items:
>> -      - description: exclusive PHY reset line
>> +      - description: APB reset line
>> +      - description: PHY reset line
> 
> That's changing the order, before first was the phy....

See reset-names below...

> 
>> +    minItems: 1
>>  
>>    reset-names:
>>      items:
>>        - const: apb
>> +      - const: phy
> 
> Although here first was apb? Quite confusing.

Confusing indeed. IMHO the description "exclusive PHY reset line" is
misleading. In the existing device trees there are hints that this is an
APB related reset. These are only hints, of course, but they are the
best info we have.

I can add a remark that we are fixing the misleading description while
at it.

> 
> Anyway "phy" reset for "phy" is pretty non-informative, please give some
> useful name.

As far as the additional reset is concerned, I do not have any info at
all what it is related to. Therefore, it is hard to give a useful name.
Also, to be frank, I have no intention to haggle around with Rockchip
support to find out a more meaningful name (I tried to do that in the
past, only to find that the time spent on that is almost definitely wasted).

Similar PHY blocks are using "phy" or "pipe" or simply "reset" as names,
so I went for that.

Any suggestions are warmly welcome. "aux"? "reset2"? ...?

Best regards,
Michael

> 
>> +    minItems: 1
> 
> Best regards,
> Krzysztof
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy