Message-ID: <s5h1t9huc45.wl-tiwai@suse.de>
Date:	Sat, 16 Jan 2016 10:06:50 +0100
From:	Takashi Iwai <tiwai@...e.de>
To:	Shuah Khan <shuahkh@....samsung.com>
Cc:	hans.verkuil@...co.com, laurent.pinchart@...asonboard.com,
	clemens@...isch.de, sakari.ailus@...ux.intel.com,
	javier@....samsung.com, mchehab@....samsung.com,
	alsa-devel@...a-project.org, arnd@...db.de,
	ricard.wanderlof@...s.com, labbott@...oraproject.org,
	chehabrafael@...il.com, misterpib@...il.com,
	prabhakar.csengg@...il.com, ricardo.ribalda@...il.com,
	ruchandani.tina@...il.com, takamichiho@...il.com,
	tvboxspy@...il.com, dominic.sacre@....de, crope@....fi,
	julian@...st.de, pierre-louis.bossart@...ux.intel.com,
	corbet@....net, joe@...po.co.uk, johan@...ud.se,
	dan.carpenter@...cle.com, pawel@...iak.com, p.zabel@...gutronix.de,
	perex@...ex.cz, stefanr@...6.in-berlin.de, inki.dae@...sung.com,
	jh1009.sung@...sung.com, k.kozlowski@...sung.com,
	kyungmin.park@...sung.com, m.szyprowski@...sung.com,
	sw0312.kim@...sung.com, elfring@...rs.sourceforge.net,
	linux-api@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-media@...r.kernel.org, linuxbugs@...tgam.net,
	gtmkramer@...all.nl, normalperson@...t.net, daniel@...que.org
Subject: Re: [PATCH v2 26/31] sound/usb: Update ALSA driver to use Managed Media Controller API

On Sat, 16 Jan 2016 01:38:27 +0100,
Shuah Khan wrote:
> 
> On 01/15/2016 06:38 AM, Takashi Iwai wrote:
> > On Thu, 14 Jan 2016 20:52:28 +0100,
> > Shuah Khan wrote:
> >>
> >> Change ALSA driver to use Managed Media Managed Controller
> >> API to share tuner with DVB and V4L2 drivers that control
> >> AU0828 media device.  Media device is created based on a
> >> newly added field value in the struct snd_usb_audio_quirk.
> >> Using this approach, the media controller API usage can be
> >> added for a specific device. In this patch, Media Controller
> >> API is enabled for AU0828 hw. snd_usb_create_quirk() will
> >> check this new field, if set will create a media device using
> >> media_device_get_devres() interface.
> >>
> >> media_device_get_devres() will allocate a new media device
> >> devres or return an existing one, if it finds one.
> >>
> >> During probe, media usb driver could have created the media
> >> device devres. It will then initialze (if necessary) and
> >> register the media device if it isn't already initialized
> >> and registered. Media device unregister is done from
> >> usb_audio_disconnect().
> >>
> >> During probe, media usb driver could have created the
> >> media device devres. It will then register the media
> >> device if it isn't already registered. Media device
> >> unregister is done from usb_audio_disconnect().
> >>
> >> New structure media_ctl is added to group the new
> >> fields to support media entity and links. This new
> >> structure is added to struct snd_usb_substream.
> >>
> >> A new entity_notify hook and a new ALSA capture media
> >> entity are registered from snd_usb_pcm_open() after
> >> setting up hardware information for the PCM device.
> >>
> >> When a new entity is registered, Media Controller API
> >> interface media_device_register_entity() invokes all
> >> registered entity_notify hooks for the media device.
> >> ALSA entity_notify hook parses all the entity list to
> >> find a link from decoder it ALSA entity. This indicates
> >> that the bridge driver created a link from decoder to
> >> ALSA capture entity.
> >>
> >> ALSA will attempt to enable the tuner to link the tuner
> >> to the decoder calling enable_source handler if one is
> >> provided by the bridge driver prior to starting Media
> >> pipeline from snd_usb_hw_params(). If enable_source returns
> >> with tuner busy condition, then snd_usb_hw_params() will fail
> >> with -EBUSY. Media pipeline is stopped from snd_usb_hw_free().
> >>
> >> Signed-off-by: Shuah Khan <shuahkh@....samsung.com>
> >> ---
> >> Changes since v1: Addressed Takasi Iwai's comments on v1
> >> - Move config defines to Kconfig and add logic
> >>   to Makefile to conditionally compile media.c
> >> - Removed extra includes from media.c
> >> - Added snd_usb_autosuspend() in error leg
> >> - Removed debug related code that was missed in v1
> >>
> >>  sound/usb/Kconfig        |   7 ++
> >>  sound/usb/Makefile       |   4 +
> >>  sound/usb/card.c         |   7 ++
> >>  sound/usb/card.h         |   1 +
> >>  sound/usb/media.c        | 190 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  sound/usb/media.h        |  56 ++++++++++++++
> >>  sound/usb/pcm.c          |  28 +++++--
> >>  sound/usb/quirks-table.h |   1 +
> >>  sound/usb/quirks.c       |   5 ++
> >>  sound/usb/stream.c       |   2 +
> >>  sound/usb/usbaudio.h     |   1 +
> >>  11 files changed, 297 insertions(+), 5 deletions(-)
> >>  create mode 100644 sound/usb/media.c
> >>  create mode 100644 sound/usb/media.h
> >>
> >> diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig
> >> index a452ad7..8d5cdab 100644
> >> --- a/sound/usb/Kconfig
> >> +++ b/sound/usb/Kconfig
> >> @@ -15,6 +15,7 @@ config SND_USB_AUDIO
> >>  	select SND_RAWMIDI
> >>  	select SND_PCM
> >>  	select BITREVERSE
> >> +	select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER && (MEDIA_SUPPORT || MEDIA_SUPPORT_MODULE)
> > 
> > This looks wrong as a Kconfig syntax.  "... if MEDIA_CONTROLLER"
> > should work, I suppose?
> 
> The conditional select syntax is used in several Kconfig
> files. You are right about (MEDIA_SUPPORT || MEDIA_SUPPORT_MODULE)
> Adding checks for that is unnecessary.

