| Message ID | 20230809202343.1098425-6-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | apq8016: camss: Update dts with various fixes | expand |
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. > Enables the sensor by default, as we are adding a standalone mezzanine > structure. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > 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. > + > +/dts-v1/; > + > +#include "apq8016-sbc.dts" > + Please also move the fixed regulators here, they're part of the mezzanine, not the DB410c. > +&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@ Thanks, Stephan
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@linaro.org> >> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> 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
On 10/08/2023 16:26, Bryan O'Donoghue wrote:
> probably good in LK
lk2nd obvs !
On Thu, Aug 10, 2023 at 04:26:34PM +0100, Bryan O'Donoghue wrote: > 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. > AFAIU there is work going on (at Linaro?) to move the qcom RBs to use U-Boot, so I guess that would be the easiest common base to work with. There is an U-Boot port for DB410c as well. > 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. > Right, yeah I think it's fine to have the separate DTB for now. I was always confused about the disabled camera parts in apq8016-sbc, having it in a separate DTB is definitely less confusing. :) > 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. Handling the GPIOs sounds complicated... I think it would be also okay to have board-specific mezzanine overlays as a first step though. (i.e. one for DB410c, others for other compatible 96boards). > > 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. > Thanks, I'm looking forward to seeing what you come up with. :D Stephan
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. + */ + +/dts-v1/; + +#include "apq8016-sbc.dts" + +&camss { + status = "okay"; + + ports { + port@0 { + reg = <0>; + csiphy0_ep: endpoint { + data-lanes = <0 2>; + remote-endpoint = <&ov5640_ep>; + status = "okay"; + }; + }; + }; +}; + +&cci { + status = "okay"; +}; + +&cci_i2c0 { + camera_rear@3b { + compatible = "ovti,ov5640"; + reg = <0x3b>; + + powerdown-gpios = <&tlmm 34 GPIO_ACTIVE_HIGH>; + reset-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>; + pinctrl-names = "default"; + pinctrl-0 = <&camera_rear_default>; + + clocks = <&gcc GCC_CAMSS_MCLK0_CLK>; + clock-names = "xclk"; + assigned-clocks = <&gcc GCC_CAMSS_MCLK0_CLK>; + assigned-clock-rates = <23880000>; + + DOVDD-supply = <&camera_vdddo_1v8>; + AVDD-supply = <&camera_vdda_2v8>; + DVDD-supply = <&camera_vddd_1v5>; + + port { + ov5640_ep: endpoint { + data-lanes = <1 2>; + remote-endpoint = <&csiphy0_ep>; + }; + }; + }; +}; diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts index ddb19709a9eee..84641925f3329 100644 --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dts +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dts @@ -250,55 +250,6 @@ &blsp_uart2 { label = "LS-UART1"; }; -&camss { - status = "okay"; - ports { - port@0 { - reg = <0>; - csiphy0_ep: endpoint { - data-lanes = <0 2>; - remote-endpoint = <&ov5640_ep>; - status = "okay"; - }; - }; - }; -}; - -&cci { - status = "okay"; -}; - -&cci_i2c0 { - camera_rear@3b { - compatible = "ovti,ov5640"; - reg = <0x3b>; - - powerdown-gpios = <&tlmm 34 GPIO_ACTIVE_HIGH>; - reset-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>; - pinctrl-names = "default"; - pinctrl-0 = <&camera_rear_default>; - - clocks = <&gcc GCC_CAMSS_MCLK0_CLK>; - clock-names = "xclk"; - assigned-clocks = <&gcc GCC_CAMSS_MCLK0_CLK>; - assigned-clock-rates = <23880000>; - - DOVDD-supply = <&camera_vdddo_1v8>; - AVDD-supply = <&camera_vdda_2v8>; - DVDD-supply = <&camera_vddd_1v5>; - - /* No camera mezzanine by default */ - status = "disabled"; - - port { - ov5640_ep: endpoint { - data-lanes = <1 2>; - remote-endpoint = <&csiphy0_ep>; - }; - }; - }; -}; - &lpass { status = "okay"; };