Hi Luca, Thanks for reviewing!
> -----Original Message----- > From: Luca Ceresoli <[email protected]> > Sent: Monday, May 25, 2020 6:44 PM > To: Vishal Sagar <[email protected]>; Hyun Kwon <[email protected]>; > [email protected]; [email protected]; > [email protected]; [email protected]; Michal Simek > <[email protected]>; [email protected]; > [email protected]; [email protected]; linux-arm- > [email protected]; [email protected]; Dinesh Kumar > <[email protected]>; Sandip Kothari <[email protected]>; Jacopo Mondi > <[email protected]> > Cc: Hyun Kwon <[email protected]> > Subject: Re: [PATCH v13 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx > Subsystem driver > > Hi Vishal, > > thanks. I have only a few minor nitpicking comments. > > On 12/05/20 17:19, 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. > > > > 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 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 <[email protected]> > > Reviewed-by: Hyun Kwon <[email protected]> > > Reviewed-by: Laurent Pinchart <[email protected]> > > [...] > > > +static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state) { > > + int ret = 0; > > + > > + /* enable core */ > > + xcsi2rxss_set(state, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE); > > + > > + ret = xcsi2rxss_soft_reset(state); > > + if (ret < 0) { > > 'if (ret)' is enough, it's a classic nonzero-on-error return value. > Agreed. I will fix it in next version. > > +/** > > + * xcsi2rxss_irq_handler - Interrupt handler for CSI-2 > > + * @irq: IRQ number > > + * @data: Pointer to device state > > + * > > + * In the interrupt handler, a list of event counters are updated for > > + * corresponding interrupts. This is useful to get status / debug. > > + * > > + * Return: IRQ_HANDLED after handling interrupts */ static > > +irqreturn_t xcsi2rxss_irq_handler(int irq, void *data) { > > + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)data; > > + struct device *dev = state->dev; > > + u32 status; > > + > > + status = xcsi2rxss_read(state, XCSI_ISR_OFFSET) & > XCSI_ISR_ALLINTR_MASK; > > + xcsi2rxss_write(state, XCSI_ISR_OFFSET, status); > > + > > + /* Received a short packet */ > > + if (status & XCSI_ISR_SPFIFONE) { > > + u32 count = 0; > > + > > + /* > > + * Drain generic short packet FIFO by reading max 31 > > + * (fifo depth) short packets from fifo or till fifo is empty. > > + */ > > + for (count = 0; count < XCSI_SPKT_FIFO_DEPTH; ++count) { > > + u32 spfifostat, spkt; > > + > > + spkt = xcsi2rxss_read(state, XCSI_SPKTR_OFFSET); > > + dev_dbg(dev, "Short packet = 0x%08x\n", spkt); > > + spfifostat = xcsi2rxss_read(state, XCSI_ISR_OFFSET); > > + spfifostat &= XCSI_ISR_SPFIFONE; > > + if (!spfifostat) > > + break; > > + xcsi2rxss_write(state, XCSI_ISR_OFFSET, spfifostat); > > + } > > + } > > + > > + /* Short packet FIFO overflow */ > > + if (status & XCSI_ISR_SPFIFOF) > > + dev_dbg_ratelimited(dev, "Short packet FIFO overflowed\n"); > > + > > + /* > > + * Stream line buffer full > > + * This means there is a backpressure from downstream IP > > + */ > > + if (status & XCSI_ISR_SLBF) { > > + dev_alert_ratelimited(dev, "Stream Line Buffer Full!\n"); > > + > > + /* disable interrupts */ > > + xcsi2rxss_clr(state, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK); > > + xcsi2rxss_clr(state, XCSI_GIER_OFFSET, XCSI_GIER_GIE); > > + > > + /* disable core */ > > + xcsi2rxss_clr(state, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE); > > + state->streaming = false; > > + > > + /* > > + * The IP needs to be hard reset before it can be used now. > > + * This will be done in streamoff. > > + */ > > + > > + /* > > + * TODO: Notify the whole pipeline with v4l2_subdev_notify() > to > > + * inform userspace. > > + */ > > + } > > + > > + /* Increment event counters */ > > + if (status & XCSI_ISR_ALLINTR_MASK) { > > + unsigned int i; > > + > > + for (i = 0; i < XCSI_NUM_EVENTS; i++) { > > + if (!(status & xcsi2rxss_events[i].mask)) > > + continue; > > + state->events[i]++; > > + dev_dbg_ratelimited(dev, "%s: %u\n", > > + xcsi2rxss_events[i].name, > > + state->events[i]); > > + } > > + > > + if (status & XCSI_ISR_VCXFE && state->en_vcx) { > > + u32 vcxstatus; > > + > > + vcxstatus = xcsi2rxss_read(state, XCSI_VCXR_OFFSET); > > + vcxstatus &= XCSI_VCXR_VCERR; > > + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) { > > + if (!(vcxstatus & (1 << i))) > > You can use BIT(i) instead of (1 << i). Yep that is a good alternative. > > > +/** > > + * xcsi2rxss_set_format - This is used to set the pad format > > + * @sd: Pointer to V4L2 Sub device structure > > + * @cfg: Pointer to sub device pad information structure > > + * @fmt: Pointer to pad level media bus format > > + * > > + * This function is used to set the pad format. Since the pad format > > +is fixed > > + * in hardware, it can't be modified on run time. So when a format > > +set is > > + * requested by application, all parameters except the format type is > > +saved > > + * for the pad and the original pad format is sent back to the application. > > + * > > + * Return: 0 on success > > + */ > > +static int xcsi2rxss_set_format(struct v4l2_subdev *sd, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd); > > + struct v4l2_mbus_framefmt *__format; > > + u32 dt; > > + > > + /* only sink pad format can be updated */ > > This comment should be placed... > > > + mutex_lock(&xcsi2rxss->lock); > > + > > + /* > > + * Only the format->code parameter matters for CSI as the > > + * CSI format cannot be changed at runtime. > > + * Ensure that format to set is copied to over to CSI pad format > > + */ > > + __format = __xcsi2rxss_get_pad_format(xcsi2rxss, cfg, > > + fmt->pad, fmt->which); > > + > > ...here. > Ok will move the comment here. > > + if (fmt->pad == XVIP_PAD_SOURCE) { > > + fmt->format = *__format; > > + mutex_unlock(&xcsi2rxss->lock); > > + return 0; > > + } > > + > > + /* > > + * RAW8 is supported in all datatypes. So if requested media bus > format > > + * is of RAW8 type, then allow to be set. In case core is configured to > > + * other RAW, YUV422 8/10 or RGB888, set appropriate media bus > format. > > + */ > > + dt = xcsi2rxss_get_dt(fmt->format.code); > > + if (dt != xcsi2rxss->datatype && dt != XCSI_DT_RAW8) { > > + dev_dbg(xcsi2rxss->dev, "Unsupported media bus format"); > > + /* set the default format for the data type */ > > + fmt->format.code = xcsi2rxss_get_nth_mbus(xcsi2rxss- > >datatype, > > + 0); > > + } > > + > > + *__format = fmt->format; > > + mutex_unlock(&xcsi2rxss->lock); > > + > > + return 0; > > +} > > + > > +/* > > + * xcsi2rxss_enum_mbus_code - Handle pixel format enumeration > > + * @sd : pointer to v4l2 subdev structure > > + * @cfg: V4L2 subdev pad configuration > > + * @code : pointer to v4l2_subdev_mbus_code_enum structure > > Remove space before colon here. > > Looks good otherwise, and my comments are minor details so: > Reviewed-by: Luca Ceresoli <[email protected]> That’s great. Thanks for reviewing this again. > > I tried to runtime test this driver as well replacing the v10 driver that I'm > using > at the moment, but ran into many problems, apparently in the media entity > navigation. The diff between v10 and v13 does not justify these problems, so > I'm assuming v13 needs a more recent kernel than the 4.19 I'm currentl stuck > on. > > -- > Luca Ceresoli Regards Vishal Sagar
