[alsa-devel] [PATCH MC Next Gen v2 2/3] sound/usb: Create media mixer function and control interface entities
Shuah Khan
shuahkh at osg.samsung.com
Sun Oct 25 22:37:34 CET 2015
On 10/22/2015 01:16 AM, Takashi Iwai wrote:
> On Wed, 21 Oct 2015 01:25:15 +0200,
> Shuah Khan wrote:
>>
>> Add support for creating MEDIA_ENT_F_AUDIO_MIXER entity for
>> each mixer and a MEDIA_INTF_T_ALSA_CONTROL control interface
>> entity that links to mixer entities. MEDIA_INTF_T_ALSA_CONTROL
>> entity corresponds to the control device for the card.
>>
>> Signed-off-by: Shuah Khan <shuahkh at osg.samsung.com>
>> ---
>> sound/usb/card.c | 5 +++
>> sound/usb/media.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> sound/usb/media.h | 20 ++++++++++++
>> sound/usb/mixer.h | 1 +
>> sound/usb/usbaudio.h | 1 +
>> 5 files changed, 116 insertions(+)
>>
>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>> index 469d2bf..d004cb4 100644
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
>> @@ -560,6 +560,9 @@ static int usb_audio_probe(struct usb_interface *intf,
>> if (err < 0)
>> goto __error;
>>
>> + /* Create media entities for mixer and control dev */
>> + media_mixer_init(chip);
>> +
>> usb_chip[chip->index] = chip;
>> chip->num_interfaces++;
>> chip->probing = 0;
>> @@ -616,6 +619,8 @@ static void usb_audio_disconnect(struct usb_interface *intf)
>> list_for_each(p, &chip->midi_list) {
>> snd_usbmidi_disconnect(p);
>> }
>> + /* delete mixer media resources */
>> + media_mixer_delete(chip);
>> /* release mixer resources */
>> list_for_each_entry(mixer, &chip->mixer_list, list) {
>> snd_usb_mixer_disconnect(mixer);
>> diff --git a/sound/usb/media.c b/sound/usb/media.c
>> index 0cbfee6..a26ea8b 100644
>> --- a/sound/usb/media.c
>> +++ b/sound/usb/media.c
>> @@ -199,4 +199,93 @@ void media_stop_pipeline(struct snd_usb_substream *subs)
>> if (mctl)
>> media_disable_source(mctl);
>> }
>> +
>> +int media_mixer_init(struct snd_usb_audio *chip)
>> +{
>> + struct device *ctl_dev = &chip->card->ctl_dev;
>> + struct media_intf_devnode *ctl_intf;
>> + struct usb_mixer_interface *mixer;
>> + struct media_device *mdev;
>> + struct media_mixer_ctl *mctl;
>> + u32 intf_type = MEDIA_INTF_T_ALSA_CONTROL;
>> + int ret;
>> +
>> + mdev = media_device_find_devres(&chip->dev->dev);
>> + if (!mdev)
>> + return -ENODEV;
>> +
>> + ctl_intf = (struct media_intf_devnode *) chip->ctl_intf_media_devnode;
>
> Why do we need cast? Can't chip->ctl_intf_media_devnode itself be
> struct media_intf_devndoe pointer?
Yeah. There is no need to cast here. I will fix it.
>
>
>> + if (!ctl_intf) {
>> + ctl_intf = (void *) media_devnode_create(mdev,
>> + intf_type, 0,
>> + MAJOR(ctl_dev->devt),
>> + MINOR(ctl_dev->devt));
>> + if (!ctl_intf)
>> + return -ENOMEM;
chip->ctl_intf_media_devnode = ctl_intf;
>> + }
>
> Is chip->ctl_intf_media_devnode assigned anywhere at all?
Thanks. Great find. I missed that one. It should be assigned
as above in media_devnode_create() success path.
>
> Not directly related with this patchset: the above uses
> media_devnode_create(), which I assume it's a device-file
> (major:minor) specific. What if sound hardware with multiple codecs
> and so on in general?
Yes. It is for the control device. I looked at the multiple
codecs case briefly, but it requires more thought. Maybe
something I could bring up at the Media Workshop today if
time permits.
>
>
>> +
>> + list_for_each_entry(mixer, &chip->mixer_list, list) {
>> +
>> + if (mixer->media_mixer_ctl)
>> + continue;
>> +
>> + /* allocate media_ctl */
>> + mctl = kzalloc(sizeof(struct media_ctl), GFP_KERNEL);
>> + if (!mctl)
>> + return -ENOMEM;
>> +
>> + mixer->media_mixer_ctl = (void *) mctl;
>> + mctl->media_dev = mdev;
>> +
>> + mctl->media_entity.function = MEDIA_ENT_F_AUDIO_MIXER;
>> + mctl->media_entity.name = chip->card->mixername;
>> + mctl->media_pad[0].flags = MEDIA_PAD_FL_SINK;
>> + mctl->media_pad[1].flags = MEDIA_PAD_FL_SOURCE;
>> + mctl->media_pad[2].flags = MEDIA_PAD_FL_SOURCE;
>> + media_entity_init(&mctl->media_entity, MEDIA_MIXER_PAD_MAX,
>> + mctl->media_pad);
>> + ret = media_device_register_entity(mctl->media_dev,
>> + &mctl->media_entity);
>> + if (ret)
>> + return ret;
>
> Will the last (unfinished) mctl be freed properly in the error path?
Should be I will make sure.
>
>> + mctl->intf_link = media_create_intf_link(&mctl->media_entity,
>> + &ctl_intf->intf,
>> + MEDIA_LNK_FL_ENABLED);
>> + if (!mctl->intf_link) {
>> + media_device_unregister_entity(&mctl->media_entity);
>> + return -ENOMEM;
>
> Ditto.
>
>> + }
>> + mctl->intf_devnode = ctl_intf;
>> + }
>> + return 0;
>> +}
>> +
>> +void media_mixer_delete(struct snd_usb_audio *chip)
>> +{
>> + struct usb_mixer_interface *mixer;
>> + struct media_device *mdev;
>> +
>> + mdev = media_device_find_devres(&chip->dev->dev);
>> + if (!mdev)
>> + return;
>> +
>> + list_for_each_entry(mixer, &chip->mixer_list, list) {
>> + struct media_mixer_ctl *mctl;
>> +
>> + mctl = (struct media_mixer_ctl *) mixer->media_mixer_ctl;
>> + if (!mixer->media_mixer_ctl)
>> + continue;
>> +
>> + media_entity_remove_links(&mctl->media_entity);
>> + media_device_unregister_entity(&mctl->media_entity);
>> + media_entity_cleanup(&mctl->media_entity);
>> + mctl->media_dev = NULL;
>> + mctl->intf_devnode = NULL;
>> + kfree(mctl);
>
> The previous two NULL are superfluous as we'll free them immediately.
Yes. I will fix that.
Thanks for the review
-- Shuah
>
>
> thanks,
>
> Takashi
>
>
>> + mixer->media_mixer_ctl = NULL;
>> + }
>> + media_devnode_remove(chip->ctl_intf_media_devnode);
>> +}
>> +
>> #endif
>> diff --git a/sound/usb/media.h b/sound/usb/media.h
>> index cdcfb80..8268e52 100644
>> --- a/sound/usb/media.h
>> +++ b/sound/usb/media.h
>> @@ -20,6 +20,7 @@
>> #ifdef USE_MEDIA_CONTROLLER
>> #include <media/media-device.h>
>> #include <media/media-entity.h>
>> +#include <sound/asound.h>
>>
>> struct media_ctl {
>> struct media_device *media_dev;
>> @@ -30,6 +31,21 @@ struct media_ctl {
>> struct media_pipeline media_pipe;
>> };
>>
>> +/* One source pad each for SNDRV_PCM_STREAM_CAPTURE and
>> + * SNDRV_PCM_STREAM_PLAYBACK. One for sink pad to link
>> + * to AUDIO Source
>> +*/
>> +#define MEDIA_MIXER_PAD_MAX (SNDRV_PCM_STREAM_LAST + 2)
>> +
>> +struct media_mixer_ctl {
>> + struct media_device *media_dev;
>> + struct media_entity media_entity;
>> + struct media_intf_devnode *intf_devnode;
>> + struct media_link *intf_link;
>> + struct media_pad media_pad[MEDIA_MIXER_PAD_MAX];
>> + struct media_pipeline media_pipe;
>> +};
>> +
>> int media_device_init(struct snd_usb_audio *chip, struct usb_interface *iface);
>> void media_device_delete(struct usb_interface *iface);
>> int media_stream_init(struct snd_usb_substream *subs, struct snd_pcm *pcm,
>> @@ -37,6 +53,8 @@ int media_stream_init(struct snd_usb_substream *subs, struct snd_pcm *pcm,
>> void media_stream_delete(struct snd_usb_substream *subs);
>> int media_start_pipeline(struct snd_usb_substream *subs);
>> void media_stop_pipeline(struct snd_usb_substream *subs);
>> +int media_mixer_init(struct snd_usb_audio *chip);
>> +void media_mixer_delete(struct snd_usb_audio *chip);
>> #else
>> static inline int media_device_init(struct snd_usb_audio *chip,
>> struct usb_interface *iface)
>> @@ -49,5 +67,7 @@ static inline void media_stream_delete(struct snd_usb_substream *subs) { }
>> static inline int media_start_pipeline(struct snd_usb_substream *subs)
>> { return 0; }
>> static inline void media_stop_pipeline(struct snd_usb_substream *subs) { }
>> +static int media_mixer_init(struct snd_usb_audio *chip) { return 0; }
>> +static void media_mixer_delete(struct snd_usb_audio *chip) { }
>> #endif
>> #endif /* __MEDIA_H */
>> diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h
>> index d3268f0..9d62a9e 100644
>> --- a/sound/usb/mixer.h
>> +++ b/sound/usb/mixer.h
>> @@ -22,6 +22,7 @@ struct usb_mixer_interface {
>> struct urb *rc_urb;
>> struct usb_ctrlrequest *rc_setup_packet;
>> u8 rc_buffer[6];
>> + void *media_mixer_ctl;
>> };
>>
>> #define MAX_CHANNELS 16 /* max logical channels */
>> diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
>> index c2dbf1d..b2be7c3 100644
>> --- a/sound/usb/usbaudio.h
>> +++ b/sound/usb/usbaudio.h
>> @@ -59,6 +59,7 @@ struct snd_usb_audio {
>> bool autoclock; /* from the 'autoclock' module param */
>>
>> struct usb_host_interface *ctrl_intf; /* the audio control interface */
>> + void *ctl_intf_media_devnode;
>> };
>>
>> #define usb_audio_err(chip, fmt, args...) \
>> --
>> 2.1.4
>>
--
Shuah Khan
Sr. Linux Kernel Developer
Samsung Research America (Silicon Valley)
shuahkh at osg.samsung.com | (970) 217-8978
More information about the Alsa-devel
mailing list