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

[Vishal Sagar]

Hi Sakari,

Thanks for the review. 

> -----Original Message-----
> From: Sakari Ailus [mailto:sakari.ailus@xxxxxxxxxxxxxxx]
> Sent: Friday, March 22, 2019 9:31 PM
> To: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> Cc: Hyun Kwon <hyunk@xxxxxxxxxx>; laurent.pinchart@xxxxxxxxxxxxxxxx;
> mchehab@xxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Michal
> Simek <michals@xxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; hans.verkuil@xxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Dinesh Kumar
> <dineshk@xxxxxxxxxx>; Sandip Kothari <sandipk@xxxxxxxxxx>; Luca Ceresoli
> <luca@xxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v7 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx
> Subsystem driver
> 
> EXTERNAL EMAIL
> 
> Hi Vishal,
> 
> On Thu, Mar 14, 2019 at 04:54:51PM +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>
> > Reviewed-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> > ---
> > v7
> > - No change
> >
> > v6
> > - No change
> >
> > v5
> > - Removed bayer and updated related parts like set default format based
> >   on Luca Cersoli's comments.
> > - Added correct YUV422 10bpc media bus format
> >
> > v4
> > - Removed irq member from core structure
> > - Consolidated IP config prints in xcsi2rxss_log_ipconfig()
> > - Return -EINVAL in case of invalid ioctl
> > - Code formatting
> > - Added reviewed by Hyun Kwon
> >
> > 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
> > - lock description updated
> > - changed to ratelimited type for all dev prints in irq handler
> > - Removed YUV 422 10bpc media format
> >
> > v2
> > - Fixed comments given by Hyun and Sakari.
> > - Made all bitmask using BIT() and GENMASK()
> > - Removed unused definitions
> > - Removed DPHY access. This will be done by separate DPHY PHY driver.
> > - Added support for CSI v2.0 for YUV 422 10bpc, RAW16, RAW20 and extra
> >   virtual channels
> > - Fixed the ports as sink and source
> > - Now use the v4l2fwnode API to get number of data-lanes
> > - Added clock framework support
> > - Removed the close() function
> > - updated the set format function
> > - support only VFB enabled configuration
> >
> >  drivers/media/platform/xilinx/Kconfig           |   10 +
> >  drivers/media/platform/xilinx/Makefile          |    1 +
> >  drivers/media/platform/xilinx/xilinx-csi2rxss.c | 1465
> +++++++++++++++++++++++
> >  include/uapi/linux/xilinx-v4l2-controls.h       |   14 +
> >  include/uapi/linux/xilinx-v4l2-events.h         |   25 +
> >  5 files changed, 1515 insertions(+)
> >  create mode 100644 drivers/media/platform/xilinx/xilinx-csi2rxss.c
> >  create mode 100644 include/uapi/linux/xilinx-v4l2-events.h
> >
> > diff --git a/drivers/media/platform/xilinx/Kconfig
> b/drivers/media/platform/xilinx/Kconfig
> > index 74ec8aa..30b4a25 100644
> > --- a/drivers/media/platform/xilinx/Kconfig
> > +++ b/drivers/media/platform/xilinx/Kconfig
> > @@ -10,6 +10,16 @@ config VIDEO_XILINX
> >
> >  if VIDEO_XILINX
> >

<snip>

