Re: [PATCH 1/7] dt-bindings: media: renesas,isp: Add ISP core function block

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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