Em Wed, 2 Mar 2016 15:57:03 -0700
Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> escreveu:
> On 03/02/2016 01:41 PM, Dan Carpenter wrote:
> > On Wed, Mar 02, 2016 at 09:50:31AM -0700, Shuah Khan wrote:
> >> + mctl = kzalloc(sizeof(*mctl), GFP_KERNEL);
> >> + if (!mctl)
> >> + return -ENOMEM;
> >> +
> >> + mctl->media_dev = mdev;
> >> + if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> >> + intf_type = MEDIA_INTF_T_ALSA_PCM_PLAYBACK;
> >> + mctl->media_entity.function = MEDIA_ENT_F_AUDIO_PLAYBACK;
> >> + mctl->media_pad.flags = MEDIA_PAD_FL_SOURCE;
> >> + mixer_pad = 1;
> >> + } else {
> >> + intf_type = MEDIA_INTF_T_ALSA_PCM_CAPTURE;
> >> + mctl->media_entity.function = MEDIA_ENT_F_AUDIO_CAPTURE;
> >> + mctl->media_pad.flags = MEDIA_PAD_FL_SINK;
> >> + mixer_pad = 2;
> >> + }
> >> + mctl->media_entity.name = pcm->name;
> >> + media_entity_pads_init(&mctl->media_entity, 1, &mctl->media_pad);
> >> + ret = media_device_register_entity(mctl->media_dev,
> >> + &mctl->media_entity);
> >> + if (ret)
> >> + goto err1;
> >
> > Could we give this label a meaningful name instead of a number?
> > goto free_mctl;
>
> I do see other places where numbered labels are used.
> Names might help with code readability.
>
> register_entity_fail probably makes more sense as a
> label than free_mctl. In any case, I can address the
> labels in a follow-on patch.
>
> >
> >> +
> >> + mctl->intf_devnode = media_devnode_create(mdev, intf_type, 0,
> >> + MAJOR(pcm_dev->devt),
> >> + MINOR(pcm_dev->devt));
> >> + if (!mctl->intf_devnode) {
> >> + ret = -ENOMEM;
> >> + goto err2;
> >
> > goto unregister_device;
> >
> >> + }
> >> + mctl->intf_link = media_create_intf_link(&mctl->media_entity,
> >> + &mctl->intf_devnode->intf,
> >> + MEDIA_LNK_FL_ENABLED);
> >> + if (!mctl->intf_link) {
> >> + ret = -ENOMEM;
> >> + goto err3;
> >
> > goto delete_devnode;
> >
> >> + }
> >> +
> >> + /* create link between mixer and audio */
> >> + media_device_for_each_entity(entity, mdev) {
> >> + switch (entity->function) {
> >> + case MEDIA_ENT_F_AUDIO_MIXER:
> >> + ret = media_create_pad_link(entity, mixer_pad,
> >> + &mctl->media_entity, 0,
> >> + MEDIA_LNK_FL_ENABLED);
> >> + if (ret)
> >> + goto err4;
> >
> > This is a bit weird because we're inside a loop. Shouldn't we call
> > media_entity_remove_links() or something if this is the second time
> > through the loop?
>
> Links are removed from media_device_unregister_entity()
> which is called in the error path.
>
> >
> > I don't understand this. The kernel has the media_entity_cleanup() stub
> > function which is supposed to do this but it hasn't been implemented
> > yet?
> >
>
> Please see above. Links are removed when entity is
> unregistered. media_entity_cleanup() is a stub. It
> isn't intended for removing links.
Shuah,
Please address the issue that Dan is pointing on a separate
patch.
Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
