Re: [PATCH 3/9] media: rzg2l-cru: Use bulk reset API in rzg2l_cru_start_streaming_vq()

Hi Jacopo,
Thanks for your review.

On Mon, Jul 27, 2026 at 11:55:17AM +0200, Jacopo Mondi wrote:
> Hi Tommaso
> 
> On Tue, Jun 16, 2026 at 07:05:33PM +0200, Tommaso Merciai wrote:
> > Replace individual reset_control_deassert() calls for aresetn and presetn
> > with reset_control_bulk_deassert(), and consolidate the error path labels
> > into a single err_assert_resets using reset_control_bulk_assert().
> >
> > No functional changes intended.
> >
> > Signed-off-by: Tommaso Merciai <[email protected]>
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 23 ++++++++-----------
> >  1 file changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index 5185a547461d..bf61a74f8f74 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -672,6 +672,10 @@ irqreturn_t rzg3e_cru_irq(int irq, void *data)
> >  static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count)
> >  {
> >  	struct rzg2l_cru_dev *cru = vb2_get_drv_priv(vq);
> > +	struct reset_control_bulk_data resets[] = {
> > +		{ .rstc = cru->aresetn },
> > +		{ .rstc = cru->presetn },
> > +	};
> >  	int ret;
> >
> >  	ret = pm_runtime_resume_and_get(cru->dev);
> > @@ -683,19 +687,12 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
> >  		goto err_pm_put;
> >
> >  	/* Release reset state */
> > -	ret = reset_control_deassert(cru->aresetn);
> > +	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
> >  	if (ret) {
> > -		dev_err(cru->dev, "failed to deassert aresetn\n");
> > +		dev_err(cru->dev, "failed to deassert resets\n");
> >  		goto err_vclk_disable;
> >  	}
> >
> > -	ret = reset_control_deassert(cru->presetn);
> > -	if (ret) {
> > -		reset_control_assert(cru->aresetn);
> > -		dev_err(cru->dev, "failed to deassert presetn\n");
> > -		goto assert_aresetn;
> > -	}
> > -
> 
> Is there any ordering requirement in the reset signal de-assertion ?

Even if there is no constraint on de-assertion ordering, I think you
are right: using reset_control_bulk_deassert() the de-assertion order
is changed.

reset_control_bulk_deassert() deasserts the last element of the array
first.

Good catch!
I will fix that in v2.


> 
> >  	/* Allocate scratch buffer */
> >  	cru->scratch = dma_alloc_coherent(cru->dev, cru->format.sizeimage,
> >  					  &cru->scratch_phys, GFP_KERNEL);
> > @@ -703,7 +700,7 @@ static int rzg2l_cru_start_streaming_vq(struct vb2_queue *vq, unsigned int count
> >  		rzg2l_cru_return_buffers(cru, VB2_BUF_STATE_QUEUED);
> >  		dev_err(cru->dev, "Failed to allocate scratch buffer\n");
> >  		ret = -ENOMEM;
> > -		goto assert_presetn;
> > +		goto err_assert_resets;
> >  	}
> >
> >  	cru->active_slot = 0;
> > @@ -722,11 +719,9 @@ 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);
> > -assert_presetn:
> > -	reset_control_assert(cru->presetn);
> >
> > -assert_aresetn:
> > -	reset_control_assert(cru->aresetn);
> > +err_assert_resets:
> > +	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
> >
> 
> Do you happen to know why rzg2l_cru_stop_streaming_vq() only assert
> 'presetn' and not 'aresetn' ?

We have:

rzg2l_cru_stop_streaming_vq()
  rzg2l_cru_set_stream(cru, 0)
    sd = remote pad subdev (= "ip" subdev)
    v4l2_subdev_call(sd, video, s_stream, 0)
      rzg2l_cru_ip_s_stream(sd, enable=0)  rzg2l-ip.c:191
        v4l2_subdev_call(cru->ip.remote, video, s_stream, 0)
        v4l2_subdev_call(cru->ip.remote, video, post_streamoff)
        rzg2l_cru_stop_image_processing(cru)
          reset_control_assert(cru->aresetn)  <-- ARESETN

  reset_control_assert(cru->presetn)          <-- PRESETN

Then both resets are asserted during rzg2l_cru_stop_streaming_vq().
Please correct me if I'm wrong.

Kind Regards,
Tommaso


> 
> >  err_vclk_disable:
> >  	clk_disable_unprepare(cru->vclk);
> > --
> > 2.54.0
> >