Well, my point was that check should be like:
   select SND_USB_AUDIO_XXX if MEDIA_CONTROLLER && MEDIA_SUPPORT

as there is no MEDIA_SUPPORT_MODULE in Kconfig.  It's
MEDIA_SUPPORT=m in Kconfig syntax.  In C code, it's
CONFIG_MEDIA_SUPPORT_MODULE, though.

> > Above all, can MEDIA_CONTROLLER be tristate?  It's currently a bool.
> > If it's a tristate, it causes a problem wrt dependency.  Imagine
> > USB-audio is built-in while MC is a module.
> 
> I don't think MEDIA_CONTROLLER will evebr tristate. I have this
> logic to the following and it works with and without MEDIA_CONTROLLER
> 
> diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig
> index a452ad7..8ccbb35 100644
> --- a/sound/usb/Kconfig
> +++ b/sound/usb/Kconfig
> @@ -15,6 +15,7 @@ config SND_USB_AUDIO
>         select SND_RAWMIDI
>         select SND_PCM
>         select BITREVERSE
> +       select SND_USB_AUDIO_USE_MEDIA_CONTROLLER       if MEDIA_CONTROLLER
>         help
>           Say Y here to include support for USB audio and USB MIDI
>           devices.
> @@ -22,6 +23,11 @@ config SND_USB_AUDIO
>           To compile this driver as a module, choose M here: the module
>           will be called snd-usb-audio.
> 
> +config SND_USB_AUDIO_USE_MEDIA_CONTROLLER
> +       bool "USB Audio/MIDI driver with Media Controller Support"
> +       depends on MEDIA_CONTROLLER
> +       default y
> +
> 
> > 
> >>  	help
> >>  	  Say Y here to include support for USB audio and USB MIDI
> >>  	  devices.
> >> @@ -22,6 +23,12 @@ config SND_USB_AUDIO
> >>  	  To compile this driver as a module, choose M here: the module
> >>  	  will be called snd-usb-audio.
> >>  
> >> +config SND_USB_AUDIO_USE_MEDIA_CONTROLLER
> >> +	bool "USB Audio/MIDI driver with Media Controller Support"
> >> +	depends on SND_USB_AUDIO && MEDIA_CONTROLLER
> >> +	depends on MEDIA_SUPPORT || MEDIA_SUPPORT_MODULE
> 
> See above. I got rid of depends on for SND_USB_AUDIO

There is a revere-select, so this menu itself doesn't play any role.

If you want this item selectable, drop select from SND_USB_AUDIO but
just have proper dependencies for this item:

config SND_USB_AUDIO_USE_MEDIA_CONTROLLER
	bool "USB Audio/MIDI driver with Media Controller Support"
	depends on SND_USB_AUDIO
	depends on MEDIA_CONTROLLER
	default y

(Whether default y or not is a remaining question: usually we keep it 
 empty (i.e. no), but I put it here since you had it.)

If you want this auto-selected unconditionally, drop the prompt and
superfluous dependency (and don't set default y):

config SND_USB_AUDIO
	....
	select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER
	....

config SND_USB_AUDIO_USE_MEDIA_CONTROLLER
	bool


Takashi

Powered by blists - more mailing lists