| Message ID | 20230330115853.1628216-11-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Separate links and async sub-devices | expand |
Hi Sakari, Thank you for the patch. On Thu, Mar 30, 2023 at 02:58:45PM +0300, Sakari Ailus wrote: > Register V4L2 device before initialising the notifier. This way the device > is available to the notifier from the beginning. Could you please explain in the commit message why this is needed ? Same comment for subsequent patches in this series. > Also fix error handling in probe. Splitting this in two patches, with the fix first, would make it easier to review. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/platform/intel/pxa_camera.c | 30 +++++++++++++---------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/intel/pxa_camera.c b/drivers/media/platform/intel/pxa_camera.c > index b848a2a9032f..31ae220ee4f3 100644 > --- a/drivers/media/platform/intel/pxa_camera.c > +++ b/drivers/media/platform/intel/pxa_camera.c > @@ -2289,6 +2289,10 @@ static int pxa_camera_probe(struct platform_device *pdev) > if (IS_ERR(pcdev->clk)) > return PTR_ERR(pcdev->clk); > > + err = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev); > + if (err) > + return err; > + > v4l2_async_nf_init(&pcdev->notifier); > pcdev->res = res; > pcdev->pdata = pdev->dev.platform_data; > @@ -2306,10 +2310,10 @@ static int pxa_camera_probe(struct platform_device *pdev) > } else if (pdev->dev.of_node) { > err = pxa_camera_pdata_from_dt(&pdev->dev, pcdev); > } else { > - return -ENODEV; > + err = -ENODEV; > } > if (err < 0) > - return err; > + goto exit_notifier_cleanup; > > if (!(pcdev->platform_flags & (PXA_CAMERA_DATAWIDTH_8 | > PXA_CAMERA_DATAWIDTH_9 | PXA_CAMERA_DATAWIDTH_10))) { > @@ -2342,8 +2346,10 @@ static int pxa_camera_probe(struct platform_device *pdev) > * Request the regions. > */ > base = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(base)) > - return PTR_ERR(base); > + if (IS_ERR(base)) { > + err = PTR_ERR(base); > + goto exit_notifier_cleanup; > + } > > pcdev->irq = irq; > pcdev->base = base; > @@ -2352,7 +2358,8 @@ static int pxa_camera_probe(struct platform_device *pdev) > pcdev->dma_chans[0] = dma_request_chan(&pdev->dev, "CI_Y"); > if (IS_ERR(pcdev->dma_chans[0])) { > dev_err(&pdev->dev, "Can't request DMA for Y\n"); > - return PTR_ERR(pcdev->dma_chans[0]); > + err = PTR_ERR(pcdev->dma_chans[0]); > + goto exit_notifier_cleanup; > } > > pcdev->dma_chans[1] = dma_request_chan(&pdev->dev, "CI_U"); > @@ -2392,23 +2399,17 @@ static int pxa_camera_probe(struct platform_device *pdev) > pxa_camera_activate(pcdev); > > platform_set_drvdata(pdev, pcdev); > - err = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev); > - if (err) > - goto exit_deactivate; > > err = pxa_camera_init_videobuf2(pcdev); > if (err) > - goto exit_notifier_cleanup; > + goto exit_deactivate; > > pcdev->notifier.ops = &pxa_camera_sensor_ops; > err = v4l2_async_nf_register(&pcdev->v4l2_dev, &pcdev->notifier); > if (err) > - goto exit_notifier_cleanup; > + goto exit_deactivate; > > return 0; > -exit_notifier_cleanup: > - v4l2_async_nf_cleanup(&pcdev->notifier); > - v4l2_device_unregister(&pcdev->v4l2_dev); > exit_deactivate: > pxa_camera_deactivate(pcdev); > tasklet_kill(&pcdev->task_eof); > @@ -2418,6 +2419,9 @@ static int pxa_camera_probe(struct platform_device *pdev) > dma_release_channel(pcdev->dma_chans[1]); > exit_free_dma_y: > dma_release_channel(pcdev->dma_chans[0]); > +exit_notifier_cleanup: > + v4l2_async_nf_cleanup(&pcdev->notifier); > + v4l2_device_unregister(&pcdev->v4l2_dev); > return err; > } >
Hi Laurent, On Tue, Apr 25, 2023 at 03:25:06AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, Mar 30, 2023 at 02:58:45PM +0300, Sakari Ailus wrote: > > Register V4L2 device before initialising the notifier. This way the device > > is available to the notifier from the beginning. > > Could you please explain in the commit message why this is needed ? Same > comment for subsequent patches in this series. Yes. > > > Also fix error handling in probe. > > Splitting this in two patches, with the fix first, would make it easier > to review. I'll address these in v2.
diff --git a/drivers/media/platform/intel/pxa_camera.c b/drivers/media/platform/intel/pxa_camera.c index b848a2a9032f..31ae220ee4f3 100644 --- a/drivers/media/platform/intel/pxa_camera.c +++ b/drivers/media/platform/intel/pxa_camera.c @@ -2289,6 +2289,10 @@ static int pxa_camera_probe(struct platform_device *pdev) if (IS_ERR(pcdev->clk)) return PTR_ERR(pcdev->clk); + err = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev); + if (err) + return err; + v4l2_async_nf_init(&pcdev->notifier); pcdev->res = res; pcdev->pdata = pdev->dev.platform_data; @@ -2306,10 +2310,10 @@ static int pxa_camera_probe(struct platform_device *pdev) } else if (pdev->dev.of_node) { err = pxa_camera_pdata_from_dt(&pdev->dev, pcdev); } else { - return -ENODEV; + err = -ENODEV; } if (err < 0) - return err; + goto exit_notifier_cleanup; if (!(pcdev->platform_flags & (PXA_CAMERA_DATAWIDTH_8 | PXA_CAMERA_DATAWIDTH_9 | PXA_CAMERA_DATAWIDTH_10))) { @@ -2342,8 +2346,10 @@ static int pxa_camera_probe(struct platform_device *pdev) * Request the regions. */ base = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(base)) - return PTR_ERR(base); + if (IS_ERR(base)) { + err = PTR_ERR(base); + goto exit_notifier_cleanup; + } pcdev->irq = irq; pcdev->base = base; @@ -2352,7 +2358,8 @@ static int pxa_camera_probe(struct platform_device *pdev) pcdev->dma_chans[0] = dma_request_chan(&pdev->dev, "CI_Y"); if (IS_ERR(pcdev->dma_chans[0])) { dev_err(&pdev->dev, "Can't request DMA for Y\n"); - return PTR_ERR(pcdev->dma_chans[0]); + err = PTR_ERR(pcdev->dma_chans[0]); + goto exit_notifier_cleanup; } pcdev->dma_chans[1] = dma_request_chan(&pdev->dev, "CI_U"); @@ -2392,23 +2399,17 @@ static int pxa_camera_probe(struct platform_device *pdev) pxa_camera_activate(pcdev); platform_set_drvdata(pdev, pcdev); - err = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev); - if (err) - goto exit_deactivate; err = pxa_camera_init_videobuf2(pcdev); if (err) - goto exit_notifier_cleanup; + goto exit_deactivate; pcdev->notifier.ops = &pxa_camera_sensor_ops; err = v4l2_async_nf_register(&pcdev->v4l2_dev, &pcdev->notifier); if (err) - goto exit_notifier_cleanup; + goto exit_deactivate; return 0; -exit_notifier_cleanup: - v4l2_async_nf_cleanup(&pcdev->notifier); - v4l2_device_unregister(&pcdev->v4l2_dev); exit_deactivate: pxa_camera_deactivate(pcdev); tasklet_kill(&pcdev->task_eof); @@ -2418,6 +2419,9 @@ static int pxa_camera_probe(struct platform_device *pdev) dma_release_channel(pcdev->dma_chans[1]); exit_free_dma_y: dma_release_channel(pcdev->dma_chans[0]); +exit_notifier_cleanup: + v4l2_async_nf_cleanup(&pcdev->notifier); + v4l2_device_unregister(&pcdev->v4l2_dev); return err; }
Register V4L2 device before initialising the notifier. This way the device is available to the notifier from the beginning. Also fix error handling in probe. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/platform/intel/pxa_camera.c | 30 +++++++++++++---------- 1 file changed, 17 insertions(+), 13 deletions(-)