Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932547AbaJUNHh (ORCPT ); Tue, 21 Oct 2014 09:07:37 -0400 Received: from cantor2.suse.de ([195.135.220.15]:49895 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932170AbaJUNHf (ORCPT ); Tue, 21 Oct 2014 09:07:35 -0400 Date: Tue, 21 Oct 2014 15:07:29 +0200 Message-ID: From: Takashi Iwai To: Hans Verkuil Cc: Shuah Khan , m.chehab@samsung.com, akpm@linux-foundation.org, gregkh@linuxfoundation.org, crope@iki.fi, olebowle@gmx.com, dheitmueller@kernellabs.com, sakari.ailus@linux.intel.com, laurent.pinchart@ideasonboard.com, perex@perex.cz, prabhakar.csengg@gmail.com, tim.gardner@canonical.com, linux@eikelenboom.it, linux-media@vger.kernel.org, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/6] media: add media token device resource framework In-Reply-To: <54464A83.7090706@xs4all.nl> References: <5446396B.9010709@xs4all.nl> <54464A83.7090706@xs4all.nl> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Tue, 21 Oct 2014 13:58:59 +0200, Hans Verkuil wrote: > > > > 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 > >>> --- > >>> 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 :-) Ah, I thought it was a boolean, but yes, this can be a module indeed. Then I don't think it's worth to put in lib. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/