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

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

Hi Jai,Changhuang,

Thanks for the reviews.

On 18/02/25 21:07, Jai Luthra wrote:
> On Tue, Feb 18, 2025 at 09:35:50AM +0000, Changhuang Liang wrote:
>> Hi Jai, Abhilash
>>
>>> Hi Abhilash,
>>>
>>> On Mon, Feb 17, 2025 at 06:30:12PM +0530, Yemike Abhilash Chandra wrote:
>>>> The Cadence CSI2RX IP exposes 3 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@ti.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Address Krzysztof's review comment to remove flexibility while adding
>>>>    interrupts.
>>>>
>>>>   .../devicetree/bindings/media/cdns,csi2rx.yaml         | 10
>>> ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>>> b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>>> index 2008a47c0580..f335429cbde9 100644
>>>> --- a/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>>> +++ b/Documentation/devicetree/bindings/media/cdns,csi2rx.yaml
>>>> @@ -24,6 +24,16 @@ properties:
>>>>     reg:
>>>>       maxItems: 1
>>>>
>>>> +  interrupts:
>>>> +    minItems: 3
>>>> +    maxItems: 3
>>>> +
>>>> +  interrupt-names:
>>>> +    items:
>>>> +      - const: info
>>>> +      - const: error
>>>> +      - const: monitor
>>>> +
>>>
>>> How many interrupt lines are actually exposed by the Cadence IP?
>>
>> I only see that the Cadence IP exposes two interrupt lines: irq and err_irq. If there are any mistakes,
>> please help correct them.
>>
> 
> Thank you Changhuang for the quick confirmation.
> This seems to match CSI_ERR_IRQ and CSI_IRQ in TI's integration.
> 
>>> It is not clear to me from the TRM [0]. From the "Table 12-1524. CSI_RX_IF
>>> Hardware Requests" it looks like there are three separate interrupt lines
>>> CSI_ERR_IRQ, CSI_IRQ and CSI_LEVEL, which are routed to the Arm core/GIC.
>>> And four lines for the ASF submodule (?) that are not routed to GIC.
>>>
> 
> Abhilash,
> 
> What is unclear to me is where the third CSI_LEVEL interrupt line is coming
> from?
> 
> The TRM description of the CSI_LEVEL interrupt says it is for PSI_L overflow
> or VP0/VP1 frame/line mismatch, both of which are outside the Cadence CSI2RX,
> instead belonging to the TI wrapper hardware, which has its own driver.

Yes, we will update the device tree bindings accordingly. We are 
planning to handle the TI-specific interrupt line separately in another 
series.

> 
>>> This does not match with the error, info and monitor line names mentioned
>>> here.
>>>
>>> In my understanding which interrupt lines are actually integrated will vary
>>> from SoC to SoC, so please check what are the actual interrupt line names
>>> exposed by the Cadence IP. Maybe Maxime knows more.
>>>
>>> But I don't think it is correct to make all 3 mandatory together, as some
>>> vendors may only integrate the error interrupt ignoring the rest.
>>
>> Agreed.
>>
> 
> I think by default there should only be two optional interrupt lines for
> cdns-csi2rx, "irq" and "err_irq" which is common across vendors.
> 
> If this third TI-specific CSI_LEVEL interrupt *has* to be handled by
> cdns-csi2rx driver, it would be better to create conditional bindings and
> match data in the driver such that the irq is only requested if
> "ti,j721e-csi2rx" compatible is present.

I will correct this in v3 by introducing bindings that allow vendors
the flexibility to use either two or three interrupt lines.

Thanks and Regards,
Yemike Abhilash Chandra

> 
>> Best Regards,
>> Changhuang
> 
> Thanks,
> Jai