Message-ID: <7dd6132f-52c2-4f0a-8eec-26791f250111@linaro.org>
Date: Tue, 8 Oct 2024 15:06:27 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
 Depeng Shao <quic_depengs@...cinc.com>, krzk+dt@...nel.org,
 Neil Armstrong <neil.armstrong@...aro.org>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel@...cinc.com, Yongsheng Li <quic_yon@...cinc.com>, mchehab@...nel.org,
 robh@...nel.org, todor.too@...il.com, rfoss@...nel.org, conor+dt@...nel.org
Subject: Re: [PATCH 07/13] dt-bindings: media: camss: Add qcom,sm8550-camss
 binding

On 08/10/2024 14:50, Vladimir Zapolskiy wrote:
> Hi Depeng.
> 
> On 9/30/24 12:26, Depeng Shao wrote:
>> Hi Bryan,
>>
>> On 9/25/2024 11:40 PM, Depeng Shao wrote:
>>> Hi Vladimir, Bryan,
>>>
>>> On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote:
>>>> Hi Bryan,
>>>>
>>>> On 9/18/24 01:40, Bryan O'Donoghue wrote:
>>>>> On 13/09/2024 06:06, Vladimir Zapolskiy wrote:
>>>>>> On 9/13/24 01:41, Bryan O'Donoghue wrote:
>>>>>>> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>>>>>>>> 3. Required not optional in the yaml
>>>>>>>>>
>>>>>>>>>         => You can't use the PHY without its regulators
>>>>>>>>
>>>>>>>> No, the supplies shall be optional, since it's absolutely 
>>>>>>>> possible to
>>>>>>>> have
>>>>>>>> such a board, where supplies are merely not connected to the SoC.
>>>>>>>
>>>>>>> For any _used_ PHY both supplies are certainly required.
>>>>>>>
>>>>>>> That's what the yaml/dts check for this should achieve.
>>>>>>
>>>>>> I believe it is technically possible by writing an enormously complex
>>>>>> scheme, when all possible "port" cases and combinations are listed.
>>>>>>
>>>>>> Do you see any simpler way? Do you insist that it is utterly needed?
>>>>>
>>>>> I asked Krzysztof about this offline.
>>>>>
>>>>> He said something like
>>>>>
>>>>> Quote:
>>>>> This is possible, but I think not between child nodes.
>>>>> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/
>>>>> devicetree/bindings/example-schema.yaml#L194
>>>>>
>>>>> You could require something in children, but not in parent node. For
>>>>> children something around:
>>>>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/
>>>>> devicetree/bindings/net/qcom,ipa.yaml#L174
>>>>>
>>>>> allOf:
>>>>>      - if:
>>>>>          required:
>>>>>            - something-in-parent
>>>>>        then:
>>>>>          properties:
>>>>>            child-node:
>>>>>              required:
>>>>>                - something-in-child
>>>>>
>>>>> I will see if I can turn that into a workable proposal/patch.
>>>>>
>>>>
>>>> thank you for pushing my review request forward.
>>>>
>>>> Overall I believe making supply properties as optional ones is
>>>> sufficient,
>>>> technically straightforward and merely good enough, thus please let me
>>>> ask you to ponder on this particular variant one more time.
>>>>
>>>
>>> So, we are discussing two things.
>>>
>>> 1# Use separate supplies for each CSI block, looks like there is no
>>> doubt about it anymore. So, I will update it just like based on 
>>> suggestion.
>>>
>>> csiphyX-vdda-phy-supply
>>> csiphyX-vdda-pll-supply
>>>
>>> Then I will need below items in the required list if they are required.
>>> required:
>>>     - csiphy0-vdda-phy-supply
>>>     - csiphy0-vdda-pll-supply
>>>     - csiphy1-vdda-phy-supply
>>>     - csiphy1-vdda-pll-supply
>>> ...
>>>     - csiphy7-vdda-phy-supply
>>>     - csiphy7-vdda-pll-supply
>>>
>>> 2# Regarding the CSI supplies, if they need to be making as optional?
>>> Looks like there is no conclusion now.
>>>
>>> @Bryan, do you agree with this?
>>>
>>
>> I'm preparing the new version patches, and will send out for reviewing
>> in few days. I will follow Vladimir's comments if you have no response,
>> it means making supply properties as optional one, so they won't be
>> added to the required list.
>>
> 
> Recently I published the change, which moves regulator supplies from CSID
> to CSIPHY, I believe it makes sense to base the SM8550 change and 
> regulators
> under discussion on top of the series:
> 
> https://lore.kernel.org/all/20240926211957.4108692-1- 
> vladimir.zapolskiy@...aro.org/
> 
> Note, that SM8250 regulators are not changed, however their names are 
> wrong,
> the correction shall be a separate change later on...
> 
> Next, I developed my opinion regarding the supply regulator property names:
> 
> 1) voltage supply regulator property names match the pattern "*v*-supply",
>     and the most common name is "vdd*-supply", the match to the pattern 
> shall
>     be preserved,
> 2) also it would be much better and it will exclude any confusion, if 
> SoC pin
>     names are put into the name, like it is done in a multitude of similar
>     cases.
> 
> So, in my opinion for SM8550 CAMSS a proposed set of voltage supply 
> regulator
> names should be this one:
> 
> - vdda-csi01-0p9-supply
> - vdda-csi01-1p2-supply
> - vdda-csi23-0p9-supply
> - vdda-csi23-1p2-supply
> - vdda-csi46-0p9-supply
> - vdda-csi46-1p2-supply
> - vdda-csi57-0p9-supply
> - vdda-csi57-1p2-supply

So I communicated to Depeng to take the patch for the regulators but, I 
still don't think the above is the right way to do this.

I will take a pass at constructing something in the schema to capture 
the case where a regulator is required if and only if it is instantiated.

May not be possible with our current syntax/tools but is 100% how the 
hardware works so IMO is the right thing to try to do.

---
bod

Powered by blists - more mailing lists