Hi All,
Thanks for the feedback.
As discussed in the thread, it make sense to request the IRQ at probe time
I will send next version based on this.
Cheers,
Biju
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Sunday, April 21, 2024 2:47 PM
> Subject: Re: [PATCH] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to
> rzg2l_cru_video_register()
>
> On Thu, Apr 18, 2024 at 11:06:27AM +0200, Geert Uytterhoeven wrote:
> > Hi Biju,
> >
> > On Mon, Feb 19, 2024 at 7:05 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > Move request_irq() to rzg2l_cru_video_register(), in order to avoid
> > > requesting IRQ during device start which happens frequently.
> > >
> > > Suggested-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> >
> > > @@ -1011,6 +1000,7 @@ void rzg2l_cru_video_unregister(struct
> > > rzg2l_cru_dev *cru) {
> > > media_device_unregister(&cru->mdev);
> > > video_unregister_device(&cru->vdev);
> > > + free_irq(cru->image_conv_irq, cru);
> > > }
> > >
> > > int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru) @@ -1018,6
> > > +1008,13 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > > struct video_device *vdev = &cru->vdev;
> > > int ret;
> > >
> > > + ret = request_irq(cru->image_conv_irq, rzg2l_cru_irq,
> > > + IRQF_SHARED, KBUILD_MODNAME, cru);
> > > + if (ret) {
> > > + dev_err(cru->dev, "failed to request irq\n");
> > > + return ret;
> > > + }
> > > +
> > > if (video_is_registered(&cru->vdev)) {
> >
> > How can this happen? Perhaps rzg2l_cru_video_register() can be called
> > multiple times through the rzg2l_cru_group_notify_complete() notifier?
>
> The notifier completion handler shouldn't be called multiple times, no.
> There's however a possibility (I think) that a subdev could disappear of the device is unbound from
> its driver. If the device is later rebound, the notifier completion handler could be called again.
>
> The issue is that rzg2l_cru_video_unregister() is called from .remove().
> I think a better fix would be to request the IRQ at probe time (or did we discuss that previously
> and concluded it could cause issues ?). I would also argue that the video devices should be
> registered at probe time, not in the notifier completion handler.
>
> > If that is true, the request_irq() should be moved after this block,
> > just before the call to video_register_device() below.
> >
> > > struct media_entity *entity;
> > >
> > > @@ -1032,14 +1029,18 @@ int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru)
> > > ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > > if (ret) {
> > > dev_err(cru->dev, "Failed to register video device\n");
> > > - return ret;
> > > + goto err_request_irq;
> > > }
> > >
> > > ret = media_device_register(&cru->mdev);
> > > - if (ret) {
> > > - video_unregister_device(&cru->vdev);
> > > - return ret;
> > > - }
> > > + if (ret)
> > > + goto err_video_unregister;
> > >
> > > return 0;
> > > +
> > > +err_video_unregister:
> > > + video_unregister_device(&cru->vdev);
> > > +err_request_irq:
> > > + free_irq(cru->image_conv_irq, cru);
> > > + return ret;
> > > }
>
> --
> Regards,
>
> Laurent Pinchart
