Hi Sakari,
Thank you for the patch.
On Thu, May 25, 2023 at 12:16:06PM +0300, Sakari Ailus wrote:
> Fix and simplify error handling in pxa_camera probe, by moving devm_*()
> functions early in the probe function and then tearing down what was set
> up on error patch.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/intel/pxa_camera.c | 48 ++++++++++++-----------
> 1 file changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/platform/intel/pxa_camera.c b/drivers/media/platform/intel/pxa_camera.c
> index f0d316d5fe27c..dad5e8d97683e 100644
> --- a/drivers/media/platform/intel/pxa_camera.c
> +++ b/drivers/media/platform/intel/pxa_camera.c
> @@ -2289,6 +2289,24 @@ static int pxa_camera_probe(struct platform_device *pdev)
> if (IS_ERR(pcdev->clk))
> return PTR_ERR(pcdev->clk);
>
> + /*
> + * Request the regions.
> + */
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + pcdev->irq = irq;
> + pcdev->base = base;
> +
> + /* request irq */
> + err = devm_request_irq(&pdev->dev, pcdev->irq, pxa_camera_irq, 0,
> + PXA_CAM_DRV_NAME, pcdev);
> + if (err) {
> + dev_err(&pdev->dev, "Camera interrupt register failed\n");
> + return err;
> + }
> +
The IRQ should not be requested before the device is initialized, to
avoid spurious IRQs at probe time. I don't think the driver currently
handles this very well, but moving IRQ registration up is the wrong
direction. As this particular change isn't needed to clean up the
notifier, I would keep the devm_request_irq() call where it is.
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> v4l2_async_nf_init(&pcdev->notifier);
> pcdev->res = res;
> pcdev->pdata = pdev->dev.platform_data;
> @@ -2338,21 +2356,12 @@ static int pxa_camera_probe(struct platform_device *pdev)
> spin_lock_init(&pcdev->lock);
> mutex_init(&pcdev->mlock);
>
> - /*
> - * Request the regions.
> - */
> - base = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(base))
> - return PTR_ERR(base);
> -
> - pcdev->irq = irq;
> - pcdev->base = base;
> -
> /* request dma */
> 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");
> @@ -2379,14 +2388,6 @@ static int pxa_camera_probe(struct platform_device *pdev)
> }
> }
>
> - /* request irq */
> - err = devm_request_irq(&pdev->dev, pcdev->irq, pxa_camera_irq, 0,
> - PXA_CAM_DRV_NAME, pcdev);
> - if (err) {
> - dev_err(&pdev->dev, "Camera interrupt register failed\n");
> - goto exit_free_dma;
> - }
> -
> tasklet_setup(&pcdev->task_eof, pxa_camera_eof);
>
> pxa_camera_activate(pcdev);
> @@ -2398,16 +2399,15 @@ static int pxa_camera_probe(struct platform_device *pdev)
>
> err = pxa_camera_init_videobuf2(pcdev);
> if (err)
> - goto exit_notifier_cleanup;
> + goto exit_v4l2_device_unregister;
>
> 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_v4l2_device_unregister;
>
> return 0;
> -exit_notifier_cleanup:
> - v4l2_async_nf_cleanup(&pcdev->notifier);
> +exit_v4l2_device_unregister:
> v4l2_device_unregister(&pcdev->v4l2_dev);
> exit_deactivate:
> pxa_camera_deactivate(pcdev);
> @@ -2418,6 +2418,8 @@ 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);
> return err;
> }
>
--
Regards,
Laurent Pinchart
