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

[Bryan O'Donoghue]

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@xxxxxxxxxx/
>
> 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