[PATCH 0/3] Media Device Allocator API

[PATCH 0/3] Media Device Allocator API

[Shuah Khan]

Media Device Allocator API to allows multiple drivers share a media device.
Using this API, drivers can allocate a media device with the shared struct
device as the key. Once the media device is allocated by a driver, other
drivers can get a reference to it. The media device is released when all
the references are released.

This patch series has been tested with au0828 and snd-usb-audio drivers.
snd-usb-audio patch isn't included in this series. Once this patch series
is reviews and gets a stable state, I will send out the snd-usb-audio
patch.

Shuah Khan (3):
  media: Media Device Allocator API
  media: add media_device_unregister_put() interface
  media: change au0828 to use Media Device Allocator API

 drivers/media/Makefile                 |   3 +-
 drivers/media/media-dev-allocator.c    | 139 +++++++++++++++++++++++++++++++++
 drivers/media/media-device.c           |  11 +++
 drivers/media/usb/au0828/au0828-core.c |  12 +--
 drivers/media/usb/au0828/au0828.h      |   1 +
 include/media/media-dev-allocator.h    | 118 ++++++++++++++++++++++++++++
 include/media/media-device.h           |  15 ++++
 7 files changed, 290 insertions(+), 9 deletions(-)
 create mode 100644 drivers/media/media-dev-allocator.c
 create mode 100644 include/media/media-dev-allocator.h

-- 
2.7.4

[PATCH 1/3] media: Media Device Allocator API

[Shuah Khan]

Media Device Allocator API to allows multiple drivers share a media device.
Using this API, drivers can allocate a media device with the shared struct
device as the key. Once the media device is allocated by a driver, other
drivers can get a reference to it. The media device is released when all
the references are released.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/Makefile              |   3 +-
 drivers/media/media-dev-allocator.c | 139 ++++++++++++++++++++++++++++++++++++
 include/media/media-dev-allocator.h | 118 ++++++++++++++++++++++++++++++
 3 files changed, 259 insertions(+), 1 deletion(-)
 create mode 100644 drivers/media/media-dev-allocator.c
 create mode 100644 include/media/media-dev-allocator.h

diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index e608bbc..b08f091 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -2,7 +2,8 @@
 # Makefile for the kernel multimedia device drivers.
 #
 
-media-objs	:= media-device.o media-devnode.o media-entity.o
+media-objs	:= media-device.o media-devnode.o media-entity.o \
+		   media-dev-allocator.o
 
 #
 # I2C drivers should come before other drivers, otherwise they'll fail
diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
new file mode 100644
index 0000000..b49ab55
--- /dev/null
+++ b/drivers/media/media-dev-allocator.c
@@ -0,0 +1,139 @@
+/*
+ * media-dev-allocator.c - Media Controller Device Allocator API
+ *
+ * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+/*
+ * This file adds a global refcounted Media Controller Device Instance API.
+ * A system wide global media device list is managed and each media device
+ * includes a kref count. The last put on the media device releases the media
+ * device instance.
+ *
+*/
+
+#include <linux/slab.h>
+#include <linux/kref.h>
+#include <linux/usb.h>
+#include <media/media-device.h>
+
+static LIST_HEAD(media_device_list);
+static DEFINE_MUTEX(media_device_lock);
+
+struct media_device_instance {
+	struct media_device mdev;
+	struct list_head list;
+	struct device *dev;
+	struct kref refcount;
+};
+
+static inline struct media_device_instance *
+to_media_device_instance(struct media_device *mdev)
+{
+	return container_of(mdev, struct media_device_instance, mdev);
+}
+
+static void media_device_instance_release(struct kref *kref)
+{
+	struct media_device_instance *mdi =
+		container_of(kref, struct media_device_instance, refcount);
+
+	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
+
+	mutex_lock(&media_device_lock);
+
+	media_device_unregister(&mdi->mdev);
+	media_device_cleanup(&mdi->mdev);
+
+	list_del(&mdi->list);
+	mutex_unlock(&media_device_lock);
+
+	kfree(mdi);
+}
+
+static struct media_device *__media_device_get(struct device *dev,
+					       bool allocate)
+{
+	struct media_device_instance *mdi;
+
+	mutex_lock(&media_device_lock);
+
+	list_for_each_entry(mdi, &media_device_list, list) {
+		if (mdi->dev == dev) {
+			kref_get(&mdi->refcount);
+			dev_dbg(dev, "%s: get mdev=%p\n",
+				 __func__, &mdi->mdev);
+			goto done;
+		}
+	}
+
+	if (!allocate) {
+		mdi = NULL;
+		goto done;
+	}
+
+	mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
+	if (!mdi)
+		goto done;
+
+	mdi->dev = dev;
+	kref_init(&mdi->refcount);
+	list_add_tail(&mdi->list, &media_device_list);
+
+	dev_dbg(dev, "%s: alloc mdev=%p\n", __func__, &mdi->mdev);
+
+done:
+	mutex_unlock(&media_device_lock);
+
+	return mdi ? &mdi->mdev : NULL;
+}
+
+struct media_device *media_device_allocate(struct device *dev)
+{
+	dev_dbg(dev, "%s\n", __func__);
+	return __media_device_get(dev, true);
+}
+EXPORT_SYMBOL_GPL(media_device_allocate);
+
+struct media_device *media_device_usb_allocate(struct usb_device *udev,
+					       char *driver_name)
+{
+	struct media_device *mdev;
+
+	mdev = media_device_allocate(&udev->dev);
+	if (!mdev)
+		return ERR_PTR(-ENOMEM);
+
+	/* check if media device is already initialized */
+	mutex_lock(&media_device_lock);
+	if (!mdev->dev)
+		__media_device_usb_init(mdev, udev, udev->product,
+					driver_name);
+	mutex_unlock(&media_device_lock);
+
+	return mdev;
+}
+EXPORT_SYMBOL_GPL(media_device_usb_allocate);
+
+/* don't allocate - get reference if one is found */
+struct media_device *media_device_get(struct media_device *mdev)
+{
+	struct media_device_instance *mdi = to_media_device_instance(mdev);
+
+	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
+	return __media_device_get(mdi->dev, false);
+}
+EXPORT_SYMBOL_GPL(media_device_get);
+
+void media_device_put(struct media_device *mdev)
+{
+	struct media_device_instance *mdi = to_media_device_instance(mdev);
+
+	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
+	kref_put(&mdi->refcount, media_device_instance_release);
+}
+EXPORT_SYMBOL_GPL(media_device_put);
diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
new file mode 100644
index 0000000..a63dfc6
--- /dev/null
+++ b/include/media/media-dev-allocator.h
@@ -0,0 +1,118 @@
+/*
+ * media-dev-allocator.h - Media Controller Device Allocator API
+ *
+ * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
+ * Copyright (c) 2016 Samsung Electronics Co., Ltd.
+ *
+ * This file is released under the GPLv2.
+ * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ */
+
+/*
+ * This file adds a global ref-counted Media Controller Device Instance API.
+ * A system wide global media device list is managed and each media device
+ * includes a kref count. The last put on the media device releases the media
+ * device instance.
+*/
+
+#ifndef _MEDIA_DEV_ALLOCTOR_H
+#define _MEDIA_DEV_ALLOCTOR_H
+
+struct usb_device;
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+/**
+ * DOC: Media Controller Device Allocator API
+ *
+ * When media device belongs to more than one driver, the shared media device
+ * is allocated with the shared struct device as the key for look ups.
+ *
+ * Shared media device should stay in registered state until the last driver
+ * unregisters it. In addition, media device should be released when all the
+ * references are released. Each driver gets a reference to the media device
+ * during probe, when it allocates the media device. If media device is already
+ * allocated, allocate API bumps up the refcount and return the existing media
+ * device. Driver puts the reference back from its disconnect routine when it
+ * calls media_device_unregister_put().
+ *
+ * Media device is unregistered and cleaned up from the kref put handler to
+ * ensure that the media device stays in registered state until the last driver
+ * unregisters the media device.
+ *
+ * Media Device can be in any the following states:
+ *
+ * - Allocated
+ * - Registered (could be tied to more than one driver)
+ * - Unregistered and released in the kref put handler.
+ *
+ * Driver Usage:
+ *
+ * Drivers should use the media-core routines to get register reference and
+ * use the media_device_unregister_put() routine to make sure the shared
+ * media device delete is handled correctly.
+ *
+ * driver probe:
+ *	Call media_device_usb_allocate() to allocate or get a reference
+ *	Call media_device_register(), if media devnode isn't registered
+ *
+ * driver disconnect:
+ *	Call media_device_unregister_put() to put the reference.
+ *
+ */
+
+/**
+ * media_device_usb_allocate() - Allocate and return media device
+ *
+ * @udev - struct usb_device pointer
+ * @driver_name
+ *
+ * This interface should be called to allocate a media device when multiple
+ * drivers share usb_device and the media device. This interface allocates
+ * media device and calls media_device_usb_init() to initialize it.
+ *
+ */
+struct media_device *media_device_usb_allocate(struct usb_device *udev,
+					       char *driver_name);
+/**
+ * media_device_allocate() - Allocate and return global media device
+ *
+ * @dev - struct device pointer
+ *
+ * This interface should be called to allocate media device. A new media
+ * device instance is created and added to the system wide media device
+ * instance list. If media device instance exists, media_device_allocate()
+ * increments the reference count and returns the media device. When more
+ * than one driver control the media device, the first driver to probe will
+ * allocate and the second driver when it calls  media_device_allocate(),
+ * it will get a reference.
+ *
+ */
+struct media_device *media_device_allocate(struct device *dev);
+/**
+ * media_device_get() -	Get reference to a registered media device.
+ *
+ * @mdev - struct media_device pointer
+ *
+ * This interface should be called to get a reference to an allocated media
+ * device.
+ */
+struct media_device *media_device_get(struct media_device *mdev);
+/**
+ * media_device_put() - Release media device. Calls kref_put().
+ *
+ * @mdev - struct media_device pointer
+ *
+ * This interface should be called to put Media Device Instance kref.
+ */
+void media_device_put(struct media_device *mdev);
+#else
+static inline struct media_device *media_device_usb_allocate(
+			struct usb_device *udev, char *driver_name)
+			{ return NULL; }
+static inline struct media_device *media_device_allocate(struct device *dev)
+			{ return NULL; }
+static inline struct media_device *media_device_get(struct media_device *mdev)
+			{ return NULL; }
+static inline void media_device_put(struct media_device *mdev) { }
+#endif /* CONFIG_MEDIA_CONTROLLER */
+#endif
-- 
2.7.4

Re: [PATCH 1/3] media: Media Device Allocator API

[Hans Verkuil]

Hi Shuah,

Some comments below:

