Re: [PATCH MC Next Gen v2 2/3] sound/usb: Create media mixer function and control interface entities
On 11/03/2015 09:23 AM, Takashi Iwai wrote: > On Tue, 03 Nov 2015 17:06:45 +0100, > Shuah Khan wrote: >> >> On 10/25/2015 03:37 PM, Shuah Khan wrote: >>> 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 > --- > 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. >> >> Sorry I misspoke. The reason for this cast is ctl_intf_media_devnode >> is void to avoid including media.h and other media files in usbaudio.h > > You can declare the struct without definition in each header file. > So just declare it and use it in usbaudio.h like: > > struct media_intf_devnode; > > struct snd_usb_audio { > > struct media_intf_devnode *ctl_intf_media_devnode; > > > And even if you're using a void pointer there instead of the explicit > struct pointer, the cast is superfluous. The implicit cast between a > void pointer and any other pointer is valid in plain C. > Yes. cast is definitely not necessary. I will drop the cast and leave the rest alone. thanks, -- Shuah -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shua...@osg.samsung.com | (970) 217-8978 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH MC Next Gen v2 2/3] sound/usb: Create media mixer function and control interface entities
On Tue, 03 Nov 2015 17:06:45 +0100, Shuah Khan wrote: > > On 10/25/2015 03:37 PM, Shuah Khan wrote: > > 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 > >>> --- > >>> 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. > > Sorry I misspoke. The reason for this cast is ctl_intf_media_devnode > is void to avoid including media.h and other media files in usbaudio.h You can declare the struct without definition in each header file. So just declare it and use it in usbaudio.h like: struct media_intf_devnode; struct snd_usb_audio { struct media_intf_devnode *ctl_intf_media_devnode; And even if you're using a void pointer there instead of the explicit struct pointer, the cast is superfluous. The implicit cast between a void pointer and any other pointer is valid in plain C. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH MC Next Gen v2 2/3] sound/usb: Create media mixer function and control interface entities
On 10/25/2015 03:37 PM, Shuah Khan wrote: > 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 >>> --- >>> 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. Sorry I misspoke. The reason for this cast is ctl_intf_media_devnode is void to avoid including media.h and other media files in usbaudio.h The same approach I took for card.h when adding media_ctl to struct snd_usb_substream Does this sound reasonable or would you rather see these to be their respective struct pointers which would require including media.h in these headers? thanks, -- Shuah -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shua...@osg.samsung.com | (970) 217-8978 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH MC Next Gen v2 2/3] sound/usb: Create media mixer function and control interface entities
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 --- 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) { +
Re: [PATCH MC Next Gen v2 2/3] sound/usb: Create media mixer function and control interface entities
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 > --- > 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? > + 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; > + } Is chip->ctl_intf_media_devnode assigned anywhere at all? 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? > + > + 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? > + 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); >