2024-01-10 14:16:58

by Julien Stephan

[permalink] [raw]
Subject: [PATCH v4 2/5] dt-bindings: media: add mediatek ISP3.0 camsv

From: Phi-bang Nguyen <[email protected]>

This adds the bindings, for the ISP3.0 camsv module embedded in
some Mediatek SoC, such as the mt8365

Signed-off-by: Phi-bang Nguyen <[email protected]>
Signed-off-by: Julien Stephan <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
.../bindings/media/mediatek,mt8365-camsv.yaml | 109 ++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 110 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml

diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml b/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
new file mode 100644
index 000000000000..097b1ab6bc72
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2023 MediaTek, BayLibre
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/mediatek,mt8365-camsv.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek CAMSV 3.0
+
+maintainers:
+ - Laurent Pinchart <[email protected]>
+ - Julien Stephan <[email protected]>
+ - Andy Hsieh <[email protected]>
+
+description:
+ The CAMSV is a set of DMA engines connected to the SENINF CSI-2
+ receivers. The number of CAMSVs depend on the SoC model.
+
+properties:
+ compatible:
+ const: mediatek,mt8365-camsv
+
+ reg:
+ items:
+ - description: camsv base
+ - description: img0 base
+ - description: tg base
+
+ interrupts:
+ maxItems: 1
+
+ power-domains:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: cam clock
+ - description: camtg clock
+ - description: camsv clock
+
+ clock-names:
+ items:
+ - const: cam
+ - const: camtg
+ - const: camsv
+
+ iommus:
+ maxItems: 1
+
+ ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+
+ properties:
+ port@0:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: connection point for camsv0
+
+ required:
+ - port@0
+
+required:
+ - compatible
+ - interrupts
+ - clocks
+ - clock-names
+ - power-domains
+ - iommus
+ - ports
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/clock/mediatek,mt8365-clk.h>
+ #include <dt-bindings/memory/mediatek,mt8365-larb-port.h>
+ #include <dt-bindings/power/mediatek,mt8365-power.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ camsv1: camsv@15050000 {
+ compatible = "mediatek,mt8365-camsv";
+ reg = <0 0x15050000 0 0x0040>,
+ <0 0x15050208 0 0x0020>,
+ <0 0x15050400 0 0x0100>;
+ interrupts = <GIC_SPI 186 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&camsys CLK_CAM>,
+ <&camsys CLK_CAMTG>,
+ <&camsys CLK_CAMSV0>;
+ clock-names = "cam", "camtg", "camsv";
+ iommus = <&iommu M4U_PORT_CAM_IMGO>;
+ power-domains = <&spm MT8365_POWER_DOMAIN_CAM>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ port@0 {
+ reg = <0>;
+ camsv1_endpoint: endpoint {
+ remote-endpoint = <&seninf_camsv1_endpoint>;
+ };
+ };
+ };
+ };
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 4444e719cf8e..3ea2158864e1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13616,6 +13616,7 @@ M: Laurent Pinchart <[email protected]>
M: Julien Stephan <[email protected]>
M: Andy Hsieh <[email protected]>
S: Supported
+F: Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
F: Documentation/devicetree/bindings/media/mediatek,mt8365-seninf.yaml

MEDIATEK SMI DRIVER
--
2.43.0



2024-01-12 07:35:17

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v4 2/5] dt-bindings: media: add mediatek ISP3.0 camsv

On 10/01/2024 15:14, Julien Stephan wrote:
> From: Phi-bang Nguyen <[email protected]>
>
> This adds the bindings, for the ISP3.0 camsv module embedded in
> some Mediatek SoC, such as the mt8365
>
> Signed-off-by: Phi-bang Nguyen <[email protected]>
> Signed-off-by: Julien Stephan <[email protected]>
> Link: https://lore.kernel.org/r/[email protected]
> ---
> .../bindings/media/mediatek,mt8365-camsv.yaml | 109 ++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 110 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml b/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
> new file mode 100644
> index 000000000000..097b1ab6bc72
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
> @@ -0,0 +1,109 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2023 MediaTek, BayLibre
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek,mt8365-camsv.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek CAMSV 3.0
> +
> +maintainers:
> + - Laurent Pinchart <[email protected]>
> + - Julien Stephan <[email protected]>
> + - Andy Hsieh <[email protected]>
> +
> +description:
> + The CAMSV is a set of DMA engines connected to the SENINF CSI-2
> + receivers. The number of CAMSVs depend on the SoC model.

