Re: [RFC/PATCH] media: Add video bus switch

[Pavel Machek]

On Thu 2016-12-22 15:32:44, Sebastian Reichel wrote:
> Hi Pavel,
> 
> On Thu, Dec 22, 2016 at 02:39:38PM +0100, Pavel Machek wrote:
> > N900 contains front and back camera, with a switch between the
> > two. This adds support for the swich component.
> > 
> > Signed-off-by: Sebastian Reichel <sre@xxxxxxxxxx>
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@xxxxxxxxx>
> > Signed-off-by: Pavel Machek <pavel@xxxxxx>
> > 
> > --
> > 
> > I see this needs dts documentation, anything else than needs to be
> > done?
> 
> Yes. This driver takes care of the switch gpio, but the cameras also
> use different bus settings. Currently omap3isp gets the bus-settings
> from the link connected to the CCP2 port in DT at probe time (*).
> 
> So there are two general problems:
> 
> 1. Settings must be applied before the streaming starts instead of
> at probe time, since the settings may change (based one the selected
> camera). That should be fairly easy to implement by just moving the
> code to the s_stream callback as far as I can see.
> 
> 2. omap3isp should try to get the bus settings from using a callback
> in the connected driver instead of loading it from DT. Then the
> video-bus-switch can load the bus-settings from its downstream links
> in DT and propagate the correct ones to omap3isp based on the
> selected port. The DT loading part should actually remain in omap3isp
> as fallback, in case it does not find a callback in the connected driver.
> That way everything is backward compatible and the DT variant is
> nice for 1-on-1 scenarios.

So... did I understood it correctly? (Needs some work to be done...)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 45c69ed..1f44da1 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -702,6 +704,33 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe,
 
 	entity = &pipe->output->video.entity;
 	while (1) {
+		struct v4l2_of_endpoint vep;
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_entity_remote_pad(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+	       	printk("Entity = %p\n", entity);
+		ret = v4l2_subdev_call(subdev, video, g_endpoint_config, &vep);
+		/* Is there better method than walking a list?
+		   Can I easily get dev and isd pointers here? */
+#if 0
+		if (ret == 0) {
+			printk("success\n");
+			/* notifier->subdevs[notifier->num_subdevs] ... contains isd */
+			isp_endpoint_to_buscfg(dev, vep, isd->bus);
+		}
+#endif
+	}
+
+	entity = &pipe->output->video.entity;
+	while (1) {
 		pad = &entity->pads[0];
 		if (!(pad->flags & MEDIA_PAD_FL_SINK))
 			break;
@@ -2099,27 +2128,8 @@ static void isp_of_parse_node_csi2(struct device *dev,
 	buscfg->bus.csi2.crc = 1;
 }
 
-static int isp_of_parse_node_endpoint(struct device *dev,
-				      struct device_node *node,
-				      struct isp_async_subdev *isd)
+static int isp_endpoint_to_buscfg(struct device *dev, struct v4l2_of_endpoint vep, struct isp_bus_cfg *buscfg)
 {
-	struct isp_bus_cfg *buscfg;
-	struct v4l2_of_endpoint vep;
-	int ret;
-
-	isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
-	if (!isd->bus)
-		return -ENOMEM;
-
-	buscfg = isd->bus;
-
-	ret = v4l2_of_parse_endpoint(node, &vep);
-	if (ret)
-		return ret;
-
-	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
-		vep.base.port);
-
 	switch (vep.base.port) {
 	case ISP_OF_PHY_PARALLEL:
 		buscfg->interface = ISP_INTERFACE_PARALLEL;
@@ -2147,10 +2157,35 @@ static int isp_of_parse_node_endpoint(struct device *dev,
 		break;
 
 	default:
+		return -1;
+	}
+	return 0;
+}
+
+static int isp_of_parse_node_endpoint(struct device *dev,
+				      struct device_node *node,
+				      struct isp_async_subdev *isd)
+{
+	struct isp_bus_cfg *buscfg;
+	struct v4l2_of_endpoint vep;
+	int ret;
+
+	isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
+	if (!isd->bus)
+		return -ENOMEM;
+
+	buscfg = isd->bus;
+
+	ret = v4l2_of_parse_endpoint(node, &vep);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
+		vep.base.port);
+
+	if (isp_endpoint_to_buscfg(dev, vep, buscfg))
 		dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
 			 vep.base.port);
-		break;
-	}
 
 	return 0;
 }
@@ -2262,6 +2297,10 @@ static int isp_of_parse_nodes(struct device *dev,
 	}
 
 	return notifier->num_subdevs;
+
+error:
+	of_node_put(node);
+	return -EINVAL;
 }
 
 static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
diff --git a/drivers/media/platform/video-bus-switch.c b/drivers/media/platform/video-bus-switch.c
index 1a5d944..3a2d442 100644
--- a/drivers/media/platform/video-bus-switch.c
+++ b/drivers/media/platform/video-bus-switch.c
@@ -247,12 +247,21 @@ static int vbs_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct v4l2_subdev *subdev = vbs_get_remote_subdev(sd);
 
+	/* FIXME: we need to set the GPIO here */
+
 	if (IS_ERR(subdev))
 		return PTR_ERR(subdev);
 
 	return v4l2_subdev_call(subdev, video, s_stream, enable);
 }
 
+static int vbs_g_endpoint_config(struct v4l2_subdev *sd, struct isp_bus_cfg *cfg)
+{
+	printk("vbs_g_endpoint_config...\n");
+	return 0;
+}
+
+
 static const struct v4l2_subdev_internal_ops vbs_internal_ops = {
 	.registered = &vbs_registered,
 };
@@ -265,6 +274,7 @@ static const struct media_entity_operations vbs_media_ops = {
 /* subdev video operations */
 static const struct v4l2_subdev_video_ops vbs_video_ops = {
 	.s_stream = vbs_s_stream,
+	.g_endpoint_config = vbs_g_endpoint_config,
 };
 
 static const struct v4l2_subdev_ops vbs_ops = {
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index cf778c5..30457b0 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -415,6 +415,8 @@ struct v4l2_subdev_video_ops {
 			     const struct v4l2_mbus_config *cfg);
 	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
 			   unsigned int *size);
+	int (*g_endpoint_config)(struct v4l2_subdev *sd,
+			    struct v4l2_of_endpoint *cfg);
 };
 
 /**





-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature