On 10/21/2014 01:51 PM, Takashi Iwai wrote:
At Tue, 21 Oct 2014 12:46:03 +0200, Hans Verkuil wrote:
Hi Shuah,
As promised, here is my review for this patch series.
On 10/14/2014 04:58 PM, Shuah Khan wrote:
Add media token device resource framework to allow sharing resources such as tuner, dma, audio etc. across media drivers and non-media sound drivers that control media hardware. The Media token resource is created at the main struct device that is common to all drivers that claim various pieces of the main media device, which allows them to find the resource using the main struct device. As an example, digital, analog, and snd-usb-audio drivers can use the media token resource API using the main struct device for the interface the media device is attached to.
A shared media tokens resource is created using devres framework for drivers to find and lock/unlock. Creating a shared devres helps avoid creating data structure dependencies between drivers. This media token resource contains media token for tuner, and audio. When tuner token is requested, audio token is issued.
Did you mean: 'tuner token is issued' instead of audio token?
I also have the same question as Takashi: why do we have an audio token in the first place? While you are streaming audio over alsa the underlying tuner must be marked as being in use. It's all about the tuner, since that's the resource that is being shared, not about audio as such.
For the remainder of my review I will ignore the audio-related code and concentrate only on the tuner part.
Subsequent token (for tuner and audio) gets from the same task and task in the same tgid succeed. This allows applications that make multiple v4l2 ioctls to work with the first call acquiring the token and applications that create separate threads to handle video and audio functions.
Signed-off-by: Shuah Khan shuahkh@osg.samsung.com
MAINTAINERS | 2 + include/linux/media_tknres.h | 50 +++++++++ lib/Makefile | 2 + lib/media_tknres.c | 237 ++++++++++++++++++++++++++++++++++++++++++
I am still not convinced myself that this should be a generic API. The only reason we need it today is for sharing tuners. Which is almost a purely media thing with USB audio as the single non-media driver that will be affected. Today I see no use case outside of tuners. I would probably want to keep this inside drivers/media.
If this is going to be expanded it can always be moved to lib later.
Well, my argument is that it should be more generic if it were intended to be put in lib. It'd be fine to put into drivers/media, but this code snippet must be a separate module. Otherwise usb-audio would grab the whole media stuff even if not needed at all.
Certainly.
(snip)
I also discovered that you are missing MODULE_AUTHOR, MODULE_DESCRIPTION and above all MODULE_LICENSE. Without the MODULE_LICENSE it won't link this module to the GPL devres_* functions. It took me some time to figure that out.
It was a code in lib, so it cannot be a module at all :)
Well, it depends on CONFIG_MEDIA_SUPPORT which is 'm' in my case, so it compiles as a module :-)
Regards,
Hans