Re: [PATCH v5 2/2] media: i2c: Add driver for AMSOSRMA Mira220

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Jai

On Tue, Jul 21, 2026 at 09:58:41PM +0530, Jai Luthra wrote:
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2026-07-21 21:37:59)
> > Hi Sashiko
> >
> > On Tue, Jul 21, 2026 at 02:36:14PM +0000, sashiko-bot@kernel.org wrote:
> > > Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
> > > - [High] Dereferencing `mira220->vblank` without checking for NULL in `mira220_init_controls()`.
> > > - [High] Missing `v4l2_subdev_cleanup()` in the driver's remove function causes an active state memory leak.
> > > - [Medium] The uninitialized stack variable `id` is evaluated and logged without checking if the I2C read succeeded.
> > > - [Medium] The active state format code is not updated when `V4L2_CID_HFLIP` or `V4L2_CID_VFLIP` are modified.
> > > - [Medium] Exposure control maximum limit exceeds the 16-bit hardware register width, leading to silent truncation.
> > > - [Medium] Hardware exposure register is not updated when `V4L2_CID_EXPOSURE` is clamped due to a `V4L2_CID_VBLANK` change.
> > > - [Medium] `mira220_power_on()` discards the actual error code when `clk_prepare_enable()` fails.
> > > - [Medium] Missing runtime PM reference hold during probe exposes the device to concurrent suspend.
> > > - [Medium] The error path in probe unconditionally powers off the device without checking its runtime PM state.
> > > - [Medium] V4L2_CID_HBLANK reports an incorrect static value when using a 1-lane MIPI CSI-2 configuration, breaking user-space framerate and exposure calculations.
> > > - [Low] Typo in user-facing control menu item: 'Vertial Gradient' instead of 'Vertical Gradient'.
> > > --
>
> [...]
>
> > > @@ -1936,6 +1936,6 @@ static int mira220_probe(struct i2c_client *client)
> > > [ ... ]
> > > > +   /* Enable runtime PM and turn off the device */
> > > > +   pm_runtime_set_autosuspend_delay(dev, 1000);
> > > > +   pm_runtime_use_autosuspend(dev);
> > > > +   pm_runtime_set_active(dev);
> > > > +   pm_runtime_enable(dev);
> > > > +
> > > > +   ret = mira220_identify_module(mira220);
> > >
> > > [Severity: Medium]
> > > Is it possible for the device to be concurrently suspended while probe
> > > continues?
> > >
> > > The device is marked active with a usage count of 0 after pm_runtime_enable().
> > > If the PM subsystem or userspace triggers a suspend (e.g. by setting
> > > autosuspend_delay_ms=0), the device could be powered down while probe executes
> > > mira220_identify_module().
> > >
> > > This could lead to I2C timeouts or bus crashes. Taking a pm_runtime reference
> > > hold could prevent this.
> > >
> >
> > By calling pm_runtime_get_noresume() (as the device is powered up
> > already) ?
> >
> >
> > > @@ -1985,5 +1985,5 @@ static int mira220_probe(struct i2c_client *client)
> > > [ ... ]
> > > > +error_power_off:
> > > > +   pm_runtime_disable(dev);
> > > > +   mira220_power_off(dev);
> > > > +   pm_runtime_set_suspended(dev);
> > > > +   return ret;
> > > > +}
> > >
> > > [Severity: Medium]
> > > Should this error path check the runtime PM state before unconditionally
> > > powering off the device?
> > >
> > > The PM core might have already suspended the device due to the zero usage count.
> > >
> > > Calling mira220_power_off() unconditionally could redundantly disable clocks
> > > and regulators that are already disabled, potentially causing double-disable
> > > warnings.
> > >
> >
> > Only in the case some suspend went on during probe. Even if unlikely,
> > guarding the call with
> >
> >         if (!pm_runtime_status_suspended(&client->dev))
> >                 mira220_power_off(&client->dev);
> >
> > costs nothing
> >
>
> For the above two issues it would be simpler to move the autosuspend just
> before you return from probe:
>
>     pm_runtime_set_active(dev)
>     pm_runtime_enable(dev)
>
>     ... rest of probe ..
>
>     pm_runtime_idle(dev)
>     pm_runtime_set_autosuspend_delay(dev, 1000);
>     pm_runtime_use_autosuspend(dev);
>
>     return 0;
>
> I saw this pattern in imx219/imx708 (sashiko was happy with this part in
>                                      the imx708 series I posted last week)
>

This is a good suggestion.

To be honest the comment

--------------------------------------------------------------------------------
@ -1936,6 +1936,6 @@ static int mira220_probe(struct i2c_client *client)
[ ... ]
> +     /* Enable runtime PM and turn off the device */
> +     pm_runtime_set_autosuspend_delay(dev, 1000);
> +     pm_runtime_use_autosuspend(dev);
> +     pm_runtime_set_active(dev);
> +     pm_runtime_enable(dev);
> +
> +     ret = mira220_identify_module(mira220);

[Severity: Medium]
Is it possible for the device to be concurrently suspended while probe
continues?

The device is marked active with a usage count of 0 after pm_runtime_enable().
If the PM subsystem or userspace triggers a suspend (e.g. by setting
autosuspend_delay_ms=0), the device could be powered down while probe executes
mira220_identify_module().
--------------------------------------------------------------------------------

Does mention "userspace triggering a suspend" (a system suspend?)
which seem to imply might happen concurrently to the driver probing
even if the device doesn't use autosuspend..

I feel like I should take your suggestion in and ignore the case of
system-suspend-while-probing ?


> Thanks,
>     Jai