Hi Dafna,
On 1/13/20 10:55 PM, Dafna Hirschfeld wrote:
> After a successful media_device_register call, call v4l2_device_get().
> and set the media_devnode release callback to a function that
> calls v4l2_device_put().
> That should ensure that the v4l2_device's release callback is called
> when the very last user of any of the registered device nodes has
> closed its fh.
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
> drivers/media/platform/vimc/vimc-core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 9d4e8bc89620..0f03e9cec075 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -214,6 +214,14 @@ static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
> kfree(vimc);
> }
>
> +static void vimc_media_device_release(struct media_devnode *devnode)
> +{
> + struct media_device *mdev = devnode->media_dev;
> + struct vimc_device *vimc = container_of(mdev, struct vimc_device, mdev);
> +
> + v4l2_device_put(&vimc->v4l2_dev);
> +}
> +
> static int vimc_register_devices(struct vimc_device *vimc)
> {
> int ret;
> @@ -252,6 +260,8 @@ static int vimc_register_devices(struct vimc_device *vimc)
> goto err_rm_subdevs;
> }
>
> + v4l2_device_get(&vimc->v4l2_dev);
> + vimc->mdev.devnode->release = vimc_media_device_release;
> /* Expose all subdev's nodes*/
> ret = v4l2_device_register_subdev_nodes(&vimc->v4l2_dev);
> if (ret) {
>
I like the idea, but I think the roles of v4l2_device and media_device should
be swapped. Logically the media device is the top-level device, and the v4l2_device
sits below it. So rather than cleaning everything up in the v4l2_device release
callback, that should be done in the mdev.devnode->release callback.
So during the probe you need a call to get_device(&mdev.devnode->dev) and in the
v4l2_device release callback you call put_device(&mdev.devnode->dev). And the
mdev.devnode->release() callback is then used to clean everything up.
I think it is a good idea to add helper functions media_device_get/put that take
a media_device pointer as argument, but I'd do that as a new last patch, replacing
any get/put_device() calls for mdev.devnode in one go. There may be some
discussion about that, so having this as the last patch makes it easier to postpone
merging it if needed.
Regards,
Hans