DMA should not go to media, but to dma

> +
> +properties:
> + compatible:
> + const: mediatek,mt8365-camsv
> +
> + reg:
> + items:
> + - description: camsv base
> + - description: img0 base
> + - description: tg base
> +
> + interrupts:
> + maxItems: 1
> +
> + power-domains:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: cam clock
> + - description: camtg clock
> + - description: camsv clock
> +
> + clock-names:
> + items:
> + - const: cam
> + - const: camtg
> + - const: camsv
> +
> + iommus:
> + maxItems: 1
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/properties/port
> + description: connection point for camsv0

This explains me nothing. What type of connection point? How does it fit
the pipeline going to the display?

It seems you represented DMA as some other device to make your drivers
easier... That's not how it works.

> +
> + required:
> + - port@0
> +
> +required:
> + - compatible
> + - interrupts
> + - clocks
> + - clock-names
> + - power-domains
> + - iommus
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/clock/mediatek,mt8365-clk.h>
> + #include <dt-bindings/memory/mediatek,mt8365-larb-port.h>
> + #include <dt-bindings/power/mediatek,mt8365-power.h>
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + camsv1: camsv@15050000 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation



Best regards,
Krzysztof


2024-01-12 07:49:51

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v4 2/5] dt-bindings: media: add mediatek ISP3.0 camsv

On 12/01/2024 08:41, Laurent Pinchart wrote:
> On Fri, Jan 12, 2024 at 08:34:45AM +0100, Krzysztof Kozlowski wrote:
>> On 10/01/2024 15:14, Julien Stephan wrote:
>>> From: Phi-bang Nguyen <[email protected]>
>>>
>>> This adds the bindings, for the ISP3.0 camsv module embedded in
>>> some Mediatek SoC, such as the mt8365
>>>
>>> Signed-off-by: Phi-bang Nguyen <[email protected]>
>>> Signed-off-by: Julien Stephan <[email protected]>
>>> Link: https://lore.kernel.org/r/[email protected]
>>> ---
>>> .../bindings/media/mediatek,mt8365-camsv.yaml | 109 ++++++++++++++++++
>>> MAINTAINERS | 1 +
>>> 2 files changed, 110 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml b/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
>>> new file mode 100644
>>> index 000000000000..097b1ab6bc72
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
>>> @@ -0,0 +1,109 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +# Copyright (c) 2023 MediaTek, BayLibre
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/mediatek,mt8365-camsv.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MediaTek CAMSV 3.0
>>> +
>>> +maintainers:
>>> + - Laurent Pinchart <[email protected]>
>>> + - Julien Stephan <[email protected]>
>>> + - Andy Hsieh <[email protected]>
>>> +
>>> +description:
>>> + The CAMSV is a set of DMA engines connected to the SENINF CSI-2
>>> + receivers. The number of CAMSVs depend on the SoC model.
>>
>> DMA should not go to media, but to dma
>
> They're not generic DMA engines. The CAMSV is a video capture device
> that includes a DMA engine, much like pretty much all the other video
> capture devices.

OK, some more explanation would be useful in description.

