Re: [PATCH 07/13] dt-bindings: media: camss: Add qcom,sm8550-camss binding

[Depeng Shao <quic_depengs@quicinc.com>]

Hi Vladimir,

On 10/8/2024 10:06 PM, Bryan O'Donoghue wrote:
> 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@linaro.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.
> 


Yes, I have picked your patch and rebased the SM8550 change based on 
your patch. I also verified them and it works good.

But I don't understand why the names are csi01, csi23, csi46, csi57. 
Could you please elaborate more?

I'm using csiphyX-vdda-phy-supply and csiphyX-vdda-pll-supply now.

Thanks,
Depeng