[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <998346fc-9228-49c3-bac7-9a701a0cc5e6@ti.com>
Date: Fri, 21 Feb 2025 10:31:51 +0530
From: Yemike Abhilash Chandra <y-abhilashchandra@...com>
To: Jai Luthra <jai.luthra@...ux.dev>,
Changhuang Liang
<changhuang.liang@...rfivetech.com>
CC: "mripard@...nel.org" <mripard@...nel.org>,
"linux-media@...r.kernel.org"
<linux-media@...r.kernel.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>,
"mchehab@...nel.org" <mchehab@...nel.org>,
"robh@...nel.org" <robh@...nel.org>,
"krzk+dt@...nel.org"
<krzk+dt@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>,
"devarsht@...com" <devarsht@...com>,
"vaishnav.a@...com" <vaishnav.a@...com>,
"r-donadkar@...com" <r-donadkar@...com>,
"u-kumar1@...com" <u-kumar1@...com>
Subject: Re: 回复: [PATCH v2 1/2] dt-bindings: media: cdns,csi2rx.yaml: Add optional interrupts for cdns-csi2rx
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@...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
Powered by blists - more mailing lists