Re: [PATCH 4/5] arm64: dts: qcom: sdm670-google-sargo: add imx355 front camera

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

On 02/07/2025 00:15, Richard Acayan wrote:
> On Tue, Jul 01, 2025 at 01:23:44PM +0100, Bryan O'Donoghue wrote:
>> On 30/06/2025 23:59, Richard Acayan wrote:
>>> The Sony IMX355 is the front camera on the Pixel 3a. It is connected to
>>> CSIPHY1 and CCI I2C1, and uses MCLK2. Add support for it.
>>>
>>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>>> ---
>>>    .../boot/dts/qcom/sdm670-google-sargo.dts     | 112 ++++++++++++++++++
>>>    1 file changed, 112 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
>>> index d01422844fbf..0af6a440ecbc 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sdm670-google-sargo.dts
>>> @@ -172,6 +172,34 @@ vreg_s2b_1p05: vreg-s2b-regulator {
>>>    		regulator-min-microvolt = <1050000>;
>>>    		regulator-max-microvolt = <1050000>;
>>>    	};
>>> +
>>> +	cam_front_ldo: cam-front-ldo-regulator {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "cam_front_ldo";
>>> +		regulator-min-microvolt = <1352000>;
>>> +		regulator-max-microvolt = <1352000>;
>>> +		regulator-enable-ramp-delay = <135>;
>>> +
>>> +		gpios = <&pm660l_gpios 4 GPIO_ACTIVE_HIGH>;
>>> +		enable-active-high;
>>> +
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&cam_front_ldo_pin>;
>>> +	};
>>> +
>>> +	cam_vio_ldo: cam-vio-ldo-regulator {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "cam_vio_ldo";
>>> +		regulator-min-microvolt = <1800000>;
>>> +		regulator-max-microvolt = <1800000>;
>>> +		regulator-enable-ramp-delay = <233>;
>>> +
>>> +		gpios = <&pm660_gpios 13 GPIO_ACTIVE_HIGH>;
>>> +		enable-active-high;
>>> +
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&cam_vio_pin>;
>>> +	};
>>>    };
>>>
>>>    &apps_rsc {
>>> @@ -392,6 +420,58 @@ vreg_bob: bob {
>>>    	};
>>>    };
>>>
>>> +&camss {
>>> +	vdda-phy-supply = <&vreg_l1a_1p225>;
>>
>> You've got your 1p2 but looks like you are missing your 0p8 supply
> 
> This is probably vreg_s6a_0p87 which supplies vreg_l1a_1p225.

Yep adds up to me.

> 
>>> +	status = "okay";
>>> +};
>>> +
>>> +&camss_endpoint1 {
>>> +	clock-lanes = <7>;
>>> +	data-lanes = <0 1 2 3>;
>>> +	remote-endpoint = <&cam_front_endpoint>;
>>> +	status = "okay";
>>> +};
>>
>> This looks not like how the other dts are upstream. Does this work and pass
>> the dt checker ?
>>
>> Right now upstream wants something like this
>>
>> &camss {
>>          vdda-phy-supply = <&vreg_l5a_0p88>;
>>          vdda-pll-supply = <&vreg_l9a_1p2>;
>>          status = "okay";
>>
>>          ports {
>>                  /* The port index denotes CSIPHY id i.e. csiphy2 */
>>                  port@2 {
>>                          csiphy2_ep: endpoint {
>>                                  clock-lanes = <7>;
>>                                  data-lanes = <0 1 2 3>;
>>                                  remote-endpoint = <&imx577_ep>;
>>                          };
>>                  };
>>          };
>> };
> 
> I misunderstood a review comment from an earlier series. We can do the
> same thing here instead of pushing a different style.

Yes. Unless/until there's a change in linux-next you can safely ignore 
all opinions/statements including mine on how things will be in the 
future...

>> Can the upstream driver actually consume the dt as you specified above ?
> 
> If you're curious, it does understand and let you go as far as using the
> camera.

Nice.

Good work.

---
bod