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

[Jai Luthra]

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@xxxxxx>
> > > ---
> > >
> > > 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.

> > 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.

> Best Regards,
> Changhuang

Thanks,
Jai

Attachment: signature.asc
Description: PGP signature