Hi Krzysztof,
Thanks for your feedback.
On 2025-03-17 12:31:51 +0100, Krzysztof Kozlowski wrote:
> On Sat, Mar 15, 2025 at 04:27:02PM +0100, Niklas Söderlund wrote:
> > diff --git a/Documentation/devicetree/bindings/media/renesas,isp.yaml b/Documentation/devicetree/bindings/media/renesas,isp.yaml
> > index c4de4555b753..de9bc739e084 100644
> > --- a/Documentation/devicetree/bindings/media/renesas,isp.yaml
> > +++ b/Documentation/devicetree/bindings/media/renesas,isp.yaml
> > @@ -25,19 +25,54 @@ properties:
> > - renesas,r8a779h0-isp # V4M
> > - const: renesas,rcar-gen4-isp # Generic R-Car Gen4
> > reg:
> > - maxItems: 1
> > + minItems: 1
> > + maxItems: 2
> > +
> > + reg-names:
> > + minItems: 1
> > + items:
> > + - const: cs
> > + - const: core
>
> All of this and further must be restricted per compatible. Otherwise
> commit msg should explain why one SoC can have it different on different
> boards.
I will expand the commit message. In short this can't be restricted per
compatible, different instances of the IP on the same board can and can
not have a core part.
>
> >
> > interrupts:
> > - maxItems: 1
> > + minItems: 1
> > + maxItems: 2
> > +
> > + interrupt-names:
> > + minItems: 1
> > + items:
> > + - const: cs
> > + - const: core
> >
> > clocks:
> > - maxItems: 1
> > + minItems: 1
> > + maxItems: 2
> > +
> > + clock-names:
> > + minItems: 1
> > + items:
> > + - const: cs
> > + - const: core
> >
> > power-domains:
> > maxItems: 1
> >
> > resets:
> > - maxItems: 1
> > + minItems: 1
> > + maxItems: 2
> > +
> > + reset-names:
> > + minItems: 1
> > + items:
> > + - const: cs
> > + - const: core
> > +
> > + renesas,vspx:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description:
> > + A phandle to the companion VSPX responsible for the Streaming Bridge
>
> But what does this device needs it for?
It's the external IP that controls the DMA of data to the ISP. I will
expand this description.
>
> > + functionality. This property is not mandatory and not all ISP devices
> > + have one attached.
>
> Drop last sentence, redundant. Instead disallow it (renesas,vspx: false)
> for all the variants not having VSPX.
I can't do that as all variants can possibly have one attached to it.
All instances of the ISP that have a core part have a VSPX. And on the
same SoC different instances of the IP can have and can not have a core
part.
>
> >
> > ports:
> > $ref: /schemas/graph.yaml#/properties/ports
> > @@ -103,10 +138,14 @@ properties:
> > required:
> > - compatible
> > - reg
> > + - reg-names
> > - interrupts
> > + - interrupt-names
> > - clocks
> > + - clock-names
> > - power-domains
> > - resets
> > + - reset-names
> > - ports
> >
> > additionalProperties: false
> > @@ -119,11 +158,16 @@ examples:
> >
> > isp1: isp@fed20000 {
> > compatible = "renesas,r8a779a0-isp", "renesas,rcar-gen4-isp";
> > - reg = <0xfed20000 0x10000>;
> > - interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>;
> > + reg = <0xfed20000 0x10000>, <0xfee00000 0x10000>;
> > + reg-names = "cs", "core";
> > + interrupts = <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>,
> > + <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "cs", "core";
> > clocks = <&cpg CPG_MOD 613>;
> > + clock-names = "cs";
>
> Why no core? The names feel inconsistent. If your block has "core" reg
> for the "ISP core" sublock, why there is no "ISP core" clock for that
> subblock?
Indeed this is wrong, there should be a core clock added here too,
thanks for catching this.
>
> Best regards,
> Krzysztof
>
--
Kind Regards,
Niklas Söderlund
