| Message ID | 20220510115859.19777-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | a314327c9d16b6a36737fc3ac8c2ba7fe65e2138 |
| Headers | show |
| Series | None | expand |
Hi Laurent, On Tue, May 10, 2022 at 02:58:11PM +0300, Laurent Pinchart wrote: > Prepare for the decoupling of the imx7-media-csi driver from the > IPUv3-based drivers by moving the imx_media_dev handling from probe() > function to separate functions. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/staging/media/imx/imx7-media-csi.c | 71 +++++++++++++--------- > 1 file changed, 42 insertions(+), 29 deletions(-) > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > index 893620e8fc03..d7c65b8bb3c9 100644 > --- a/drivers/staging/media/imx/imx7-media-csi.c > +++ b/drivers/staging/media/imx/imx7-media-csi.c > @@ -1175,11 +1175,45 @@ static int imx7_csi_async_register(struct imx7_csi *csi) > return v4l2_async_register_subdev(&csi->sd); > } > > +static void imx7_csi_media_cleanup(struct imx7_csi *csi) > +{ > + struct imx_media_dev *imxmd = csi->imxmd; I know that later on we embed this media dev in csi, but meanwhile we can reach here where csi->imxmd is NULL, because... > + > + v4l2_device_unregister(&imxmd->v4l2_dev); > + media_device_unregister(&imxmd->md); > + media_device_cleanup(&imxmd->md); > +} > + > +static int imx7_csi_media_init(struct imx7_csi *csi) > +{ > + struct imx_media_dev *imxmd; > + int ret; > + > + /* add media device */ > + imxmd = imx_media_dev_init(csi->dev, NULL); > + if (IS_ERR(imxmd)) > + return PTR_ERR(imxmd); here we get out and later remove() is called, or... > + > + ret = imx_media_of_add_csi(imxmd, csi->dev->of_node); > + if (ret < 0 && ret != -ENODEV && ret != -EEXIST) { > + imx7_csi_media_cleanup(csi); here we only set csi->imxmd = imxmd bellow. > + return ret; > + } > + > + ret = imx_media_dev_notifier_register(imxmd, NULL); > + if (ret < 0) { > + imx7_csi_media_cleanup(csi); ditto > + return ret; > + } > + > + csi->imxmd = imxmd; > + > + return 0; > +} > + > static int imx7_csi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > - struct device_node *node = dev->of_node; > - struct imx_media_dev *imxmd; > struct imx7_csi *csi; > int i, ret; > > @@ -1193,6 +1227,7 @@ static int imx7_csi_probe(struct platform_device *pdev) > spin_lock_init(&csi->irqlock); > mutex_init(&csi->lock); > > + /* Acquire resources and install interrupt handler. */ > csi->mclk = devm_clk_get(&pdev->dev, "mclk"); > if (IS_ERR(csi->mclk)) { > ret = PTR_ERR(csi->mclk); > @@ -1214,7 +1249,6 @@ static int imx7_csi_probe(struct platform_device *pdev) > > csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev); > > - /* install interrupt handler */ > ret = devm_request_irq(dev, csi->irq, imx7_csi_irq_handler, 0, "csi", > (void *)csi); > if (ret < 0) { > @@ -1222,22 +1256,11 @@ static int imx7_csi_probe(struct platform_device *pdev) > goto destroy_mutex; > } > > - /* add media device */ > - imxmd = imx_media_dev_init(dev, NULL); > - if (IS_ERR(imxmd)) { > - ret = PTR_ERR(imxmd); > + /* Initialize all the media device infrastructure. */ > + ret = imx7_csi_media_init(csi); > + if (ret) > goto destroy_mutex; > - } > > - ret = imx_media_of_add_csi(imxmd, node); > - if (ret < 0 && ret != -ENODEV && ret != -EEXIST) > - goto cleanup; > - > - ret = imx_media_dev_notifier_register(imxmd, NULL); > - if (ret < 0) > - goto cleanup; > - > - csi->imxmd = imxmd; > v4l2_subdev_init(&csi->sd, &imx7_csi_subdev_ops); > v4l2_set_subdevdata(&csi->sd, csi); > csi->sd.internal_ops = &imx7_csi_internal_ops; > @@ -1269,11 +1292,7 @@ static int imx7_csi_probe(struct platform_device *pdev) > v4l2_async_nf_cleanup(&csi->notifier); > > cleanup: > - v4l2_async_nf_unregister(&imxmd->notifier); > - v4l2_async_nf_cleanup(&imxmd->notifier); Didn't this notifier got dropped in cleanup() function? or am I missing something? Cheers, Rui > - v4l2_device_unregister(&imxmd->v4l2_dev); > - media_device_unregister(&imxmd->md); > - media_device_cleanup(&imxmd->md); > + imx7_csi_media_cleanup(csi); > > destroy_mutex: > mutex_destroy(&csi->lock); > @@ -1285,14 +1304,8 @@ static int imx7_csi_remove(struct platform_device *pdev) > { > struct v4l2_subdev *sd = platform_get_drvdata(pdev); > struct imx7_csi *csi = v4l2_get_subdevdata(sd); > - struct imx_media_dev *imxmd = csi->imxmd; > > - v4l2_async_nf_unregister(&imxmd->notifier); > - v4l2_async_nf_cleanup(&imxmd->notifier); > - > - media_device_unregister(&imxmd->md); > - v4l2_device_unregister(&imxmd->v4l2_dev); > - media_device_cleanup(&imxmd->md); > + imx7_csi_media_cleanup(csi); > > v4l2_async_nf_unregister(&csi->notifier); > v4l2_async_nf_cleanup(&csi->notifier); > -- > Regards, > > Laurent Pinchart >
Hi Rui, On Tue, May 10, 2022 at 03:09:02PM +0100, Rui Miguel Silva wrote: > On Tue, May 10, 2022 at 02:58:11PM +0300, Laurent Pinchart wrote: > > Prepare for the decoupling of the imx7-media-csi driver from the > > IPUv3-based drivers by moving the imx_media_dev handling from probe() > > function to separate functions. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/staging/media/imx/imx7-media-csi.c | 71 +++++++++++++--------- > > 1 file changed, 42 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > > index 893620e8fc03..d7c65b8bb3c9 100644 > > --- a/drivers/staging/media/imx/imx7-media-csi.c > > +++ b/drivers/staging/media/imx/imx7-media-csi.c > > @@ -1175,11 +1175,45 @@ static int imx7_csi_async_register(struct imx7_csi *csi) > > return v4l2_async_register_subdev(&csi->sd); > > } > > > > +static void imx7_csi_media_cleanup(struct imx7_csi *csi) > > +{ > > + struct imx_media_dev *imxmd = csi->imxmd; > > I know that later on we embed this media dev in csi, but meanwhile we > can reach here where csi->imxmd is NULL, because... > > > + > > + v4l2_device_unregister(&imxmd->v4l2_dev); > > + media_device_unregister(&imxmd->md); > > + media_device_cleanup(&imxmd->md); > > +} > > + > > +static int imx7_csi_media_init(struct imx7_csi *csi) > > +{ > > + struct imx_media_dev *imxmd; > > + int ret; > > + > > + /* add media device */ > > + imxmd = imx_media_dev_init(csi->dev, NULL); > > + if (IS_ERR(imxmd)) > > + return PTR_ERR(imxmd); > > here we get out and later remove() is called, or... This can't happen, as if probe() fails, remove() isn't called. > > + > > + ret = imx_media_of_add_csi(imxmd, csi->dev->of_node); > > + if (ret < 0 && ret != -ENODEV && ret != -EEXIST) { > > + imx7_csi_media_cleanup(csi); > > here we only set csi->imxmd = imxmd bellow. > > > + return ret; > > + } > > + > > + ret = imx_media_dev_notifier_register(imxmd, NULL); > > + if (ret < 0) { > > + imx7_csi_media_cleanup(csi); > > ditto Those two are valid concerns. I'll fix them. > > + return ret; > > + } > > + > > + csi->imxmd = imxmd; > > + > > + return 0; > > +} > > + > > static int imx7_csi_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > - struct device_node *node = dev->of_node; > > - struct imx_media_dev *imxmd; > > struct imx7_csi *csi; > > int i, ret; > > > > @@ -1193,6 +1227,7 @@ static int imx7_csi_probe(struct platform_device *pdev) > > spin_lock_init(&csi->irqlock); > > mutex_init(&csi->lock); > > > > + /* Acquire resources and install interrupt handler. */ > > csi->mclk = devm_clk_get(&pdev->dev, "mclk"); > > if (IS_ERR(csi->mclk)) { > > ret = PTR_ERR(csi->mclk); > > @@ -1214,7 +1249,6 @@ static int imx7_csi_probe(struct platform_device *pdev) > > > > csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev); > > > > - /* install interrupt handler */ > > ret = devm_request_irq(dev, csi->irq, imx7_csi_irq_handler, 0, "csi", > > (void *)csi); > > if (ret < 0) { > > @@ -1222,22 +1256,11 @@ static int imx7_csi_probe(struct platform_device *pdev) > > goto destroy_mutex; > > } > > > > - /* add media device */ > > - imxmd = imx_media_dev_init(dev, NULL); > > - if (IS_ERR(imxmd)) { > > - ret = PTR_ERR(imxmd); > > + /* Initialize all the media device infrastructure. */ > > + ret = imx7_csi_media_init(csi); > > + if (ret) > > goto destroy_mutex; > > - } > > > > - ret = imx_media_of_add_csi(imxmd, node); > > - if (ret < 0 && ret != -ENODEV && ret != -EEXIST) > > - goto cleanup; > > - > > - ret = imx_media_dev_notifier_register(imxmd, NULL); > > - if (ret < 0) > > - goto cleanup; > > - > > - csi->imxmd = imxmd; > > v4l2_subdev_init(&csi->sd, &imx7_csi_subdev_ops); > > v4l2_set_subdevdata(&csi->sd, csi); > > csi->sd.internal_ops = &imx7_csi_internal_ops; > > @@ -1269,11 +1292,7 @@ static int imx7_csi_probe(struct platform_device *pdev) > > v4l2_async_nf_cleanup(&csi->notifier); > > > > cleanup: > > - v4l2_async_nf_unregister(&imxmd->notifier); > > - v4l2_async_nf_cleanup(&imxmd->notifier); > > Didn't this notifier got dropped in cleanup() function? or am I > missing something? They should be kept in imx7_csi_media_cleanup() and removed later. I'll fix it too. > > - v4l2_device_unregister(&imxmd->v4l2_dev); > > - media_device_unregister(&imxmd->md); > > - media_device_cleanup(&imxmd->md); > > + imx7_csi_media_cleanup(csi); > > > > destroy_mutex: > > mutex_destroy(&csi->lock); > > @@ -1285,14 +1304,8 @@ static int imx7_csi_remove(struct platform_device *pdev) > > { > > struct v4l2_subdev *sd = platform_get_drvdata(pdev); > > struct imx7_csi *csi = v4l2_get_subdevdata(sd); > > - struct imx_media_dev *imxmd = csi->imxmd; > > > > - v4l2_async_nf_unregister(&imxmd->notifier); > > - v4l2_async_nf_cleanup(&imxmd->notifier); > > - > > - media_device_unregister(&imxmd->md); > > - v4l2_device_unregister(&imxmd->v4l2_dev); > > - media_device_cleanup(&imxmd->md); > > + imx7_csi_media_cleanup(csi); > > > > v4l2_async_nf_unregister(&csi->notifier); > > v4l2_async_nf_cleanup(&csi->notifier);
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c index 893620e8fc03..d7c65b8bb3c9 100644 --- a/drivers/staging/media/imx/imx7-media-csi.c +++ b/drivers/staging/media/imx/imx7-media-csi.c @@ -1175,11 +1175,45 @@ static int imx7_csi_async_register(struct imx7_csi *csi) return v4l2_async_register_subdev(&csi->sd); } +static void imx7_csi_media_cleanup(struct imx7_csi *csi) +{ + struct imx_media_dev *imxmd = csi->imxmd; + + v4l2_device_unregister(&imxmd->v4l2_dev); + media_device_unregister(&imxmd->md); + media_device_cleanup(&imxmd->md); +} + +static int imx7_csi_media_init(struct imx7_csi *csi) +{ + struct imx_media_dev *imxmd; + int ret; + + /* add media device */ + imxmd = imx_media_dev_init(csi->dev, NULL); + if (IS_ERR(imxmd)) + return PTR_ERR(imxmd); + + ret = imx_media_of_add_csi(imxmd, csi->dev->of_node); + if (ret < 0 && ret != -ENODEV && ret != -EEXIST) { + imx7_csi_media_cleanup(csi); + return ret; + } + + ret = imx_media_dev_notifier_register(imxmd, NULL); + if (ret < 0) { + imx7_csi_media_cleanup(csi); + return ret; + } + + csi->imxmd = imxmd; + + return 0; +} + static int imx7_csi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *node = dev->of_node; - struct imx_media_dev *imxmd; struct imx7_csi *csi; int i, ret; @@ -1193,6 +1227,7 @@ static int imx7_csi_probe(struct platform_device *pdev) spin_lock_init(&csi->irqlock); mutex_init(&csi->lock); + /* Acquire resources and install interrupt handler. */ csi->mclk = devm_clk_get(&pdev->dev, "mclk"); if (IS_ERR(csi->mclk)) { ret = PTR_ERR(csi->mclk); @@ -1214,7 +1249,6 @@ static int imx7_csi_probe(struct platform_device *pdev) csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev); - /* install interrupt handler */ ret = devm_request_irq(dev, csi->irq, imx7_csi_irq_handler, 0, "csi", (void *)csi); if (ret < 0) { @@ -1222,22 +1256,11 @@ static int imx7_csi_probe(struct platform_device *pdev) goto destroy_mutex; } - /* add media device */ - imxmd = imx_media_dev_init(dev, NULL); - if (IS_ERR(imxmd)) { - ret = PTR_ERR(imxmd); + /* Initialize all the media device infrastructure. */ + ret = imx7_csi_media_init(csi); + if (ret) goto destroy_mutex; - } - ret = imx_media_of_add_csi(imxmd, node); - if (ret < 0 && ret != -ENODEV && ret != -EEXIST) - goto cleanup; - - ret = imx_media_dev_notifier_register(imxmd, NULL); - if (ret < 0) - goto cleanup; - - csi->imxmd = imxmd; v4l2_subdev_init(&csi->sd, &imx7_csi_subdev_ops); v4l2_set_subdevdata(&csi->sd, csi); csi->sd.internal_ops = &imx7_csi_internal_ops; @@ -1269,11 +1292,7 @@ static int imx7_csi_probe(struct platform_device *pdev) v4l2_async_nf_cleanup(&csi->notifier); cleanup: - v4l2_async_nf_unregister(&imxmd->notifier); - v4l2_async_nf_cleanup(&imxmd->notifier); - v4l2_device_unregister(&imxmd->v4l2_dev); - media_device_unregister(&imxmd->md); - media_device_cleanup(&imxmd->md); + imx7_csi_media_cleanup(csi); destroy_mutex: mutex_destroy(&csi->lock); @@ -1285,14 +1304,8 @@ static int imx7_csi_remove(struct platform_device *pdev) { struct v4l2_subdev *sd = platform_get_drvdata(pdev); struct imx7_csi *csi = v4l2_get_subdevdata(sd); - struct imx_media_dev *imxmd = csi->imxmd; - v4l2_async_nf_unregister(&imxmd->notifier); - v4l2_async_nf_cleanup(&imxmd->notifier); - - media_device_unregister(&imxmd->md); - v4l2_device_unregister(&imxmd->v4l2_dev); - media_device_cleanup(&imxmd->md); + imx7_csi_media_cleanup(csi); v4l2_async_nf_unregister(&csi->notifier); v4l2_async_nf_cleanup(&csi->notifier);
Prepare for the decoupling of the imx7-media-csi driver from the IPUv3-based drivers by moving the imx_media_dev handling from probe() function to separate functions. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/staging/media/imx/imx7-media-csi.c | 71 +++++++++++++--------- 1 file changed, 42 insertions(+), 29 deletions(-)