Re: [PATCH 2/3] media: i2c: ov9282: Switch to using the sub-device state lock

[xiaolei wang]

On 3/1/26 02:27, Tarang Raval wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> Hi Xiaolei,
>
> > Switch to using the sub-device state lock and properly call
> > v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
> > remove().
> >
> > Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>
> ...
>
> > @@ -1458,16 +1442,20 @@ static int ov9282_probe(struct i2c_client *client)
> >          pm_runtime_enable(ov9282->dev);
> >          pm_runtime_idle(ov9282->dev);
> >
> > +       ret = v4l2_async_register_subdev_sensor(&ov9282->sd);
> > +       if (ret < 0)
> > +               goto v4l2_subdev_cleanup;
> > +
> >          return 0;
> >
> > +v4l2_subdev_cleanup:
> > +       v4l2_subdev_cleanup(&ov9282->sd);
>
> If v4l2_async_register_subdev_sensor() fails, runtime PM remains enabled.
> Can you please disable runtime PM (and set suspended state) on that error path?
Hi Tarang,

Thank you for the review!

You're right, I missed the runtime PM cleanup in the error path.
I'll fix it in v2:

v4l2_subdev_cleanup:
    v4l2_subdev_cleanup(&ov9282->sd);
    pm_runtime_disable(ov9282->dev);
    pm_runtime_set_suspended(ov9282->dev);
    goto error_media_entity;

This ensures runtime PM is properly disabled if subdev registration fails.

Best Regards,
Xiaolei
> >   error_media_entity:
> >          media_entity_cleanup(&ov9282->sd.entity);
> >   error_handler_free:
> >          v4l2_ctrl_handler_free(ov9282->sd.ctrl_handler);
> >   error_power_off:
> >          ov9282_power_off(ov9282->dev);
> > -error_mutex_destroy:
> > -       mutex_destroy(&ov9282->mutex);
> >
> >          return ret;
> >   }
> > @@ -1481,9 +1469,9 @@ static int ov9282_probe(struct i2c_client *client)
> >   static void ov9282_remove(struct i2c_client *client)
> >   {
> >          struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > -       struct ov9282 *ov9282 = to_ov9282(sd);
> >
> >          v4l2_async_unregister_subdev(sd);
> > +       v4l2_subdev_cleanup(sd);
> >          media_entity_cleanup(&sd->entity);
> >          v4l2_ctrl_handler_free(sd->ctrl_handler);
> >
> > @@ -1491,8 +1479,6 @@ static void ov9282_remove(struct i2c_client *client)
> >          if (!pm_runtime_status_suspended(&client->dev))
> >                  ov9282_power_off(&client->dev);
> >          pm_runtime_set_suspended(&client->dev);
> > -
> > -       mutex_destroy(&ov9282->mutex);
> >   }
> >
> >   static const struct dev_pm_ops ov9282_pm_ops = {
> > --
> > 2.43.0
> with above change
>
> Reviewed-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
>
> Best Regards,
> Tarang