| Message ID | 20241217140656.965235-5-quic_vikramsa@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: qcom: camss: Add sc7280 support | expand |
On 17.12.2024 3:40 PM, Vladimir Zapolskiy wrote: > On 12/17/24 16:06, Vikram Sharma wrote: >> The Vision Mezzanine for the RB3 ships with an imx577 camera sensor. >> Enable the IMX577 on the vision mezzanine. >> >> An example media-ctl pipeline for the imx577 is: >> >> media-ctl --reset >> media-ctl -v -V '"imx577 '19-001a'":0[fmt:SRGGB10/4056x3040 field:none]' >> media-ctl -V '"msm_csiphy3":0[fmt:SRGGB10/4056x3040]' >> media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4056x3040]' >> media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4056x3040]' >> media-ctl -l '"msm_csiphy3":1->"msm_csid0":0[1]' >> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]' >> >> yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F /dev/video0 >> >> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com> >> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >> --- [...] >> + rst-pins { >> + pins = "gpio78"; >> + function = "gpio"; >> + drive-strength = <2>; >> + bias-pull-down; >> + output-low; >> + }; > > I have doubts that it's proper to embed a reset gpio into driver's > pinctrl suspend/resume power management. > > Konrad, can you please confirm that it's really accepted? > > I'd rather ask to remove this reset pin control. There's certainly some appearances of this in the tree. You could make the argument that it makes sense to prevent misconfiguration (i.e. the bootloader may set the pin in input mode), but then the counter argument is that the (Linux) gpiod APIs request OUT_LOW/HIGH, and we would expect that the driver uses that if the GPIO is requested through e.g. reset-gpios. I'm not particularly sure what to recommend here. Krzysztof? Konrad
On 12/19/24 21:06, Konrad Dybcio wrote: > On 17.12.2024 3:40 PM, Vladimir Zapolskiy wrote: >> On 12/17/24 16:06, Vikram Sharma wrote: >>> The Vision Mezzanine for the RB3 ships with an imx577 camera sensor. >>> Enable the IMX577 on the vision mezzanine. >>> >>> An example media-ctl pipeline for the imx577 is: >>> >>> media-ctl --reset >>> media-ctl -v -V '"imx577 '19-001a'":0[fmt:SRGGB10/4056x3040 field:none]' >>> media-ctl -V '"msm_csiphy3":0[fmt:SRGGB10/4056x3040]' >>> media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4056x3040]' >>> media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4056x3040]' >>> media-ctl -l '"msm_csiphy3":1->"msm_csid0":0[1]' >>> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]' >>> >>> yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F /dev/video0 >>> >>> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com> >>> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >>> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >>> --- > > [...] > >>> + rst-pins { >>> + pins = "gpio78"; >>> + function = "gpio"; >>> + drive-strength = <2>; >>> + bias-pull-down; >>> + output-low; >>> + }; >> >> I have doubts that it's proper to embed a reset gpio into driver's >> pinctrl suspend/resume power management. >> >> Konrad, can you please confirm that it's really accepted? >> >> I'd rather ask to remove this reset pin control. > > There's certainly some appearances of this in the tree. > > You could make the argument that it makes sense to prevent misconfiguration > (i.e. the bootloader may set the pin in input mode), but then the counter > argument is that the (Linux) gpiod APIs request OUT_LOW/HIGH, and we would > expect that the driver uses that if the GPIO is requested through > e.g. reset-gpios. > > I'm not particularly sure what to recommend here. Krzysztof? > I'm worried by a possibility that a device reset/shutdown control GPIO could be turned off by entering the "sleep" pinctrl setup. If a particular GPIO/pin is off, is it still continuously functional as a control GPIO of some device? I believe it is not anymore in general, this is my concern here. -- Best wishes, Vladimir
diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile index 4686f2a8ddd8..a7e88fcabded 100644 --- a/arch/arm64/boot/dts/qcom/Makefile +++ b/arch/arm64/boot/dts/qcom/Makefile @@ -115,6 +115,10 @@ dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-4000.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs615-ride.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2.dtb + +qcs6490-rb3gen2-vision-mezzanine-dtbs := qcs6490-rb3gen2.dtb qcs6490-rb3gen2-vision-mezzanine.dtbo + +dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2-vision-mezzanine.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs8300-ride.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs8550-aim300-aiot.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs9100-ride.dtb diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso new file mode 100644 index 000000000000..7782c4aee576 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. + */ + +/* + * Camera Sensor overlay on top of rb3gen2 core kit. + */ + +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/clock/qcom,camcc-sc7280.h> + +/dts-v1/; +/plugin/; + +&camss { + vdda-phy-supply = <&vreg_l10c_0p88>; + vdda-pll-supply = <&vreg_l6b_1p2>; + + status = "okay"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + /* The port index denotes CSIPHY id i.e. csiphy3 */ + port@3 { + reg = <3>; + + csiphy3_ep: endpoint { + clock-lanes = <7>; + data-lanes = <0 1 2 3>; + remote-endpoint = <&imx577_ep>; + }; + }; + }; +}; + +&cci1 { + status = "okay"; +}; + +&cci1_i2c1 { + #address-cells = <1>; + #size-cells = <0>; + + camera@1a { + compatible = "sony,imx577"; + + reg = <0x1a>; + + reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>; + pinctrl-names = "default", "suspend"; + pinctrl-0 = <&cam2_default>; + pinctrl-1 = <&cam2_suspend>; + + clocks = <&camcc CAM_CC_MCLK3_CLK>; + assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>; + assigned-clock-rates = <24000000>; + + dovdd-supply = <&vreg_l18b_1p8>; + avdd-supply = <&vph_pwr>; + dvdd-supply = <&vph_pwr>; + + port { + imx577_ep: endpoint { + clock-lanes = <7>; + link-frequencies = /bits/ 64 <600000000>; + data-lanes = <0 1 2 3>; + remote-endpoint = <&csiphy3_ep>; + }; + }; + }; +}; + +&tlmm { + cam2_default: cam2-default-state { + mclk-pins { + pins = "gpio67"; + function = "cam_mclk"; + drive-strength = <2>; + bias-disable; + }; + + rst-pins { + pins = "gpio78"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + }; + + cam2_suspend: cam2-suspend-state { + mclk-pins { + pins = "gpio67"; + function = "cam_mclk"; + drive-strength = <2>; + bias-pull-down; + }; + + rst-pins { + pins = "gpio78"; + function = "gpio"; + drive-strength = <2>; + bias-pull-down; + output-low; + }; + }; +};