Re: [PATCH v2 1/6] media: add media token device resource framework

[Takashi Iwai]

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 <shuahkh@xxxxxxxxxxxxxxx>
> >>> ---
> >>>    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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/