Re: [PATCH v2 2/5] arm64: dts: renesas: sparrow-hawk: Add overlay for IMX219 on J1

Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx> · Fri, 4 Jul 2025 11:52:25 +0200

Hello Laurent,

Thanks for your time to review this work.

On 2025-07-04 02:03:19 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> The comments below apply to 3/5 where applicable.
> 
> On Tue, Jul 01, 2025 at 01:26:09PM +0200, Niklas Söderlund wrote:
> > Add an overlay to connect an IMX219 camera sensor to the J1 connector.
> > The IMX219 utilizes 2 CSI-2 D-PHY lanes. This enables the video capture
> > pipeline behind the CSI40 Rx to be enabled to process images from the
> > sensor.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > ---
> > * Changes since v1
> > - Drop 'status = "okay"' property for the fixed regulators.
> > ---
> >  arch/arm64/boot/dts/renesas/Makefile          |   3 +
> >  ...8a779g3-sparrow-hawk-camera-j1-imx219.dtso | 118 ++++++++++++++++++
> >  2 files changed, 121 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-camera-j1-imx219.dtso
> > 
> > diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
> > index 47e46ef99d36..73218f7ec9af 100644
> > --- a/arch/arm64/boot/dts/renesas/Makefile
> > +++ b/arch/arm64/boot/dts/renesas/Makefile
> > @@ -96,7 +96,10 @@ dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g2-white-hawk-single-ard-audio-da7212.dtb
> >  
> >  DTC_FLAGS_r8a779g3-sparrow-hawk += -Wno-spi_bus_bridge
> >  dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g3-sparrow-hawk.dtb
> > +dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g3-sparrow-hawk-camera-j1-imx219.dtbo
> >  dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g3-sparrow-hawk-fan-pwm.dtbo
> > +r8a779g3-sparrow-hawk-camera-j1-imx219-dtbs := r8a779g3-sparrow-hawk.dtb r8a779g3-sparrow-hawk-camera-j1-imx219.dtbo
> > +dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g3-sparrow-hawk-camera-j1-imx219.dtb
> >  r8a779g3-sparrow-hawk-fan-pwm-dtbs := r8a779g3-sparrow-hawk.dtb r8a779g3-sparrow-hawk-fan-pwm.dtbo
> >  dtb-$(CONFIG_ARCH_R8A779G0) += r8a779g3-sparrow-hawk-fan-pwm.dtb
> >  
> > diff --git a/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-camera-j1-imx219.dtso b/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-camera-j1-imx219.dtso
> > new file mode 100644
> > index 000000000000..a9089d3a4b29
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-camera-j1-imx219.dtso
> > @@ -0,0 +1,118 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +/*
> > + * Device Tree Overlay for an IMX219 camera sensor in connector J1 on R-Car V4H
> > + * ES3.0 Sparrow Hawk board.
> > + *
> > + * Copyright 2025 Renesas Electronics Corp.
> > + * Copyright 2025 Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>
> > + */
> > +
> > +/dts-v1/;
> > +/plugin/;
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/media/video-interfaces.h>
> > +
> > +&{/} {
> > +	clk_cam_j1: clk_cam_j1 {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <24000000>;
> > +		status = "okay";
> 
> No need for status.
> 
> > +	};
> > +
> > +	/* Page 29 / CSI_IF_CN / J1 */
> > +	reg_cam_j1: reg_cam_j1 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "reg_cam_j1";
> > +		enable-active-high;
> > +		gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> 
> No need for a pinctrl entry ?

As far as I can tell, no. No other user of gpio in the renesas DTS sets 
pinctrl for gpio. Inspecting the pinmux configuration further makes me 
believe this is correct.

Without this overlay loaded

  # grep gpio /sys/kernel/debug/pinctrl/e6050000.pinctrl-sh-pfc/pinmux-pins
  Format: pin (name): mux_owner gpio_owner hog?
  pin 65 (GP_2_1): (MUX UNCLAIMED) e6058180.gpio:561
  pin 107 (GP_3_11): (MUX UNCLAIMED) e6058980.gpio:591
  pin 149 (GP_4_21): (MUX UNCLAIMED) e6060180.gpio:631
  pin 150 (GP_4_22): (MUX UNCLAIMED) e6060180.gpio:632
  pin 234 (GP_7_10): (MUX UNCLAIMED) e6061980.gpio:687
  pin 269 (GP_8_13): (MUX UNCLAIMED) e6068180.gpio:711

With this overlay loaded.

  # grep gpio /sys/kernel/debug/pinctrl/e6050000.pinctrl-sh-pfc/pinmux-pins
  Format: pin (name): mux_owner gpio_owner hog?
  pin 1 (GP_0_1): (MUX UNCLAIMED) e6050180.gpio:513
  pin 65 (GP_2_1): (MUX UNCLAIMED) e6058180.gpio:561
  pin 107 (GP_3_11): (MUX UNCLAIMED) e6058980.gpio:591
  pin 149 (GP_4_21): (MUX UNCLAIMED) e6060180.gpio:631
  pin 150 (GP_4_22): (MUX UNCLAIMED) e6060180.gpio:632
  pin 234 (GP_7_10): (MUX UNCLAIMED) e6061980.gpio:687
  pin 269 (GP_8_13): (MUX UNCLAIMED) e6068180.gpio:711

The GP_0_1 pin seems to behave just like the other gpio pins in the system.

I agree with all other comments, thanks!

> 
> > +	};
> > +
> > +	reg_cam_j1_dummy: reg_cam_j1_dummy {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "reg_cam_j1_dummy";
> > +	};
> > +};
> > +
> > +&i2c1 {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	status = "okay";
> > +
> > +	imx219_j1: imx219@10 {
> 
> cam@10, and drop the label (it's unused).
> 
> > +		compatible = "sony,imx219";
> > +		reg = <0x10>;
> > +		status = "okay";
> 
> Drop status.
> 
> > +
> > +		clocks = <&clk_cam_j1>;
> > +
> > +		VANA-supply = <&reg_cam_j1>;
> > +		VDIG-supply = <&reg_cam_j1_dummy>;
> > +		VDDL-supply = <&reg_cam_j1_dummy>;
> 
> You could use the same regulator for the three supplies, and drop
> reg_cam_j1_dummy.
> 
> > +
> > +		orientation = <2>;
> > +		rotation = <0>;
> > +
> > +		port {
> > +			imx219_j1_out: endpoint {
> > +				clock-noncontinuous;
> > +				link-frequencies = /bits/ 64 <456000000>;
> > +				data-lanes = <1 2>;
> > +				remote-endpoint = <&csi40_in>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +/* Page 29 / CSI_IF_CN */
> > +&csi40 {
> > +	status = "okay";
> > +
> > +	ports {
> > +		port {
> > +			csi40_in: endpoint {
> > +				bus-type = <MEDIA_BUS_TYPE_CSI2_DPHY>;
> > +				clock-lanes = <0>;
> > +				data-lanes = <1 2>;
> > +				remote-endpoint = <&imx219_j1_out>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&isp0 {
> > +	status = "okay";
> > +};
> > +
> > +&vin00 {
> > +	status = "okay";
> > +};
> > +
> > +&vin01 {
> > +	status = "okay";
> > +};
> > +
> > +&vin02 {
> > +	status = "okay";
> > +};
> > +
> > +&vin03 {
> > +	status = "okay";
> > +};
> > +
> > +&vin04 {
> > +	status = "okay";
> > +};
> > +
> > +&vin05 {
> > +	status = "okay";
> > +};
> > +
> > +&vin06 {
> > +	status = "okay";
> > +};
> > +
> > +&vin07 {
> > +	status = "okay";
> > +};
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Kind Regards,
Niklas Söderlund