Re: [PATCH v3 1/3] media: i2c: cvs: Add driver of Intel Computer Vision Sensing Controller(CVS)

[Mehdi Djait <mehdi.djait@linux.intel.com>]

Hi Miguel,

On Sat, May 23, 2026 at 04:37:41PM -0700, Vadillo, Miguel wrote:
> Hi Mehdi,
> 
> On 5/22/26 1:39 AM, Mehdi Djait wrote:
> > Hi Miguel,
> > 
> > Thank you for the patch.
> > 
> > On Thu, May 21, 2026 at 03:23:57PM -0700, Miguel Vadillo wrote:
> > > Add driver for Intel Computer Vision Sensing (CVS) devices found on
> > 
> > [..]
> > 
> > > +fail_i2c:
> > > +	ret = cvs_csi_init(ctx, dev, i2c);
> > > +	if (ret) {
> > > +		dev_err_probe(dev, ret, "CSI init failed\n");
> > > +		goto err_put_ipu;
> > > +	}
> > > +
> > > +	dev_set_drvdata(dev, ctx);
> > > +	pm_runtime_set_autosuspend_delay(dev, 1000);
> > > +	pm_runtime_use_autosuspend(dev);
> > > +	pm_runtime_enable(dev);
> > > +	pm_runtime_idle(dev);
> > > +
> > > +	/*
> > > +	 * Create a PM runtime device link with IPU as consumer and CVS as
> > > +	 * supplier. When the IPU runtime-resumes to start streaming, the PM
> > > +	 * framework automatically resumes CVS first, triggering
> > > +	 * cvs_runtime_resume() which hands CSI-2 link ownership to the host.
> > > +	 */
> > > +	ctx->ipu_link = device_link_add(&ipu->dev, dev,
> > > +					DL_FLAG_PM_RUNTIME |
> > > +					DL_FLAG_RPM_ACTIVE |
> > > +					DL_FLAG_STATELESS);
> > > +	put_device(&ipu->dev);
> > > +	if (!ctx->ipu_link) {
> > > +		dev_err(dev, "IPU device link failed\n");
> > > +		ret = -ENODEV;
> > > +		goto err_csi_remove;
> > > +	}
> > > +
> > > +	if (has_acpi_companion(dev))
> > > +		acpi_dev_clear_dependencies(ACPI_COMPANION(dev));
> > > +
> > > +	return 0;
> > > +
> > > +err_csi_remove:
> > > +	if (ctx->ipu_link)
> > > +		device_link_del(ctx->ipu_link);
> > > +	cvs_csi_remove(ctx);
> > > +	pm_runtime_dont_use_autosuspend(dev);
> > > +	pm_runtime_disable(dev);
> > > +	pm_runtime_set_suspended(dev);
> > > +	return ret;
> > 
> > this return ret seems wrong here.
> 
> Are you referring to just the space before return?
> @@ -1318,10 +1318,12 @@ static int cvs_core_probe(struct device *dev, struct
> i2c_client *i2c)
>         pm_runtime_dont_use_autosuspend(dev);
>         pm_runtime_disable(dev);
>         pm_runtime_set_suspended(dev);
> +
>         return ret;
> 
>  err_put_ipu:
>         put_device(&ipu->dev);
> +
>         return ret;
>  }
> 
> put_device(ipu) was already done after the ipu_link so no need to waterfall
> all the way down. Please clarify if I am misunderstanding your comment.

that's the expected code flow with goto err_x, goto err_y; -> That you do the
error handling in reverse order of allocation/init and then finish by return ret.

So in this case how about you hold on the the ipu-dev a bit longer,
remove the return ret of err_csi_remove and put_device() at the end if everything goes without error:

diff --git a/drivers/media/i2c/cvs/core.c b/drivers/media/i2c/cvs/core.c
index 9db56eb77645..b8d270c001b3 100644
--- a/drivers/media/i2c/cvs/core.c
+++ b/drivers/media/i2c/cvs/core.c
@@ -815,7 +815,6 @@ static int cvs_core_probe(struct device *dev, struct i2c_client *i2c)
                                        DL_FLAG_PM_RUNTIME |
                                        DL_FLAG_RPM_ACTIVE |
                                        DL_FLAG_STATELESS);
-       put_device(&ipu->dev);
        if (!ctx->ipu_link) {
                dev_err(dev, "IPU device link failed\n");
                ret = -ENODEV;
@@ -825,6 +824,8 @@ static int cvs_core_probe(struct device *dev, struct i2c_client *i2c)
        if (has_acpi_companion(dev))
                acpi_dev_clear_dependencies(ACPI_COMPANION(dev));
 
+       put_device(&ipu->dev);
+
        return 0;
 
 err_csi_remove:
@@ -834,7 +835,6 @@ static int cvs_core_probe(struct device *dev, struct i2c_client *i2c)
        pm_runtime_dont_use_autosuspend(dev);
        pm_runtime_disable(dev);
        pm_runtime_set_suspended(dev);
-       return ret;
 
 err_put_ipu:
        put_device(&ipu->dev);


(Actually if you want to go just a tiny bit the extra mile and see if we
can solve this without holding a reference to the dev for the whole
probe function will actually simplify the error handling a lot, this is
of course not a priority at all and I am ok with whatever you choose to
do)

--
Kind Regards
Mehdi Djait