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

From: Takashi Iwai
Date: Sat Jan 16 2016 - 04:08:41 EST


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@xxxxxxxxxxxxxxx>
> >> ---
> >> 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