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

[Richard Acayan <mailingradian@gmail.com>]

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.

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

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