On 05/13/2016 07:09 PM, Shuah Khan wrote:
> Media Device Allocator API to allows multiple drivers share a media device.
> Using this API, drivers can allocate a media device with the shared struct
> device as the key. Once the media device is allocated by a driver, other
> drivers can get a reference to it. The media device is released when all
> the references are released.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/Makefile              |   3 +-
>  drivers/media/media-dev-allocator.c | 139 ++++++++++++++++++++++++++++++++++++
>  include/media/media-dev-allocator.h | 118 ++++++++++++++++++++++++++++++
>  3 files changed, 259 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/media-dev-allocator.c
>  create mode 100644 include/media/media-dev-allocator.h
> 
> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> index e608bbc..b08f091 100644
> --- a/drivers/media/Makefile
> +++ b/drivers/media/Makefile
> @@ -2,7 +2,8 @@
>  # Makefile for the kernel multimedia device drivers.
>  #
>  
> -media-objs	:= media-device.o media-devnode.o media-entity.o
> +media-objs	:= media-device.o media-devnode.o media-entity.o \
> +		   media-dev-allocator.o
>  
>  #
>  # I2C drivers should come before other drivers, otherwise they'll fail
> diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
> new file mode 100644
> index 0000000..b49ab55
> --- /dev/null
> +++ b/drivers/media/media-dev-allocator.c
> @@ -0,0 +1,139 @@
> +/*
> + * media-dev-allocator.c - Media Controller Device Allocator API
> + *
> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + *
> + * This file is released under the GPLv2.
> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +/*
> + * This file adds a global refcounted Media Controller Device Instance API.
> + * A system wide global media device list is managed and each media device
> + * includes a kref count. The last put on the media device releases the media
> + * device instance.
> + *
> +*/
> +
> +#include <linux/slab.h>
> +#include <linux/kref.h>
> +#include <linux/usb.h>
> +#include <media/media-device.h>
> +
> +static LIST_HEAD(media_device_list);
> +static DEFINE_MUTEX(media_device_lock);
> +
> +struct media_device_instance {
> +	struct media_device mdev;
> +	struct list_head list;
> +	struct device *dev;
> +	struct kref refcount;
> +};
> +
> +static inline struct media_device_instance *
> +to_media_device_instance(struct media_device *mdev)
> +{
> +	return container_of(mdev, struct media_device_instance, mdev);
> +}
> +
> +static void media_device_instance_release(struct kref *kref)
> +{
> +	struct media_device_instance *mdi =
> +		container_of(kref, struct media_device_instance, refcount);
> +
> +	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
> +
> +	mutex_lock(&media_device_lock);
> +
> +	media_device_unregister(&mdi->mdev);
> +	media_device_cleanup(&mdi->mdev);
> +
> +	list_del(&mdi->list);
> +	mutex_unlock(&media_device_lock);
> +
> +	kfree(mdi);
> +}
> +
> +static struct media_device *__media_device_get(struct device *dev,
> +					       bool allocate)
> +{
> +	struct media_device_instance *mdi;
> +
> +	mutex_lock(&media_device_lock);
> +
> +	list_for_each_entry(mdi, &media_device_list, list) {
> +		if (mdi->dev == dev) {
> +			kref_get(&mdi->refcount);
> +			dev_dbg(dev, "%s: get mdev=%p\n",
> +				 __func__, &mdi->mdev);
> +			goto done;
> +		}
> +	}
> +
> +	if (!allocate) {
> +		mdi = NULL;
> +		goto done;
> +	}
> +
> +	mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
> +	if (!mdi)
> +		goto done;
> +
> +	mdi->dev = dev;
> +	kref_init(&mdi->refcount);
> +	list_add_tail(&mdi->list, &media_device_list);
> +
> +	dev_dbg(dev, "%s: alloc mdev=%p\n", __func__, &mdi->mdev);
> +
> +done:
> +	mutex_unlock(&media_device_lock);
> +
> +	return mdi ? &mdi->mdev : NULL;
> +}
> +
> +struct media_device *media_device_allocate(struct device *dev)
> +{
> +	dev_dbg(dev, "%s\n", __func__);
> +	return __media_device_get(dev, true);
> +}
> +EXPORT_SYMBOL_GPL(media_device_allocate);

Do we need this function? Whenever a media device is allocated, you also need
to initialize the media device with the media_device_lock held (as happens
below in media_device_usb_allocate). You can't really use this function
standalone as far as I can see.

An alternative might be to pass a callback function with media_device_allocate
that initialized the media device. That would ensure that there are no race
conditions since media_device_allocate can lock media_device_lock before
calling the callback.

