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

[xiaolei wang <xiaolei.wang@windriver.com>]

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@windriver.com>
> ...
>
>> @@ -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@siliconsignals.io>
>
> Best Regards,
> Tarang