Hi Hyun, Thanks for reviewing. Apologies for the delayed response. > -----Original Message----- > From: Hyun Kwon [mailto:hyun.kwon@xxxxxxxxxx] > Sent: Thursday, February 14, 2019 1:16 AM > To: Vishal Sagar <vishal.sagar@xxxxxxxxxx> > Cc: 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: [v3,2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem > > Hi Vishal, > > Thanks for the patch. Sorry for the delay. > > On Fri, 2019-02-01 at 18:26:06 +0530, 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 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> > > --- > > v3 > > - Fixed comments given by Hyun. > > - Removed DPHY 200 MHz clock. This will be controlled by DPHY driver > > - Minor code formatting > > - en_csi_v20 and vfb members removed from struct and made local to dt > parsing > > [snip] > > > +}; > > + > > +/* > > + * struct xcsi2rxss_core - Core configuration CSI2 Rx Subsystem device > structure > > + * @dev: Platform structure > > + * @iomem: Base address of subsystem > > + * @irq: requested irq number > > + * @enable_active_lanes: If number of active lanes can be modified > > + * @max_num_lanes: Maximum number of lanes present > > + * @datatype: Data type filter > > + * @bayer: bayer pattern > > + * @events: Structure to maintain event logs > > + * @vcx_events: Structure to maintain VCX event logs > > + * @en_vcx: If more than 4 VC are enabled > > + * @lite_aclk: AXI4-Lite interface clock > > + * @video_aclk: Video clock > > + */ > > +struct xcsi2rxss_core { > > + struct device *dev; > > + void __iomem *iomem; > > + int irq; > > This doesn't have to be stored. Agree. I will remove it in next version. > > > + bool enable_active_lanes; > > + u32 max_num_lanes; > > + u32 datatype; > > + u32 bayer; > > + struct xcsi2rxss_event *events; > > + struct xcsi2rxss_event *vcx_events; > > + bool en_vcx; > > + struct clk *lite_aclk; > > + struct clk *video_aclk; > > +}; > > + > > +/** > > + * struct xcsi2rxss_state - CSI2 Rx Subsystem device structure > > + * @core: Core structure for MIPI CSI2 Rx Subsystem > > + * @subdev: The v4l2 subdev structure > > [snip] > > > + > > +/* Print event counters */ > > +static void xcsi2rxss_log_counters(struct xcsi2rxss_state *state) > > +{ > > + struct xcsi2rxss_core *core = &state->core; > > + int i; > > + > > + for (i = 0; i < XCSI_NUM_EVENTS; i++) { > > + if (core->events[i].counter > 0) > > Does checkpatch warn if putting {} here, and > > > + dev_info(core->dev, "%s events: %d\n", > > + core->events[i].name, > > + core->events[i].counter); > > + } > > + > > + if (core->en_vcx) > > here, and > > > + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) { > > + if (core->vcx_events[i].counter > 0) > > here? But up to you. No checkpatch doesn't give warning. > > > + dev_info(core->dev, > > + "VC %d Frame %s err vcx events: %d\n", > > + (i / 2) + XCSI_VCX_START, > > + i & 1 ? "Sync" : "Level", > > + core->vcx_events[i].counter); > > + } > > +} > > + > > +/** > > + * xcsi2rxss_log_status - Logs the status of the CSI-2 Receiver > > + * @sd: Pointer to V4L2 subdevice structure > > + * > > + * This function prints the current status of Xilinx MIPI CSI-2 > > [snip] > > > + > > +/** > > + * xcsi2rxss_s_ctrl - This is used to set the Xilinx MIPI CSI-2 V4L2 controls > > + * @ctrl: V4L2 control to be set > > + * > > + * This function is used to set the V4L2 controls for the Xilinx MIPI > > + * CSI-2 Rx Subsystem. It is used to set the active lanes in the system. > > + * The event counters can be reset. > > + * > > + * Return: 0 on success, errors otherwise > > + */ > > +static int xcsi2rxss_s_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct xcsi2rxss_state *xcsi2rxss = > > + container_of(ctrl->handler, struct xcsi2rxss_state, > > + ctrl_handler); > > + struct xcsi2rxss_core *core = &xcsi2rxss->core; > > + int ret = 0; > > + > > + mutex_lock(&xcsi2rxss->lock); > > + > > + switch (ctrl->id) { > > + case V4L2_CID_XILINX_MIPICSISS_ACT_LANES: > > + /* > > + * This will be called only when "Enable Active Lanes" parameter > > + * is set in design > > + */ > > + if (core->enable_active_lanes) { > > + u32 active_lanes; > > + > > + xcsi2rxss_clr_and_set(core, XCSI_PCR_OFFSET, > > + XCSI_PCR_ACTLANES_MASK, > > + ctrl->val - 1); > > + /* > > + * This delay is to allow the value to reflect as write > > + * and read paths are different. > > + */ > > + udelay(1); > > + active_lanes = xcsi2rxss_read(core, XCSI_PCR_OFFSET); > > + active_lanes &= XCSI_PCR_ACTLANES_MASK; > > + active_lanes++; > > + if (active_lanes != ctrl->val) > > + dev_info(core->dev, "RxByteClkHS absent\n"); > > + dev_dbg(core->dev, "active lanes = %d\n", ctrl->val); > > + } else { > > + ret = -EINVAL; > > + } > > + break; > > + case V4L2_CID_XILINX_MIPICSISS_RESET_COUNTERS: > > + xcsi2rxss_reset_event_counters(xcsi2rxss); > > + break; > > + default: > > -EINVAL? Agree. I will add it in next version. > > > + break; > > + } > > + > > + mutex_unlock(&xcsi2rxss->lock); > > + > > + return ret; > > +} > > + > > [snip] > > > + > > +static void xcsi2rxss_clk_disable(struct xcsi2rxss_core *core) > > +{ > > + clk_disable_unprepare(core->video_aclk); > > + clk_disable_unprepare(core->lite_aclk); > > +} > > + > > +static int xcsi2rxss_parse_of(struct xcsi2rxss_state *xcsi2rxss) > > +{ > > + struct xcsi2rxss_core *core = &xcsi2rxss->core; > > + struct device_node *node = xcsi2rxss->core.dev->of_node; > > + struct device_node *ports = NULL; > > + struct device_node *port = NULL; > > + unsigned int nports; > > + bool en_csi_v20, vfb; > > + int ret; > > + > > + en_csi_v20 = of_property_read_bool(node, "xlnx,en-csi-v2-0"); > > + if (en_csi_v20) { > > + core->en_vcx = of_property_read_bool(node, "xlnx,en-vcx"); > > + dev_dbg(core->dev, "vcx %s", core->en_vcx ? "enabled" : > > + "disabled"); > > + } > > + > > + dev_dbg(core->dev, "en_csi_v20 %s", en_csi_v20 ? "enabled" : > > + "disabled"); > > Would it be better to have these options visible with log? > It can be printed as a part of final banner in probe. Up to you. > Ok. Will put it under a new function xcsi2rxss_log_ipconfig() > > + > > + core->enable_active_lanes = > > + of_property_read_bool(node, "xlnx,en-active-lanes"); > > + dev_dbg(core->dev, "Enable active lanes property = %s\n", > > + core->enable_active_lanes ? "Present" : "Absent"); > > + > > + ret = of_property_read_u32(node, "xlnx,csi-pxl-format", > > + &core->datatype); > > + if (ret < 0) { > > + dev_err(core->dev, "missing xlnx,csi-pxl-format property\n"); > > + return ret; > > + } > > + > > + switch (core->datatype) { > > + case XCSI_DT_YUV4228B: > > + case XCSI_DT_RGB444: > > + case XCSI_DT_RGB555: > > + case XCSI_DT_RGB565: > > + case XCSI_DT_RGB666: > > + case XCSI_DT_RGB888: > > + case XCSI_DT_RAW6: > > + case XCSI_DT_RAW7: > > + case XCSI_DT_RAW8: > > + case XCSI_DT_RAW10: > > + case XCSI_DT_RAW12: > > + case XCSI_DT_RAW14: > > + break; > > + case XCSI_DT_YUV42210B: > > + case XCSI_DT_RAW16: > > + case XCSI_DT_RAW20: > > + if (!en_csi_v20) { > > + ret = -EINVAL; > > + dev_dbg(core->dev, "enable csi v2 for this pixel > format"); > > + } > > + break; > > + default: > > + ret = -EINVAL; > > + } > > + > > + if (ret < 0) { > > + dev_err(core->dev, "invalid csi-pxl-format property!\n"); > > + return ret; > > + } > > + > > + dev_dbg(core->dev, "pixel format set as 0x%x\n", core->datatype); > > + > > + vfb = of_property_read_bool(node, "xlnx,vfb"); > > + dev_dbg(core->dev, "Video Format Bridge property = %s\n", > > + vfb ? "Present" : "Absent"); > > I'd collect all these print in one print or one location. But up to you. Ok. Will put it under a new function xcsi2rxss_log_ipconfig() > > > + > > + if (!vfb) { > > + dev_err(core->dev, "failed as VFB is disabled!\n"); > > + return -EINVAL; > > + } > > [snip] > > > + /* Register interrupt handler */ > > + core->irq = irq_of_parse_and_map(node, 0); > > + ret = devm_request_irq(core->dev, core->irq, xcsi2rxss_irq_handler, > > + IRQF_SHARED, "xilinx-csi2rxss", xcsi2rxss); > > + if (ret) { > > + dev_err(core->dev, "Err = %d Interrupt handler reg failed!\n", > > + ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int xcsi2rxss_probe(struct platform_device *pdev) > > +{ > > + struct v4l2_subdev *subdev; > > + struct xcsi2rxss_state *xcsi2rxss; > > + struct xcsi2rxss_core *core; > > + struct resource *res; > > + int ret, num_ctrls, i; > > + > > + xcsi2rxss = devm_kzalloc(&pdev->dev, sizeof(*xcsi2rxss), GFP_KERNEL); > > + if (!xcsi2rxss) > > + return -ENOMEM; > > + > > + core = &xcsi2rxss->core; > > + core->dev = &pdev->dev; > > + > > + mutex_init(&xcsi2rxss->lock); > > + > > + ret = xcsi2rxss_parse_of(xcsi2rxss); > > + if (ret < 0) > > + return ret; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + core->iomem = devm_ioremap_resource(core->dev, res); > > + if (IS_ERR(core->iomem)) > > + return PTR_ERR(core->iomem); > > + > > + ret = xcsi2rxss_clk_get(core); > > + if (ret < 0) > > + return ret; > > + > > + ret = xcsi2rxss_clk_enable(core); > > + if (ret < 0) > > + return ret; > > Not sure if I mentioned, please consider having these with > stream on / off later. We don't have a mechanism to reset the video path through software if it is connected to a Processor System Reset. So for now I will keep it here. > > I only have minor comments, and it looks fine to me. Please take a look at > those comments, then > > Reviewed-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx> > > Thanks, > -hyun Thanks, Vishal
