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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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