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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ec20238c-1cd1-493b-8df5-5be055576f14@ti.com>
Date: Wed, 26 Feb 2025 11:16:44 +0530
From: Yemike Abhilash Chandra <y-abhilashchandra@...com>
To: Jai Luthra <jai.luthra@...ux.dev>,
        "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>
CC: <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <mripard@...nel.org>,
        <mchehab@...nel.org>, <robh@...nel.org>, <conor+dt@...nel.org>,
        <devarsht@...com>, <vaishnav.a@...com>, <r-donadkar@...com>,
        <u-kumar1@...com>
Subject: Re: [PATCH v3 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional
 interrupts for cdns-csi2rx

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@...com>
>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
>> ---
>>
>> 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@...01000: 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@...01000: 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@...01000: 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
>>