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
