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,
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
