[PATCH V10 2/2] media: i2c: dw9768: Add DW9768 VCM driver

[Tomasz Figa]

On Wed, Sep 2, 2020 at 7:35 AM Sergey Senozhatsky
<sergey.senozhatsky.work at gmail.com> wrote:
>
> On (20/07/03 16:04), Dongchun Zhu wrote:
> [..]
> > +static int dw9768_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct dw9768 *dw9768;
> > +     unsigned int i;
> > +     int ret;
> > +
> > +     dw9768 = devm_kzalloc(dev, sizeof(*dw9768), GFP_KERNEL);
> > +     if (!dw9768)
> > +             return -ENOMEM;
> > +
> > +     /* Initialize subdev */
> > +     v4l2_i2c_subdev_init(&dw9768->sd, client, &dw9768_ops);
> > +
> [..]
> > +     dw9768->sd.entity.function = MEDIA_ENT_F_LENS;
> > +
> > +     pm_runtime_enable(dev);
> > +     if (!pm_runtime_enabled(dev)) {
> > +             ret = dw9768_runtime_resume(dev);
> > +             if (ret < 0) {
> > +                     dev_err(dev, "failed to power on: %d\n", ret);
> > +                     goto err_clean_entity;
> > +             }
> > +     }
> > +
> > +     ret = v4l2_async_register_subdev(&dw9768->sd);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to register V4L2 subdev: %d", ret);
> > +             goto err_power_off;
> > +     }
>
> I would expect to see a slightly different order here: first set
> everything up, then expose the device to PM subsystem.
>

Thanks for chiming in. However, the code above is correct. Runtime PM
callbacks don't usually rely on the availability of any
userspace-facing components, while the opposite is true - when the
device is registered to the userspace, it must have all the
initialization aspects completed and that includes initialization of
runtime PM.

> [..]
> > +static int dw9768_remove(struct i2c_client *client)
> > +{
> > +     struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +     struct dw9768 *dw9768 = sd_to_dw9768(sd);
> > +
> > +     v4l2_async_unregister_subdev(&dw9768->sd);
> > +     v4l2_ctrl_handler_free(&dw9768->ctrls);
> > +     media_entity_cleanup(&dw9768->sd.entity);
> > +     pm_runtime_disable(&client->dev);
> > +     if (!pm_runtime_status_suspended(&client->dev))
> > +             dw9768_runtime_suspend(&client->dev);
> > +     pm_runtime_set_suspended(&client->dev);
>
> Ditto. Shall we first disable PM (so that we won't get any unexpected
> PM callbacks) and then destroy the device?

Ditto. +/- the opposite order

Best regards,
Tomasz