Message ID | 20210915120240.21572-1-p.yadav@ti.com |
---|---|
Headers | show |
Series | CSI2RX support on J721E | expand |
Hi, On 15/09/21 05:32PM, Pratyush Yadav wrote: > Hi, > > This series adds support for CSI2 capture on J721E. It includes some > fixes to the Cadence CSI2RX driver, re-structures the TI platform > drivers, and finally adds the TI CSI2RX wrapper driver. > > This series used to include the DPHY and DMA engine patches as well, but > they have been split off to facilitate easier merging. > > Tested on TI's J721E with OV5640 sensor. > > The branch with all the patches needed to enable testing (dts nodes, > OV5640 dropped patch, etc.) can be found here at > https://github.com/prati0100/linux-next/ branch "capture". There are no pending comments on this series. Can this be merged please? > > Changes in v4: > - Drop the call to set PHY submode. It is now being done via compatible > on the DPHY side. > - Acquire the media device's graph_mutex before starting the graph walk. > - Call media_graph_walk_init() and media_graph_walk_cleanup() when > starting and ending the graph walk respectively. > - Reduce max frame height and width in enum_framesizes. Currently they > are set to UINT_MAX but they must be a multiple of step_width, so they > need to be rounded down. Also, these values are absurdly large which > causes some userspace applications like gstreamer to trip up. While it > is not generally right to change the kernel for an application bug, it > is not such a big deal here. This change is replacing one set of > absurdly large arbitrary values with another set of smaller but still > absurdly large arbitrary values. Both limits are unlikely to be hit in > practice. > - Add power-domains property. > - Drop maxItems from clock-names. > - Drop the type for data-lanes. > - Drop uniqueItems from data-lanes. Move it to video-interfaces.yaml > instead. > - Drop OV5640 runtime pm patch. It seems to be a bit complicated and it > is not exactly necessary for this series. Any CSI-2 camera will work > just fine, OV5640 just happens to be the one I tested with. I don't > want it to block this series. I will submit it as a separate patch > later. > > Changes in v3: > - Use v4l2_get_link_freq() to calculate pixel clock. > - Move DMA related fields in struct ti_csi2rx_dma. > - Protect DMA buffer queue with a spinlock to make sure the queue buffer > and DMA callback don't race on it. > - Track the current DMA state. It might go idle because of a lack of > buffers. This state can be used to restart it if needed. > - Do not include the current buffer in the pending queue. It is slightly > better modelling than leaving it at the head of the pending queue. > - Use the buffer as the callback argument, and add a reference to csi in it. > - If queueing a buffer to DMA fails, the buffer gets leaked and DMA gets > stalled with. Instead, report the error to vb2 and queue the next > buffer in the pending queue. > - DMA gets stalled if we run out of buffers since the callback is the > only one that fires subsequent transfers and it is no longer being > called. Check for that when queueing buffers and restart DMA if > needed. > - Do not put of node until we are done using the fwnode. > - Set inital format to UYVY 640x480. > - Add compatible: contains: const: cdns,csi2rx to allow SoC specific > compatible. > - Add more constraints for data-lanes property. > > Changes in v2: > - Use phy_pm_runtime_get_sync() and phy_pm_runtime_put() before making > calls to set PHY mode, etc. to make sure it is ready. > - Use dmaengine_get_dma_device() instead of directly accessing > dma->device->dev. > - Do not set dst_addr_width when configuring slave DMA. > - Move to a separate subdir and rename to j721e-csi2rx.c > - Convert compatible to ti,j721e-csi2rx. > - Move to use Media Controller centric APIs. > - Improve cleanup in probe when one of the steps fails. > - Add colorspace to formats database. > - Set hw_revision on media_device. > - Move video device initialization to probe time instead of register time. > - Rename to ti,j721e-csi2rx.yaml > - Add an entry in MAINTAINERS. > - Add a description for the binding. > - Change compatible to ti,j721e-csi2rx to make it SoC specific. > - Remove description from dmas, reg, power-domains. > - Remove a limit of 2 from #address-cells and #size-cells. > - Fix add ^ to csi-bridge subnode regex. > - Make ranges mandatory. > - Add unit address in example. > - Add a reference to cdns,csi2rx in csi-bridge subnode. > - Expand the example to include the csi-bridge subnode as well. > - Re-order subject prefixes. > - Convert OV5640 to use runtime PM and drop Cadence CSI2RX s_power patch. > - Drop subdev call wrappers from cdns-csi2rx. > - Move VPE and CAL to a separate subdir. > - Rename ti-csi2rx.c to j721e-csi2rx.c > > Pratyush Yadav (11): > media: cadence: csi2rx: Unregister v4l2 async notifier > media: cadence: csi2rx: Add external DPHY support > media: cadence: csi2rx: Soft reset the streams before starting capture > media: cadence: csi2rx: Set the STOP bit when stopping a stream > media: cadence: csi2rx: Fix stream data configuration > media: cadence: csi2rx: Populate subdev devnode > media: Re-structure TI platform drivers > media: ti: Add CSI2RX support for J721E > media: dt-bindings: Make sure items in data-lanes are unique > media: dt-bindings: Add DT bindings for TI J721E CSI2RX driver > media: dt-bindings: Convert Cadence CSI2RX binding to YAML > > .../devicetree/bindings/media/cdns,csi2rx.txt | 100 -- > .../bindings/media/cdns,csi2rx.yaml | 169 +++ > .../bindings/media/ti,j721e-csi2rx.yaml | 101 ++ > .../bindings/media/video-interfaces.yaml | 1 + > MAINTAINERS | 10 +- > drivers/media/platform/Kconfig | 12 + > drivers/media/platform/Makefile | 2 +- > drivers/media/platform/cadence/cdns-csi2rx.c | 184 ++- > drivers/media/platform/ti/Makefile | 4 + > drivers/media/platform/ti/cal/Makefile | 3 + > .../{ti-vpe => ti/cal}/cal-camerarx.c | 0 > .../platform/{ti-vpe => ti/cal}/cal-video.c | 0 > .../media/platform/{ti-vpe => ti/cal}/cal.c | 0 > .../media/platform/{ti-vpe => ti/cal}/cal.h | 0 > .../platform/{ti-vpe => ti/cal}/cal_regs.h | 0 > .../media/platform/ti/j721e-csi2rx/Makefile | 2 + > .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 1008 +++++++++++++++++ > .../platform/{ti-vpe => ti/vpe}/Makefile | 4 - > .../media/platform/{ti-vpe => ti/vpe}/csc.c | 0 > .../media/platform/{ti-vpe => ti/vpe}/csc.h | 0 > .../media/platform/{ti-vpe => ti/vpe}/sc.c | 0 > .../media/platform/{ti-vpe => ti/vpe}/sc.h | 0 > .../platform/{ti-vpe => ti/vpe}/sc_coeff.h | 0 > .../media/platform/{ti-vpe => ti/vpe}/vpdma.c | 0 > .../media/platform/{ti-vpe => ti/vpe}/vpdma.h | 0 > .../platform/{ti-vpe => ti/vpe}/vpdma_priv.h | 0 > .../media/platform/{ti-vpe => ti/vpe}/vpe.c | 0 > .../platform/{ti-vpe => ti/vpe}/vpe_regs.h | 0 > 28 files changed, 1480 insertions(+), 120 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.txt > create mode 100644 Documentation/devicetree/bindings/media/cdns,csi2rx.yaml > create mode 100644 Documentation/devicetree/bindings/media/ti,j721e-csi2rx.yaml > create mode 100644 drivers/media/platform/ti/Makefile > create mode 100644 drivers/media/platform/ti/cal/Makefile > rename drivers/media/platform/{ti-vpe => ti/cal}/cal-camerarx.c (100%) > rename drivers/media/platform/{ti-vpe => ti/cal}/cal-video.c (100%) > rename drivers/media/platform/{ti-vpe => ti/cal}/cal.c (100%) > rename drivers/media/platform/{ti-vpe => ti/cal}/cal.h (100%) > rename drivers/media/platform/{ti-vpe => ti/cal}/cal_regs.h (100%) > create mode 100644 drivers/media/platform/ti/j721e-csi2rx/Makefile > create mode 100644 drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c > rename drivers/media/platform/{ti-vpe => ti/vpe}/Makefile (78%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/csc.c (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/csc.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/sc.c (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/sc.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/sc_coeff.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma.c (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma_priv.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpe.c (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpe_regs.h (100%) > > -- > 2.33.0 >
Hi Pratyush, On Wed, Sep 15, 2021 at 05:32:31PM +0530, Pratyush Yadav wrote: > + ret = phy_pm_runtime_get_sync(csi2rx->dphy); Note that this will return 1 if the device was already resumed. That is not an error. > + if (ret == -ENOTSUPP) > + got_pm = false; > + else if (ret) > + return ret;
Hi Pratyush, On Wed, Sep 15, 2021 at 05:32:37PM +0530, Pratyush Yadav wrote: ... > +/* > + * Find the input format. This is done by finding the first device in the > + * pipeline which can tell us the current format. This could be the sensor, or > + * this could be another device in the middle which is capable of format > + * conversions. > + */ > +static int ti_csi2rx_validate_pipeline(struct ti_csi2rx_dev *csi) > +{ > + struct media_pipeline *pipe = &csi->pipe; > + struct media_entity *entity; > + struct v4l2_subdev *sd; > + struct v4l2_subdev_format fmt; > + struct v4l2_pix_format *pix = &csi->v_fmt.fmt.pix; > + struct media_device *mdev = &csi->mdev; > + const struct ti_csi2rx_fmt *ti_fmt; > + int ret; > + > + mutex_lock(&mdev->graph_mutex); > + ret = media_graph_walk_init(&pipe->graph, mdev); > + if (ret) { > + mutex_unlock(&mdev->graph_mutex); > + return ret; > + } > + > + media_graph_walk_start(&pipe->graph, &csi->vdev.entity); > + > + while ((entity = media_graph_walk_next(&pipe->graph))) { > + if (!is_media_entity_v4l2_subdev(entity)) > + continue; You shouldn't rely on media_graph_walk_next() to return entities in a particular order. I'd suggest approach taken in isp_video_check_external_subdevs() (in drivers/media/platform/omap3isp/ispvideo.c). > + > + sd = media_entity_to_v4l2_subdev(entity); > + > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + fmt.pad = media_get_pad_index(entity, 0, PAD_SIGNAL_DEFAULT); > + > + ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt); > + if (ret && ret != -ENOIOCTLCMD) { > + media_graph_walk_cleanup(&pipe->graph); > + mutex_unlock(&mdev->graph_mutex); > + return ret; > + } > + > + if (!ret) > + break; > + } > + > + media_graph_walk_cleanup(&pipe->graph); > + mutex_unlock(&mdev->graph_mutex); > + > + /* Could not find input format. */ > + if (!entity) > + return -EPIPE; > + > + if (fmt.format.width != pix->width) > + return -EPIPE; > + if (fmt.format.height != pix->height) > + return -EPIPE; Pipeline validation should take place during media_pipeline_start(). Why are you doing it here? > + > + ti_fmt = find_format_by_pix(pix->pixelformat); > + if (WARN_ON(!ti_fmt)) > + return -EINVAL; > + > + if (fmt.format.code == MEDIA_BUS_FMT_YUYV8_2X8 || > + fmt.format.code == MEDIA_BUS_FMT_VYUY8_2X8 || > + fmt.format.code == MEDIA_BUS_FMT_YVYU8_2X8) { > + dev_err(csi->dev, > + "Only UYVY input allowed for YUV422 8-bit. Output format can be configured.\n"); > + return -EPIPE; > + } > + > + if (fmt.format.code == MEDIA_BUS_FMT_UYVY8_2X8) { > + /* Format conversion between YUV422 formats can be done. */ > + if (ti_fmt->code != MEDIA_BUS_FMT_UYVY8_2X8 && > + ti_fmt->code != MEDIA_BUS_FMT_YUYV8_2X8 && > + ti_fmt->code != MEDIA_BUS_FMT_VYUY8_2X8 && > + ti_fmt->code != MEDIA_BUS_FMT_YVYU8_2X8) > + return -EPIPE; > + } else if (fmt.format.code != ti_fmt->code) { > + return -EPIPE; > + } > + > + if (fmt.format.field != V4L2_FIELD_NONE && > + fmt.format.field != V4L2_FIELD_ANY) > + return -EPIPE; > + > + return 0; > +} > + > +static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > +{ > + struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq); > + struct ti_csi2rx_dma *dma = &csi->dma; > + struct ti_csi2rx_buffer *buf, *tmp; > + unsigned long flags = 0; No need to assign flags here. > + int ret = 0; > +
On 06/10/21 11:28PM, Sakari Ailus wrote: > Hi Pratyush, > > On Wed, Sep 15, 2021 at 05:32:37PM +0530, Pratyush Yadav wrote: > ... > > +/* > > + * Find the input format. This is done by finding the first device in the > > + * pipeline which can tell us the current format. This could be the sensor, or > > + * this could be another device in the middle which is capable of format > > + * conversions. > > + */ > > +static int ti_csi2rx_validate_pipeline(struct ti_csi2rx_dev *csi) > > +{ > > + struct media_pipeline *pipe = &csi->pipe; > > + struct media_entity *entity; > > + struct v4l2_subdev *sd; > > + struct v4l2_subdev_format fmt; > > + struct v4l2_pix_format *pix = &csi->v_fmt.fmt.pix; > > + struct media_device *mdev = &csi->mdev; > > + const struct ti_csi2rx_fmt *ti_fmt; > > + int ret; > > + > > + mutex_lock(&mdev->graph_mutex); > > + ret = media_graph_walk_init(&pipe->graph, mdev); > > + if (ret) { > > + mutex_unlock(&mdev->graph_mutex); > > + return ret; > > + } > > + > > + media_graph_walk_start(&pipe->graph, &csi->vdev.entity); > > + > > + while ((entity = media_graph_walk_next(&pipe->graph))) { > > + if (!is_media_entity_v4l2_subdev(entity)) > > + continue; > > You shouldn't rely on media_graph_walk_next() to return entities in a > particular order. Ah, right. Need to drop this. > > I'd suggest approach taken in isp_video_check_external_subdevs() (in > drivers/media/platform/omap3isp/ispvideo.c). > > > + > > + sd = media_entity_to_v4l2_subdev(entity); > > + > > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > + fmt.pad = media_get_pad_index(entity, 0, PAD_SIGNAL_DEFAULT); > > + > > + ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt); > > + if (ret && ret != -ENOIOCTLCMD) { > > + media_graph_walk_cleanup(&pipe->graph); > > + mutex_unlock(&mdev->graph_mutex); > > + return ret; > > + } > > + > > + if (!ret) > > + break; > > + } > > + > > + media_graph_walk_cleanup(&pipe->graph); > > + mutex_unlock(&mdev->graph_mutex); > > + > > + /* Could not find input format. */ > > + if (!entity) > > + return -EPIPE; > > + > > + if (fmt.format.width != pix->width) > > + return -EPIPE; > > + if (fmt.format.height != pix->height) > > + return -EPIPE; > > Pipeline validation should take place during media_pipeline_start(). Why > are you doing it here? How would be do that? Via the link_validate callback? > > > + > > + ti_fmt = find_format_by_pix(pix->pixelformat); > > + if (WARN_ON(!ti_fmt)) > > + return -EINVAL; > > + > > + if (fmt.format.code == MEDIA_BUS_FMT_YUYV8_2X8 || > > + fmt.format.code == MEDIA_BUS_FMT_VYUY8_2X8 || > > + fmt.format.code == MEDIA_BUS_FMT_YVYU8_2X8) { > > + dev_err(csi->dev, > > + "Only UYVY input allowed for YUV422 8-bit. Output format can be configured.\n"); > > + return -EPIPE; > > + } > > + > > + if (fmt.format.code == MEDIA_BUS_FMT_UYVY8_2X8) { > > + /* Format conversion between YUV422 formats can be done. */ > > + if (ti_fmt->code != MEDIA_BUS_FMT_UYVY8_2X8 && > > + ti_fmt->code != MEDIA_BUS_FMT_YUYV8_2X8 && > > + ti_fmt->code != MEDIA_BUS_FMT_VYUY8_2X8 && > > + ti_fmt->code != MEDIA_BUS_FMT_YVYU8_2X8) > > + return -EPIPE; > > + } else if (fmt.format.code != ti_fmt->code) { > > + return -EPIPE; > > + } > > + > > + if (fmt.format.field != V4L2_FIELD_NONE && > > + fmt.format.field != V4L2_FIELD_ANY) > > + return -EPIPE; > > + > > + return 0; > > +} > > + > > +static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count) > > +{ > > + struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq); > > + struct ti_csi2rx_dma *dma = &csi->dma; > > + struct ti_csi2rx_buffer *buf, *tmp; > > + unsigned long flags = 0; > > No need to assign flags here. Ok. > > > + int ret = 0; > > + > > -- > Kind regards, > > Sakari Ailus
On 06/10/21 11:19PM, Sakari Ailus wrote: > Hi Pratyush, > > On Wed, Sep 15, 2021 at 05:32:31PM +0530, Pratyush Yadav wrote: > > + ret = phy_pm_runtime_get_sync(csi2rx->dphy); > > Note that this will return 1 if the device was already resumed. That is not > an error. Thanks. Will fix. > > > + if (ret == -ENOTSUPP) > > + got_pm = false; > > + else if (ret) > > + return ret; > > -- > Sakari Ailus
On Thu, Oct 07, 2021 at 02:31:43AM +0530, Pratyush Yadav wrote: > On 06/10/21 11:28PM, Sakari Ailus wrote: > > Hi Pratyush, > > > > On Wed, Sep 15, 2021 at 05:32:37PM +0530, Pratyush Yadav wrote: > > ... > > > +/* > > > + * Find the input format. This is done by finding the first device in the > > > + * pipeline which can tell us the current format. This could be the sensor, or > > > + * this could be another device in the middle which is capable of format > > > + * conversions. > > > + */ > > > +static int ti_csi2rx_validate_pipeline(struct ti_csi2rx_dev *csi) > > > +{ > > > + struct media_pipeline *pipe = &csi->pipe; > > > + struct media_entity *entity; > > > + struct v4l2_subdev *sd; > > > + struct v4l2_subdev_format fmt; > > > + struct v4l2_pix_format *pix = &csi->v_fmt.fmt.pix; > > > + struct media_device *mdev = &csi->mdev; > > > + const struct ti_csi2rx_fmt *ti_fmt; > > > + int ret; > > > + > > > + mutex_lock(&mdev->graph_mutex); > > > + ret = media_graph_walk_init(&pipe->graph, mdev); > > > + if (ret) { > > > + mutex_unlock(&mdev->graph_mutex); > > > + return ret; > > > + } > > > + > > > + media_graph_walk_start(&pipe->graph, &csi->vdev.entity); > > > + > > > + while ((entity = media_graph_walk_next(&pipe->graph))) { > > > + if (!is_media_entity_v4l2_subdev(entity)) > > > + continue; > > > > You shouldn't rely on media_graph_walk_next() to return entities in a > > particular order. > > Ah, right. Need to drop this. > > > > > I'd suggest approach taken in isp_video_check_external_subdevs() (in > > drivers/media/platform/omap3isp/ispvideo.c). > > > > > + > > > + sd = media_entity_to_v4l2_subdev(entity); > > > + > > > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > > + fmt.pad = media_get_pad_index(entity, 0, PAD_SIGNAL_DEFAULT); > > > + > > > + ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt); > > > + if (ret && ret != -ENOIOCTLCMD) { > > > + media_graph_walk_cleanup(&pipe->graph); > > > + mutex_unlock(&mdev->graph_mutex); > > > + return ret; > > > + } > > > + > > > + if (!ret) > > > + break; > > > + } > > > + > > > + media_graph_walk_cleanup(&pipe->graph); > > > + mutex_unlock(&mdev->graph_mutex); > > > + > > > + /* Could not find input format. */ > > > + if (!entity) > > > + return -EPIPE; > > > + > > > + if (fmt.format.width != pix->width) > > > + return -EPIPE; > > > + if (fmt.format.height != pix->height) > > > + return -EPIPE; > > > > Pipeline validation should take place during media_pipeline_start(). Why > > are you doing it here? > > How would be do that? Via the link_validate callback? Yes, please. See other drivers for examples --- such as omap3isp.
Hi Pratyush, Thank you for the patch. On Wed, Sep 15, 2021 at 05:32:30PM +0530, Pratyush Yadav wrote: > The notifier is added to the global notifier list when registered. When > the module is removed, the struct csi2rx_priv in which the notifier is > embedded, is destroyed. As a result the notifier list has a reference to > a notifier that no longer exists. This causes invalid memory accesses > when the list is iterated over. Similar for when the probe fails. > > Unregister and clean up the notifier to avoid this. > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Note that there are other issues in the driver in cleanup paths, in particular a missing v4l2_async_notifier_cleanup() call in csi2rx_parse_dt() when v4l2_async_notifier_add_fwnode_remote_subdev() fails (moving the one from the other error path to an err label would be best), and missing media_entity_cleanup() calls in both the probe error path and the remove handler. Would you like to submit fixes for those ? > --- > > (no changes since v3) > > Changes in v3: > - New in v3. > > drivers/media/platform/cadence/cdns-csi2rx.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 7b44ab2b8c9a..d60975f905d6 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -469,6 +469,7 @@ static int csi2rx_probe(struct platform_device *pdev) > return 0; > > err_cleanup: > + v4l2_async_nf_unregister(&csi2rx->notifier); > v4l2_async_nf_cleanup(&csi2rx->notifier); > err_free_priv: > kfree(csi2rx); > @@ -479,6 +480,8 @@ static int csi2rx_remove(struct platform_device *pdev) > { > struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev); > > + v4l2_async_nf_unregister(&csi2rx->notifier); > + v4l2_async_nf_cleanup(&csi2rx->notifier); > v4l2_async_unregister_subdev(&csi2rx->subdev); > kfree(csi2rx); >
Hi Pratyush, Thank you for the patch. On Wed, Sep 15, 2021 at 05:32:31PM +0530, Pratyush Yadav wrote: > Some platforms like TI's J721E can have the CSI2RX paired with an > external DPHY. Add support to enable and configure the DPHY using the > generic PHY framework. > > Get the pixel rate and bpp from the subdev and pass them on to the DPHY > along with the number of lanes. All other settings are left to their > default values. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > Changes in v4: > - Drop the call to set PHY submode. It is now being done via compatible > on the DPHY side. > > Changes in v3: > - Use v4l2_get_link_freq() to calculate pixel clock. > > Changes in v2: > - Use phy_pm_runtime_get_sync() and phy_pm_runtime_put() before making > calls to set PHY mode, etc. to make sure it is ready. > > drivers/media/platform/cadence/cdns-csi2rx.c | 143 +++++++++++++++++-- > 1 file changed, 133 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index d60975f905d6..c06e039a1aa8 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -30,6 +30,12 @@ > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > +#define CSI2RX_DPHY_CL_RST BIT(16) > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > +#define CSI2RX_DPHY_CL_EN BIT(4) > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > + > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > @@ -54,6 +60,11 @@ enum csi2rx_pads { > CSI2RX_PAD_MAX, > }; > > +struct csi2rx_fmt { > + u32 code; > + u8 bpp; > +}; > + > struct csi2rx_priv { > struct device *dev; > unsigned int count; > @@ -85,6 +96,37 @@ struct csi2rx_priv { > int source_pad; > }; > > +static const struct csi2rx_fmt formats[] = { > + { > + .code = MEDIA_BUS_FMT_YUYV8_2X8, On CSI-2 we use the _1X16 media bus codes (it's a convention, neither the 1X16 nor th 2X8 actually described accurately how data is transported on the bus). > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > + .bpp = 16, > + }, > +}; > + > +static u8 csi2rx_get_bpp(u32 code) > +{ > + int i; i only takes positive values, it can be an unsigned int. > + > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > + if (formats[i].code == code) > + return formats[i].bpp; > + } > + > + return 0; > +} It's a bit convoluted for a function that returns 16 for all formats :-) I understand it's meant to be extended, so that fine. > + > static inline > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > { > @@ -101,6 +143,66 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > } > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > +{ > + union phy_configure_opts opts = { }; > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > + struct v4l2_subdev_format sd_fmt; > + s64 pixel_clock; > + int ret; > + u8 bpp; > + bool got_pm = true; > + > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + sd_fmt.pad = 0; > + > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, > + &sd_fmt); Subdevs should not call into their neighbours (except to start/stop streaming). You should instead use the format configured on the sink pad the the csi2rx. This means you'll need to implement .get_fmt() and .set_fmt(), which should be done anyway. > + if (ret) > + return ret; > + > + bpp = csi2rx_get_bpp(sd_fmt.format.code); > + if (!bpp) > + return -EINVAL; > + > + /* > + * Do not divide by the number of lanes here. That will be done by > + * phy_mipi_dphy_get_default_config(). > + */ > + pixel_clock = v4l2_get_link_freq(csi2rx->source_subdev->ctrl_handler, > + 1, 2); > + if (pixel_clock < 0) > + return pixel_clock; > + > + ret = phy_mipi_dphy_get_default_config(pixel_clock, bpp, > + csi2rx->num_lanes, cfg); > + if (ret) > + return ret; > + > + ret = phy_pm_runtime_get_sync(csi2rx->dphy); > + if (ret == -ENOTSUPP) > + got_pm = false; > + else if (ret) > + return ret; > + > + ret = phy_power_on(csi2rx->dphy); > + if (ret) > + goto out; > + > + ret = phy_configure(csi2rx->dphy, &opts); > + if (ret) { > + /* Can't do anything if it fails. Ignore the return value. */ > + phy_power_off(csi2rx->dphy); > + goto out; > + } > + > +out: > + if (got_pm) > + phy_pm_runtime_put(csi2rx->dphy); > + > + return ret; > +} > + > static int csi2rx_start(struct csi2rx_priv *csi2rx) > { > unsigned int i; > @@ -139,6 +241,17 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > if (ret) > goto err_disable_pclk; > > + /* Enable DPHY clk and data lanes. */ > + if (csi2rx->dphy) { > + reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST; > + for (i = 0; i < csi2rx->num_lanes; i++) { > + reg |= CSI2RX_DPHY_DL_EN(csi2rx->lanes[i] - 1); > + reg |= CSI2RX_DPHY_DL_RST(csi2rx->lanes[i] - 1); > + } > + > + writel(reg, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > + } > + > /* > * Create a static mapping between the CSI virtual channels > * and the output stream. > @@ -169,10 +282,21 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > if (ret) > goto err_disable_pixclk; > > + if (csi2rx->dphy) { > + ret = csi2rx_configure_external_dphy(csi2rx); > + if (ret) { > + dev_err(csi2rx->dev, > + "Failed to configure external DPHY: %d\n", ret); > + goto err_disable_sysclk; > + } > + } > + > clk_disable_unprepare(csi2rx->p_clk); > > return 0; > > +err_disable_sysclk: > + clk_disable_unprepare(csi2rx->sys_clk); > err_disable_pixclk: > for (; i > 0; i--) > clk_disable_unprepare(csi2rx->pixel_clk[i - 1]); > @@ -200,6 +324,13 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > + > + if (csi2rx->dphy) { > + writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > + > + if (phy_power_off(csi2rx->dphy)) > + dev_warn(csi2rx->dev, "Couldn't power off DPHY\n"); > + } > } > > static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > @@ -307,15 +438,6 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > return PTR_ERR(csi2rx->dphy); > } > > - /* > - * FIXME: Once we'll have external D-PHY support, the check > - * will need to be removed. > - */ > - if (csi2rx->dphy) { > - dev_err(&pdev->dev, "External D-PHY not supported yet\n"); > - return -EINVAL; > - } > - > ret = clk_prepare_enable(csi2rx->p_clk); > if (ret) { > dev_err(&pdev->dev, "Couldn't prepare and enable P clock\n"); > @@ -345,7 +467,7 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > * FIXME: Once we'll have internal D-PHY support, the check > * will need to be removed. > */ > - if (csi2rx->has_internal_dphy) { > + if (!csi2rx->dphy && csi2rx->has_internal_dphy) { > dev_err(&pdev->dev, "Internal D-PHY not supported yet\n"); > return -EINVAL; > } > @@ -464,6 +586,7 @@ static int csi2rx_probe(struct platform_device *pdev) > dev_info(&pdev->dev, > "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n", > csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams, > + csi2rx->dphy ? "external" : > csi2rx->has_internal_dphy ? "internal" : "no"); > > return 0;
Hi Pratyush, Thank you for the patch. On Wed, Sep 15, 2021 at 05:32:32PM +0530, Pratyush Yadav wrote: > This resets the stream state machines and FIFOs, giving them a clean > slate. On J721E if the streams are not reset before starting the > capture, the captured frame gets wrapped around vertically on every run > after the first. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > > (no changes since v1) > > drivers/media/platform/cadence/cdns-csi2rx.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index c06e039a1aa8..e05d76394cd6 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -39,6 +39,7 @@ > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > +#define CSI2RX_STREAM_CTRL_SOFT_RST BIT(4) > #define CSI2RX_STREAM_CTRL_START BIT(0) > > #define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x008) > @@ -135,12 +136,22 @@ struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > > static void csi2rx_reset(struct csi2rx_priv *csi2rx) > { > + int i; i is always positive, it can be an unsigned int. With this, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT, > csi2rx->base + CSI2RX_SOFT_RESET_REG); > > udelay(10); > > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > + > + /* Reset individual streams. */ > + for (i = 0; i < csi2rx->max_streams; i++) { > + writel(CSI2RX_STREAM_CTRL_SOFT_RST, > + csi2rx->base + CSI2RX_STREAM_CTRL_REG(i)); > + usleep_range(10, 20); > + writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i)); > + } > } > > static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx)
Hi Pratyush, Thank you for the patch. On Wed, Sep 15, 2021 at 05:32:33PM +0530, Pratyush Yadav wrote: > The stream stop procedure says that the STOP bit should be set when the > stream is to be stopped, and then the ready bit in stream status > register polled to make sure the STOP operation is finished. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > > (no changes since v1) > > drivers/media/platform/cadence/cdns-csi2rx.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index e05d76394cd6..3730e8beee48 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -8,6 +8,7 @@ > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/io.h> > +#include <linux/iopoll.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_graph.h> > @@ -40,8 +41,12 @@ > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > #define CSI2RX_STREAM_CTRL_SOFT_RST BIT(4) > +#define CSI2RX_STREAM_CTRL_STOP BIT(1) > #define CSI2RX_STREAM_CTRL_START BIT(0) > > +#define CSI2RX_STREAM_STATUS_REG(n) (CSI2RX_STREAM_BASE(n) + 0x004) > +#define CSI2RX_STREAM_STATUS_RDY BIT(31) > + > #define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x008) > #define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT BIT(31) > #define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n) BIT((n) + 16) > @@ -321,12 +326,23 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > static void csi2rx_stop(struct csi2rx_priv *csi2rx) > { > unsigned int i; > + u32 val; > + int ret; > > clk_prepare_enable(csi2rx->p_clk); > clk_disable_unprepare(csi2rx->sys_clk); > > for (i = 0; i < csi2rx->max_streams; i++) { > - writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i)); > + writel(CSI2RX_STREAM_CTRL_STOP, > + csi2rx->base + CSI2RX_STREAM_CTRL_REG(i)); > + > + ret = readl_relaxed_poll_timeout(csi2rx->base + > + CSI2RX_STREAM_STATUS_REG(i), > + val, > + (val & CSI2RX_STREAM_STATUS_RDY), > + 10, 10000); > + if (ret) > + dev_warn(csi2rx->dev, "Failed to stop stream%d\n", i); %u for unsigned int. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > clk_disable_unprepare(csi2rx->pixel_clk[i]); > }
Hi Pratyush, Thank you for the patch. On Wed, Sep 15, 2021 at 05:32:34PM +0530, Pratyush Yadav wrote: > Firstly, there is no VC_EN bit present in the STREAM_DATA_CFG register. > Bit 31 is part of the VL_SELECT field. Remove it completely. > > Secondly, it makes little sense to enable ith virtual channel for ith > stream. Sure, there might be a use-case that demands it. But there might > also be a use case that demands all streams to use the 0th virtual > channel. Prefer this case over the former because it is less arbitrary > and also makes it very clear what the limitations of the current driver > is instead of giving a false impression that multiple virtual channels > are supported. No issue with that. Hopefully we'll support multiple streams for real in the not too distant future :-) > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > (no changes since v1) > > drivers/media/platform/cadence/cdns-csi2rx.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 3730e8beee48..edd56c5f2e89 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -48,7 +48,6 @@ > #define CSI2RX_STREAM_STATUS_RDY BIT(31) > > #define CSI2RX_STREAM_DATA_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x008) > -#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT BIT(31) > #define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n) BIT((n) + 16) > > #define CSI2RX_STREAM_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x00c) > @@ -286,8 +285,11 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF, > csi2rx->base + CSI2RX_STREAM_CFG_REG(i)); > > - writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT | > - CSI2RX_STREAM_DATA_CFG_VC_SELECT(i), > + /* > + * Enable one virtual channel. When multiple virtual channels > + * are supported this will have to be changed. > + */ > + writel(CSI2RX_STREAM_DATA_CFG_VC_SELECT(0), > csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i)); > > writel(CSI2RX_STREAM_CTRL_START,
Hi Pratyush, Thank you for the patch. On Wed, Sep 15, 2021 at 05:32:35PM +0530, Pratyush Yadav wrote: > The devnode can be used by media-ctl and other userspace tools to > perform configurations on the subdev. Without it, media-ctl returns > ENOENT when setting format on the sensor subdev. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > (no changes since v2) > > Changes in v2: > - New in v2. > > drivers/media/platform/cadence/cdns-csi2rx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index edd56c5f2e89..7c3183069db0 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -602,6 +602,7 @@ static int csi2rx_probe(struct platform_device *pdev) > csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK; > for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) > csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE; > + csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX, > csi2rx->pads);
Hi Pratyush, Thank you for the patch. On Wed, Sep 15, 2021 at 05:32:36PM +0530, Pratyush Yadav wrote: > The ti-vpe/ sub-directory does not only contain the VPE-specific things. > It also contains the CAL driver, which is a completely different > subsystem. This is also not a good place to add new drivers for other TI > platforms since they will all get mixed up. > > Separate the VPE and CAL parts into different sub-directories and rename > the ti-vpe/ sub-directory to ti/. This is now the place where new TI > platform drivers can be added. That looks much better :-) > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Compile tested only. There should be no functional change. > > (no changes since v3) > > Changes in v3: > - Add Tomi's R-by. > > Changes in v2: > - New in v2. > > MAINTAINERS | 3 ++- > drivers/media/platform/Makefile | 2 +- > drivers/media/platform/ti/Makefile | 3 +++ > drivers/media/platform/ti/cal/Makefile | 3 +++ > drivers/media/platform/{ti-vpe => ti/cal}/cal-camerarx.c | 0 > drivers/media/platform/{ti-vpe => ti/cal}/cal-video.c | 0 > drivers/media/platform/{ti-vpe => ti/cal}/cal.c | 0 > drivers/media/platform/{ti-vpe => ti/cal}/cal.h | 0 > drivers/media/platform/{ti-vpe => ti/cal}/cal_regs.h | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/Makefile | 4 ---- > drivers/media/platform/{ti-vpe => ti/vpe}/csc.c | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/csc.h | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/sc.c | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/sc.h | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/sc_coeff.h | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/vpdma.c | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/vpdma.h | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/vpdma_priv.h | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/vpe.c | 0 > drivers/media/platform/{ti-vpe => ti/vpe}/vpe_regs.h | 0 > 20 files changed, 9 insertions(+), 6 deletions(-) > create mode 100644 drivers/media/platform/ti/Makefile > create mode 100644 drivers/media/platform/ti/cal/Makefile > rename drivers/media/platform/{ti-vpe => ti/cal}/cal-camerarx.c (100%) > rename drivers/media/platform/{ti-vpe => ti/cal}/cal-video.c (100%) > rename drivers/media/platform/{ti-vpe => ti/cal}/cal.c (100%) > rename drivers/media/platform/{ti-vpe => ti/cal}/cal.h (100%) > rename drivers/media/platform/{ti-vpe => ti/cal}/cal_regs.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/Makefile (78%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/csc.c (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/csc.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/sc.c (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/sc.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/sc_coeff.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma.c (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpdma_priv.h (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpe.c (100%) > rename drivers/media/platform/{ti-vpe => ti/vpe}/vpe_regs.h (100%) > > diff --git a/MAINTAINERS b/MAINTAINERS > index cad1289793db..62bc4a949ae1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -18829,7 +18829,8 @@ W: http://linuxtv.org/ > Q: http://patchwork.linuxtv.org/project/linux-media/list/ > F: Documentation/devicetree/bindings/media/ti,cal.yaml > F: Documentation/devicetree/bindings/media/ti,vpe.yaml > -F: drivers/media/platform/ti-vpe/ > +F: drivers/media/platform/ti/cal/ > +F: drivers/media/platform/ti/vpe/ > > TI WILINK WIRELESS DRIVERS > L: linux-wireless@vger.kernel.org > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile > index 73ce083c2fc6..26d15b377a79 100644 > --- a/drivers/media/platform/Makefile > +++ b/drivers/media/platform/Makefile > @@ -15,7 +15,7 @@ obj-$(CONFIG_VIDEO_PXA27x) += pxa_camera.o > > obj-$(CONFIG_VIDEO_VIU) += fsl-viu.o > > -obj-y += ti-vpe/ > +obj-y += ti/ > > obj-$(CONFIG_VIDEO_MX2_EMMAPRP) += mx2_emmaprp.o > obj-$(CONFIG_VIDEO_CODA) += coda/ > diff --git a/drivers/media/platform/ti/Makefile b/drivers/media/platform/ti/Makefile > new file mode 100644 > index 000000000000..bbc737ccbbea > --- /dev/null > +++ b/drivers/media/platform/ti/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-y += cal/ > +obj-y += vpe/ > diff --git a/drivers/media/platform/ti/cal/Makefile b/drivers/media/platform/ti/cal/Makefile > new file mode 100644 > index 000000000000..45ac35585f0b > --- /dev/null > +++ b/drivers/media/platform/ti/cal/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_VIDEO_TI_CAL) += ti-cal.o > +ti-cal-y := cal.o cal-camerarx.o cal-video.o > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c > similarity index 100% > rename from drivers/media/platform/ti-vpe/cal-camerarx.c > rename to drivers/media/platform/ti/cal/cal-camerarx.c > diff --git a/drivers/media/platform/ti-vpe/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c > similarity index 100% > rename from drivers/media/platform/ti-vpe/cal-video.c > rename to drivers/media/platform/ti/cal/cal-video.c > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti/cal/cal.c > similarity index 100% > rename from drivers/media/platform/ti-vpe/cal.c > rename to drivers/media/platform/ti/cal/cal.c > diff --git a/drivers/media/platform/ti-vpe/cal.h b/drivers/media/platform/ti/cal/cal.h > similarity index 100% > rename from drivers/media/platform/ti-vpe/cal.h > rename to drivers/media/platform/ti/cal/cal.h > diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti/cal/cal_regs.h > similarity index 100% > rename from drivers/media/platform/ti-vpe/cal_regs.h > rename to drivers/media/platform/ti/cal/cal_regs.h > diff --git a/drivers/media/platform/ti-vpe/Makefile b/drivers/media/platform/ti/vpe/Makefile > similarity index 78% > rename from drivers/media/platform/ti-vpe/Makefile > rename to drivers/media/platform/ti/vpe/Makefile > index ad624056e039..3fadfe084f87 100644 > --- a/drivers/media/platform/ti-vpe/Makefile > +++ b/drivers/media/platform/ti/vpe/Makefile > @@ -10,7 +10,3 @@ ti-sc-y := sc.o > ti-csc-y := csc.o > > ccflags-$(CONFIG_VIDEO_TI_VPE_DEBUG) += -DDEBUG > - > -obj-$(CONFIG_VIDEO_TI_CAL) += ti-cal.o > - > -ti-cal-y := cal.o cal-camerarx.o cal-video.o > diff --git a/drivers/media/platform/ti-vpe/csc.c b/drivers/media/platform/ti/vpe/csc.c > similarity index 100% > rename from drivers/media/platform/ti-vpe/csc.c > rename to drivers/media/platform/ti/vpe/csc.c > diff --git a/drivers/media/platform/ti-vpe/csc.h b/drivers/media/platform/ti/vpe/csc.h > similarity index 100% > rename from drivers/media/platform/ti-vpe/csc.h > rename to drivers/media/platform/ti/vpe/csc.h > diff --git a/drivers/media/platform/ti-vpe/sc.c b/drivers/media/platform/ti/vpe/sc.c > similarity index 100% > rename from drivers/media/platform/ti-vpe/sc.c > rename to drivers/media/platform/ti/vpe/sc.c > diff --git a/drivers/media/platform/ti-vpe/sc.h b/drivers/media/platform/ti/vpe/sc.h > similarity index 100% > rename from drivers/media/platform/ti-vpe/sc.h > rename to drivers/media/platform/ti/vpe/sc.h > diff --git a/drivers/media/platform/ti-vpe/sc_coeff.h b/drivers/media/platform/ti/vpe/sc_coeff.h > similarity index 100% > rename from drivers/media/platform/ti-vpe/sc_coeff.h > rename to drivers/media/platform/ti/vpe/sc_coeff.h > diff --git a/drivers/media/platform/ti-vpe/vpdma.c b/drivers/media/platform/ti/vpe/vpdma.c > similarity index 100% > rename from drivers/media/platform/ti-vpe/vpdma.c > rename to drivers/media/platform/ti/vpe/vpdma.c > diff --git a/drivers/media/platform/ti-vpe/vpdma.h b/drivers/media/platform/ti/vpe/vpdma.h > similarity index 100% > rename from drivers/media/platform/ti-vpe/vpdma.h > rename to drivers/media/platform/ti/vpe/vpdma.h > diff --git a/drivers/media/platform/ti-vpe/vpdma_priv.h b/drivers/media/platform/ti/vpe/vpdma_priv.h > similarity index 100% > rename from drivers/media/platform/ti-vpe/vpdma_priv.h > rename to drivers/media/platform/ti/vpe/vpdma_priv.h > diff --git a/drivers/media/platform/ti-vpe/vpe.c b/drivers/media/platform/ti/vpe/vpe.c > similarity index 100% > rename from drivers/media/platform/ti-vpe/vpe.c > rename to drivers/media/platform/ti/vpe/vpe.c > diff --git a/drivers/media/platform/ti-vpe/vpe_regs.h b/drivers/media/platform/ti/vpe/vpe_regs.h > similarity index 100% > rename from drivers/media/platform/ti-vpe/vpe_regs.h > rename to drivers/media/platform/ti/vpe/vpe_regs.h
On 07/10/21 02:31AM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Wed, Sep 15, 2021 at 05:32:30PM +0530, Pratyush Yadav wrote: > > The notifier is added to the global notifier list when registered. When > > the module is removed, the struct csi2rx_priv in which the notifier is > > embedded, is destroyed. As a result the notifier list has a reference to > > a notifier that no longer exists. This causes invalid memory accesses > > when the list is iterated over. Similar for when the probe fails. > > > > Unregister and clean up the notifier to avoid this. > > > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Note that there are other issues in the driver in cleanup paths, in > particular a missing v4l2_async_notifier_cleanup() call in > csi2rx_parse_dt() when v4l2_async_notifier_add_fwnode_remote_subdev() > fails (moving the one from the other error path to an err label would be > best), and missing media_entity_cleanup() calls in both the probe error > path and the remove handler. Would you like to submit fixes for those ? Sure, will do. > > > --- > > > > (no changes since v3) > > > > Changes in v3: > > - New in v3. > > > > drivers/media/platform/cadence/cdns-csi2rx.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index 7b44ab2b8c9a..d60975f905d6 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -469,6 +469,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > return 0; > > > > err_cleanup: > > + v4l2_async_nf_unregister(&csi2rx->notifier); > > v4l2_async_nf_cleanup(&csi2rx->notifier); > > err_free_priv: > > kfree(csi2rx); > > @@ -479,6 +480,8 @@ static int csi2rx_remove(struct platform_device *pdev) > > { > > struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev); > > > > + v4l2_async_nf_unregister(&csi2rx->notifier); > > + v4l2_async_nf_cleanup(&csi2rx->notifier); > > v4l2_async_unregister_subdev(&csi2rx->subdev); > > kfree(csi2rx); > > > > -- > Regards, > > Laurent Pinchart