| Message ID | 20230525091615.2324824-24-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Separate links and async sub-devices | expand |
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@linux.intel.com> > --- > 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@ideasonboard.com> > 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; > } >
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; + } + 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; }
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@linux.intel.com> --- drivers/media/platform/intel/pxa_camera.c | 48 ++++++++++++----------- 1 file changed, 25 insertions(+), 23 deletions(-)