RE: [PATCH v3 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem

[Vishal Sagar]

Hi Luca,

Thanks for reviewing this. 

> -----Original Message-----
> From: Luca Ceresoli [mailto:luca@xxxxxxxxxxxxxxxx]
> Sent: Monday, February 11, 2019 4:12 PM
> To: Vishal Sagar <vishal.sagar@xxxxxxxxxx>; Hyun Kwon <hyunk@xxxxxxxxxx>;
> laurent.pinchart@xxxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx;
> robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal Simek
> <michals@xxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx;
> hans.verkuil@xxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Dinesh Kumar <dineshk@xxxxxxxxxx>; Sandip Kothari
> <sandipk@xxxxxxxxxx>
> Subject: Re: [PATCH v3 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
> Subsystem
> 
> EXTERNAL EMAIL
> 
> Hi Vishal,
> 
> On 01/02/19 13:56, Vishal Sagar wrote:
> > The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
> > from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
> > for image processing. Please refer to PG232 for details.
> 
> For those unused to Xilinx documentation I'd use the full document name
> ("MIPI CSI-2 Receiver Subsystem v4.0") or, even better, a stable URL if
> available.
> 

Ok. I will add the full documentation name here and URL.

> > The driver is used to set the number of active lanes, if enabled
> > in hardware. The CSI2 Rx controller filters out all packets except for
> > the packets with data type fixed in hardware. RAW8 packets are always
> > allowed to pass through.
> >
> > It is also used to setup and handle interrupts and enable the core. It
> > logs all the events in respective counters between streaming on and off.
> > The generic short packets received are notified to application via
> > v4l2_events.
> >
> > The driver supports only the video format bridge enabled configuration.
> > Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when
> the
> > CSI v2.0 feature is enabled in design. When the VCX feature is enabled,
> > the maximum number of virtual channels becomes 16 from 4.
> >
> > Signed-off-by: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> 
> ...
> 
> > +/**
> > + * xcsi2rxss_reset - Does a soft reset of the MIPI CSI2 Rx Subsystem
> > + * @core: Core Xilinx CSI2 Rx Subsystem structure pointer
> > + *
> > + * Core takes less than 100 video clock cycles to reset.
> > + * So a larger timeout value is chosen for margin.
> > + *
> > + * Return: 0 - on success OR -ETIME if reset times out
> > + */
> > +static int xcsi2rxss_reset(struct xcsi2rxss_core *core)
> > +{
> > +     u32 timeout = XCSI_TIMEOUT_VAL;
> 
> The comment about the timeout is duplicated here and at the #define
> line. Why not removing the define above and just putting
> 
>   u32 timeout = 1000; /* us */
> 
> here? It would make the entire timeout logic appear in a unique place.
> 

Agree. It was kept like that as the timeout value was being used in some other place in earlier patch versions.
I will do as suggested in next version.

> > +static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state)
> > +{
> > +     struct xcsi2rxss_core *core = &state->core;
> > +     int ret = 0;
> > +
> > +     xcsi2rxss_enable(core);
> > +
> > +     ret = xcsi2rxss_reset(core);
> > +     if (ret < 0) {
> > +             state->streaming = false;
> > +             return ret;
> > +     }
> > +
> > +     xcsi2rxss_intr_enable(core);
> > +     state->streaming = true;
> 
> Shouldn't you propagate s_stream to the upstream subdev here calling
> v4l2_subdev_call(..., ..., s_stream, 1)?
> 

This is done by the xvip_pipeline_start_stop() in xilinx-dma.c for Xilinx Video pipeline.

> > +     return ret;
> > +}
> 
> 
> --
> Luca

Regards
Vishal Sagar