Hi Laurent,
Thanks for the feedback.
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Friday, August 23, 2024 4:36 PM
> Subject: Re: [PATCH v2] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to probe()
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Wed, Jun 05, 2024 at 06:50:10PM +0100, Biju Das wrote:
> > Move request_irq() to probe(), in order to avoid requesting IRQ during
> > device start which happens frequently. As this function is in probe(),
> > it is better to replace it with its devm variant for managing the
> > resource efficiently.
> >
> > Suggested-by: Laurent Pinchart
> > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> > * Updated commit header and description.
> > * Moved rzg2l_cru_irq from rzg2l-video.c->rzg2l-core.c and introduced
> > rzg2l_cru_process_irq() in video.c to process irq.
> > * Dropped image_conv_irq from struct rzg2l_cru_dev
> > * Replaced request_irq with its devm variant.
> > ---
> > .../platform/renesas/rzg2l-cru/rzg2l-core.c | 20 +++++++++++++++----
> > .../platform/renesas/rzg2l-cru/rzg2l-cru.h | 5 +----
> > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 18 +++--------------
> > 3 files changed, 20 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > index 280efd2a8185..b80e5960b88b 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-core.c
> > @@ -239,10 +239,17 @@ static int rzg2l_cru_media_init(struct rzg2l_cru_dev *cru)
> > return 0;
> > }
> >
> > +static irqreturn_t rzg2l_cru_irq(int irq, void *data) {
> > + struct rzg2l_cru_dev *cru = data;
> > +
> > + return IRQ_RETVAL(rzg2l_cru_process_irq(cru));
> > +}
> > +
>
> Is there a neew to introduce this intermediate wrapper function ? Can't the existing rzg2l_cru_irq()
> function from rzg2l-video.c be used, just dropping the static keyword ? With that fixed,
OK, will fix this in next version.
Cheers,
Biju
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>
> > static int rzg2l_cru_probe(struct platform_device *pdev) {
> > struct rzg2l_cru_dev *cru;
> > - int ret;
> > + int irq, ret;
> >
> > cru = devm_kzalloc(&pdev->dev, sizeof(*cru), GFP_KERNEL);
> > if (!cru)
> > @@ -270,9 +277,14 @@ static int rzg2l_cru_probe(struct platform_device *pdev)
> > cru->dev = &pdev->dev;
> > cru->info = of_device_get_match_data(&pdev->dev);
> >
> > - cru->image_conv_irq = platform_get_irq(pdev, 0);
> > - if (cru->image_conv_irq < 0)
> > - return cru->image_conv_irq;
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return irq;
> > +
> > + ret = devm_request_irq(&pdev->dev, irq, rzg2l_cru_irq, IRQF_SHARED,
> > + KBUILD_MODNAME, cru);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret, "failed to request irq\n");
> >
> > platform_set_drvdata(pdev, cru);
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > index a5a99b004322..72405e632aca 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> > @@ -68,8 +68,6 @@ struct rzg2l_cru_ip {
> > *
> > * @vclk: CRU Main clock
> > *
> > - * @image_conv_irq: Holds image conversion interrupt number
> > - *
> > * @vdev: V4L2 video device associated with CRU
> > * @v4l2_dev: V4L2 device
> > * @num_buf: Holds the current number of buffers enabled
> > @@ -105,8 +103,6 @@ struct rzg2l_cru_dev {
> >
> > struct clk *vclk;
> >
> > - int image_conv_irq;
> > -
> > struct video_device vdev;
> > struct v4l2_device v4l2_dev;
> > u8 num_buf;
> > @@ -141,6 +137,7 @@ void rzg2l_cru_dma_unregister(struct rzg2l_cru_dev
> > *cru);
> >
> > int rzg2l_cru_video_register(struct rzg2l_cru_dev *cru); void
> > rzg2l_cru_video_unregister(struct rzg2l_cru_dev *cru);
> > +unsigned int rzg2l_cru_process_irq(struct rzg2l_cru_dev *cru);
> >
> > const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32
> > format);
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index b16b8af6e8f8..1512844fecb0 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -527,9 +527,8 @@ static void rzg2l_cru_stop_streaming(struct rzg2l_cru_dev *cru)
> > rzg2l_cru_set_stream(cru, 0);
> > }
> >
> > -static irqreturn_t rzg2l_cru_irq(int irq, void *data)
> > +unsigned int rzg2l_cru_process_irq(struct rzg2l_cru_dev *cru)
> > {
> > - struct rzg2l_cru_dev *cru = data;
> > unsigned int handled = 0;
> > unsigned long flags;
> > u32 irq_status;
> > @@ -607,7 +606,7 @@ static irqreturn_t rzg2l_cru_irq(int irq, void
> > *data)
> > done:
> > spin_unlock_irqrestore(&cru->qlock, flags);
> >
> > - return IRQ_RETVAL(handled);
> > + return handled;
> > }
> >
> > static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq,
> > unsigned int count) @@ -637,13 +636,6 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue
> *vq, unsigned int count
> > goto assert_aresetn;
> > }
> >
> > - 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");
> > - goto assert_presetn;
> > - }
> > -
> > /* Allocate scratch buffer. */
> > cru->scratch = dma_alloc_coherent(cru->dev, cru->format.sizeimage,
> > &cru->scratch_phys, GFP_KERNEL); @@ -651,7 +643,7 @@ static
> > int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
> > return_unused_buffers(cru, VB2_BUF_STATE_QUEUED);
> > dev_err(cru->dev, "Failed to allocate scratch buffer\n");
> > ret = -ENOMEM;
> > - goto free_image_conv_irq;
> > + goto assert_presetn;
> > }
> >
> > cru->sequence = 0;
> > @@ -670,9 +662,6 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
> > if (ret)
> > dma_free_coherent(cru->dev, cru->format.sizeimage, cru->scratch,
> > cru->scratch_phys);
> > -free_image_conv_irq:
> > - free_irq(cru->image_conv_irq, cru);
> > -
> > assert_presetn:
> > reset_control_assert(cru->presetn);
> >
> > @@ -698,7 +687,6 @@ static void rzg2l_cru_stop_streaming_vq(struct vb2_queue *vq)
> > dma_free_coherent(cru->dev, cru->format.sizeimage,
> > cru->scratch, cru->scratch_phys);
> >
> > - free_irq(cru->image_conv_irq, cru);
> > return_unused_buffers(cru, VB2_BUF_STATE_ERROR);
> >
> > reset_control_assert(cru->presetn);
>
> --
> Regards,
>
> Laurent Pinchart
