Re: [PATCH v3 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx

[Yemike Abhilash Chandra]

Hi Jai,
Thanks for the detailed review for the patch series.

On 24/02/25 12:50, Jai Luthra wrote:
> Hi Abhilash,
>
> Thanks for the patch.
>
> On Fri, Feb 21, 2025 at 05:33:36PM +0530, Yemike Abhilash Chandra wrote:
> > The Cadence CSI2RX IP exposes 2 interrupts [0] 12.7 camera subsystem.
> > Enabling these interrupts will provide additional information about a CSI
> > packet or an individual frame. So, add support for optional interrupts
> > and interrupt-names properties.
> >
> > [0]: http://www.ti.com/lit/pdf/spruil1
> >
> > Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@xxxxxx>
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > ---
> >
> > Changes in v3:
> > - Address Krzysztof's review comment to drop minItems from the bindings.
> > - Collect Acked-by from Krzysztof.
> >
> >   Documentation/devicetree/bindings/media/cdns,csi2rx.yaml | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > index 2008a47c0580..e8d7eaf443d1 100644
> > --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> > @@ -24,6 +24,14 @@ properties:
> >     reg:
> >       maxItems: 1
> >   
> > +  interrupts:
> > +    maxItems: 2
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: irq
> > +      - const: error_irq
> > +
>
> If I test these bindings with only one interrupt (error_irq) defined in the
> device tree, I get these errors:
>
>    DTC [C] arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb
> /home/darkapex/dev/linux2/out_clang/arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb: csi-bridge@30101000: interrupts: [[0, 187, 4]] is too short
>          from schema $id: http://devicetree.org/schemas/media/cdns,csi2rx.yaml#
> /home/darkapex/dev/linux2/out_clang/arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb: csi-bridge@30101000: interrupt-names:0: 'irq' was expected
>          from schema $id: http://devicetree.org/schemas/media/cdns,csi2rx.yaml#
> /home/darkapex/dev/linux2/out_clang/arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb: csi-bridge@30101000: interrupt-names: ['error_irq'] is too short
>          from schema $id: http://devicetree.org/schemas/media/cdns,csi2rx.yaml#
> make[1]: Leaving directory '/home/darkapex/dev/linux2/out_clang'
>
> There could be cases where only the error interrupt is integrated by the SoC,
> and the second interrupt is unconnected. IMHO it would make sense to keep the
> other interrupt optional:

Initially, I had the flexibilty, but I removed that based on Krzysztof's
feedback, I was also not clear how these interrupts are integrated by
different vendors at that time.

But since this driver is shared among vendors, I think it is better to
have the flexibility. Since this is a change in dt-bindings I would I 
like to have Krzysztof's view on this discussion.

> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> index e8d7eaf443d1..054ed4b94312 100644
> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
> @@ -25,12 +25,14 @@ properties:
>       maxItems: 1
>
>     interrupts:
> +    minItems: 1
>       maxItems: 2
>
>     interrupt-names:
> +    minItems: 1
>       items:
> -      - const: irq
>         - const: error_irq
> +      - const: irq

Krzysztof, If you agree to the same I will use the above binidngs in
next version of the series

Thanks and Regards,
Yemike Abhilash Chandra.

>     clocks:
>       items:
>
> >     clocks:
> >       items:
> >         - description: CSI2Rx system clock
> > --
> > 2.34.1