diff mbox series

[v2,06/10] media: dt-bindings: Document SAMA5D4 VDEC bindings

Message ID 20210311154055.3496076-7-emil.l.velikov@gmail.com
State Superseded
Headers show
Series Microship SAMA5D4 VPU support et al | expand

Commit Message

Emil Velikov March 11, 2021, 3:40 p.m. UTC
From: Emil Velikov <emil.velikov@collabora.com>

Add devicetree binding documentation for the Hantro G1/G2 VDEC on
the Microchip SAMAS5D4 SoC.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: devicetree@vger.kernel.org>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
v2
 - Newly introduced
 - s/Atmel/Microchip/ (Nicolas)
 - Drop leading 0 in node name/address
---
 .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

Comments

Rob Herring (Arm) March 24, 2021, 3:17 p.m. UTC | #1
On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>

> 

> Add devicetree binding documentation for the Hantro G1/G2 VDEC on

> the Microchip SAMAS5D4 SoC.

> 

> Cc: Rob Herring <robh+dt@kernel.org>

> Cc: Frank Rowand <frowand.list@gmail.com>

> Cc: devicetree@vger.kernel.org>

> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

> ---

> v2

>  - Newly introduced

>  - s/Atmel/Microchip/ (Nicolas)

>  - Drop leading 0 in node name/address

> ---

>  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++

>  1 file changed, 59 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> 

> diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> new file mode 100644

> index 000000000000..9cb2c0295d54

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> @@ -0,0 +1,59 @@

> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)

> +

> +%YAML 1.2

> +---

> +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"

> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

> +

> +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs

> +

> +maintainers:

> +  - Emil Velikov <emil.velikov@collabora.com>

> +

> +description:

> +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.

> +

> +properties:

> +  compatible:

> +    const: microchip,sama5d4-vdec

> +

> +  reg:

> +    maxItems: 1

> +

> +  interrupts:

> +    maxItems: 1

> +

> +  interrupt-names:

> +    items:

> +      - const: vdec


Why do you need a name? *-names are used to distinguish multiple entries 
and don't add anything if only a single entry.

> +

> +  clocks:

> +    maxItems: 1

> +

> +  clock-names:

> +    items:

> +      - const: vdec_clk


And here too? These are typically named for either the name of input 
signal (hclk, aclk, etc.) or function ('core', 'bus', etc.). 

> +

> +required:

> +  - compatible

> +  - reg

> +  - interrupts

> +  - interrupt-names

> +  - clocks

> +  - clock-names

> +

> +additionalProperties: false

> +

> +examples:

> +  - |

> +        #include <dt-bindings/clock/at91.h>

> +        #include <dt-bindings/interrupt-controller/irq.h>

> +

> +        vdec0: vdec@300000 {

> +                compatible = "microchip,sama5d4-vdec";

> +                reg = <0x00300000 0x100000>;

> +                interrupts = <19 IRQ_TYPE_LEVEL_HIGH 4>;

> +                interrupt-names = "vdec";

> +                clocks = <&pmc PMC_TYPE_PERIPHERAL 19>;

> +                clock-names = "vdec_clk";

> +        };

> -- 

> 2.30.1

>
Emil Velikov March 25, 2021, 2:14 p.m. UTC | #2
On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > Add devicetree binding documentation for the Hantro G1/G2 VDEC on
> > the Microchip SAMAS5D4 SoC.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org>
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> > v2
> >  - Newly introduced
> >  - s/Atmel/Microchip/ (Nicolas)
> >  - Drop leading 0 in node name/address
> > ---
> >  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++
> >  1 file changed, 59 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > new file mode 100644
> > index 000000000000..9cb2c0295d54
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs
> > +
> > +maintainers:
> > +  - Emil Velikov <emil.velikov@collabora.com>
> > +
> > +description:
> > +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    const: microchip,sama5d4-vdec
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: vdec
>
> Why do you need a name? *-names are used to distinguish multiple entries
> and don't add anything if only a single entry.
>
On one hand the names are used to describe the hardware - the SAMA
board uses designated clock and interrupt lines - both called "vdec".
Additionally the names are ultimately required by the underlying API -
platform_get_irq_byname and  devm_clk_bulk_get respectively.
How can we get the respective entries without the name?

Skimming through the existing dts file -
arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of
examples where the device tree binding contains the name for a single
clock/interrupt.

Thanks
Emil
Ezequiel Garcia March 26, 2021, 2:28 p.m. UTC | #3
Hi Emil,

On Thu, 2021-03-25 at 14:14 +0000, Emil Velikov wrote:
> On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote:

> > 

> > On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:

> > > From: Emil Velikov <emil.velikov@collabora.com>

> > > 

> > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on

> > > the Microchip SAMAS5D4 SoC.

> > > 

> > > Cc: Rob Herring <robh+dt@kernel.org>

> > > Cc: Frank Rowand <frowand.list@gmail.com>

> > > Cc: devicetree@vger.kernel.org>

> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

> > > ---

> > > v2

> > >  - Newly introduced

> > >  - s/Atmel/Microchip/ (Nicolas)

> > >  - Drop leading 0 in node name/address

> > > ---

> > >  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++

> > >  1 file changed, 59 insertions(+)

> > >  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> > > 

> > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-

> > > vdec.yaml

> > > new file mode 100644

> > > index 000000000000..9cb2c0295d54

> > > --- /dev/null

> > > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> > > @@ -0,0 +1,59 @@

> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)

> > > +

> > > +%YAML 1.2

> > > +---

> > > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"

> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

> > > +

> > > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs

> > > +

> > > +maintainers:

> > > +  - Emil Velikov <emil.velikov@collabora.com>

> > > +

> > > +description:

> > > +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.

> > > +

> > > +properties:

> > > +  compatible:

> > > +    const: microchip,sama5d4-vdec

> > > +

> > > +  reg:

> > > +    maxItems: 1

> > > +

> > > +  interrupts:

> > > +    maxItems: 1

> > > +

> > > +  interrupt-names:

> > > +    items:

> > > +      - const: vdec

> > 

> > Why do you need a name? *-names are used to distinguish multiple entries

> > and don't add anything if only a single entry.

> > 

> On one hand the names are used to describe the hardware - the SAMA

> board uses designated clock and interrupt lines - both called "vdec".

> Additionally the names are ultimately required by the underlying API -

> platform_get_irq_byname and  devm_clk_bulk_get respectively.

> How can we get the respective entries without the name?

> 


There are APIs to get the resource by index as opposed to by name.
E.g. platform_get_irq.

However, you are confusing here internal kernel implementation
with devicetree bindings. The latter is just a representation
of the hardware.

> Skimming through the existing dts file -

> arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of

> examples where the device tree binding contains the name for a single

> clock/interrupt.

> 



Note that dtsi and dts are the device-tree files, not the bindings.
And even if you find examples, the fact that there exist examples
doesn't mean it's the right thing to do.

Thanks,
Ezequiel
Ezequiel Garcia March 26, 2021, 2:33 p.m. UTC | #4
Hi Rob,

Thanks for reviewing.

On Wed, 2021-03-24 at 09:17 -0600, Rob Herring wrote:
> On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:

> > From: Emil Velikov <emil.velikov@collabora.com>

> > 

> > Add devicetree binding documentation for the Hantro G1/G2 VDEC on

> > the Microchip SAMAS5D4 SoC.

> > 

> > Cc: Rob Herring <robh+dt@kernel.org>

> > Cc: Frank Rowand <frowand.list@gmail.com>

> > Cc: devicetree@vger.kernel.org>

> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

> > ---

> > v2

> >  - Newly introduced

> >  - s/Atmel/Microchip/ (Nicolas)

> >  - Drop leading 0 in node name/address

> > ---

> >  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++

> >  1 file changed, 59 insertions(+)

> >  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> > 

> > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-

> > vdec.yaml

> > new file mode 100644

> > index 000000000000..9cb2c0295d54

> > --- /dev/null

> > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> > @@ -0,0 +1,59 @@

> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)

> > +

> > +%YAML 1.2

> > +---

> > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"

> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

> > +

> > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs

> > +

> > +maintainers:

> > +  - Emil Velikov <emil.velikov@collabora.com>

> > +

> > +description:

> > +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.

> > +

> > +properties:

> > +  compatible:

> > +    const: microchip,sama5d4-vdec

> > +

> > +  reg:

> > +    maxItems: 1

> > +

> > +  interrupts:

> > +    maxItems: 1

> > +

> > +  interrupt-names:

> > +    items:

> > +      - const: vdec

> 

> Why do you need a name? *-names are used to distinguish multiple entries 

> and don't add anything if only a single entry.

> 


I guess you are right.

I was about to argue that it makes backwards compatibility easier,
but I suspect that's not the case either.

> > +

> > +  clocks:

> > +    maxItems: 1

> > +

> > +  clock-names:

> > +    items:

> > +      - const: vdec_clk

> 

> And here too? These are typically named for either the name of input 

> signal (hclk, aclk, etc.) or function ('core', 'bus', etc.). 

> 


Indeed, "core" might be a better choice. But it seems we don't need
names here.

Thanks,
Ezequiel
Emil Velikov March 31, 2021, 5:42 p.m. UTC | #5
On Fri, 26 Mar 2021 at 14:29, Ezequiel Garcia <ezequiel@collabora.com> wrote:
>

