Re: [v1 5/6] arm64: dts: sdm845-db845c: Add ov8856 & ov7251 camera nodes

From: Bjorn Andersson
Date: Thu Mar 12 2020 - 01:35:02 EST

Next message: Amol Grover: "[PATCH] net: caif: Add lockdep expression to RCU traversal primitive"
Previous message: Chen-Yu Tsai: "Re: [linux-sunxi] [PATCH 0/8] media: sunxi: Add DE2 rotate driver"
In reply to: Robert Foss: "[v1 5/6] arm64: dts: sdm845-db845c: Add ov8856 & ov7251 camera nodes"
Next in thread: Robert Foss: "[v1 1/6] arm64: dts: msm8916: Add i2c-qcom-cci node"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed 11 Mar 05:35 PDT 2020, Robert Foss wrote:

> Enable the ov8856 main camera and the ov7251 b/w tracking camera
> used on the Qualcomm RB3 kit.
>
> Currently the camera nodes have not yet been attached to an to a
> CSI2 endpoint, since no driver currently supports the ISP that the the
> SDM845/db845c ships with.
>
> Signed-off-by: Robert Foss <robert.foss@xxxxxxxxxx>
> ---
> arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 239 +++++++++++++++++++++
> 1 file changed, 239 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> index e8c056d02ace..660550197ce9 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
> @@ -110,6 +110,53 @@
> // enable-active-high;
> };
>
> + cam0_dvdd_1v2: reg_cam0_dvdd_1v2 {

cam0_dvdd_1v2: cam0-dvdd-1v2 {

> + compatible = "regulator-fixed";
> + regulator-name = "CAM0_DVDD_1V2";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + enable-active-high;
> + gpio = <&pm8998_gpio 12 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&cam0_dvdd_1v2_en_default>;
> + vin-supply = <&vbat>;
> + };
> +
> + cam0_avdd_2v8: reg_cam0_avdd_2v8 {
> + compatible = "regulator-fixed";
> + regulator-name = "CAM0_AVDD_2V8";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + enable-active-high;
> + gpio = <&pm8998_gpio 10 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&cam0_avdd_2v8_en_default>;
> + vin-supply = <&vbat>;
> + };
> +
> + /* This regulator is enabled when the VREG_LVS1A_1P8 trace is enabled */
> + cam3_avdd_2v8: reg_cam3_avdd_2v8 {
> + compatible = "regulator-fixed";
> + regulator-name = "CAM3_AVDD_2V8";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + regulator-always-on;
> + vin-supply = <&vbat>;
> + };
> +
> + /* This regulator does not really exits, but a 'vddd-supply' is
> + * required for the ov7251 driver, but no 'vddd' regulator is used
> + * in the schematic
> + */

Looking at the driver you should be able to just omit vddd-supply from
the DT node, in which case the driver will get a dummy regulator and
should function properly.

Presumably you can then skip defining this dummy regulator as well.

> + cam3_vddd_1v2: reg_cam3_vddd_1v2 {
> + compatible = "regulator-fixed";
> + regulator-name = "CAM3_VDDD_1V2_DUMMY";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + vin-supply = <&vbat>;
> + };
> +
> pcie0_3p3v_dual: vldo-3v3-regulator {
> compatible = "regulator-fixed";
> regulator-name = "VLDO_3V3";
> @@ -406,6 +453,81 @@
> };
>
> &tlmm {
> + pcie0_default_state: pcie0-default {
> + clkreq {
> + pins = "gpio36";
> + function = "pci_e0";
> + bias-pull-up;
> + };
> +
> + reset-n {
> + pins = "gpio35";
> + function = "gpio";
> +
> + drive-strength = <2>;
> + output-low;
> + bias-pull-down;
> + };
> +
> + wake-n {
> + pins = "gpio37";
> + function = "gpio";
> +
> + drive-strength = <2>;
> + bias-pull-up;
> + };
> + };

This looks like leftovers from your workspace.

> +
> + cam0_default: cam0_default {
> + mux_rst {

Please combine *_rst into one "rst" subnode and *_mclk0 into a mclk {}.

> + function = "gpio";
> + pins = "gpio9";
> + };
> + config_rst {
> + pins = "gpio9";
> + drive-strength = <16>;
> + bias-disable;
> + };
> +
> + mux_mclk0 {
> + function = "cam_mclk";
> + pins = "gpio13";
> + };
> + config_mclk0 {
> + pins = "gpio13";
> + drive-strength = <16>;
> + bias-disable;
> + };
> + };
> +
> + cam3_default: cam3_default {

Ditto.

Also, please check the indentation of this block.

> + mux_rst {
> + function = "gpio";
> + pins = "gpio21";
> + };
> + config_rst {
> + pins = "gpio21";
> + drive-strength = <16>;
> + bias-disable;
> + };
> +
> + mux_mclk3 {
> + function = "cam_mclk";
> + pins = "gpio16";
> + };
> + config_mclk3 {
> + pins = "gpio16";
> + drive-strength = <16>;
> + bias-disable;
> + };
> + };
> +
> + lt9611_irq_pin: lt9611-irq {
> + pins = "gpio84";
> + function = "gpio";
> + bias-disable;
> + };

This node shouldn't be here either.

> +
> pcie0_pwren_state: pcie0-pwren {
> pins = "gpio90";
> function = "gpio";
> @@ -612,8 +734,125 @@
> "PM845_GPIO24",
> "OPTION2",
> "PM845_SLB";
> +
> + cam0_dvdd_1v2_en_default: cam0_dvdd_1v2_en_pinctrl {

Use '-' in the node name, and you can drop the _pinctrl suffix, given
that the name only has to be unique in this parent node.

> + pins = "gpio12";
> + function = "normal";
> +
> + bias-pull-up;
> + drive-push-pull;
> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> + };
> +
> + cam0_avdd_2v8_en_default: cam0_avdd_2v8_en_pinctrl {
> + pins = "gpio10";
> + function = "normal";
> +
> + bias-pull-up;
> + drive-push-pull;
> + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
> + };
> };
>
> &cci {
> status = "ok";
> +
> + i2c-bus@0 {

Please reference this by label instead.

> + cam0@10 {

camera@10

> + compatible = "ovti,ov8856";
> +
> + /* The Qualcomm RB3 camera mezzanine schematic lists
> + * 0x20 as I2C address of this device, but the Linux
> + * kernel documentation lists 0x10 I2C address.
> + */

This is a normal discrepancy in how different people lists
addresses. Feel free to omit this comment.

> + reg = <0x10>;
> +
> + // CAM0_RST_N
> + reset-gpios = <&tlmm 9 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&cam0_default>;
> + gpios = <&tlmm 13 0>,
> + <&tlmm 9 0>;

s/0/GPIO_ACTIVE_HIGH/g

> +
> + clocks = <&clock_camcc CAM_CC_MCLK0_CLK>;
> + clock-names = "xvclk";
> + clock-frequency = <19200000>;
> +
> +

Extra newline.

> + /* The &vreg_s4a_1p8 trace is powered on as a
> + * part of the TITAN_TOP_GDSC power domain.

Rather vreg_s4a_1p8 is simply always on, unrelated to TITAN_TOP_GDSC.
The GDSC is likely to control the power to the CCI controller itself
though.

> + * So it is represented by a fixed regulator.
> + *
> + * The 2.8V vdda-supply and 1.2V vddd-supply regulators
> + * both have to be enabled through the power management
> + * gpios.
> + */
> + power-domains = <&clock_camcc TITAN_TOP_GDSC>;
> +
> + dovdd-supply = <&vreg_lvs1a_1p8>;
> + avdd-supply = <&cam0_avdd_2v8>;
> + dvdd-supply = <&cam0_dvdd_1v2>;
> +
> + /* No camera mezzanine by default */

I think it's fine to assume that everyone has their mezzanine mounted on
their db845c, so feel free to omit this comment and the status below.

(Assuming that not having the camera connected will be handled somewhat
gracefully)

Given though that we're lacking the rest of the camera subsystem it
might be suitable to status = "disable" this for now.

> + status = "ok";
> +
> + port {
> + ov8856_ep: endpoint {
> + clock-lanes = <1>;
> + link-frequencies = /bits/ 64
> + <360000000 180000000>;
> + data-lanes = <1 2 3 4>;
> +// remote-endpoint = <&csiphy0_ep>;
> + };
> + };
> + };
> + };
> +
> + i2c-bus@1 {

Same comments as for the first bus...

Regards,
Bjorn

> + cam3@60 {
> + compatible = "ovti,ov7251";
> +
> + // I2C address as per ov7251.txt linux documentation
> + reg = <0x60>;
> +
> + // CAM3_RST_N
> + enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&cam3_default>;
> + gpios = <&tlmm 16 0>,
> + <&tlmm 21 0>;
> +
> + clocks = <&clock_camcc CAM_CC_MCLK3_CLK>;
> + clock-names = "xclk";
> + clock-frequency = <24000000>;
> +
> + /* The &vreg_s4a_1p8 trace is powered on as a
> + * part of the TITAN_TOP_GDSC power domain.
> + * So it is represented by a fixed regulator.
> + *
> + * The 2.8V vdda-supply regulator is enabled when the
> + * vreg_s4a_1p8 trace is pulled high.
> + * It too is represented by a fixed regulator.
> + *
> + * No 1.2V vddd-supply regulator is used, a fixed
> + * regulator represents it.
> + */
> + power-domains = <&clock_camcc TITAN_TOP_GDSC>;
> +
> + vdddo-supply = <&vreg_lvs1a_1p8>;
> + vdda-supply = <&cam3_avdd_2v8>;
> + vddd-supply = <&cam3_vddd_1v2>;
> +
> + /* No camera mezzanine by default */
> + status = "ok";
> +
> + port {
> + ov7251_ep: endpoint {
> + clock-lanes = <1>;
> + data-lanes = <0 1>;
> +// remote-endpoint = <&csiphy3_ep>;
> + };
> + };
> + };
> + };
> };
> --
> 2.20.1
>

Next message: Amol Grover: "[PATCH] net: caif: Add lockdep expression to RCU traversal primitive"
Previous message: Chen-Yu Tsai: "Re: [linux-sunxi] [PATCH 0/8] media: sunxi: Add DE2 rotate driver"
In reply to: Robert Foss: "[v1 5/6] arm64: dts: sdm845-db845c: Add ov8856 & ov7251 camera nodes"
Next in thread: Robert Foss: "[v1 1/6] arm64: dts: msm8916: Add i2c-qcom-cci node"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]