diff mbox series

[3/3] arm64: dts: mediatek: mt8365-pumpkin: Add overlays for thp7312 cameras

Message ID 20230905233118.183140-4-paul.elder@ideasonboard.com
State New
Headers show
Series media: i2c: Add driver for THine THP7312 ISP | expand

Commit Message

Paul Elder Sept. 5, 2023, 11:31 p.m. UTC 
Add overlays for the Pumpkin i350 to support THP7312 cameras.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 arch/arm64/boot/dts/mediatek/Makefile         |  4 +
 .../mt8365-pumpkin-common-thp7312.dtsi        | 23 ++++++
 .../mt8365-pumpkin-csi0-thp7312-imx258.dtso   | 73 +++++++++++++++++++
 .../mt8365-pumpkin-csi1-thp7312-imx258.dtso   | 73 +++++++++++++++++++
 4 files changed, 173 insertions(+)
 create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
 create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
 create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi1-thp7312-imx258.dtso

Comments

Krzysztof Kozlowski Sept. 6, 2023, 7:27 a.m. UTC | #1 
On 06/09/2023 01:31, Paul Elder wrote:
> Add overlays for the Pumpkin i350 to support THP7312 cameras.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  arch/arm64/boot/dts/mediatek/Makefile         |  4 +
>  .../mt8365-pumpkin-common-thp7312.dtsi        | 23 ++++++
>  .../mt8365-pumpkin-csi0-thp7312-imx258.dtso   | 73 +++++++++++++++++++
>  .../mt8365-pumpkin-csi1-thp7312-imx258.dtso   | 73 +++++++++++++++++++
>  4 files changed, 173 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi1-thp7312-imx258.dtso
> 
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index 20570bc40de8..ceaf24105001 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -56,4 +56,8 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-evk.dtb
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-pumpkin.dtb
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8516-pumpkin.dtb
>  
> +mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi0-thp7312-imx258.dtbo
> +mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi1-thp7312-imx258.dtbo
>  mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-ethernet-usb.dtbo
> +
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mtk-mt8365-pumpkin.dtb
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
> new file mode 100644
> index 000000000000..478697552617
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023 Ideas on Board
> + * Author: Paul Elder <paul.elder@ideasonboard.com>
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +&{/} {
> +	vsys_v4p2: regulator@0 {

Hm? Is this a bus?

> +		compatible = "regulator-fixed";
> +		regulator-name = "vsys-v4p2";
> +		regulator-min-microvolt = <4200000>;
> +		regulator-max-microvolt = <4200000>;
> +	};
> +
> +	camera61_clk: cam_clk24m {

And this is not on a bus? It's the same / node!

Please work on mainline, which means take mainline code and change it to
your needs. Do not take downstream poor code and change it...

No underscores in node names. Also generic node names, so at least with
generic prefix or suffix.


> +		compatible = "fixed-clock";
> +		clock-frequency = <24000000>;
> +		#clock-cells = <0>;
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
> new file mode 100644
> index 000000000000..740d14a19d75
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2023 Ideas on Board
> + * Author: Paul Elder <paul.elder@ideasonboard.com>
> + */
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/pinctrl/mt8365-pinfunc.h>
> +#include "mt8365-pumpkin-common-thp7312.dtsi"
> +
> +&i2c3 {
> +	camera@61 {
> +		compatible = "thine,thp7312";
> +		reg = <0x61>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&cam0_pins_default>;
> +		reset-gpios = <&pio 118 GPIO_ACTIVE_LOW>;
> +		clocks = <&camera61_clk>;
> +
> +		vddcore-supply = <&vsys_v4p2>;
> +		vhtermrx-supply = <&vsys_v4p2>;
> +		vddtx-supply = <&vsys_v4p2>;
> +		vddhost-supply = <&vsys_v4p2>;
> +		vddcmos-supply = <&vsys_v4p2>;
> +		vddgpio_0-supply = <&vsys_v4p2>;
> +		vddgpio_1-supply = <&vsys_v4p2>;
> +		DOVDD-supply = <&vsys_v4p2>;
> +		AVDD-supply = <&vsys_v4p2>;
> +		DVDD-supply = <&vsys_v4p2>;
> +
> +		orientation = <0>;
> +		rotation = <0>;
> +
> +		thine,rx,data-lanes = <4 1 3 2>;

NAK for this property.


> +
> +		port {
> +			isp1_out: endpoint {
> +				remote-endpoint = <&seninf_in1>;
> +				data-lanes = <4 2 1 3>;
> +			};
> +		};
> +	};
> +};
> +
> +&pio {
> +	cam0_pins_default: cam0_pins_default {

No underscores in node names.

> +		pins_rst {

Ditto


Best regards,
Krzysztof
Laurent Pinchart Sept. 6, 2023, 9 a.m. UTC | #2 
On Wed, Sep 06, 2023 at 10:48:23AM +0200, Krzysztof Kozlowski wrote:
> On 06/09/2023 10:32, Laurent Pinchart wrote:
> > On Wed, Sep 06, 2023 at 09:27:07AM +0200, Krzysztof Kozlowski wrote:
> >> On 06/09/2023 01:31, Paul Elder wrote:
> >>> Add overlays for the Pumpkin i350 to support THP7312 cameras.
> >>>
> >>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >>> ---
> >>>  arch/arm64/boot/dts/mediatek/Makefile         |  4 +
> >>>  .../mt8365-pumpkin-common-thp7312.dtsi        | 23 ++++++
> >>>  .../mt8365-pumpkin-csi0-thp7312-imx258.dtso   | 73 +++++++++++++++++++
> >>>  .../mt8365-pumpkin-csi1-thp7312-imx258.dtso   | 73 +++++++++++++++++++
> >>>  4 files changed, 173 insertions(+)
> >>>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
> >>>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
> >>>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi1-thp7312-imx258.dtso
> >>>
> >>> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> >>> index 20570bc40de8..ceaf24105001 100644
> >>> --- a/arch/arm64/boot/dts/mediatek/Makefile
> >>> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> >>> @@ -56,4 +56,8 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-evk.dtb
> >>>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-pumpkin.dtb
> >>>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8516-pumpkin.dtb
> >>>  
> >>> +mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi0-thp7312-imx258.dtbo
> >>> +mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi1-thp7312-imx258.dtbo
> >>>  mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-ethernet-usb.dtbo
> >>> +
> >>> +dtb-$(CONFIG_ARCH_MEDIATEK) += mtk-mt8365-pumpkin.dtb
> >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
> >>> new file mode 100644
> >>> index 000000000000..478697552617
> >>> --- /dev/null
> >>> +++ b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
> >>> @@ -0,0 +1,23 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Copyright (c) 2023 Ideas on Board
> >>> + * Author: Paul Elder <paul.elder@ideasonboard.com>
> >>> + */
> >>> +
> >>> +/dts-v1/;
> >>> +/plugin/;
> >>> +
> >>> +&{/} {
> >>> +	vsys_v4p2: regulator@0 {
> >>
> >> Hm? Is this a bus?
> > 
> > There are multiple instances of "numbered" regulators in upstream DT
> > files, for instance arch/arm/boot/dts/nxp/imx/imx6qdl-nitrogen6_max.dtsi
> 
> That's the only example I saw... I fixed it now.
> 
> > has a regulator@0. There are similar instances for clocks.
> > 
> > I understand why it may not be a good idea, and how the root node is
> > indeed not a bus. In some cases, those regulators and clocks are grouped
> > in a regulators or clocks node that has a "simple-bus" compatible. I'm
> > not sure if that's a good idea, but at least it should validate.
> > 
> > What's the best practice for discrete board-level clocks and regulators
> > in overlays ? How do we ensure that their node name will not conflict
> > with the board to which the overlay is attached ?
> 
> Top-level nodes (so under /) do not have unit addresses. If they have -
> it's an error, because it is not a bus. Also, unit address requires reg.
> No reg? No unit address. DTC reports this as warnings as well.

I agree with all that, but what's the recommended practice to add
top-level clocks and regulators in overlays, in a way that avoids
namespace clashes with the base board ?

> >>> +		orientation = <0>;
> >>> +		rotation = <0>;
> >>> +
> >>> +		thine,rx,data-lanes = <4 1 3 2>;
> >>
> >> NAK for this property.
> > 
> > Please explain why. You commented very briefly in the bindings review,
> > and it wasn't clear to me if you were happy or not with the property,
> > and if not, why.
> 
> Because it is duplicating endpoint. At least from the description.

The THP7312 is an external ISP. At the hardware level, it has an input
side, with a CSI-2 receiver and an I2C master controller, and an output
side, with a CSI-2 transmitter and an I2C slave controller. A raw camera
sensor is connected on the input side, transmitting image data to the
THP7312, and being controlled over I2C by the firmware running on the
THP7312. From a Linux point of view, only the output side of the THP7312
is visible, and the combination of the raw camera sensor and the THP7312
acts as a smart camera sensor, producing YUV images.

As there are two CSI-2 buses, the data lanes configuration needs to be
specified for both sides. On the output side, connected to the SoC and
visible to Linux, the bindings use a port node with an endpoint and the
standard data-lanes property. On the input side, which is invisible to
Linux, the bindings use the vendor-specific thine,rx,data-lanes
property. Its semantics is identical to the standard data-lanes
property, but it's not located in an endpoint as there's no port for the
input side.
Laurent Pinchart Sept. 6, 2023, 9:35 a.m. UTC | #3 
On Wed, Sep 06, 2023 at 11:21:31AM +0200, Krzysztof Kozlowski wrote:
> On 06/09/2023 11:00, Laurent Pinchart wrote:
> >>> has a regulator@0. There are similar instances for clocks.
> >>>
> >>> I understand why it may not be a good idea, and how the root node is
> >>> indeed not a bus. In some cases, those regulators and clocks are grouped
> >>> in a regulators or clocks node that has a "simple-bus" compatible. I'm
> >>> not sure if that's a good idea, but at least it should validate.
> >>>
> >>> What's the best practice for discrete board-level clocks and regulators
> >>> in overlays ? How do we ensure that their node name will not conflict
> >>> with the board to which the overlay is attached ?
> >>
> >> Top-level nodes (so under /) do not have unit addresses. If they have -
> >> it's an error, because it is not a bus. Also, unit address requires reg.
> >> No reg? No unit address. DTC reports this as warnings as well.
> > 
> > I agree with all that, but what's the recommended practice to add
> > top-level clocks and regulators in overlays, in a way that avoids
> > namespace clashes with the base board ?
> 
> Whether you use regulator@0 or regulator-0, you have the same chances of
> clash.

No disagreement there. My question is whether there's a recommended
practice to avoid clashes, or if it's an unsolved problem that gets
ignored for now because there's only 36h in a day and there are more
urgent things to do.

> >>>>> +		orientation = <0>;
> >>>>> +		rotation = <0>;
> >>>>> +
> >>>>> +		thine,rx,data-lanes = <4 1 3 2>;
> >>>>
> >>>> NAK for this property.
> >>>
> >>> Please explain why. You commented very briefly in the bindings review,
> >>> and it wasn't clear to me if you were happy or not with the property,
> >>> and if not, why.
> >>
> >> Because it is duplicating endpoint. At least from the description.
> > 
> > The THP7312 is an external ISP. At the hardware level, it has an input
> > side, with a CSI-2 receiver and an I2C master controller, and an output
> > side, with a CSI-2 transmitter and an I2C slave controller. A raw camera
> > sensor is connected on the input side, transmitting image data to the
> > THP7312, and being controlled over I2C by the firmware running on the
> > THP7312. From a Linux point of view, only the output side of the THP7312
> > is visible, and the combination of the raw camera sensor and the THP7312
> > acts as a smart camera sensor, producing YUV images.
> 
> None of this was explained in the device description or property field.

I agree this can be improved. Paul, can you expand the description to
make it clearer in the next version ?

> I probably judged to fast but it just looked like duplicated property.
> Then shouldn't it have two ports, even if camera side is not visible for
> the Linux?

I'm in two minds about this. On one hand, using ports means we can reuse
standard properties, as well as helper code in the kernel, which is
nice. On the other hand, it means we would also need to add a DT node
to model the sensor, but the sensor isn't exposed to Linux, so we don't
want that node to cause a device being instantiated.

I think we'll need to add more properties related to the camera sensor
in the future. Coupled with the fact that the THP7312 actually has two
inputs to support two sensors at the same time (which neither the
bindings nor the driver curently support, but that's fine, they can be
added later), it would be nice to group all properties related to a
particular THP7312 input in a node. I've given this a try for the AP1302
(another external ISP) a while ago. The bindings have been posted in
https://lore.kernel.org/linux-media/20211006113254.3470-2-anil.mamidala@xilinx.com/.
It still doesn't connect the sensors to the ISP in DT, but it nicely
groups all sensor-related properties together. Is this something that
you would be happier with ?

> > As there are two CSI-2 buses, the data lanes configuration needs to be
> > specified for both sides. On the output side, connected to the SoC and
> > visible to Linux, the bindings use a port node with an endpoint and the
> > standard data-lanes property. On the input side, which is invisible to
> > Linux, the bindings use the vendor-specific thine,rx,data-lanes
> > property. Its semantics is identical to the standard data-lanes
> > property, but it's not located in an endpoint as there's no port for the
> > input side.
> 
> And how does the property support multiple sensors? What if they data
> lanes are also different between each other?

Ignoring for a moment that the THP7312 has two inputs, there would be no
problem I think, as only one sensor is connected to the input. Different
sensor models can be used on different boards, but only one at a time.

To support the second input, we could add a thine,rx2,data-lanes
property. It's not great, but not that bad either if it stopped there.
However, if we later have to add additional sensor-related properties
(such as regulators for instance), it could become ugly. Grouping
sensor-related properties in child sensor nodes would be nicer I
believe.
Kieran Bingham Sept. 6, 2023, 11:01 a.m. UTC | #4 
Quoting Laurent Pinchart (2023-09-06 10:35:31)
> On Wed, Sep 06, 2023 at 11:21:31AM +0200, Krzysztof Kozlowski wrote:
> > On 06/09/2023 11:00, Laurent Pinchart wrote:
> > >>> has a regulator@0. There are similar instances for clocks.
> > >>>
> > >>> I understand why it may not be a good idea, and how the root node is
> > >>> indeed not a bus. In some cases, those regulators and clocks are grouped
> > >>> in a regulators or clocks node that has a "simple-bus" compatible. I'm
> > >>> not sure if that's a good idea, but at least it should validate.
> > >>>
> > >>> What's the best practice for discrete board-level clocks and regulators
> > >>> in overlays ? How do we ensure that their node name will not conflict
> > >>> with the board to which the overlay is attached ?
> > >>
> > >> Top-level nodes (so under /) do not have unit addresses. If they have -
> > >> it's an error, because it is not a bus. Also, unit address requires reg.
> > >> No reg? No unit address. DTC reports this as warnings as well.
> > > 
> > > I agree with all that, but what's the recommended practice to add
> > > top-level clocks and regulators in overlays, in a way that avoids
> > > namespace clashes with the base board ?
> > 
> > Whether you use regulator@0 or regulator-0, you have the same chances of
> > clash.
> 
> No disagreement there. My question is whether there's a recommended
> practice to avoid clashes, or if it's an unsolved problem that gets
> ignored for now because there's only 36h in a day and there are more
> urgent things to do.

Should an overlay add these items to a simple-bus specific to that
overlay/device that is being supported?

That would 'namespace' the added fixed-clocks/fixed-regulators etc...

But maybe it's overengineering or mis-using the simple-bus.

And the items are still not on a 'bus' with an address - they just exist
on a presumably externally provided board....

--
Kieran
Laurent Pinchart Sept. 6, 2023, 11:14 a.m. UTC | #5 
On Wed, Sep 06, 2023 at 12:01:43PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2023-09-06 10:35:31)
> > On Wed, Sep 06, 2023 at 11:21:31AM +0200, Krzysztof Kozlowski wrote:
> > > On 06/09/2023 11:00, Laurent Pinchart wrote:
> > > >>> has a regulator@0. There are similar instances for clocks.
> > > >>>
> > > >>> I understand why it may not be a good idea, and how the root node is
> > > >>> indeed not a bus. In some cases, those regulators and clocks are grouped
> > > >>> in a regulators or clocks node that has a "simple-bus" compatible. I'm
> > > >>> not sure if that's a good idea, but at least it should validate.
> > > >>>
> > > >>> What's the best practice for discrete board-level clocks and regulators
> > > >>> in overlays ? How do we ensure that their node name will not conflict
> > > >>> with the board to which the overlay is attached ?
> > > >>
> > > >> Top-level nodes (so under /) do not have unit addresses. If they have -
> > > >> it's an error, because it is not a bus. Also, unit address requires reg.
> > > >> No reg? No unit address. DTC reports this as warnings as well.
> > > > 
> > > > I agree with all that, but what's the recommended practice to add
> > > > top-level clocks and regulators in overlays, in a way that avoids
> > > > namespace clashes with the base board ?
> > > 
> > > Whether you use regulator@0 or regulator-0, you have the same chances of
> > > clash.
> > 
> > No disagreement there. My question is whether there's a recommended
> > practice to avoid clashes, or if it's an unsolved problem that gets
> > ignored for now because there's only 36h in a day and there are more
> > urgent things to do.
> 
> Should an overlay add these items to a simple-bus specific to that
> overlay/device that is being supported?
> 
> That would 'namespace' the added fixed-clocks/fixed-regulators etc...
> 
> But maybe it's overengineering or mis-using the simple-bus.

You would still need to name the node that groups the regulators and
clocks in a way that wouldn't clash between multiple overlays and the
base board. It would be nice to have nodes that are "private" to an
overlay.

> And the items are still not on a 'bus' with an address - they just exist
> on a presumably externally provided board....
Paul Elder Sept. 7, 2023, 2:55 p.m. UTC | #6 
On Wed, Sep 06, 2023 at 02:14:29PM +0300, Laurent Pinchart wrote:
> On Wed, Sep 06, 2023 at 12:01:43PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2023-09-06 10:35:31)
> > > On Wed, Sep 06, 2023 at 11:21:31AM +0200, Krzysztof Kozlowski wrote:
> > > > On 06/09/2023 11:00, Laurent Pinchart wrote:
> > > > >>> has a regulator@0. There are similar instances for clocks.
> > > > >>>
> > > > >>> I understand why it may not be a good idea, and how the root node is
> > > > >>> indeed not a bus. In some cases, those regulators and clocks are grouped
> > > > >>> in a regulators or clocks node that has a "simple-bus" compatible. I'm
> > > > >>> not sure if that's a good idea, but at least it should validate.
> > > > >>>
> > > > >>> What's the best practice for discrete board-level clocks and regulators
> > > > >>> in overlays ? How do we ensure that their node name will not conflict
> > > > >>> with the board to which the overlay is attached ?
> > > > >>
> > > > >> Top-level nodes (so under /) do not have unit addresses. If they have -
> > > > >> it's an error, because it is not a bus. Also, unit address requires reg.
> > > > >> No reg? No unit address. DTC reports this as warnings as well.
> > > > > 
> > > > > I agree with all that, but what's the recommended practice to add
> > > > > top-level clocks and regulators in overlays, in a way that avoids
> > > > > namespace clashes with the base board ?
> > > > 
> > > > Whether you use regulator@0 or regulator-0, you have the same chances of
> > > > clash.
> > > 
> > > No disagreement there. My question is whether there's a recommended
> > > practice to avoid clashes, or if it's an unsolved problem that gets
> > > ignored for now because there's only 36h in a day and there are more
> > > urgent things to do.
> > 
> > Should an overlay add these items to a simple-bus specific to that
> > overlay/device that is being supported?
> > 
> > That would 'namespace' the added fixed-clocks/fixed-regulators etc...
> > 
> > But maybe it's overengineering or mis-using the simple-bus.
> 
> You would still need to name the node that groups the regulators and
> clocks in a way that wouldn't clash between multiple overlays and the
> base board. It would be nice to have nodes that are "private" to an
> overlay.

What's the best solution to this then :/


Paul

> 
> > And the items are still not on a 'bus' with an address - they just exist
> > on a presumably externally provided board....
Laurent Pinchart Sept. 7, 2023, 3:04 p.m. UTC | #7 
On Thu, Sep 07, 2023 at 11:55:13PM +0900, Paul Elder wrote:
> On Wed, Sep 06, 2023 at 02:14:29PM +0300, Laurent Pinchart wrote:
> > On Wed, Sep 06, 2023 at 12:01:43PM +0100, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart (2023-09-06 10:35:31)
> > > > On Wed, Sep 06, 2023 at 11:21:31AM +0200, Krzysztof Kozlowski wrote:
> > > > > On 06/09/2023 11:00, Laurent Pinchart wrote:
> > > > > >>> has a regulator@0. There are similar instances for clocks.
> > > > > >>>
> > > > > >>> I understand why it may not be a good idea, and how the root node is
> > > > > >>> indeed not a bus. In some cases, those regulators and clocks are grouped
> > > > > >>> in a regulators or clocks node that has a "simple-bus" compatible. I'm
> > > > > >>> not sure if that's a good idea, but at least it should validate.
> > > > > >>>
> > > > > >>> What's the best practice for discrete board-level clocks and regulators
> > > > > >>> in overlays ? How do we ensure that their node name will not conflict
> > > > > >>> with the board to which the overlay is attached ?
> > > > > >>
> > > > > >> Top-level nodes (so under /) do not have unit addresses. If they have -
> > > > > >> it's an error, because it is not a bus. Also, unit address requires reg.
> > > > > >> No reg? No unit address. DTC reports this as warnings as well.
> > > > > > 
> > > > > > I agree with all that, but what's the recommended practice to add
> > > > > > top-level clocks and regulators in overlays, in a way that avoids
> > > > > > namespace clashes with the base board ?
> > > > > 
> > > > > Whether you use regulator@0 or regulator-0, you have the same chances of
> > > > > clash.
> > > > 
> > > > No disagreement there. My question is whether there's a recommended
> > > > practice to avoid clashes, or if it's an unsolved problem that gets
> > > > ignored for now because there's only 36h in a day and there are more
> > > > urgent things to do.
> > > 
> > > Should an overlay add these items to a simple-bus specific to that
> > > overlay/device that is being supported?
> > > 
> > > That would 'namespace' the added fixed-clocks/fixed-regulators etc...
> > > 
> > > But maybe it's overengineering or mis-using the simple-bus.
> > 
> > You would still need to name the node that groups the regulators and
> > clocks in a way that wouldn't clash between multiple overlays and the
> > base board. It would be nice to have nodes that are "private" to an
> > overlay.
> 
> What's the best solution to this then :/

It seems we don't have a good solution. For now, I'd recommend just
picking a name for the regulator that has a high chance to be unique,
like reg-thp7312-1v2 for instance.

> > > And the items are still not on a 'bus' with an address - they just exist
> > > on a presumably externally provided board....
Laurent Pinchart Sept. 7, 2023, 3:04 p.m. UTC | #8 
On Thu, Sep 07, 2023 at 06:04:09PM +0300, Laurent Pinchart wrote:
> On Thu, Sep 07, 2023 at 11:55:13PM +0900, Paul Elder wrote:
> > On Wed, Sep 06, 2023 at 02:14:29PM +0300, Laurent Pinchart wrote:
> > > On Wed, Sep 06, 2023 at 12:01:43PM +0100, Kieran Bingham wrote:
> > > > Quoting Laurent Pinchart (2023-09-06 10:35:31)
> > > > > On Wed, Sep 06, 2023 at 11:21:31AM +0200, Krzysztof Kozlowski wrote:
> > > > > > On 06/09/2023 11:00, Laurent Pinchart wrote:
> > > > > > >>> has a regulator@0. There are similar instances for clocks.
> > > > > > >>>
> > > > > > >>> I understand why it may not be a good idea, and how the root node is
> > > > > > >>> indeed not a bus. In some cases, those regulators and clocks are grouped
> > > > > > >>> in a regulators or clocks node that has a "simple-bus" compatible. I'm
> > > > > > >>> not sure if that's a good idea, but at least it should validate.
> > > > > > >>>
> > > > > > >>> What's the best practice for discrete board-level clocks and regulators
> > > > > > >>> in overlays ? How do we ensure that their node name will not conflict
> > > > > > >>> with the board to which the overlay is attached ?
> > > > > > >>
> > > > > > >> Top-level nodes (so under /) do not have unit addresses. If they have -
> > > > > > >> it's an error, because it is not a bus. Also, unit address requires reg.
> > > > > > >> No reg? No unit address. DTC reports this as warnings as well.
> > > > > > > 
> > > > > > > I agree with all that, but what's the recommended practice to add
> > > > > > > top-level clocks and regulators in overlays, in a way that avoids
> > > > > > > namespace clashes with the base board ?
> > > > > > 
> > > > > > Whether you use regulator@0 or regulator-0, you have the same chances of
> > > > > > clash.
> > > > > 
> > > > > No disagreement there. My question is whether there's a recommended
> > > > > practice to avoid clashes, or if it's an unsolved problem that gets
> > > > > ignored for now because there's only 36h in a day and there are more
> > > > > urgent things to do.
> > > > 
> > > > Should an overlay add these items to a simple-bus specific to that
> > > > overlay/device that is being supported?
> > > > 
> > > > That would 'namespace' the added fixed-clocks/fixed-regulators etc...
> > > > 
> > > > But maybe it's overengineering or mis-using the simple-bus.
> > > 
> > > You would still need to name the node that groups the regulators and
> > > clocks in a way that wouldn't clash between multiple overlays and the
> > > base board. It would be nice to have nodes that are "private" to an
> > > overlay.
> > 
> > What's the best solution to this then :/
> 
> It seems we don't have a good solution. For now, I'd recommend just
> picking a name for the regulator that has a high chance to be unique,
> like reg-thp7312-1v2 for instance.

Or reg-cam-1v2, or ... The name doesn't matter much really, as long as
it's not extremely generic with a high risk of conflict.

> > > > And the items are still not on a 'bus' with an address - they just exist
> > > > on a presumably externally provided board....
Rob Herring Sept. 8, 2023, 8:52 p.m. UTC | #9 
On Wed, Sep 06, 2023 at 08:31:18AM +0900, Paul Elder wrote:
> Add overlays for the Pumpkin i350 to support THP7312 cameras.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  arch/arm64/boot/dts/mediatek/Makefile         |  4 +
>  .../mt8365-pumpkin-common-thp7312.dtsi        | 23 ++++++
>  .../mt8365-pumpkin-csi0-thp7312-imx258.dtso   | 73 +++++++++++++++++++
>  .../mt8365-pumpkin-csi1-thp7312-imx258.dtso   | 73 +++++++++++++++++++
>  4 files changed, 173 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
>  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi1-thp7312-imx258.dtso
> 
> diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> index 20570bc40de8..ceaf24105001 100644
> --- a/arch/arm64/boot/dts/mediatek/Makefile
> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> @@ -56,4 +56,8 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-evk.dtb
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-pumpkin.dtb
>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8516-pumpkin.dtb
>  
> +mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi0-thp7312-imx258.dtbo
> +mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi1-thp7312-imx258.dtbo
>  mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-ethernet-usb.dtbo

This has no effect. You are assigning the same variable 3 times. It 
needs to be every overlay applied to its base is a *-dtbs variable and 
then those are all added to 'dtb-y'. IOW, we don't allow overlays which 
can't be applied at build time.

Assuming the overlays aren't mutually exclusive, you could do:

mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-ethernet-usb.dtbo
mtk-mt8365-pumpkin-dtbs += mt8365-pumpkin-csi0-thp7312-imx258.dtbo
mtk-mt8365-pumpkin-dtbs += mt8365-pumpkin-csi1-thp7312-imx258.dtbo

This works the same way as '-objs' variables to group .o files into a 
module.

> +
> +dtb-$(CONFIG_ARCH_MEDIATEK) += mtk-mt8365-pumpkin.dtb

Looks like mtk-mt8365-pumpkin.dtb failed to get built before. In any 
case, that's a terrible name. What's the difference between 
mt8365-pumpkin.dtb and mtk-mt8365-pumpkin.dtb? There's no clue.

Rob
Laurent Pinchart Sept. 9, 2023, 3:37 p.m. UTC | #10 
On Fri, Sep 08, 2023 at 03:52:22PM -0500, Rob Herring wrote:
> On Wed, Sep 06, 2023 at 08:31:18AM +0900, Paul Elder wrote:
> > Add overlays for the Pumpkin i350 to support THP7312 cameras.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  arch/arm64/boot/dts/mediatek/Makefile         |  4 +
> >  .../mt8365-pumpkin-common-thp7312.dtsi        | 23 ++++++
> >  .../mt8365-pumpkin-csi0-thp7312-imx258.dtso   | 73 +++++++++++++++++++
> >  .../mt8365-pumpkin-csi1-thp7312-imx258.dtso   | 73 +++++++++++++++++++
> >  4 files changed, 173 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi1-thp7312-imx258.dtso
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
> > index 20570bc40de8..ceaf24105001 100644
> > --- a/arch/arm64/boot/dts/mediatek/Makefile
> > +++ b/arch/arm64/boot/dts/mediatek/Makefile
> > @@ -56,4 +56,8 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-evk.dtb
> >  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-pumpkin.dtb
> >  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8516-pumpkin.dtb
> >  
> > +mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi0-thp7312-imx258.dtbo
> > +mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi1-thp7312-imx258.dtbo
> >  mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-ethernet-usb.dtbo
> 
> This has no effect. You are assigning the same variable 3 times. It 
> needs to be every overlay applied to its base is a *-dtbs variable and 
> then those are all added to 'dtb-y'. IOW, we don't allow overlays which 
> can't be applied at build time.
> 
> Assuming the overlays aren't mutually exclusive, you could do:
> 
> mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-ethernet-usb.dtbo
> mtk-mt8365-pumpkin-dtbs += mt8365-pumpkin-csi0-thp7312-imx258.dtbo
> mtk-mt8365-pumpkin-dtbs += mt8365-pumpkin-csi1-thp7312-imx258.dtbo
> 
> This works the same way as '-objs' variables to group .o files into a 
> module.
> 
> > +
> > +dtb-$(CONFIG_ARCH_MEDIATEK) += mtk-mt8365-pumpkin.dtb
> 
> Looks like mtk-mt8365-pumpkin.dtb failed to get built before. In any 
> case, that's a terrible name. What's the difference between 
> mt8365-pumpkin.dtb and mtk-mt8365-pumpkin.dtb? There's no clue.

That's a bad name indeed. The cover letter indicates that this patch
depends on currently out-of-tree DT changes, including a
work-in-progress commit that adds mt8365-pumpkin-ethernet-usb.dtbo. The
commits order makes this patch look weird.

Paul, in your v2, please indicate in the commit message of this patch
that it isn't intended to be merged yet. A "[DNI]" prefix in the subject
line will help.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
index 20570bc40de8..ceaf24105001 100644
--- a/arch/arm64/boot/dts/mediatek/Makefile
+++ b/arch/arm64/boot/dts/mediatek/Makefile
@@ -56,4 +56,8 @@  dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-evk.dtb
 dtb-$(CONFIG_ARCH_MEDIATEK) += mt8365-pumpkin.dtb
 dtb-$(CONFIG_ARCH_MEDIATEK) += mt8516-pumpkin.dtb
 
+mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi0-thp7312-imx258.dtbo
+mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-csi1-thp7312-imx258.dtbo
 mtk-mt8365-pumpkin-dtbs := mt8365-pumpkin.dtb mt8365-pumpkin-ethernet-usb.dtbo
+
+dtb-$(CONFIG_ARCH_MEDIATEK) += mtk-mt8365-pumpkin.dtb
diff --git a/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
new file mode 100644
index 000000000000..478697552617
--- /dev/null
+++ b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-common-thp7312.dtsi
@@ -0,0 +1,23 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023 Ideas on Board
+ * Author: Paul Elder <paul.elder@ideasonboard.com>
+ */
+
+/dts-v1/;
+/plugin/;
+
+&{/} {
+	vsys_v4p2: regulator@0 {
+		compatible = "regulator-fixed";
+		regulator-name = "vsys-v4p2";
+		regulator-min-microvolt = <4200000>;
+		regulator-max-microvolt = <4200000>;
+	};
+
+	camera61_clk: cam_clk24m {
+		compatible = "fixed-clock";
+		clock-frequency = <24000000>;
+		#clock-cells = <0>;
+	};
+};
diff --git a/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
new file mode 100644
index 000000000000..740d14a19d75
--- /dev/null
+++ b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi0-thp7312-imx258.dtso
@@ -0,0 +1,73 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023 Ideas on Board
+ * Author: Paul Elder <paul.elder@ideasonboard.com>
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pinctrl/mt8365-pinfunc.h>
+#include "mt8365-pumpkin-common-thp7312.dtsi"
+
+&i2c3 {
+	camera@61 {
+		compatible = "thine,thp7312";
+		reg = <0x61>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&cam0_pins_default>;
+		reset-gpios = <&pio 118 GPIO_ACTIVE_LOW>;
+		clocks = <&camera61_clk>;
+
+		vddcore-supply = <&vsys_v4p2>;
+		vhtermrx-supply = <&vsys_v4p2>;
+		vddtx-supply = <&vsys_v4p2>;
+		vddhost-supply = <&vsys_v4p2>;
+		vddcmos-supply = <&vsys_v4p2>;
+		vddgpio_0-supply = <&vsys_v4p2>;
+		vddgpio_1-supply = <&vsys_v4p2>;
+		DOVDD-supply = <&vsys_v4p2>;
+		AVDD-supply = <&vsys_v4p2>;
+		DVDD-supply = <&vsys_v4p2>;
+
+		orientation = <0>;
+		rotation = <0>;
+
+		thine,rx,data-lanes = <4 1 3 2>;
+
+		port {
+			isp1_out: endpoint {
+				remote-endpoint = <&seninf_in1>;
+				data-lanes = <4 2 1 3>;
+			};
+		};
+	};
+};
+
+&pio {
+	cam0_pins_default: cam0_pins_default {
+		pins_rst {
+			pinmux = <MT8365_PIN_118_DMIC0_DAT0__FUNC_GPIO118>;
+		};
+	};
+};
+
+&seninf {
+	status = "okay";
+
+	ports {
+		port@0 {
+			seninf_in1: endpoint {
+				remote-endpoint = <&isp1_out>;
+				clock-lanes = <2>;
+				data-lanes = <1 3 0 4>;
+			};
+		};
+	};
+};
+
+&camsv1 {
+	status = "okay";
+};
+
+&mipi_csi0 {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi1-thp7312-imx258.dtso b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi1-thp7312-imx258.dtso
new file mode 100644
index 000000000000..2ebe4e9b56fa
--- /dev/null
+++ b/arch/arm64/boot/dts/mediatek/mt8365-pumpkin-csi1-thp7312-imx258.dtso
@@ -0,0 +1,73 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023 Ideas on Board
+ * Author: Paul Elder <paul.elder@ideasonboard.com>
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pinctrl/mt8365-pinfunc.h>
+#include "mt8365-pumpkin-common-thp7312.dtsi"
+
+&i2c2 {
+	camera@61 {
+		compatible = "thine,thp7312";
+		reg = <0x61>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&cam1_pins_default>;
+		reset-gpios = <&pio 119 GPIO_ACTIVE_LOW>;
+		clocks = <&camera61_clk>;
+
+		vddcore-supply = <&vsys_v4p2>;
+		vhtermrx-supply = <&vsys_v4p2>;
+		vddtx-supply = <&vsys_v4p2>;
+		vddhost-supply = <&vsys_v4p2>;
+		vddcmos-supply = <&vsys_v4p2>;
+		vddgpio_0-supply = <&vsys_v4p2>;
+		vddgpio_1-supply = <&vsys_v4p2>;
+		DOVDD-supply = <&vsys_v4p2>;
+		AVDD-supply = <&vsys_v4p2>;
+		DVDD-supply = <&vsys_v4p2>;
+
+		orientation = <0>;
+		rotation = <0>;
+
+		thine,rx,data-lanes = <4 1 3 2>;
+
+		port {
+			isp2_out: endpoint {
+				remote-endpoint = <&seninf_in2>;
+				data-lanes = <4 2 1 3>;
+			};
+		};
+	};
+};
+
+&pio {
+	cam1_pins_default: cam1_pins_default {
+		pins_rst {
+			pinmux = <MT8365_PIN_119_DMIC0_DAT1__FUNC_GPIO119>;
+		};
+	};
+};
+
+&seninf {
+	status = "okay";
+
+	ports {
+		port@1 {
+			seninf_in2: endpoint {
+				remote-endpoint = <&isp2_out>;
+				clock-lanes = <2>;
+				data-lanes = <1 3 0 4>;
+			};
+		};
+	};
+};
+
+&camsv2 {
+	status = "okay";
+};
+
+&mipi_csi1 {
+	status = "okay";
+};