Re: [PATCH v2 26/31] sound/usb: Update ALSA driver to use Managed Media Controller API

[Takashi Iwai <tiwai@suse.de>]

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@osg.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