> Hi Emil,

>

> On Thu, 2021-03-25 at 14:14 +0000, Emil Velikov wrote:

> > On Wed, 24 Mar 2021 at 15:17, Rob Herring <robh@kernel.org> wrote:

> > >

> > > On Thu, Mar 11, 2021 at 03:40:51PM +0000, Emil Velikov wrote:

> > > > From: Emil Velikov <emil.velikov@collabora.com>

> > > >

> > > > Add devicetree binding documentation for the Hantro G1/G2 VDEC on

> > > > the Microchip SAMAS5D4 SoC.

> > > >

> > > > Cc: Rob Herring <robh+dt@kernel.org>

> > > > Cc: Frank Rowand <frowand.list@gmail.com>

> > > > Cc: devicetree@vger.kernel.org>

> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

> > > > ---

> > > > v2

> > > >  - Newly introduced

> > > >  - s/Atmel/Microchip/ (Nicolas)

> > > >  - Drop leading 0 in node name/address

> > > > ---

> > > >  .../media/microchip,sama5d4-vdec.yaml         | 59 +++++++++++++++++++

> > > >  1 file changed, 59 insertions(+)

> > > >  create mode 100644 Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> > > >

> > > > diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-

> > > > vdec.yaml

> > > > new file mode 100644

> > > > index 000000000000..9cb2c0295d54

> > > > --- /dev/null

> > > > +++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml

> > > > @@ -0,0 +1,59 @@

> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)

> > > > +

> > > > +%YAML 1.2

> > > > +---

> > > > +$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"

> > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"

> > > > +

> > > > +title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs

> > > > +

> > > > +maintainers:

> > > > +  - Emil Velikov <emil.velikov@collabora.com>

> > > > +

> > > > +description:

> > > > +  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.

> > > > +

> > > > +properties:

> > > > +  compatible:

> > > > +    const: microchip,sama5d4-vdec

> > > > +

> > > > +  reg:

> > > > +    maxItems: 1

> > > > +

> > > > +  interrupts:

> > > > +    maxItems: 1

> > > > +

> > > > +  interrupt-names:

> > > > +    items:

> > > > +      - const: vdec

> > >

> > > Why do you need a name? *-names are used to distinguish multiple entries

> > > and don't add anything if only a single entry.

> > >

> > On one hand the names are used to describe the hardware - the SAMA

> > board uses designated clock and interrupt lines - both called "vdec".

> > Additionally the names are ultimately required by the underlying API -

> > platform_get_irq_byname and  devm_clk_bulk_get respectively.

> > How can we get the respective entries without the name?

> >

>

> There are APIs to get the resource by index as opposed to by name.

> E.g. platform_get_irq.

>

> However, you are confusing here internal kernel implementation

> with devicetree bindings. The latter is just a representation

> of the hardware.

>

My train of thought was that if we're missing API to fetch the entries
via non-name means, then there's a reasonable precedent to expose the
name. Clearly I missed the API, so my argument is just off.

> > Skimming through the existing dts file -

> > arch/arm/boot/dts/sama5d4.dtsi and other dts files - there are lots of

> > examples where the device tree binding contains the name for a single

> > clock/interrupt.

> >

>

>

> Note that dtsi and dts are the device-tree files, not the bindings.

> And even if you find examples, the fact that there exist examples

> doesn't mean it's the right thing to do.

>

Fully agreed. Thanks for the correction and hints.

The v3, just submitted, lacks the names all together.

-Emil
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
new file mode 100644
index 000000000000..9cb2c0295d54
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/microchip,sama5d4-vdec.yaml
@@ -0,0 +1,59 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/media/microchip,sama5d4-vdec.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Hantro G1 VPU codec implemented on Microchip SAMA5D4 SoCs
+
+maintainers:
+  - Emil Velikov <emil.velikov@collabora.com>
+
+description:
+  Hantro G1 video decode accelerator present on Microchip SAMA5D4 SoCs.
+
+properties:
+  compatible:
+    const: microchip,sama5d4-vdec
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    items:
+      - const: vdec
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: vdec_clk
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+        #include <dt-bindings/clock/at91.h>
+        #include <dt-bindings/interrupt-controller/irq.h>
+
+        vdec0: vdec@300000 {
+                compatible = "microchip,sama5d4-vdec";
+                reg = <0x00300000 0x100000>;
+                interrupts = <19 IRQ_TYPE_LEVEL_HIGH 4>;
+                interrupt-names = "vdec";
+                clocks = <&pmc PMC_TYPE_PERIPHERAL 19>;
+                clock-names = "vdec_clk";
+        };