>
>>> +
>>> +properties:
>>> + compatible:
>>> + const: mediatek,mt8365-camsv
>>> +
>>> + reg:
>>> + items:
>>> + - description: camsv base
>>> + - description: img0 base
>>> + - description: tg base
>>> +
>>> + interrupts:
>>> + maxItems: 1
>>> +
>>> + power-domains:
>>> + maxItems: 1
>>> +
>>> + clocks:
>>> + items:
>>> + - description: cam clock
>>> + - description: camtg clock
>>> + - description: camsv clock
>>> +
>>> + clock-names:
>>> + items:
>>> + - const: cam
>>> + - const: camtg
>>> + - const: camsv
>>> +
>>> + iommus:
>>> + maxItems: 1
>>> +
>>> + ports:
>>> + $ref: /schemas/graph.yaml#/properties/ports
>>> +
>>> + properties:
>>> + port@0:
>>> + $ref: /schemas/graph.yaml#/properties/port
>>> + description: connection point for camsv0
>>
>> This explains me nothing. What type of connection point? How does it fit
>> the pipeline going to the display?
>
> The description seems wrong, it should state
>
> description: Connection to the SENINF output
>
> or something similar.

I am still not sure whether DMA engine should be connected via graphs.

Best regards,
Krzysztof


2024-01-12 07:50:35

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v4 2/5] dt-bindings: media: add mediatek ISP3.0 camsv

On Fri, Jan 12, 2024 at 08:34:45AM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2024 15:14, Julien Stephan wrote:
> > From: Phi-bang Nguyen <[email protected]>
> >
> > This adds the bindings, for the ISP3.0 camsv module embedded in
> > some Mediatek SoC, such as the mt8365
> >
> > Signed-off-by: Phi-bang Nguyen <[email protected]>
> > Signed-off-by: Julien Stephan <[email protected]>
> > Link: https://lore.kernel.org/r/[email protected]
> > ---
> > .../bindings/media/mediatek,mt8365-camsv.yaml | 109 ++++++++++++++++++
> > MAINTAINERS | 1 +
> > 2 files changed, 110 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml b/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
> > new file mode 100644
> > index 000000000000..097b1ab6bc72
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mt8365-camsv.yaml
> > @@ -0,0 +1,109 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2023 MediaTek, BayLibre
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/mediatek,mt8365-camsv.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek CAMSV 3.0
> > +
> > +maintainers:
> > + - Laurent Pinchart <[email protected]>
> > + - Julien Stephan <[email protected]>
> > + - Andy Hsieh <[email protected]>
> > +
> > +description:
> > + The CAMSV is a set of DMA engines connected to the SENINF CSI-2
> > + receivers. The number of CAMSVs depend on the SoC model.
>
> DMA should not go to media, but to dma

They're not generic DMA engines. The CAMSV is a video capture device
that includes a DMA engine, much like pretty much all the other video
capture devices.

> > +
> > +properties:
> > + compatible:
> > + const: mediatek,mt8365-camsv
> > +
> > + reg:
> > + items:
> > + - description: camsv base
> > + - description: img0 base
> > + - description: tg base
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + power-domains:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: cam clock
> > + - description: camtg clock
> > + - description: camsv clock
> > +
> > + clock-names:
> > + items:
> > + - const: cam
> > + - const: camtg
> > + - const: camsv
> > +
> > + iommus:
> > + maxItems: 1
> > +
> > + ports:
> > + $ref: /schemas/graph.yaml#/properties/ports
> > +
> > + properties:
> > + port@0:
> > + $ref: /schemas/graph.yaml#/properties/port
> > + description: connection point for camsv0
>
> This explains me nothing. What type of connection point? How does it fit
> the pipeline going to the display?

The description seems wrong, it should state

description: Connection to the SENINF output

or something similar.

> It seems you represented DMA as some other device to make your drivers
> easier... That's not how it works.
>
> > +
> > + required:
> > + - port@0
> > +
> > +required:
> > + - compatible
> > + - interrupts
> > + - clocks
> > + - clock-names
> > + - power-domains
> > + - iommus
> > + - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/interrupt-controller/arm-gic.h>
> > + #include <dt-bindings/interrupt-controller/irq.h>
> > + #include <dt-bindings/clock/mediatek,mt8365-clk.h>
> > + #include <dt-bindings/memory/mediatek,mt8365-larb-port.h>
> > + #include <dt-bindings/power/mediatek,mt8365-power.h>
> > +
> > + soc {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > +
> > + camsv1: camsv@15050000 {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

--
Regards,

Laurent Pinchart