Re: [PATCH v2 5/6] arm64: dts: qcom: apq8016-sbc-d3-camera-mezzanine: Move default ov5640 to a standalone dts

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/08/2023 16:11, Stephan Gerhold wrote:
Hi,

just some nitpicks. Some of them were present before already but maybe
you can prepend or append another cleanup patch while at it. :)

On Wed, Aug 09, 2023 at 09:23:42PM +0100, Bryan O'Donoghue wrote:
At the moment we define a single ov5640 sensor in the apq8016-sbc and
disable that sensor.

The sensor mezzanine for this is a D3 Engineering Dual ov5640 mezzanine
card. Move the definition from the apq8016-sbc where it shouldn't be to a
standalone dts.


I wonder what would be required to implement this using a DT overlay,
rather than a standalone separate DT? Seems like there are some .dtso
files in upstream Linux.

I'm also fine with the separate DTB personally, though.

So, we've discussed that previously and its a good model, which I like and which works well for RPI as an example.

AFAIK though the runtime dtbo overlay is still missing at least one upstream commit and the state of dtbo in qcom bootloaders is variable, probably good in LK, good in u-boot and then I'd say nothing doing.

I'm hoping to transition the mezzanine dtb files to something "generic" for boards that support 96 boards interfaces.

Its a bit out of scope for this series as, all I really want to do here is fixup obvious errors as I find them in camss and its dtbs.

So anyway the idea would be to define labels in the core board dts files for stuff like "powerdown-gpios = <&tlmm 34 GPIO_ACTIVE_HIGH>;" I'm not sure that's really feasible until its tried though.

Basically any mezzanine board would ideally only be defined once, with 96boards supporting baseboards providing the necessary additional detail on pins and regulators for the mezzanine to consume..

Come to think of it though you'd have to #include "myboard.dts" so maybe, probably, that idea not feasible.

dtbo would be better still but like I say I'm not presupposing a decent bootloader that can apply the overlay.

I/we will look again at dtbo since its just a neater model really.

Enables the sensor by default, as we are adding a standalone mezzanine
structure.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
---
  arch/arm64/boot/dts/qcom/Makefile             |  1 +
  .../qcom/apq8016-sbc-d3-camera-mezzanine.dts  | 55 +++++++++++++++++++
  arch/arm64/boot/dts/qcom/apq8016-sbc.dts      | 49 -----------------
  3 files changed, 56 insertions(+), 49 deletions(-)
  create mode 100644 arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts

diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
index f15548dbfa56e..19016765ba4c6 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -1,5 +1,6 @@
  # SPDX-License-Identifier: GPL-2.0
  dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc.dtb
+dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc-d3-camera-mezzanine.dtb
  dtb-$(CONFIG_ARCH_QCOM)	+= apq8039-t2.dtb
  dtb-$(CONFIG_ARCH_QCOM)	+= apq8094-sony-xperia-kitakami-karin_windy.dtb
  dtb-$(CONFIG_ARCH_QCOM)	+= apq8096-db820c.dtb
diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
new file mode 100644
index 0000000000000..ef0e76e424898
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2023, Linaro Ltd.
+ */

I assume you have permission from the original contributor to relicense
this? apq8016-sbc.dts is GPL.

Eh it was Loic @ Linaro but, TBH I just added a boilerplate here ...

No you're right this should be // SPDX-License-Identifier: GPL-2.0-only


+
+/dts-v1/;
+
+#include "apq8016-sbc.dts"
+

Please also move the fixed regulators here, they're part of the
mezzanine, not the DB410c.

ack, true


+&camss {
+	status = "okay";
+
+	ports {
+		port@0 {
+			reg = <0>;
+			csiphy0_ep: endpoint {
+				data-lanes = <0 2>;
+				remote-endpoint = <&ov5640_ep>;
+				status = "okay";

Should be unneeded since it's not set to disabled anywhere?

+			};
+		};
+	};
+};
+
+&cci {
+	status = "okay";
+};
+
+&cci_i2c0 {
+	camera_rear@3b {

camera@

sure

---
bod




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux