Re: [PATCH v3 01/19] media: dt-bindings: media: camss: Add qcom,sm8250-camss binding

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 04/01/2022 13:42, Vladimir Zapolskiy wrote:
> Hi Bryan, Robert,
> 
> On 12/22/21 2:37 AM, Bryan O'Donoghue wrote:
>> From: Jonathan Marek <jonathan@marek.ca>
>>
>> Add bindings for qcom,sm8250-camss in order to support the camera
>> subsystem for SM8250.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>   .../bindings/media/qcom,sm8250-camss.yaml     | 450 ++++++++++++++++++
>>   1 file changed, 450 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml 
>> b/Documentation/devicetree/bindings/media/qcom,sm8250-camss.yaml
> 
> <snip>
> 
>> +required:
>> +  - clock-names
>> +  - clocks
>> +  - compatible
>> +  - interconnects
>> +  - interconnect-names
>> +  - interrupts
>> +  - interrupt-names
>> +  - iommus
>> +  - power-domains
>> +  - reg
>> +  - reg-names
>> +
>> +additionalProperties: false
> 
> I've discovered that there is a noticeable difference between this bindings
> and all the previous ones, for instance see qcom,sdm845-camss.yaml
> 
> There is no required 'vdda-supply' property on the list, and fwiw I believe
> there should be two supply properties for 0p9 and 1p2 supplies in fact.
> Similarly, two separate supplies should be present in sdm845 camss 
> bindings.

The 1p2 supply is defined in the camera sensor section as dvdd-supply

https://git.linaro.org/people/bryan.odonoghue/kernel.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?h=v5.16-rc6-sm8250-camss-imx577-only-v3

1p2 connects to dvdd-supply as per Miura(865-RB5)-Camera_NAV_Mezz page 
10 - thundercomm registration required to view

arch/arm64/boot/dts/qcom/qrb5165-rb5.dts::&cci_i2c2::camera@1a
{
     dovdd-supply  = <&vreg_s4a_1p8>;
     avdd-supply = <&vreg_l7f_1p8>;
     dvdd-supply = <&vreg_l9a_1p2>;
}

similarly to

arch/arm64/boot/dts/qcom/sdm845-db845c.dts::&cci_i2c0::camera@10 {
     dovdd-supply = <&vreg_lvs1a_1p8>;
     avdd-supply = <&cam0_avdd_2v8>;
     dvdd-supply = <&cam0_dvdd_1v2>;
}

and

arch/arm64/boot/dts/qcom/apq8016-sbc.dts::&cci_i2c0::camera_rear@3b
{
     vdddo-supply = <&camera_vdddo_1v8>;
     vdda-supply = <&camera_vdda_2v8>;
     vddd-supply = <&camera_vddd_1v5>; /* bod: here */
}

The IMX sensor needs to have the regulator_bulk_enable() stuff added, 
I'll post this patch it works standalone

https://git.linaro.org/people/bryan.odonoghue/kernel.git/commit/?h=v5.16-rc6-sm8250-camss-imx577-only-v3&id=e82fc1b29d9227cad3ad7dcab362c39dd4a63bdb

Simiar to 0c2c7a1e0d69 ("media: ov8856: Add devicetree support")

Downstream points the CPA to "camss-vdd-supply = <&titan_top_gdsc>;" 
which is covered by our TITAN_TOP_GDSC power-domain and 
"mipi-csi-vdd-supply = <&pm8150_l9>;"

regulator-pm8150-l9 == rpmh-regulator-ldoa9 == ldoa9 == pmic5-ldo

aka l9a upstream

"dvdd-supply = <&vreg_l9a_1p2>;"

vreg_l9a_1p2: ldo9 {
     regulator-name = "vreg_l9a_1p2";
     regulator-min-microvolt = <1200000>;
     regulator-max-microvolt = <1200000>;
     regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
};

Not sure I see on the schematic or in the downstream dts how 0p9 is 
connected to the camera - seems to be a pcie and or mdss-dsi regulator.

If vdda-supply is a required property of the camera and not the sensor 
then what regulator do you think it should point to ?

> At the moment the driver operates with 'vdda' supply only, commit 9e5d1581
> introduced undocumented 'vdd_sec' for sdm660, but, if I'm not mistaken,
> it's unused.

I agree with you there - vdd_sec is either unspecified in the 
Documentation or not required.

---
bod