Hi Laurent,
Thanks for the review.
On Tue, May 30, 2023 at 07:51:25AM +0300, Laurent Pinchart wrote:
> 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>
Fair enough. I'll move this to just before registering the async
sub-device.
devm_request_irq() is also problematic as an IRQ may still happen once the
driver has executed much of its remove function. I this case this isn't
probably too much of an issue though.
--
Regards,
Sakari Ailus
