Messages in this thread |  | | Date | Tue, 21 Oct 2014 13:58:59 +0200 | From | Hans Verkuil <> | Subject | Re: [PATCH v2 1/6] media: add media token device resource framework |
| |
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
|  |