| Message ID | 20240605175010.405638-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] media: platform: rzg2l-cru: rzg2l-video: Move request_irq() to probe() | expand |
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@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > 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, Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > 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);
Hi Laurent, Thanks for the feedback. > -----Original Message----- > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > 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@ideasonboard.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > 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@ideasonboard.com> > > > 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
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)); +} + 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);
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@ideasonboard.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- 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(-)