[alsa-devel] [PATCH 20/31] media: au0828 change to register/unregister entity_notify hook
Mauro Carvalho Chehab
mchehab at osg.samsung.com
Thu Jan 28 17:36:10 CET 2016
Em Wed, 6 Jan 2016 13:27:09 -0700
Shuah Khan <shuahkh at osg.samsung.com> 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 <shuahkh at osg.samsung.com>
> ---
> 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
> };
>
More information about the Alsa-devel
mailing list