Hi Sakari,
On Wed 09 Dec 20, 01:34, Sakari Ailus wrote:
> Hi Paul,
>
> On Sat, Nov 28, 2020 at 03:33:50PM +0100, Paul Kocialkowski wrote:
> ...
> > + if (ret)
> > + goto error_ctrls;
> > +
> > + /* V4L2 subdev register */
> > +
> > + ret = v4l2_async_register_subdev_sensor_common(subdev);
>
> The driver's device node may be already available to the user here...
>
> > + if (ret)
> > + goto error_ctrls;
> > +
> > + /* Runtime PM */
> > +
> > + pm_runtime_enable(sensor->dev);
> > + pm_runtime_set_suspended(sensor->dev);
>
> but runtime PM is enabled here.
>
> This needs to be done in a different order. Otherwise chances are that the
> device node is accessed before the device is powered on.
Oh that makes sense yes. Good catch :)
> > +
> > + return 0;
> > +
> > +error_ctrls:
> > + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> > +
> > +error_mutex:
> > + mutex_destroy(&sensor->mutex);
> > +
> > +error_entity:
> > + media_entity_cleanup(&sensor->subdev.entity);
> > +
> > +error_endpoint:
> > + v4l2_fwnode_endpoint_free(&sensor->endpoint);
> > +
> > + return ret;
> > +}
> > +
> > +static int ov5648_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > + struct ov5648_sensor *sensor = ov5648_subdev_sensor(subdev);
> > +
> > + clk_rate_exclusive_put(sensor->xvclk);
> > +
> > + v4l2_async_unregister_subdev(subdev);
> > + mutex_destroy(&sensor->mutex);
> > + media_entity_cleanup(&subdev->entity);
> > + v4l2_device_unregister_subdev(subdev);
> > + pm_runtime_disable(sensor->dev);
> > +
> > + ov5648_sensor_power(sensor, false);
>
> This needs to go, too, as there's no corresponding operation that powered
> on the device.
>
> Also don't forget to release the control handler.
>
> I believe these apply to both of the two drivers.
Fair enough and I think runtime PM will have called that already anyways.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