> +
> +struct media_device *media_device_usb_allocate(struct usb_device *udev,
> +					       char *driver_name)
> +{
> +	struct media_device *mdev;
> +
> +	mdev = media_device_allocate(&udev->dev);
> +	if (!mdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* check if media device is already initialized */
> +	mutex_lock(&media_device_lock);
> +	if (!mdev->dev)
> +		__media_device_usb_init(mdev, udev, udev->product,
> +					driver_name);
> +	mutex_unlock(&media_device_lock);
> +
> +	return mdev;
> +}
> +EXPORT_SYMBOL_GPL(media_device_usb_allocate);
> +
> +/* don't allocate - get reference if one is found */
> +struct media_device *media_device_get(struct media_device *mdev)
> +{
> +	struct media_device_instance *mdi = to_media_device_instance(mdev);
> +
> +	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
> +	return __media_device_get(mdi->dev, false);
> +}
> +EXPORT_SYMBOL_GPL(media_device_get);

Do we need this anywhere? It makes no sense to call this standalone, at least, not
that I can see. You just have different drivers that all call media_device_usb_allocate,
and they also call media_device_put when they are done with it. See also my comments
for the next patch.

> +
> +void media_device_put(struct media_device *mdev)
> +{
> +	struct media_device_instance *mdi = to_media_device_instance(mdev);
> +
> +	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
> +	kref_put(&mdi->refcount, media_device_instance_release);
> +}
> +EXPORT_SYMBOL_GPL(media_device_put);
> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
> new file mode 100644
> index 0000000..a63dfc6
> --- /dev/null
> +++ b/include/media/media-dev-allocator.h
> @@ -0,0 +1,118 @@
> +/*
> + * media-dev-allocator.h - Media Controller Device Allocator API
> + *
> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + *
> + * This file is released under the GPLv2.
> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + */
> +
> +/*
> + * This file adds a global ref-counted Media Controller Device Instance API.
> + * A system wide global media device list is managed and each media device
> + * includes a kref count. The last put on the media device releases the media
> + * device instance.
> +*/
> +
> +#ifndef _MEDIA_DEV_ALLOCTOR_H
> +#define _MEDIA_DEV_ALLOCTOR_H
> +
> +struct usb_device;
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +/**
> + * DOC: Media Controller Device Allocator API
> + *
> + * When media device belongs to more than one driver, the shared media device
> + * is allocated with the shared struct device as the key for look ups.
> + *
> + * Shared media device should stay in registered state until the last driver
> + * unregisters it. In addition, media device should be released when all the
> + * references are released. Each driver gets a reference to the media device
> + * during probe, when it allocates the media device. If media device is already
> + * allocated, allocate API bumps up the refcount and return the existing media
> + * device. Driver puts the reference back from its disconnect routine when it
> + * calls media_device_unregister_put().
> + *
> + * Media device is unregistered and cleaned up from the kref put handler to
> + * ensure that the media device stays in registered state until the last driver
> + * unregisters the media device.
> + *
> + * Media Device can be in any the following states:
> + *
> + * - Allocated

I don't think this is a real state. It's either unregistered or allocated and
registered. Just 'allocated' doesn't really make sense. This ties in to my
comment about media_device_allocate().

> + * - Registered (could be tied to more than one driver)
> + * - Unregistered and released in the kref put handler.

I would probably drop these 5 lines altogether.

> + *
> + * Driver Usage:
> + *
> + * Drivers should use the media-core routines to get register reference and
> + * use the media_device_unregister_put() routine to make sure the shared
> + * media device delete is handled correctly.
> + *
> + * driver probe:
> + *	Call media_device_usb_allocate() to allocate or get a reference
> + *	Call media_device_register(), if media devnode isn't registered
> + *
> + * driver disconnect:
> + *	Call media_device_unregister_put() to put the reference.

There is no media_device_unregister_put() (yet). That should be in the
next patch.

> + *
> + */
> +
> +/**
> + * media_device_usb_allocate() - Allocate and return media device
> + *
> + * @udev - struct usb_device pointer
> + * @driver_name
> + *
> + * This interface should be called to allocate a media device when multiple
> + * drivers share usb_device and the media device. This interface allocates
> + * media device and calls media_device_usb_init() to initialize it.
> + *
> + */
> +struct media_device *media_device_usb_allocate(struct usb_device *udev,
> +					       char *driver_name);
> +/**
> + * media_device_allocate() - Allocate and return global media device
> + *
> + * @dev - struct device pointer
> + *
> + * This interface should be called to allocate media device. A new media
> + * device instance is created and added to the system wide media device
> + * instance list. If media device instance exists, media_device_allocate()
> + * increments the reference count and returns the media device. When more
> + * than one driver control the media device, the first driver to probe will
> + * allocate and the second driver when it calls  media_device_allocate(),
> + * it will get a reference.
> + *
> + */
> +struct media_device *media_device_allocate(struct device *dev);
> +/**
> + * media_device_get() -	Get reference to a registered media device.
> + *
> + * @mdev - struct media_device pointer
> + *
> + * This interface should be called to get a reference to an allocated media
> + * device.
> + */
> +struct media_device *media_device_get(struct media_device *mdev);
> +/**
> + * media_device_put() - Release media device. Calls kref_put().
> + *
> + * @mdev - struct media_device pointer
> + *
> + * This interface should be called to put Media Device Instance kref.
> + */
> +void media_device_put(struct media_device *mdev);
> +#else
> +static inline struct media_device *media_device_usb_allocate(
> +			struct usb_device *udev, char *driver_name)
> +			{ return NULL; }
> +static inline struct media_device *media_device_allocate(struct device *dev)
> +			{ return NULL; }
> +static inline struct media_device *media_device_get(struct media_device *mdev)
> +			{ return NULL; }
> +static inline void media_device_put(struct media_device *mdev) { }
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +#endif
> 

Regards,

	Hans

Re: [PATCH 1/3] media: Media Device Allocator API

[Shuah Khan]

On 05/23/2016 05:26 AM, Hans Verkuil wrote:
> Hi Shuah,
> 
> Some comments below:

Thanks for the review.

> 
> On 05/13/2016 07:09 PM, Shuah Khan wrote:
>> Media Device Allocator API to allows multiple drivers share a media device.
>> Using this API, drivers can allocate a media device with the shared struct
>> device as the key. Once the media device is allocated by a driver, other
>> drivers can get a reference to it. The media device is released when all
>> the references are released.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/Makefile              |   3 +-
>>  drivers/media/media-dev-allocator.c | 139 ++++++++++++++++++++++++++++++++++++
>>  include/media/media-dev-allocator.h | 118 ++++++++++++++++++++++++++++++
>>  3 files changed, 259 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/media/media-dev-allocator.c
>>  create mode 100644 include/media/media-dev-allocator.h
>>
>> diff --git a/drivers/media/Makefile b/drivers/media/Makefile
>> index e608bbc..b08f091 100644
>> --- a/drivers/media/Makefile
>> +++ b/drivers/media/Makefile
>> @@ -2,7 +2,8 @@
>>  # Makefile for the kernel multimedia device drivers.
>>  #
>>  
>> -media-objs	:= media-device.o media-devnode.o media-entity.o
>> +media-objs	:= media-device.o media-devnode.o media-entity.o \
>> +		   media-dev-allocator.o
>>  
>>  #
>>  # I2C drivers should come before other drivers, otherwise they'll fail
>> diff --git a/drivers/media/media-dev-allocator.c b/drivers/media/media-dev-allocator.c
>> new file mode 100644
>> index 0000000..b49ab55
>> --- /dev/null
>> +++ b/drivers/media/media-dev-allocator.c
>> @@ -0,0 +1,139 @@
>> +/*
>> + * media-dev-allocator.c - Media Controller Device Allocator API
>> + *
>> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
>> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
>> + *
>> + * This file is released under the GPLv2.
>> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> + */
>> +
>> +/*
>> + * This file adds a global refcounted Media Controller Device Instance API.
>> + * A system wide global media device list is managed and each media device
>> + * includes a kref count. The last put on the media device releases the media
>> + * device instance.
>> + *
>> +*/
>> +
>> +#include <linux/slab.h>
>> +#include <linux/kref.h>
>> +#include <linux/usb.h>
>> +#include <media/media-device.h>
>> +
>> +static LIST_HEAD(media_device_list);
>> +static DEFINE_MUTEX(media_device_lock);
>> +
>> +struct media_device_instance {
>> +	struct media_device mdev;
>> +	struct list_head list;
>> +	struct device *dev;
>> +	struct kref refcount;
>> +};
>> +
>> +static inline struct media_device_instance *
>> +to_media_device_instance(struct media_device *mdev)
>> +{
>> +	return container_of(mdev, struct media_device_instance, mdev);
>> +}
>> +
>> +static void media_device_instance_release(struct kref *kref)
>> +{
>> +	struct media_device_instance *mdi =
>> +		container_of(kref, struct media_device_instance, refcount);
>> +
>> +	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
>> +
>> +	mutex_lock(&media_device_lock);
>> +
>> +	media_device_unregister(&mdi->mdev);
>> +	media_device_cleanup(&mdi->mdev);
>> +
>> +	list_del(&mdi->list);
>> +	mutex_unlock(&media_device_lock);
>> +
>> +	kfree(mdi);
>> +}
>> +
>> +static struct media_device *__media_device_get(struct device *dev,
>> +					       bool allocate)
>> +{
>> +	struct media_device_instance *mdi;
>> +
>> +	mutex_lock(&media_device_lock);
>> +
>> +	list_for_each_entry(mdi, &media_device_list, list) {
>> +		if (mdi->dev == dev) {
>> +			kref_get(&mdi->refcount);
>> +			dev_dbg(dev, "%s: get mdev=%p\n",
>> +				 __func__, &mdi->mdev);
>> +			goto done;
>> +		}
>> +	}
>> +
>> +	if (!allocate) {
>> +		mdi = NULL;
>> +		goto done;
>> +	}
>> +
>> +	mdi = kzalloc(sizeof(*mdi), GFP_KERNEL);
>> +	if (!mdi)
>> +		goto done;
>> +
>> +	mdi->dev = dev;
>> +	kref_init(&mdi->refcount);
>> +	list_add_tail(&mdi->list, &media_device_list);
>> +
>> +	dev_dbg(dev, "%s: alloc mdev=%p\n", __func__, &mdi->mdev);
>> +
>> +done:
>> +	mutex_unlock(&media_device_lock);
>> +
>> +	return mdi ? &mdi->mdev : NULL;
>> +}
>> +
>> +struct media_device *media_device_allocate(struct device *dev)
>> +{
>> +	dev_dbg(dev, "%s\n", __func__);
>> +	return __media_device_get(dev, true);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_allocate);
> 
> Do we need this function? Whenever a media device is allocated, you also need
> to initialize the media device with the media_device_lock held (as happens
> below in media_device_usb_allocate). You can't really use this function
> standalone as far as I can see.
> 
> An alternative might be to pass a callback function with media_device_allocate
> that initialized the media device. That would ensure that there are no race
> conditions since media_device_allocate can lock media_device_lock before
> calling the callback.

You are right, it is not used and all the usb drivers will use the
media_device_usb_allocate() and for pci drivers that have the need
to share the media device, we could add media_device_pci_allocate().
I think deleting this and collapsing the logic media_device_usb_allocate()
will help with the locking as well. I will fix this. I can delete this
routine all together and make some changes to __media_device_get() to
expect to be called media_device_lock held. That should work well.

> 
>> +
>> +struct media_device *media_device_usb_allocate(struct usb_device *udev,
>> +					       char *driver_name)
>> +{
>> +	struct media_device *mdev;
>> +
>> +	mdev = media_device_allocate(&udev->dev);
>> +	if (!mdev)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	/* check if media device is already initialized */
>> +	mutex_lock(&media_device_lock);
>> +	if (!mdev->dev)
>> +		__media_device_usb_init(mdev, udev, udev->product,
>> +					driver_name);
>> +	mutex_unlock(&media_device_lock);
>> +
>> +	return mdev;
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_usb_allocate);
>> +
>> +/* don't allocate - get reference if one is found */
>> +struct media_device *media_device_get(struct media_device *mdev)
>> +{
>> +	struct media_device_instance *mdi = to_media_device_instance(mdev);
>> +
>> +	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
>> +	return __media_device_get(mdi->dev, false);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_get);
> 
> Do we need this anywhere? It makes no sense to call this standalone, at least, not
> that I can see. You just have different drivers that all call media_device_usb_allocate,
> and they also call media_device_put when they are done with it. See also my comments
> for the next patch.

You are right. It was needed when I originally did this patch series
and used this to fix the lifetime issue. media_device_get() used to
get called from media_device_open() and media_device_put() from
media_device_close(). I left it in there just in case, there is a
need to get a reference, but I think it can be added later when the
need arises. I will delete this.

>> +
>> +void media_device_put(struct media_device *mdev)
>> +{
>> +	struct media_device_instance *mdi = to_media_device_instance(mdev);
>> +
>> +	dev_dbg(mdi->mdev.dev, "%s: mdev=%p\n", __func__, &mdi->mdev);
>> +	kref_put(&mdi->refcount, media_device_instance_release);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_put);
>> diff --git a/include/media/media-dev-allocator.h b/include/media/media-dev-allocator.h
>> new file mode 100644
>> index 0000000..a63dfc6
>> --- /dev/null
>> +++ b/include/media/media-dev-allocator.h
>> @@ -0,0 +1,118 @@
>> +/*
>> + * media-dev-allocator.h - Media Controller Device Allocator API
>> + *
>> + * Copyright (c) 2016 Shuah Khan <shuahkh@osg.samsung.com>
>> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
>> + *
>> + * This file is released under the GPLv2.
>> + * Credits: Suggested by Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> + */
>> +
>> +/*
>> + * This file adds a global ref-counted Media Controller Device Instance API.
>> + * A system wide global media device list is managed and each media device
>> + * includes a kref count. The last put on the media device releases the media
>> + * device instance.
>> +*/
>> +
>> +#ifndef _MEDIA_DEV_ALLOCTOR_H
>> +#define _MEDIA_DEV_ALLOCTOR_H
>> +
>> +struct usb_device;
>> +
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +/**
>> + * DOC: Media Controller Device Allocator API
>> + *
>> + * When media device belongs to more than one driver, the shared media device
>> + * is allocated with the shared struct device as the key for look ups.
>> + *
>> + * Shared media device should stay in registered state until the last driver
>> + * unregisters it. In addition, media device should be released when all the
>> + * references are released. Each driver gets a reference to the media device
>> + * during probe, when it allocates the media device. If media device is already
>> + * allocated, allocate API bumps up the refcount and return the existing media
>> + * device. Driver puts the reference back from its disconnect routine when it
>> + * calls media_device_unregister_put().
>> + *
>> + * Media device is unregistered and cleaned up from the kref put handler to
>> + * ensure that the media device stays in registered state until the last driver
>> + * unregisters the media device.
>> + *
>> + * Media Device can be in any the following states:
>> + *
>> + * - Allocated
> 
> I don't think this is a real state. It's either unregistered or allocated and
> registered. Just 'allocated' doesn't really make sense. This ties in to my
> comment about media_device_allocate().
> 
>> + * - Registered (could be tied to more than one driver)
>> + * - Unregistered and released in the kref put handler.
> 
> I would probably drop these 5 lines altogether.
> 
>> + *
>> + * Driver Usage:
>> + *
>> + * Drivers should use the media-core routines to get register reference and
>> + * use the media_device_unregister_put() routine to make sure the shared
>> + * media device delete is handled correctly.
>> + *
>> + * driver probe:
>> + *	Call media_device_usb_allocate() to allocate or get a reference
>> + *	Call media_device_register(), if media devnode isn't registered
>> + *
>> + * driver disconnect:
>> + *	Call media_device_unregister_put() to put the reference.
> 
> There is no media_device_unregister_put() (yet). That should be in the
> next patch.
> 
>> + *
>> + */
>> +
>> +/**
>> + * media_device_usb_allocate() - Allocate and return media device
>> + *
>> + * @udev - struct usb_device pointer
>> + * @driver_name
>> + *
>> + * This interface should be called to allocate a media device when multiple
>> + * drivers share usb_device and the media device. This interface allocates
>> + * media device and calls media_device_usb_init() to initialize it.
>> + *
>> + */
>> +struct media_device *media_device_usb_allocate(struct usb_device *udev,
>> +					       char *driver_name);
>> +/**
>> + * media_device_allocate() - Allocate and return global media device
>> + *
>> + * @dev - struct device pointer
>> + *
>> + * This interface should be called to allocate media device. A new media
>> + * device instance is created and added to the system wide media device
>> + * instance list. If media device instance exists, media_device_allocate()
>> + * increments the reference count and returns the media device. When more
>> + * than one driver control the media device, the first driver to probe will
>> + * allocate and the second driver when it calls  media_device_allocate(),
>> + * it will get a reference.
>> + *
>> + */
>> +struct media_device *media_device_allocate(struct device *dev);
>> +/**
>> + * media_device_get() -	Get reference to a registered media device.
>> + *
>> + * @mdev - struct media_device pointer
>> + *
>> + * This interface should be called to get a reference to an allocated media
>> + * device.
>> + */
>> +struct media_device *media_device_get(struct media_device *mdev);
>> +/**
>> + * media_device_put() - Release media device. Calls kref_put().
>> + *
>> + * @mdev - struct media_device pointer
>> + *
>> + * This interface should be called to put Media Device Instance kref.
>> + */
>> +void media_device_put(struct media_device *mdev);
>> +#else
>> +static inline struct media_device *media_device_usb_allocate(
>> +			struct usb_device *udev, char *driver_name)
>> +			{ return NULL; }
>> +static inline struct media_device *media_device_allocate(struct device *dev)
>> +			{ return NULL; }
>> +static inline struct media_device *media_device_get(struct media_device *mdev)
>> +			{ return NULL; }
>> +static inline void media_device_put(struct media_device *mdev) { }
>> +#endif /* CONFIG_MEDIA_CONTROLLER */
>> +#endif
>>
> 
> Regards,
> 
> 	Hans
> 

[PATCH 2/3] media: add media_device_unregister_put() interface

[Shuah Khan]

Add media_device_unregister_put() interface to release reference to a media
device allocated using the Media Device Allocator API. The media device is
unregistered and freed when the last driver that holds the reference to the
media device releases the reference. The media device is unregistered and
freed in the kref put handler.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/media-device.c | 11 +++++++++++
 include/media/media-device.h | 15 +++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 33a9952..b5c279a 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -36,6 +36,7 @@
 #include <media/media-device.h>
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
+#include <media/media-dev-allocator.h>
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 
@@ -818,6 +819,16 @@ void media_device_unregister(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_unregister);
 
+void media_device_unregister_put(struct media_device *mdev)
+{
+	if (mdev == NULL)
+		return;
+
+	dev_dbg(mdev->dev, "%s: mdev %p\n", __func__, mdev);
+	media_device_put(mdev);
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_put);
+
 static void media_device_release_devres(struct device *dev, void *res)
 {
 }
diff --git a/include/media/media-device.h b/include/media/media-device.h
index f743ae2..8bd836e 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -499,6 +499,18 @@ int __must_check __media_device_register(struct media_device *mdev,
 void media_device_unregister(struct media_device *mdev);
 
 /**
+ * media_device_unregister_put() - Unregisters a media device element
+ *
+ * @mdev:	pointer to struct &media_device
+ *
+ * Should be called to unregister media device allocated with Media Device
+ * Allocator API media_device_get() interface.
+ * It is safe to call this function on an unregistered (but initialised)
+ * media device.
+ */
+void media_device_unregister_put(struct media_device *mdev);
+
+/**
  * media_device_register_entity() - registers a media entity inside a
  *	previously registered media device.
  *
@@ -658,6 +670,9 @@ static inline int media_device_register(struct media_device *mdev)
 static inline void media_device_unregister(struct media_device *mdev)
 {
 }
+static inline void media_device_unregister_put(struct media_device *mdev)
+{
+}
 static inline int media_device_register_entity(struct media_device *mdev,
 						struct media_entity *entity)
 {
-- 
2.7.4

Re: [PATCH 2/3] media: add media_device_unregister_put() interface

[Hans Verkuil]

On 05/13/2016 07:09 PM, Shuah Khan wrote:
> Add media_device_unregister_put() interface to release reference to a media
> device allocated using the Media Device Allocator API. The media device is
> unregistered and freed when the last driver that holds the reference to the
> media device releases the reference. The media device is unregistered and
> freed in the kref put handler.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/media-device.c | 11 +++++++++++
>  include/media/media-device.h | 15 +++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 33a9952..b5c279a 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -36,6 +36,7 @@
>  #include <media/media-device.h>
>  #include <media/media-devnode.h>
>  #include <media/media-entity.h>
> +#include <media/media-dev-allocator.h>
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  
> @@ -818,6 +819,16 @@ void media_device_unregister(struct media_device *mdev)
>  }
>  EXPORT_SYMBOL_GPL(media_device_unregister);
>  
> +void media_device_unregister_put(struct media_device *mdev)
> +{
> +	if (mdev == NULL)
> +		return;
> +
> +	dev_dbg(mdev->dev, "%s: mdev %p\n", __func__, mdev);
> +	media_device_put(mdev);
> +}
> +EXPORT_SYMBOL_GPL(media_device_unregister_put);
> +

I don't really see the need for a new unregister_put function. The only thing
it adds compared to media_device_put is the 'if (mdev == NULL)' check.

Is that check needed at all?

I would probably go for an API like this:

<brainstorm mode on>

/* Not sure if there should be a void *priv as the last argument */
int (*mdev_init_fnc)(struct media_device *mdev, struct device *dev);

/* Perhaps a void *priv is needed to pass to mdev_init_fnc?
   The callback is there to initialize the media_device and it's called
   with the lock held.
*/
struct media_device *media_device_allocate(struct device *dev, mdev_init_fnc fnc);

/* Helper function for usb devices */
struct media_device *media_device_usb_allocate(struct usb_device *udev,
					       char *driver_name);

/* counterpart of media_device_allocate, that makes more sense than allocate/put IMHO */
void media_device_release(struct media_device *mdev);

<brainstorm mode off>

Regards,

	Hans

>  static void media_device_release_devres(struct device *dev, void *res)
>  {
>  }
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index f743ae2..8bd836e 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -499,6 +499,18 @@ int __must_check __media_device_register(struct media_device *mdev,
>  void media_device_unregister(struct media_device *mdev);
>  
>  /**
> + * media_device_unregister_put() - Unregisters a media device element
> + *
> + * @mdev:	pointer to struct &media_device
> + *
> + * Should be called to unregister media device allocated with Media Device
> + * Allocator API media_device_get() interface.
> + * It is safe to call this function on an unregistered (but initialised)
> + * media device.
> + */
> +void media_device_unregister_put(struct media_device *mdev);
> +
> +/**
>   * media_device_register_entity() - registers a media entity inside a
>   *	previously registered media device.
>   *
> @@ -658,6 +670,9 @@ static inline int media_device_register(struct media_device *mdev)
>  static inline void media_device_unregister(struct media_device *mdev)
>  {
>  }
> +static inline void media_device_unregister_put(struct media_device *mdev)
> +{
> +}
>  static inline int media_device_register_entity(struct media_device *mdev,
>  						struct media_entity *entity)
>  {
> 

Re: [PATCH 2/3] media: add media_device_unregister_put() interface

[Shuah Khan]

On 05/23/2016 05:39 AM, Hans Verkuil wrote:
> On 05/13/2016 07:09 PM, Shuah Khan wrote:
>> Add media_device_unregister_put() interface to release reference to a media
>> device allocated using the Media Device Allocator API. The media device is
>> unregistered and freed when the last driver that holds the reference to the
>> media device releases the reference. The media device is unregistered and
>> freed in the kref put handler.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/media-device.c | 11 +++++++++++
>>  include/media/media-device.h | 15 +++++++++++++++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 33a9952..b5c279a 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -36,6 +36,7 @@
>>  #include <media/media-device.h>
>>  #include <media/media-devnode.h>
>>  #include <media/media-entity.h>
>> +#include <media/media-dev-allocator.h>
>>  
>>  #ifdef CONFIG_MEDIA_CONTROLLER
>>  
>> @@ -818,6 +819,16 @@ void media_device_unregister(struct media_device *mdev)
>>  }
>>  EXPORT_SYMBOL_GPL(media_device_unregister);
>>  
>> +void media_device_unregister_put(struct media_device *mdev)
>> +{
>> +	if (mdev == NULL)
>> +		return;
>> +
>> +	dev_dbg(mdev->dev, "%s: mdev %p\n", __func__, mdev);
>> +	media_device_put(mdev);
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_unregister_put);
>> +
> 
> I don't really see the need for a new unregister_put function. The only thing
> it adds compared to media_device_put is the 'if (mdev == NULL)' check.
> 
> Is that check needed at all?

Yeah. My reasoning for keeping this for symmetry in drivers
with _register() and _unregister(). However, drivers already
required to call media_device_put() in _register error legs.

> 
> I would probably go for an API like this:
> 
> <brainstorm mode on>
> 
> /* Not sure if there should be a void *priv as the last argument */
> int (*mdev_init_fnc)(struct media_device *mdev, struct device *dev);
> 
> /* Perhaps a void *priv is needed to pass to mdev_init_fnc?
>    The callback is there to initialize the media_device and it's called
>    with the lock held.
> */
> struct media_device *media_device_allocate(struct device *dev, mdev_init_fnc fnc);
> 
> /* Helper function for usb devices */
> struct media_device *media_device_usb_allocate(struct usb_device *udev,
> 					       char *driver_name);
> 
> /* counterpart of media_device_allocate, that makes more sense than allocate/put IMHO */
> void media_device_release(struct media_device *mdev);

We already have a media_device_release() that gets used as devnode->release
back. I will use a different name, media_device_usb_free() or something
similar.

> 
> <brainstorm mode off>
> 
> Regards,
> 
> 	Hans
> 
>>  static void media_device_release_devres(struct device *dev, void *res)
>>  {
>>  }
>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>> index f743ae2..8bd836e 100644
>> --- a/include/media/media-device.h
>> +++ b/include/media/media-device.h
>> @@ -499,6 +499,18 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  void media_device_unregister(struct media_device *mdev);
>>  
>>  /**
>> + * media_device_unregister_put() - Unregisters a media device element
>> + *
>> + * @mdev:	pointer to struct &media_device
>> + *
>> + * Should be called to unregister media device allocated with Media Device
>> + * Allocator API media_device_get() interface.
>> + * It is safe to call this function on an unregistered (but initialised)
>> + * media device.
>> + */
>> +void media_device_unregister_put(struct media_device *mdev);
>> +
>> +/**
>>   * media_device_register_entity() - registers a media entity inside a
>>   *	previously registered media device.
>>   *
>> @@ -658,6 +670,9 @@ static inline int media_device_register(struct media_device *mdev)
>>  static inline void media_device_unregister(struct media_device *mdev)
>>  {
>>  }
>> +static inline void media_device_unregister_put(struct media_device *mdev)
>> +{
>> +}
>>  static inline int media_device_register_entity(struct media_device *mdev,
>>  						struct media_entity *entity)
>>  {
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 3/3] media: change au0828 to use Media Device Allocator API

[Shuah Khan]

Change au0828 to use Media Device Allocator API to allocate media device
with the parent usb struct device as the key, so it can be shared with the
snd usb audio driver.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/usb/au0828/au0828-core.c | 12 ++++--------
 drivers/media/usb/au0828/au0828.h      |  1 +
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index bf53553..3f7a521 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -157,9 +157,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
 	dev->media_dev->enable_source = NULL;
 	dev->media_dev->disable_source = NULL;
 
-	media_device_unregister(dev->media_dev);
-	media_device_cleanup(dev->media_dev);
-	kfree(dev->media_dev);
+	media_device_unregister_put(dev->media_dev);
 	dev->media_dev = NULL;
 #endif
 }
@@ -212,14 +210,10 @@ static int au0828_media_device_init(struct au0828_dev *dev,
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_device *mdev;
 
-	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
+	mdev = media_device_usb_allocate(udev, KBUILD_MODNAME);
 	if (!mdev)
 		return -ENOMEM;
 
-	/* check if media device is already initialized */
-	if (!mdev->dev)
-		media_device_usb_init(mdev, udev, udev->product);
-
 	dev->media_dev = mdev;
 #endif
 	return 0;
@@ -487,6 +481,8 @@ static int au0828_media_device_register(struct au0828_dev *dev,
 		/* register media device */
 		ret = media_device_register(dev->media_dev);
 		if (ret) {
+			media_device_put(dev->media_dev);
+			dev->media_dev = NULL;
 			dev_err(&udev->dev,
 				"Media Device Register Error: %d\n", ret);
 			return ret;
diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h
index dd7b378..4bf1b0c 100644
--- a/drivers/media/usb/au0828/au0828.h
+++ b/drivers/media/usb/au0828/au0828.h
@@ -35,6 +35,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
 #include <media/media-device.h>
+#include <media/media-dev-allocator.h>
 
 /* DVB */
 #include "demux.h"
-- 
2.7.4