Re: [RFC PATCH] dt-bindings: phy: Add Qualcomm MIPI C-/D-PHY schema for CSIPHY IPs

[Krzysztof Kozlowski <krzk@kernel.org>]

On 14/05/2025 21:30, Vladimir Zapolskiy wrote:
> Hello Krzysztof.
> 
> On 5/14/25 13:25, Krzysztof Kozlowski wrote:
>> On 13/05/2025 16:39, Vladimir Zapolskiy wrote:
>>> Add dt-binding schema for the CAMSS CSIPHY IPs, which provides
>>> MIPI C-/D-PHY interfaces on Qualcomm SoCs.
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>> ---
>>>   .../devicetree/bindings/phy/qcom,csiphy.yaml  | 110 ++++++++++++++++++
>>>   1 file changed, 110 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/phy/qcom,csiphy.yaml
>>
>>
>> Looks like not tested, so limited review follows.
>>
>> Filename matching compatible.
>>
> 
> Thank you for the review, the change is deliberately tagged as RFC.
> 
> I read this review comment as the displayed generic compatible 'qcom,csiphy'
> shall be added to the list of compatibles.

No. The comment is about filename. You must rename the filename to match
the compatible. How this could mean anything else?

> 
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,csiphy.yaml b/Documentation/devicetree/bindings/phy/qcom,csiphy.yaml
>>> new file mode 100644
>>> index 000000000000..ef712c5442ec
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/qcom,csiphy.yaml
>>
>> Please post the driver or any other user. Or explain why this is RFC or
>> what you expect here from us.
>>
> 
> The CSIPHY driver agnostic CAMSS changes are on the linux-media list [1], the CSIPHY
> driver specific changes will be added on top of these changes, however I believe
> it makes sense to review these two different CAMSS changesets independently.

Do not introduce your own rules. It is ALWAYS expected to post binding
and its driver user together.

> 
> Here the RFC tag is given explicitly to get change reviews for the dt binding
> documentation part, and the first user is the example embedded into the change.
> 
>>> @@ -0,0 +1,110 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/phy/qcom,csiphy.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm CSI PHY
>>
>> SM8250 ?
>>
> 
> It's supposed to be a generic device tree binding, and it covers SM8250
> CAMSS CSIPHY IP as well, which could be quite handly for testing/review.

It's not generic. It's specific to SM8250.



Best regards,
Krzysztof