Em Wed, 6 Jan 2016 13:27:09 -0700 Shuah Khan <[email protected]> escreveu:
> au0828 registers entity_notify hook to create media graph for > the device. This handler runs whenvere a new entity gets added typo: whenever. > to the media device. It creates necessary links from video, vbi, > and ALSA entities to decoder and links tuner and decoder entities. > As this handler runs as entities get added, it has to maintain > state on the links it already created. New fields are added to > au0828_dev to keep this state information. entity_notify gets > unregistered before media_device unregister. Bty, please avoid long paragraphs at the patch description, and please try to be clearer on your patch descriptions... That makes boring to read everything. :-; > > Signed-off-by: Shuah Khan <[email protected]> > --- > drivers/media/usb/au0828/au0828-core.c | 104 > +++++++++++++++++++++++---------- > drivers/media/usb/au0828/au0828.h | 6 ++ > 2 files changed, 78 insertions(+), 32 deletions(-) > > diff --git a/drivers/media/usb/au0828/au0828-core.c > b/drivers/media/usb/au0828/au0828-core.c > index 6ef177c..a381660 100644 > --- a/drivers/media/usb/au0828/au0828-core.c > +++ b/drivers/media/usb/au0828/au0828-core.c > @@ -137,6 +137,8 @@ static void au0828_unregister_media_device(struct > au0828_dev *dev) > #ifdef CONFIG_MEDIA_CONTROLLER > if (dev->media_dev && > media_devnode_is_registered(&dev->media_dev->devnode)) { > + media_device_unregister_entity_notify(dev->media_dev, > + &dev->entity_notify); > media_device_unregister(dev->media_dev); > media_device_cleanup(dev->media_dev); > dev->media_dev = NULL; > @@ -263,11 +265,16 @@ static int au0828_create_media_graph(struct au0828_dev > *dev) > struct media_device *mdev = dev->media_dev; > struct media_entity *entity; > struct media_entity *tuner = NULL, *decoder = NULL; > + struct media_entity *audio_capture = NULL; > int i, ret; > > if (!mdev) > return 0; > > + if (dev->tuner_linked && dev->vdev_linked && dev->vbi_linked && > + dev->audio_capture_linked) > + return 0; > + > media_device_for_each_entity(entity, mdev) { > switch (entity->function) { > case MEDIA_ENT_F_TUNER: > @@ -276,6 +283,9 @@ static int au0828_create_media_graph(struct au0828_dev > *dev) > case MEDIA_ENT_F_ATV_DECODER: > decoder = entity; > break; > + case MEDIA_ENT_F_AUDIO_CAPTURE: > + audio_capture = entity; > + break; > } > } > > @@ -285,60 +295,77 @@ static int au0828_create_media_graph(struct au0828_dev > *dev) > if (!decoder) > return -EINVAL; > > - if (tuner) { > + if (tuner && !dev->tuner_linked) { > + dev->tuner = tuner; > ret = media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, > decoder, 0, > MEDIA_LNK_FL_ENABLED); > if (ret) > return ret; > + dev->tuner_linked = 1; > } > - if (dev->vdev.entity.graph_obj.mdev) { > + if (dev->vdev.entity.graph_obj.mdev && !dev->vdev_linked) { > ret = media_create_pad_link(decoder, AU8522_PAD_VID_OUT, > &dev->vdev.entity, 0, > MEDIA_LNK_FL_ENABLED); > if (ret) > return ret; > + dev->vdev_linked = 1; > } > - if (dev->vbi_dev.entity.graph_obj.mdev) { > + if (dev->vbi_dev.entity.graph_obj.mdev && !dev->vbi_linked) { > ret = media_create_pad_link(decoder, AU8522_PAD_VBI_OUT, > &dev->vbi_dev.entity, 0, > MEDIA_LNK_FL_ENABLED); > if (ret) > return ret; > - } > - > - for (i = 0; i < AU0828_MAX_INPUT; i++) { > - struct media_entity *ent = &dev->input_ent[i]; > + dev->vbi_linked = 1; > > - if (!ent->graph_obj.mdev) > - continue; > + /* > + * Input entities are registered before vbi entity, > + * create graph nodes for them after vbi is created > + */ > + for (i = 0; i < AU0828_MAX_INPUT; i++) { > + struct media_entity *ent = &dev->input_ent[i]; > > - if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED) > - break; > + if (!ent->graph_obj.mdev) > + continue; > > - switch (AUVI_INPUT(i).type) { > - case AU0828_VMUX_CABLE: > - case AU0828_VMUX_TELEVISION: > - case AU0828_VMUX_DVB: > - if (!tuner) > + if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED) > break; > > - ret = media_create_pad_link(ent, 0, tuner, > - TUNER_PAD_RF_INPUT, > - MEDIA_LNK_FL_ENABLED); > - if (ret) > - return ret; > - break; > - case AU0828_VMUX_COMPOSITE: > - case AU0828_VMUX_SVIDEO: > - default: /* AU0828_VMUX_DEBUG */ > - /* FIXME: fix the decoder PAD */ > - ret = media_create_pad_link(ent, 0, decoder, 0, 0); > - if (ret) > - return ret; > - break; > + switch (AUVI_INPUT(i).type) { > + case AU0828_VMUX_CABLE: > + case AU0828_VMUX_TELEVISION: > + case AU0828_VMUX_DVB: > + if (!tuner) > + break; > + > + ret = media_create_pad_link(ent, 0, tuner, > + TUNER_PAD_RF_INPUT, > + MEDIA_LNK_FL_ENABLED); > + if (ret) > + return ret; > + break; > + case AU0828_VMUX_COMPOSITE: > + case AU0828_VMUX_SVIDEO: > + default: /* AU0828_VMUX_DEBUG */ > + /* FIXME: fix the decoder PAD */ > + ret = media_create_pad_link(ent, 0, decoder, > + 0, 0); > + if (ret) > + return ret; > + break; > + } > } > } > + if (audio_capture && !dev->audio_capture_linked) { > + ret = media_create_pad_link(decoder, AU8522_PAD_AUDIO_OUT, > + audio_capture, 0, > + MEDIA_LNK_FL_ENABLED); > + if (ret) > + return ret; > + dev->audio_capture_linked = 1; > + } > #endif > return 0; > } > @@ -349,8 +376,10 @@ static int au0828_media_device_register(struct > au0828_dev *dev, > #ifdef CONFIG_MEDIA_CONTROLLER > int ret; > > - if (dev->media_dev && > - !media_devnode_is_registered(&dev->media_dev->devnode)) { > + if (!dev->media_dev) > + return 0; > + > + if (!media_devnode_is_registered(&dev->media_dev->devnode)) { > > /* register media device */ > ret = media_device_register(dev->media_dev); > @@ -360,6 +389,17 @@ static int au0828_media_device_register(struct > au0828_dev *dev, > return ret; > } > } > + /* register entity_notify callback */ > + dev->entity_notify.notify_data = (void *) dev; > + dev->entity_notify.notify = (void *) au0828_create_media_graph; > + ret = media_device_register_entity_notify(dev->media_dev, > + &dev->entity_notify); > + if (ret) { > + dev_err(&udev->dev, > + "Media Device register entity_notify Error: %d\n", > + ret); > + return ret; > + } > #endif > return 0; > } > diff --git a/drivers/media/usb/au0828/au0828.h > b/drivers/media/usb/au0828/au0828.h > index 8276072..cfb6d58 100644 > --- a/drivers/media/usb/au0828/au0828.h > +++ b/drivers/media/usb/au0828/au0828.h > @@ -283,6 +283,12 @@ struct au0828_dev { > struct media_entity *decoder; > struct media_entity input_ent[AU0828_MAX_INPUT]; > struct media_pad input_pad[AU0828_MAX_INPUT]; > + struct media_entity_notify entity_notify; > + struct media_entity *tuner; > + bool tuner_linked; > + bool vdev_linked; > + bool vbi_linked; > + bool audio_capture_linked; Hmm... now I understood why you did the changes on patch 13/31. I see what you're doing, but not sure if this is a good idea to have one bool for each possible device. On au0828, the topology is actually simpler than on other devices, as it currently supports a very few set of I2C devices, but on other drivers, things can be messier. See, for example, two graphs for em28xx-based devices: https://mchehab.fedorapeople.org/mc-next-gen/wintv_usb2.png https://mchehab.fedorapeople.org/mc-next-gen/hvr_950.png On the first graph, the tuner is not connected directly to the analog demod, but, instead, to two other elements: - tda9887 - for video - msp3400 - for audio IMHO, the best way to handle graph setup is that each driver should handle the links that belong only to them synchronously, after creating/registering all the entities. So, only the links between two drivers would be asynchronously created. So, In the case of au0828: - au0828 core will create the connector entities; - I2C drivers will create their own entities; - DVB core will create the DVB entities/interfaces; - V4L core will create V4L interfaces and I/O entities; - au0828 V4L driver will create all V4L links, after ensuring that the needed I2C drivers were bound; - snd-usb-audio will create all ALSA-specific entities links; The V4L->ALSA links will either be created by au0828-core, via the notification handler. With that in mind, I don't see any need to touch at au0828_create_media_graph(). It will need an extra function to handle the notification when ALSA gets registered (or when the entities there are added, whatever works best). Regards, Mauro > #endif > }; >