> > + *
> > + * Return: 0 on success, errors otherwise
> > + */
> > +static int xcsi2rxss_subscribe_event(struct v4l2_subdev *sd,
> > +                                  struct v4l2_fh *fh,
> > +                                  struct v4l2_event_subscription *sub)
> > +{
> > +     struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> > +     int ret;
> > +
> > +     mutex_lock(&xcsi2rxss->lock);
> > +
> > +     switch (sub->type) {
> > +     case V4L2_EVENT_XLNXCSIRX_SPKT:
> > +     case V4L2_EVENT_XLNXCSIRX_SPKT_OVF:
> > +     case V4L2_EVENT_XLNXCSIRX_SLBF:
> > +             ret = v4l2_event_subscribe(fh, sub, XCSI_MAX_SPKT_EVENT, NULL);
> 
> What's your use case for notifying the user about the short packets?

I don't have a use case.
My motivation was that v4l2 events would be a good way to notify and share short packet with application.

> Generally these are control messages of some kind that do not need handling
> by the user.
> 
> If it's just debugging, you could print them using dev_dbg().
> 
> Same for the frame counter.
> 

Ok. I will be removing these in the next version.

> > +             break;
> > +     default:
> > +             ret = -EINVAL;
> > +     }
> > +
> > +     mutex_unlock(&xcsi2rxss->lock);
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * xcsi2rxss_unsubscribe_event - Unsubscribe from all events registered
> > + * @sd: V4L2 Sub device
> > + * @fh: V4L2 file handle
> > + * @sub: pointer to Event unsubscription structure
> > + *

<snip>

> > +static struct v4l2_mbus_framefmt *
> > +__xcsi2rxss_get_pad_format(struct xcsi2rxss_state *xcsi2rxss,
> > +                        struct v4l2_subdev_pad_config *cfg,
> > +                        unsigned int pad, u32 which)
> > +{
> > +     switch (which) {
> > +     case V4L2_SUBDEV_FORMAT_TRY:
> > +             return v4l2_subdev_get_try_format(&xcsi2rxss->subdev, cfg,
> > +                                               pad);
> 
> Fits on a single line.
> 

Agree. I will fix this in next version.

> > +     case V4L2_SUBDEV_FORMAT_ACTIVE:
> > +             return &xcsi2rxss->format;
> > +     default:
> > +             return NULL;
> > +     }
> > +}
> > +
> > +/**

<snip>

> > +static int xcsi2rxss_clk_get(struct xcsi2rxss_core *core)
> > +{
> > +     int ret;
> > +
> > +     core->lite_aclk = devm_clk_get(core->dev, "lite_aclk");
> > +     if (IS_ERR(core->lite_aclk)) {
> > +             ret = PTR_ERR(core->lite_aclk);
> > +             dev_err(core->dev, "failed to get lite_aclk (%d)\n",
> > +                     ret);
> > +             return ret;
> > +     }
> > +
> > +     core->video_aclk = devm_clk_get(core->dev, "video_aclk");
> > +     if (IS_ERR(core->video_aclk)) {
> > +             ret = PTR_ERR(core->video_aclk);
> > +             dev_err(core->dev, "failed to get video_aclk (%d)\n",
> > +                     ret);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int xcsi2rxss_clk_enable(struct xcsi2rxss_core *core)
> > +{
> > +     int ret;
> > +
> > +     ret = clk_prepare_enable(core->lite_aclk);
> > +     if (ret) {
> > +             dev_err(core->dev, "failed enabling lite_aclk (%d)\n",
> > +                     ret);
> > +             return ret;
> > +     }
> > +
> > +     ret = clk_prepare_enable(core->video_aclk);
> > +     if (ret) {
> > +             dev_err(core->dev, "failed enabling video_aclk (%d)\n",
> > +                     ret);
> > +             clk_disable_unprepare(core->lite_aclk);
> > +             return ret;
> > +     }
> > +
> 
> Could you use the *_clk_bulk_* APIs instead in the two functions? Below,
> too.
> 

Ok. I will fix this in next version.

> > +     return ret;
> > +}
> > +
> > +static void xcsi2rxss_clk_disable(struct xcsi2rxss_core *core)
> > +{
> > +     clk_disable_unprepare(core->video_aclk);
> > +     clk_disable_unprepare(core->lite_aclk);
> > +}
> > +

<snip>

> > +/* Short packet FIFO overflow */
> > +#define V4L2_EVENT_XLNXCSIRX_SPKT_OVF
> (V4L2_EVENT_XLNXCSIRX_CLASS | 0x2)
> > +/* Stream Line Buffer full */
> > +#define V4L2_EVENT_XLNXCSIRX_SLBF    (V4L2_EVENT_XLNXCSIRX_CLASS |
> 0x3)
> > +
> > +#endif /* __UAPI_XILINX_V4L2_EVENTS_H__ */
> 
> --
> Regards,
> 
> Sakari Ailus
> sakari.ailus@xxxxxxxxxxxxxxx


Regards
Vishal Sagar