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.

+
+	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.

-- 
Kind regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel