On Wed, Sep 2, 2020 at 7:35 AM Sergey Senozhatsky
<sergey.senozhatsky.work@xxxxxxxxx> 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
