Re: [PATCH 2/2] arm64: dts: ti: k3-j722s-evm: Add overlay for TEVI OV5640

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

Hi Udit,
Thanks for the review.

On 07/05/25 13:50, Kumar, Udit wrote:
> Hello Vaishnav/Abhilash
> 
> Thanks for patch
> 
> On 5/5/2025 5:27 PM, Yemike Abhilash Chandra wrote:
>> From: Vaishnav Achath <vaishnav.a@ti.com>
>>
>> TechNexion TEVI OV5640 camera is a 5MP camera that can be used with
>> J722S EVM through the 22-pin CSI-RX connector. Add a reference overlay
>> for quad TEVI OV5640 modules on J722S EVM.
>>
>> Signed-off-by: Vaishnav Achath <vaishnav.a@ti.com>
>> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> ---
>>   arch/arm64/boot/dts/ti/Makefile               |   4 +
>>   .../k3-j722s-evm-csi2-quad-tevi-ov5640.dtso   | 358 ++++++++++++++++++
>>   2 files changed, 362 insertions(+)
>>   create mode 100644 
>> arch/arm64/boot/dts/ti/k3-j722s-evm-csi2-quad-tevi-ov5640.dtso
>>
>> diff --git a/arch/arm64/boot/dts/ti/Makefile 
>> b/arch/arm64/boot/dts/ti/Makefile
>> index 829a3b028466..76b750e4b8a8 100644
>> --- a/arch/arm64/boot/dts/ti/Makefile
>> +++ b/arch/arm64/boot/dts/ti/Makefile
>> @@ -123,6 +123,7 @@ dtb-$(CONFIG_ARCH_K3) += k3-j721s2-evm-pcie1-ep.dtbo
>>   dtb-$(CONFIG_ARCH_K3) += k3-am67a-beagley-ai.dtb
>>   dtb-$(CONFIG_ARCH_K3) += k3-j722s-evm.dtb
>>   dtb-$(CONFIG_ARCH_K3) += k3-j722s-evm-csi2-quad-rpi-cam-imx219.dtbo
>> [..]
>> diff --git 
>> a/arch/arm64/boot/dts/ti/k3-j722s-evm-csi2-quad-tevi-ov5640.dtso 
>> b/arch/arm64/boot/dts/ti/k3-j722s-evm-csi2-quad-tevi-ov5640.dtso
>> new file mode 100644
>> index 000000000000..537224ea60e3
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/ti/k3-j722s-evm-csi2-quad-tevi-ov5640.dtso
>> @@ -0,0 +1,358 @@
>> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
>> +/*
>> + * 4 x TEVI OV5640 MIPI Camera module on RPI camera connector.
>> + *
>> + * Copyright (C) 2024 Texas Instruments Incorporated - 
>> https://www.ti.com/
>> + */
>> +
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include "k3-pinctrl.h"
>> +
>> +&{/} {
>> +    clk_ov5640_fixed: clock-24000000 {
>> +        compatible = "fixed-clock";
>> +        #clock-cells = <0>;
>> +        clock-frequency = <24000000>;
>> +    };
>> +
> 
> Please check once , this is clock is 25M or 24M .
> 
> As I see CDC6CE025000ADLXT/U28 is marked as 25M OSC
> 

This is the crystal clock with in the sensor.

>> +    reg_2p8v: regulator-2p8v {
>> +        compatible = "regulator-fixed";
>> +        regulator-name = "2P8V";
>> +        regulator-min-microvolt = <2800000>;
>> +        regulator-max-microvolt = <2800000>;
>> +        vin-supply = <&vdd_sd_dv>;
>> +        regulator-always-on;
>> +    };
>> +
>> +    reg_1p8v: regulator-1p8v {
>> +        compatible = "regulator-fixed";
>> +        regulator-name = "1P8V";
>> +        regulator-min-microvolt = <1800000>;
>> +        regulator-max-microvolt = <1800000>;
>> +        vin-supply = <&vdd_sd_dv>;
>> +        regulator-always-on;
>> +    };
>> +
>> +    reg_3p3v: regulator-3p3v {
>> +        compatible = "regulator-fixed";
>> +        regulator-name = "3P3V";
>> +        regulator-min-microvolt = <3300000>;
>> +        regulator-max-microvolt = <3300000>;
>> +        vin-supply = <&vdd_sd_dv>;
>> +        regulator-always-on;
>> +    };
> 
> Sorry but on connector QSH-020-01-L-D-DP-A-K/J27, I see only 3v3 available.
> 
> May be I am missing something here but how you are getting 1v8 and 2v8 
> supply
> 

Yes, 3.3V is provided as the main input supply to the sensor.
However, required 1.8V and 2.8V rails from the 3.3V input.
Please refer to [1] for detailed information.

> 
>> +};
>> +
>> +
>> +&main_pmx0 {
>> +    cam0_reset_pins_default: cam0-default-reset-pins {
>> +        pinctrl-single,pins = <
>> +            J722S_IOPAD(0x03c, PIN_OUTPUT, 7)
>> +        >;
>> +    };
>> +
> 
> Please mark GPIO you are using for this in comment
> 
> example
> 
> J722S_IOPAD(0x1dc, PIN_INPUT, 0) /* (C22) MCAN0_RX */
> 

Sure. Will do that.

> 
>> +    cam1_reset_pins_default: cam1-default-reset-pins {
>> +        pinctrl-single,pins = <
>> +            J722S_IOPAD(0x044, PIN_OUTPUT, 7)
>> +        >;
>> +    };
>> +
>> +    cam2_reset_pins_default: cam2-default-reset-pins {
>> +        pinctrl-single,pins = <
>> +            J722S_IOPAD(0x04c, PIN_OUTPUT, 7)
>> +        >;
>> +    };
>> +
>> +    cam3_reset_pins_default: cam3-default-reset-pins {
>> +        pinctrl-single,pins = <
>> +            J722S_IOPAD(0x054, PIN_OUTPUT, 7)
>> +        >;
>> +    };
>> +};
>> +
>> +&exp1 {
>> +    p06-hog{
>> +        /* P06 - CSI01_MUX_SEL_2 */
>> +        gpio-hog;
>> +        gpios = <6 GPIO_ACTIVE_HIGH>;
>> +        output-high;
>> +        line-name = "CSI01_MUX_SEL_2";
> 
> As per table on page 29
> 
> CSI01_MUX_SEL_2 as low means, INPUT<-- A Port [CSI2 Connector]
> 
> Please check, if you want to drive this line high or low

These are RPI connectors, It should be high.

> 
>> +    };
>> +
>> +    p07-hog{
>> +        /* P01 - CSI23_MUX_SEL_2 */
>> +        gpio-hog;
>> +        gpios = <7 GPIO_ACTIVE_HIGH>;
>> +        output-high;
>> +        line-name = "CSI23_MUX_SEL_2";
>> +    };
> 
> Same as for CSI01_MUX_SEL_2, table on page-30
> 
> 
>> +};
>> +
>> +&main_gpio0 {
>> +    p15-hog {
>> +        /* P15 - CSI2_CAMERA_GPIO1 */
>> +        gpio-hog;
>> +        gpios = <15 GPIO_ACTIVE_HIGH>;
>> +        output-high;
>> +        line-name = "CSI2_CAMERA_GPIO1";
>> +    };
>> +
>> +    p17-hog {
>> +        /* P17 - CSI2_CAMERA_GPIO2 */
>> +        gpio-hog;
>> +        gpios = <17 GPIO_ACTIVE_HIGH>;
>> +        output-high;
>> +        line-name = "CSI2_CAMERA_GPIO2";
>> +    };
>> +
>> +    p19-hog {
>> +        /* P19 - CSI2_CAMERA_GPIO3 */
>> +        gpio-hog;
>> +        gpios = <19 GPIO_ACTIVE_HIGH>;
>> +        output-high;
>> +        line-name = "CSI2_CAMERA_GPIO3";
>> +    };
>> +
>> +    p21-hog {
>> +        /* P21 - CSI2_CAMERA_GPIO4 */
>> +        gpio-hog;
>> +        gpios = <21 GPIO_ACTIVE_HIGH>;
>> +        output-high;
>> +        line-name = "CSI2_CAMERA_GPIO4";
>> +    };
> 
> Please check once if P21 and rest above are pin number of SOC ?
> 

Yes

> 
>> +};
>> +
>> +&pca9543_0 {
>> +    #address-cells = <1>;
>> +    #size-cells = <0>;
>> +
>> +    i2c@0 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        reg = <0>;
>> +
>> +        ov5640_0: camera@3c {
>> +            compatible = "ovti,ov5640";
>> +            reg = <0x3c>;
>> +            clocks = <&clk_ov5640_fixed>;
>> +            clock-names = "xclk";
>> +
>> +            AVDD-supply = <&reg_2p8v>;
>> +            DOVDD-supply = <&reg_1p8v>;
>> +            DVDD-supply = <&reg_3p3v>;
>> +
>> +            pinctrl-names = "default";
>> +            pinctrl-0 = <&cam0_reset_pins_default>;
> 
> do you think, you should have reset-gpios as property for this ov5640 
> and other nodes too ?
> 

Thanks for pointing this out. I will add that in the next revision.

Thanks and Regards
Yemike Abhilash Chandra


[1]:https://www.technexion.com/wp-content/uploads/2023/09/product-brief_tevi-ov5640.pdf

> 
>> +
>> [..]