Re: [PATCH v2 20/22] media: au0828 add enable, disable source handlers

Em Wed, 03 Feb 2016 21:03:52 -0700
Shuah Khan [off-list ref] escreveu:

This is wrong. There are devices without tuner (capture devices) and
without analog decoder (pure DVB devices).

In the case of pure DVB devices (e. g. no dev->decoder), it should
just enable the DVB path.

In the case of devices without tuner, it should use the same logic
needed to handle the S-Video/Composite connector inputs.

Btw, I'm not seeing how this logic would do the right thing if the user
selects either S-Video or Composite connectors.

+
+	/*
+	 * For Audio and V4L2 entity, find the link to which decoder
+	 * is the sink. Look for an active link between decoder and
+	 * tuner, if one exists, nothing to do. If not, look for any
+	 * active links between tuner and any other entity. If one
+	 * exists, tuner is busy. If tuner is free, setup link and
+	 * start pipeline from source (tuner).
+	 * For DVB FE entity, the source for the link is the tuner.
+	 * Check if tuner is available and setup link and start
+	 * pipeline.
+	*/
+	if (entity->function != MEDIA_ENT_F_DTV_DEMOD)
+		sink = dev->decoder;
+	else
+		sink = entity;
+
+	/* Is an active link between sink and tuner */
+	if (dev->active_link) {
+		if (dev->active_link->sink->entity == sink &&
+		    dev->active_link->source->entity == dev->tuner) {
+			ret = 0;
+			goto end;
+		} else {
+			ret = -EBUSY;
+			goto end;
+		}
+	}
+
+	list_for_each_entry(link, &sink->links, list) {
+		/* Check sink, and source */
+		if (link->sink->entity == sink &&
+		    link->source->entity == dev->tuner) {
+			found_link = link;
+			break;
+		}
+	}
+
+	if (!found_link) {
+		ret = -ENODEV;
+		goto end;
+	}
+
+	/* activate link between source and sink and start pipeline */
+	source = found_link->source->entity;
+	ret = __media_entity_setup_link(found_link, MEDIA_LNK_FL_ENABLED);
+	if (ret) {
+		pr_err(
+			"Activate tuner link %s->%s. Error %d\n",
+			source->name, sink->name, ret);
+		goto end;
+	}
+
+	ret = __media_entity_pipeline_start(entity, pipe);
+	if (ret) {
+		pr_err("Start Pipeline: %s->%s Error %d\n",
+			source->name, entity->name, ret);
+		ret = __media_entity_setup_link(found_link, 0);
+		pr_err("Deactive link Error %d\n", ret);
+		goto end;
+	}

Hmm... isn't it to early to activate the pipeline here? My original
guess is that, on the analog side, this should happen only at the stream
on code. Wouldn't this break apps like mythTV?

+	/*
+	 * save active link and active link owner to avoid audio
+	 * deactivating video owned link from disable_source and
+	 * vice versa
+	*/
+	dev->active_link = found_link;
+	dev->active_link_owner = entity;
+end:
+	mutex_unlock(&mdev->graph_mutex);
+	pr_debug("au0828_enable_source() end %s %d %d\n",
+		entity->name, entity->function, ret);
+	return ret;
+#endif
+	return 0;
+}
+
+static void au0828_disable_source(struct media_entity *entity)
+{
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_entity *sink;
+	int ret = 0;
+	struct media_device *mdev = entity->graph_obj.mdev;
+	struct au0828_dev *dev;
+
+	if (!mdev)
+		return;
+
+	mutex_lock(&mdev->graph_mutex);
+	dev = mdev->source_priv;
+	if (!dev->tuner || !dev->decoder || !dev->active_link) {
+		ret = -ENODEV;
+		goto end;
+	}

Same note as before.

+
+	if (entity->function != MEDIA_ENT_F_DTV_DEMOD)
+		sink = dev->decoder;
+	else
+		sink = entity;
+
+	/* link is active - stop pipeline from source (tuner) */
+	if (dev->active_link && dev->active_link->sink->entity == sink &&
+	    dev->active_link->source->entity == dev->tuner) {
+		/*
+		 * prevent video from deactivating link when audio
+		 * has active pipeline
+		*/
+		if (dev->active_link_owner != entity)
+			goto end;
+		__media_entity_pipeline_stop(entity);
+		ret = __media_entity_setup_link(dev->active_link, 0);
+		if (ret)
+			pr_err("Deactive link Error %d\n", ret);
+		dev->active_link = NULL;
+		dev->active_link_owner = NULL;
+	}

Most code here looks like the one at au0828_enable_source(). Wouldn't
be simpler to merge those code and add a "bool enable" to the function
parameters?