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@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.
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
