Hi Yong,
On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi <yong.zhi@xxxxxxxxx> wrote:
[snip]
> +static int ipu3_vidioc_enum_input(struct file *file, void *fh,
> + struct v4l2_input *input)
> +{
> + if (input->index > 0)
> + return -EINVAL;
> + strlcpy(input->name, "camera", sizeof(input->name));
> + input->type = V4L2_INPUT_TYPE_CAMERA;
> +
> + return 0;
> +}
> +
> +static int ipu3_vidioc_g_input(struct file *file, void *fh, unsigned int *input)
> +{
> + *input = 0;
> +
> + return 0;
> +}
> +
> +static int ipu3_vidioc_s_input(struct file *file, void *fh, unsigned int input)
> +{
> + return input == 0 ? 0 : -EINVAL;
> +}
> +
> +static int ipu3_vidioc_enum_output(struct file *file, void *fh,
> + struct v4l2_output *output)
> +{
> + if (output->index > 0)
> + return -EINVAL;
> + strlcpy(output->name, "camera", sizeof(output->name));
> + output->type = V4L2_INPUT_TYPE_CAMERA;
> +
> + return 0;
> +}
> +
> +static int ipu3_vidioc_g_output(struct file *file, void *fh,
> + unsigned int *output)
> +{
> + *output = 0;
> +
> + return 0;
> +}
> +
> +static int ipu3_vidioc_s_output(struct file *file, void *fh,
> + unsigned int output)
> +{
> + return output == 0 ? 0 : -EINVAL;
> +}
Do we really need to implement the 6 functions above? They don't seem
to be doing anything useful.
[snip]
> +int ipu3_v4l2_register(struct imgu_device *imgu)
> +{
> + struct v4l2_mbus_framefmt def_bus_fmt = { 0 };
> + struct v4l2_pix_format_mplane def_pix_fmt = { 0 };
> +
> + int i, r;
> +
> + /* Initialize miscellaneous variables */
> + imgu->streaming = false;
> +
> + /* Set up media device */
> + imgu->media_dev.dev = &imgu->pci_dev->dev;
> + strlcpy(imgu->media_dev.model, IMGU_NAME,
> + sizeof(imgu->media_dev.model));
> + snprintf(imgu->media_dev.bus_info, sizeof(imgu->media_dev.bus_info),
> + "%s", dev_name(&imgu->pci_dev->dev));
> + imgu->media_dev.hw_revision = 0;
> + media_device_init(&imgu->media_dev);
> + r = media_device_register(&imgu->media_dev);
> + if (r) {
> + dev_err(&imgu->pci_dev->dev,
> + "failed to register media device (%d)\n", r);
> + return r;
> + }
Shouldn't we register the media device at the end, after all video
nodes are registered below? Otherwise, since media_device_register()
exposes the media node to userspace, we risk a race, when userspace
opens the media device before all the entities are created and linked.
[snip]
> +int ipu3_v4l2_unregister(struct imgu_device *imgu)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < IMGU_NODE_NUM; i++) {
> + video_unregister_device(&imgu->nodes[i].vdev);
> + media_entity_cleanup(&imgu->nodes[i].vdev.entity);
> + mutex_destroy(&imgu->nodes[i].lock);
> + }
> +
> + v4l2_device_unregister_subdev(&imgu->subdev);
> + media_entity_cleanup(&imgu->subdev.entity);
> + kfree(imgu->subdev_pads);
> + v4l2_device_unregister(&imgu->v4l2_dev);
> + media_device_unregister(&imgu->media_dev);
Should unregister media device at the beginning, so that it cannot be
used when we continue to clean up the entities.
> + media_device_cleanup(&imgu->media_dev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ipu3_v4l2_unregister);
Best regards,
Tomasz
