media: Driver for Toshiba et8ek8 5MP sensor

PATCH Superseded Pavel Machek 8 participants 97 messages Oct 23, 2016 – Dec 22, 2017 Page 2 of 2

Subsystems (4)

Device tree bindings Media (V4L/DVB) Media controller framework Nokia N900 camera support (ET8EK8 sensor, AD5820 focus)

lore

Share & tools

lore

Sakari Ailus

maintainer

2017-02-03 22:25 UTC #60 lore raw

Re: [PATCH] media: Add video bus switch

In reply to Pavel Machek

Hi Pavel,

Quoted 134 lines · Pavel Machek

Quoted 8 lines · Pavel Machek

N900 contains front and back camera, with a switch between the two. This adds support for the switch component, and it is now possible to select between front and back cameras during runtime.
Signed-off-by Sebastian Reichel <[email protected]>
Signed-off-by Ivaylo Dimitrov <[email protected]>
Signed-off-by Pavel Machek <[email protected]>

Can I get some feedback here? This works for me -- I can use both cameras during one boot -- can I get it applied?

Thanks,
Pavel

Quoted 118 lines

Documentation/devicetree/bindings/media/video-bus-switch.txt new file +63 -0

new file mode 100644
index 0000000..1b9f8e0
@@ -0,0 +1,63 @@
+Video Bus Switch Binding
+========================
+
+This is a binding for a gpio controlled switch for camera interfaces. Such a
+device is used on some embedded devices to connect two cameras to the same
+interface of a image signal processor.
+
+Required properties
+===================
+
+compatible	: must contain "video-bus-switch"
+switch-gpios	: GPIO specifier for the gpio, which can toggle the
+		  selected camera. The GPIO should be configured, so
+		  that a disabled GPIO means, that the first port is
+		  selected.
+
+Required Port nodes
+===================
+
+More documentation on these bindings is available in
+video-interfaces.txt in the same directory.
+
+reg		: The interface:
+		  0 - port for image signal processor
+		  1 - port for first camera sensor
+		  2 - port for second camera sensor
+
+Example
+=======
+
+video-bus-switch {
+	compatible = "video-bus-switch"
+	switch-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			csi_switch_in: endpoint {
+				remote-endpoint = <&csi_isp>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			csi_switch_out1: endpoint {
+				remote-endpoint = <&csi_cam1>;
+			};
+		};
+
+		port@2 {
+			reg = <2>;
+
+			csi_switch_out2: endpoint {
+				remote-endpoint = <&csi_cam2>;
+			};
+		};
+	};
+};

drivers/media/platform/Kconfig +10 -0

index ce4a96f..a4b509e 100644
@@ -91,6 +91,16 @@ config VIDEO_OMAP3_DEBUG
 	---help---
 	  Enable debug messages on OMAP 3 camera controller driver.
 
+config VIDEO_BUS_SWITCH
+	tristate "Video Bus switch"
+	depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+	depends on MEDIA_CONTROLLER
+	depends on OF
+	---help---
+	  Driver for a GPIO controlled video bus switch, which is used to
+	  connect two camera sensors to the same port a the image signal
+	  processor.
+
 config VIDEO_PXA27x
 	tristate "PXA27x Quick Capture Interface driver"
 	depends on VIDEO_DEV && HAS_DMA

drivers/media/platform/Makefile +2 -0

index 40b18d1..8eafc27 100644
@@ -11,6 +11,8 @@ obj-$(CONFIG_VIDEO_MMP_CAMERA) += marvell-ccic/
 obj-$(CONFIG_VIDEO_OMAP3)	+= omap3isp/
 obj-$(CONFIG_VIDEO_PXA27x)	+= pxa_camera.o
 
+obj-$(CONFIG_VIDEO_BUS_SWITCH) += video-bus-switch.o
+
 obj-$(CONFIG_VIDEO_VIU) += fsl-viu.o
 
 obj-$(CONFIG_VIDEO_VIVID)		+= vivid/

drivers/media/platform/video-bus-switch.c new file +9 -0

new file mode 100644
index 0000000..6400cfc
@@ -0,0 +1,387 @@
+/*
+ * Generic driver for video bus switches
+ *
+ * Copyright (C) 2015 Sebastian Reichel <[email protected]>
+ * Copyright (C) 2016 Pavel Machek <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.

Quoted 8 lines

drivers/media/platform/video-bus-switch.c line 17

Quoted 8 lines

drivers/media/platform/video-bus-switch.c new file +8 -0

+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#define DEBUG

Please remove.

Quoted 6 lines

drivers/media/platform/video-bus-switch.c line 23
Quoted 6 lines
drivers/media/platform/video-bus-switch.c new file +6 -0
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/gpio/consumer.h>

Alphabetical order, please.

Quoted 11 lines

drivers/media/platform/video-bus-switch.c line 34

Quoted 11 lines

drivers/media/platform/video-bus-switch.c new file +11 -0

+#include <media/v4l2-async.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-of.h>
+
+/*
+ * TODO:
+ * isp_subdev_notifier_complete() calls v4l2_device_register_subdev_nodes()
+ */
+
+#define CSI_SWITCH_SUBDEVS 2
+#define CSI_SWITCH_PORTS 3

This could go to the enum below.

I guess the CSI_SWITCH_SUBDEVS could be (CSI_SWITCH_PORTS - 1).

I'd just replace CSI_SWITCH by VBS. The bus could be called differently.

Quoted 83 lines

drivers/media/platform/video-bus-switch.c new file +83 -0

+
+enum vbs_state {
+	CSI_SWITCH_DISABLED = 0,
+	CSI_SWITCH_PORT_1 = 1,
+	CSI_SWITCH_PORT_2 = 2,
+};
+
+struct vbs_src_pads {
+	struct media_entity *src;
+	int src_pad;
+};
+
+struct vbs_data {
+	struct gpio_desc *swgpio;
+	struct v4l2_subdev subdev;
+	struct v4l2_async_notifier notifier;
+	struct media_pad pads[CSI_SWITCH_PORTS];
+	struct vbs_src_pads src_pads[CSI_SWITCH_PORTS];
+	struct v4l2_of_endpoint vep[CSI_SWITCH_PORTS];
+	enum vbs_state state;
+};
+
+struct vbs_async_subdev {
+	struct v4l2_subdev *sd;
+	struct v4l2_async_subdev asd;
+	u8 port;
+};
+
+static int vbs_of_parse_nodes(struct device *dev, struct vbs_data *pdata)
+{
+	struct v4l2_async_notifier *notifier = &pdata->notifier;
+	struct device_node *node = NULL;
+
+	notifier->subdevs = devm_kcalloc(dev, CSI_SWITCH_SUBDEVS,
+		sizeof(*notifier->subdevs), GFP_KERNEL);
+	if (!notifier->subdevs)
+		return -ENOMEM;
+
+	notifier->num_subdevs = 0;
+	while (notifier->num_subdevs < CSI_SWITCH_SUBDEVS &&
+	       (node = of_graph_get_next_endpoint(dev->of_node, node))) {
+		struct v4l2_of_endpoint vep;
+		struct vbs_async_subdev *ssd;
+
+		/* skip first port (connected to isp) */
+		v4l2_of_parse_endpoint(node, &vep);
+		if (vep.base.port == 0) {
+			struct device_node *ispnode;
+
+			ispnode = of_graph_get_remote_port_parent(node);
+			if (!ispnode) {
+				dev_warn(dev, "bad remote port parent\n");
+				return -EINVAL;
+			}
+
+			of_node_put(node);
+			continue;
+		}
+
+		ssd = devm_kzalloc(dev, sizeof(*ssd), GFP_KERNEL);
+		if (!ssd) {
+			of_node_put(node);
+			return -ENOMEM;
+		}
+
+		ssd->port = vep.base.port;
+
+		notifier->subdevs[notifier->num_subdevs] = &ssd->asd;
+
+		ssd->asd.match.of.node = of_graph_get_remote_port_parent(node);
+		of_node_put(node);
+		if (!ssd->asd.match.of.node) {
+			dev_warn(dev, "bad remote port parent\n");
+			return -EINVAL;
+		}
+
+		ssd->asd.match_type = V4L2_ASYNC_MATCH_OF;
+		pdata->vep[notifier->num_subdevs] = vep;
+		notifier->num_subdevs++;
+	}
+
+	return notifier->num_subdevs;
+}

Quoted 8 lines

drivers/media/platform/video-bus-switch.c line 125
Quoted 8 lines
drivers/media/platform/video-bus-switch.c new file +8 -0
+
+static int vbs_registered(struct v4l2_subdev *sd)
+{
+	struct v4l2_device *v4l2_dev = sd->v4l2_dev;
+	struct vbs_data *pdata;
+	int err;
+
+	dev_dbg(sd->dev, "registered, init notifier...\n");

Looks like a development time debug message. :-)

Quoted 56 lines

drivers/media/platform/video-bus-switch.c new file +56 -0

+
+	pdata = v4l2_get_subdevdata(sd);
+
+	err = v4l2_async_notifier_register(v4l2_dev, &pdata->notifier);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static struct v4l2_subdev *vbs_get_remote_subdev(struct v4l2_subdev *sd)
+{
+	struct vbs_data *pdata = v4l2_get_subdevdata(sd);
+	struct media_entity *src;
+
+	if (pdata->state == CSI_SWITCH_DISABLED)
+		return ERR_PTR(-ENXIO);
+
+	src = pdata->src_pads[pdata->state].src;
+
+	return media_entity_to_v4l2_subdev(src);
+}
+
+static int vbs_link_setup(struct media_entity *entity,
+			  const struct media_pad *local,
+			  const struct media_pad *remote, u32 flags)
+{
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct vbs_data *pdata = v4l2_get_subdevdata(sd);
+	bool enable = flags & MEDIA_LNK_FL_ENABLED;
+
+	if (local->index > CSI_SWITCH_PORTS - 1)
+		return -ENXIO;
+
+	/* no configuration needed on source port */
+	if (local->index == 0)
+		return 0;
+
+	if (!enable) {
+		if (local->index == pdata->state) {
+			pdata->state = CSI_SWITCH_DISABLED;
+
+			/* Make sure we have both cameras enabled */
+			gpiod_set_value(pdata->swgpio, 1);
+			return 0;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	/* there can only be one active sink at the same time */
+	if (pdata->state != CSI_SWITCH_DISABLED)
+		return -EBUSY;
+
+	dev_dbg(sd->dev, "Link setup: going to config %d\n", local->index);
+

Quoted 8 lines

drivers/media/platform/video-bus-switch.c line 189
Quoted 8 lines
drivers/media/platform/video-bus-switch.c new file +8 -0
+	gpiod_set_value(pdata->swgpio, local->index == CSI_SWITCH_PORT_2);
+	pdata->state = local->index;
+
+	sd = vbs_get_remote_subdev(sd);
+	if (IS_ERR(sd))
+		return PTR_ERR(sd);
+
+	pdata->subdev.ctrl_handler = sd->ctrl_handler;

This is ugly. You're exposing all the controls through another sub-device.

How does link validation work now?

I wonder if it'd be less so if you just pass through the LINK_FREQ and PIXEL_RATE controls. It'll certainly be more code though.

I think the link frequency could be something that goes to the frame descriptor as well. Then we wouldn't need to worry about the controls separately, just passing the frame descriptor would be enough.

I apologise that I don't have patches quite ready for posting to the list.

Quoted 51 lines

drivers/media/platform/video-bus-switch.c new file +51 -0

+
+	return 0;
+}
+
+static int vbs_subdev_notifier_bound(struct v4l2_async_notifier *async,
+				     struct v4l2_subdev *subdev,
+				     struct v4l2_async_subdev *asd)
+{
+	struct vbs_data *pdata = container_of(async,
+		struct vbs_data, notifier);
+	struct vbs_async_subdev *ssd =
+		container_of(asd, struct vbs_async_subdev, asd);
+	struct media_entity *sink = &pdata->subdev.entity;
+	struct media_entity *src = &subdev->entity;
+	int sink_pad = ssd->port;
+	int src_pad;
+
+	if (sink_pad >= sink->num_pads) {
+		dev_err(pdata->subdev.dev, "no sink pad in internal entity!\n");
+		return -EINVAL;
+	}
+
+	for (src_pad = 0; src_pad < subdev->entity.num_pads; src_pad++) {
+		if (subdev->entity.pads[src_pad].flags & MEDIA_PAD_FL_SOURCE)
+			break;
+	}
+
+	if (src_pad >= src->num_pads) {
+		dev_err(pdata->subdev.dev, "no source pad in external entity\n");
+		return -EINVAL;
+	}
+
+	pdata->src_pads[sink_pad].src = src;
+	pdata->src_pads[sink_pad].src_pad = src_pad;
+	ssd->sd = subdev;
+
+	return 0;
+}
+
+static int vbs_subdev_notifier_complete(struct v4l2_async_notifier *async)
+{
+	struct vbs_data *pdata = container_of(async, struct vbs_data, notifier);
+	struct media_entity *sink = &pdata->subdev.entity;
+	int sink_pad;
+
+	for (sink_pad = 1; sink_pad < CSI_SWITCH_PORTS; sink_pad++) {
+		struct media_entity *src = pdata->src_pads[sink_pad].src;
+		int src_pad = pdata->src_pads[sink_pad].src_pad;
+		int err;
+
+		err = media_create_pad_link(src, src_pad, sink, sink_pad, 0);

Quoted 8 lines

drivers/media/platform/video-bus-switch.c line 248
Quoted 8 lines
drivers/media/platform/video-bus-switch.c new file +8 -0
+		if (err < 0)
+			return err;
+
+		dev_dbg(pdata->subdev.dev, "create link: %s[%d] -> %s[%d])\n",
+			src->name, src_pad, sink->name, sink_pad);
+	}
+
+	return v4l2_device_register_subdev_nodes(pdata->subdev.v4l2_dev);

The ISP driver's complete() callback calls v4l2_device_register_subdev_nodes() already. Currently it cannot handle being called more than once --- that needs to be fixed.

Quoted 14 lines

drivers/media/platform/video-bus-switch.c new file +14 -0

+}
+
+static int vbs_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct v4l2_subdev *subdev = vbs_get_remote_subdev(sd);
+
+	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 v4l2_of_endpoint *cfg)
+{

Quoted 8 lines

drivers/media/platform/video-bus-switch.c line 270
Quoted 8 lines
drivers/media/platform/video-bus-switch.c new file +8 -0
+	struct vbs_data *pdata = v4l2_get_subdevdata(sd);
+	dev_dbg(sd->dev, "vbs_g_endpoint_config... active port is %d\n", pdata->state);
+	*cfg = pdata->vep[pdata->state - 1];
+
+	return 0;
+}
+
+

I'd say that's an extra newline.

Quoted 52 lines

drivers/media/platform/video-bus-switch.c new file +52 -0

+static const struct v4l2_subdev_internal_ops vbs_internal_ops = {
+	.registered = &vbs_registered,
+};
+
+static const struct media_entity_operations vbs_media_ops = {
+	.link_setup = vbs_link_setup,
+	.link_validate = v4l2_subdev_link_validate,
+};
+
+/* 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 = {
+	.video = &vbs_video_ops,
+};
+
+static int video_bus_switch_probe(struct platform_device *pdev)
+{
+	struct vbs_data *pdata;
+	int err = 0;
+
+	/* platform data */
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_dbg(&pdev->dev, "video-bus-switch: not enough memory\n");
+		return -ENOMEM;
+	}
+	platform_set_drvdata(pdev, pdata);
+
+	/* switch gpio */
+	pdata->swgpio = devm_gpiod_get(&pdev->dev, "switch", GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->swgpio)) {
+		err = PTR_ERR(pdata->swgpio);
+		dev_err(&pdev->dev, "Failed to request gpio: %d\n", err);
+		return err;
+	}
+
+	/* find sub-devices */
+	err = vbs_of_parse_nodes(&pdev->dev, pdata);
+	if (err < 0) {
+		dev_err(&pdev->dev, "Failed to parse nodes: %d\n", err);
+		return err;
+	}
+
+	pdata->state = CSI_SWITCH_DISABLED;
+	pdata->notifier.bound = vbs_subdev_notifier_bound;
+	pdata->notifier.complete = vbs_subdev_notifier_complete;
+
+	/* setup subdev */

Quoted 8 lines

drivers/media/platform/video-bus-switch.c line 330

Quoted 8 lines

drivers/media/platform/video-bus-switch.c new file +8 -0

+	pdata->pads[0].flags = MEDIA_PAD_FL_SOURCE;
+	pdata->pads[1].flags = MEDIA_PAD_FL_SINK;
+	pdata->pads[2].flags = MEDIA_PAD_FL_SINK;
+
+	v4l2_subdev_init(&pdata->subdev, &vbs_ops);
+	pdata->subdev.dev = &pdev->dev;
+	pdata->subdev.owner = pdev->dev.driver->owner;
+	strncpy(pdata->subdev.name, dev_name(&pdev->dev), V4L2_SUBDEV_NAME_SIZE);

How about sizeof(pdata->subdev.name) ?

I'm not sure I like V4L2_SUBDEV_NAME_SIZE in general. It could be even removed. But not by this patch. :-)

Quoted 3 lines

drivers/media/platform/video-bus-switch.c line 333
Quoted 3 lines
drivers/media/platform/video-bus-switch.c new file +3 -0
+	v4l2_set_subdevdata(&pdata->subdev, pdata);
+	pdata->subdev.entity.function = MEDIA_ENT_F_SWITCH;
+	pdata->subdev.entity.flags |= MEDIA_ENT_F_SWITCH;

MEDIA_ENT_FL_*

Quoted 10 lines

drivers/media/platform/video-bus-switch.c new file +10 -0

+	pdata->subdev.entity.ops = &vbs_media_ops;
+	pdata->subdev.internal_ops = &vbs_internal_ops;
+	err = media_entity_pads_init(&pdata->subdev.entity, CSI_SWITCH_PORTS,
+				pdata->pads);
+	if (err < 0) {
+		dev_err(&pdev->dev, "Failed to init media entity: %d\n", err);
+		return err;
+	}
+
+	/* register subdev */

Quoted 8 lines

drivers/media/platform/video-bus-switch.c line 351

Quoted 8 lines

drivers/media/platform/video-bus-switch.c new file +8 -0

+	err = v4l2_async_register_subdev(&pdata->subdev);
+	if (err < 0) {
+		dev_err(&pdev->dev, "Failed to register v4l2 subdev: %d\n", err);
+		media_entity_cleanup(&pdata->subdev.entity);
+		return err;
+	}
+
+	dev_info(&pdev->dev, "video-bus-switch registered\n");

How about dev_dbg()?

Quoted 9 lines

drivers/media/platform/video-bus-switch.c line 360
Quoted 9 lines
drivers/media/platform/video-bus-switch.c new file +9 -0
+
+	return 0;
+}
+
+static int video_bus_switch_remove(struct platform_device *pdev)
+{
+	struct vbs_data *pdata = platform_get_drvdata(pdev);
+
+	v4l2_async_notifier_unregister(&pdata->notifier);

Shouldn't you unregister the notifier in the .unregister() callback?

Quoted 37 lines

drivers/media/platform/video-bus-switch.c new file +27 -0

+	v4l2_async_unregister_subdev(&pdata->subdev);
+	media_entity_cleanup(&pdata->subdev.entity);
+
+	return 0;
+}
+
+static const struct of_device_id video_bus_switch_of_match[] = {
+	{ .compatible = "video-bus-switch" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, video_bus_switch_of_match);
+
+static struct platform_driver video_bus_switch_driver = {
+	.driver = {
+		.name	= "video-bus-switch",
+		.of_match_table = video_bus_switch_of_match,
+	},
+	.probe		= video_bus_switch_probe,
+	.remove		= video_bus_switch_remove,
+};
+
+module_platform_driver(video_bus_switch_driver);
+
+MODULE_AUTHOR("Sebastian Reichel <[email protected]>");
+MODULE_DESCRIPTION("Video Bus Switch");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:video-bus-switch");

include/media/v4l2-subdev.h +1 -0

index cf778c5..448dbb5 100644
@@ -25,6 +25,7 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-mediabus.h>
+#include <media/v4l2-of.h>

Quoted 8 lines

include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 420

Quoted 8 lines

include/media/v4l2-subdev.h +2 -0

 /* generic v4l2_device notify callback notification values */
 #define V4L2_SUBDEV_IR_RX_NOTIFY		_IOW('v', 0, u32)

@@ -415,6 +416,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);

This should be in a separate patch --- assuming we'll add this one.

Quoted 12 lines

include/uapi/linux/media.h struct media_device_info { · line 150

Quoted 12 lines

include/media/v4l2-subdev.h +0 -0

 };
 
 /**

include/uapi/linux/media.h +1 -0

index 4890787..94648ab 100644
@@ -147,6 +147,7 @@ struct media_device_info {
  * MEDIA_ENT_F_IF_VID_DECODER and/or MEDIA_ENT_F_IF_AUD_DECODER.
  */
 #define MEDIA_ENT_F_TUNER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
+#define MEDIA_ENT_F_SWITCH		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 6)

I wonder if MEDIA_ENT_F_PROC_ would be a better prefix.
We shouldn't have new entries in MEDIA_ENT_F_OLD_SUBDEV_BASE anymore.

Quoted 6 lines

Quoted 5 lines
include/uapi/linux/media.h +0 -0
 #define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_F_OLD_SUBDEV_BASE
 
 

Signature · 4 lines

Kind regards,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]

Sebastian Reichel

2017-02-04 01:04 UTC #79 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Pavel Machek

Hi,

Quoted 6 lines · Pavel Machek

Quoted 5 lines · Pali Rohár
Quoted 3 lines · On Friday 03 February 2017 13:35:08 Pavel Machek wrote:
N900 contains front and back camera, with a switch between the two. This adds support for the switch component, and it is now possible to select between front and back cameras during runtime.

Quoted 6 lines

Quoted 2 lines
IIRC for controlling cameras on N900 there are two GPIOs. Should not you have both in switch driver?
I guess you recall wrongly :-). Switch seems to work. The issue was with switch GPIO also serving as reset GPIO for one sensor, or something like that, if _I_ recall correctly ;-).

I have a schematic in my master thesis, which shows how the camera sensors are connected to the SoC. The PDF is available here:

https://www.uni-oldenburg.de/fileadmin/user_upload/informatik/ag/svs/download/thesis/Reichel_Sebastian.pdf

The schematic is on page 37 (or 45 if your PDF reader does not use different numbers for the preamble stuff).

--Sebastian

Pavel Machek

author

2017-02-04 21:56 UTC #67 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Sakari Ailus

Hi!

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+Required properties
+===================
+
+compatible	: must contain "video-bus-switch"
How generic is this? Should we have e.g. nokia,video-bus-switch? And if so, change the file name accordingly.

Quoted 4 lines

Quoted 2 lines
Generic for "single GPIO controls the switch", AFAICT. But that should be common enough...
Um, yes. Then... how about: video-bus-switch-gpio? No Nokia prefix.

Ok, done. I also fixed the english a bit.

Quoted 10 lines

Quoted 10 lines
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+reg		: The interface:
+		  0 - port for image signal processor
+		  1 - port for first camera sensor
+		  2 - port for second camera sensor
I'd say this must be pretty much specific to the one in N900. You could have more ports. Or you could say that ports beyond 0 are camera sensors. I guess this is good enough for now though, it can be changed later on with the source if a need arises.

Quoted 8 lines

Quoted 3 lines
Well, I'd say that selecting between two sensors is going to be the common case. If someone needs more than two, it will no longer be simple GPIO, so we'll have some fixing to do.
It could be two GPIOs --- that's how the GPIO I2C mux works.
But I'd be surprised if someone ever uses something like that again. ;-)

I'd say.. lets handle that when we see hardware like that.

Quoted 8 lines

Quoted 8 lines
Quoted 4 lines
Btw. was it still considered a problem that the endpoint properties for the sensors can be different? With the g_routing() pad op which is to be added, the ISP driver (should actually go to a framework somewhere) could parse the graph and find the proper endpoint there.
I don't know about g_routing. I added g_endpoint_config method that passes the configuration, and that seems to work for me.

Quoted 8 lines

Quoted 1 line
I don't see g_routing in next-20170201 . Is there place to look?
I think there was a patch by Laurent to LMML quite some time ago. I suppose that set will be repicked soonish.
I don't really object using g_endpoint_config() as a temporary solution; I'd like to have Laurent's opinion on that though. Another option is to wait, but we've already waited a looong time (as in total).

Laurent, do you have some input here? We have simple "2 cameras connected to one signal processor" situation here. We need some way of passing endpoint configuration from the sensors through the switch. I did this:

Quoted 6 lines

include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 420

Quoted 6 lines

include/media/v4l2-subdev.h +2 -0

@@ -415,6 +416,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);

Google of g_routing tells me:

9) Highly reconfigurable hardware - Julien Beraud

- 44 sub-devices connected with an interconnect.
- As long as formats match, any sub-device could be connected to any
- other sub-device through a link.
- The result is 44 * 44 links at worst.
- A switch sub-device proposed as the solution to model the
- interconnect. The sub-devices are connected to the switch
- sub-devices through the hardware links that connect to the
- interconnect.
- The switch would be controlled through new IOCTLs S_ROUTING and
- G_ROUTING.
- Patches available:
http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip

but the patches are from 2005. So I guess I'll need some guidance here...

Quoted 1 line

I'll reply to the other patch containing the code.

Ok, thanks.
Pavel

Signature · 2 lines

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

Sakari Ailus

maintainer

2017-02-04 22:33 UTC #68 lore raw

Re: [PATCH] devicetree: Add video bus switch

Hi Pavel,

Quoted 31 lines · Pavel Machek

Hi!
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+Required properties
+===================
+
+compatible	: must contain "video-bus-switch"
How generic is this? Should we have e.g. nokia,video-bus-switch? And if so, change the file name accordingly.
Quoted 4 lines
Quoted 2 lines
Generic for "single GPIO controls the switch", AFAICT. But that should be common enough...
Um, yes. Then... how about: video-bus-switch-gpio? No Nokia prefix.
Ok, done. I also fixed the english a bit.
Quoted 14 lines
Quoted 13 lines
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+reg		: The interface:
+		  0 - port for image signal processor
+		  1 - port for first camera sensor
+		  2 - port for second camera sensor
I'd say this must be pretty much specific to the one in N900. You could have more ports. Or you could say that ports beyond 0 are camera sensors. I guess this is good enough for now though, it can be changed later on with the source if a need arises.
Well, I'd say that selecting between two sensors is going to be the common case. If someone needs more than two, it will no longer be simple GPIO, so we'll have some fixing to do.

Quoted 6 lines

Quoted 4 lines
It could be two GPIOs --- that's how the GPIO I2C mux works.
But I'd be surprised if someone ever uses something like that again. ;-)
I'd say.. lets handle that when we see hardware like that.

Yes. :-)

Quoted 47 lines

Quoted 9 lines
Quoted 9 lines
Quoted 5 lines
Btw. was it still considered a problem that the endpoint properties for the sensors can be different? With the g_routing() pad op which is to be added, the ISP driver (should actually go to a framework somewhere) could parse the graph and find the proper endpoint there.
I don't know about g_routing. I added g_endpoint_config method that passes the configuration, and that seems to work for me.
Quoted 8 lines
Quoted 1 line
I don't see g_routing in next-20170201 . Is there place to look?
I think there was a patch by Laurent to LMML quite some time ago. I suppose that set will be repicked soonish.
I don't really object using g_endpoint_config() as a temporary solution; I'd like to have Laurent's opinion on that though. Another option is to wait, but we've already waited a looong time (as in total).
Laurent, do you have some input here? We have simple "2 cameras connected to one signal processor" situation here. We need some way of passing endpoint configuration from the sensors through the switch. I did this:
Quoted 6 lines
include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 420
Quoted 6 lines
include/media/v4l2-subdev.h +2 -0
@@ -415,6 +416,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);
Google of g_routing tells me:
9) Highly reconfigurable hardware - Julien Beraud
- 44 sub-devices connected with an interconnect.
- As long as formats match, any sub-device could be connected to any
- other sub-device through a link.
- The result is 44 * 44 links at worst.
- A switch sub-device proposed as the solution to model the
- interconnect. The sub-devices are connected to the switch
- sub-devices through the hardware links that connect to the
- interconnect.
- The switch would be controlled through new IOCTLs S_ROUTING and
- G_ROUTING.
- Patches available:
http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip

Quoted 1 line

but the patches are from 2005. So I guess I'll need some guidance here...

Yeah, that's where it began (2015?), but right now I can only suggest to wait until there's more. My estimate is within next couple of weeks / months. But it won't be years.

Signature · 8 lines

Kind regards,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pavel Machek

author

2017-02-05 21:12 UTC #69 lore raw

Re: [PATCH] devicetree: Add video bus switch

Hi!

Quoted 15 lines

Quoted 15 lines
9) Highly reconfigurable hardware - Julien Beraud
- 44 sub-devices connected with an interconnect.
- As long as formats match, any sub-device could be connected to any
- other sub-device through a link.
- The result is 44 * 44 links at worst.
- A switch sub-device proposed as the solution to model the
- interconnect. The sub-devices are connected to the switch
- sub-devices through the hardware links that connect to the
- interconnect.
- The switch would be controlled through new IOCTLs S_ROUTING and
- G_ROUTING.
- Patches available:
http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip

Quoted 5 lines

Quoted 1 line
but the patches are from 2005. So I guess I'll need some guidance here...
Yeah, that's where it began (2015?), but right now I can only suggest to wait until there's more. My estimate is within next couple of weeks / months. But it won't be years.

Ok, week or two would be ok, couple of months is not. And all I need is single hook in common structure.

So if g_endpoint_config hook looks sane to _you_, I suggest we simply proceed. Now, maybe Mauro Carvalho Chehab <[email protected]> or Laurent or Julien will want a different solution, but then... they'll have to suggest something doable now, not in couple of months.

Does that sound like a plan?

Mauro added to cc list, so we can get some input.

Best regards,
Pavel

Signature · 2 lines

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

Pavel Machek

author

2017-02-05 22:16 UTC #61 lore raw

Re: [PATCH] media: Add video bus switch

In reply to Sakari Ailus

Hi!

I lost my original reply... so this will be slightly brief.

Quoted 9 lines

Quoted 7 lines
drivers/media/platform/video-bus-switch.c line 17
Quoted 7 lines
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
drivers/media/platform/video-bus-switch.c new file +2 -0
+
+#define DEBUG
Please remove.

Ok.

Quoted 4 lines

Quoted 2 lines
drivers/media/platform/video-bus-switch.c line 23
Quoted 2 lines
drivers/media/platform/video-bus-switch.c new file +2 -0
+#include <linux/of_graph.h>
+#include <linux/gpio/consumer.h>
Alphabetical order, please.

Ok. (But let me make unhappy noise, because these rules are inconsistent across kernel.)

Quoted 9 lines

Quoted 6 lines
drivers/media/platform/video-bus-switch.c line 34
Quoted 6 lines
+ * TODO:
+ * isp_subdev_notifier_complete() calls v4l2_device_register_subdev_nodes()
+ */
drivers/media/platform/video-bus-switch.c new file +3 -0
+
+#define CSI_SWITCH_SUBDEVS 2
+#define CSI_SWITCH_PORTS 3
This could go to the enum below.

Quoted 4 lines

I guess the CSI_SWITCH_SUBDEVS could be (CSI_SWITCH_PORTS - 1).
I'd just replace CSI_SWITCH by VBS. The bus could be called differently.

Ok.

Quoted 9 lines

Quoted 7 lines
drivers/media/platform/video-bus-switch.c line 125
Quoted 7 lines
drivers/media/platform/video-bus-switch.c new file +7 -0
+static int vbs_registered(struct v4l2_subdev *sd)
+{
+	struct v4l2_device *v4l2_dev = sd->v4l2_dev;
+	struct vbs_data *pdata;
+	int err;
+
+	dev_dbg(sd->dev, "registered, init notifier...\n");
Looks like a development time debug message. :-)

ex-development message ;-).

Quoted 16 lines

Quoted 8 lines
drivers/media/platform/video-bus-switch.c line 189
Quoted 8 lines
drivers/media/platform/video-bus-switch.c new file +8 -0
+	gpiod_set_value(pdata->swgpio, local->index == CSI_SWITCH_PORT_2);
+	pdata->state = local->index;
+
+	sd = vbs_get_remote_subdev(sd);
+	if (IS_ERR(sd))
+		return PTR_ERR(sd);
+
+	pdata->subdev.ctrl_handler = sd->ctrl_handler;
This is ugly. You're exposing all the controls through another sub-device.
How does link validation work now?
I wonder if it'd be less so if you just pass through the LINK_FREQ and PIXEL_RATE controls. It'll certainly be more code though.

Quoted 6 lines

I think the link frequency could be something that goes to the frame descriptor as well. Then we wouldn't need to worry about the controls separately, just passing the frame descriptor would be enough.
I apologise that I don't have patches quite ready for posting to the list.

(Plus of course question is "what is link validation".)

Ok, let me play with this one. Solution you are suggesting is to make a custom harndler that only passes certain data through, right?

Quoted 9 lines

Quoted 5 lines
drivers/media/platform/video-bus-switch.c line 248
Quoted 5 lines
drivers/media/platform/video-bus-switch.c new file +5 -0
+		dev_dbg(pdata->subdev.dev, "create link: %s[%d] -> %s[%d])\n",
+			src->name, src_pad, sink->name, sink_pad);
+	}
+
+	return v4l2_device_register_subdev_nodes(pdata->subdev.v4l2_dev);
The ISP driver's complete() callback calls v4l2_device_register_subdev_nodes() already. Currently it cannot handle being called more than once --- that needs to be fixed.

I may have patches for that. Let me check.

Quoted 5 lines

Quoted 3 lines
drivers/media/platform/video-bus-switch.c line 93
Quoted 3 lines
drivers/media/platform/video-bus-switch.c new file +3 -0
+}
+
+
I'd say that's an extra newline.

Not any more.

Quoted 6 lines

Quoted 4 lines
drivers/media/platform/video-bus-switch.c line 330
Quoted 4 lines
drivers/media/platform/video-bus-switch.c new file +4 -0
+	v4l2_subdev_init(&pdata->subdev, &vbs_ops);
+	pdata->subdev.dev = &pdev->dev;
+	pdata->subdev.owner = pdev->dev.driver->owner;
+	strncpy(pdata->subdev.name, dev_name(&pdev->dev), V4L2_SUBDEV_NAME_SIZE);
How about sizeof(pdata->subdev.name) ?

Ok.

Quoted 8 lines

I'm not sure I like V4L2_SUBDEV_NAME_SIZE in general. It could be even removed. But not by this patch. :-)
Quoted 3 lines
drivers/media/platform/video-bus-switch.c line 333
Quoted 3 lines
drivers/media/platform/video-bus-switch.c new file +3 -0
+	v4l2_set_subdevdata(&pdata->subdev, pdata);
+	pdata->subdev.entity.function = MEDIA_ENT_F_SWITCH;
+	pdata->subdev.entity.flags |= MEDIA_ENT_F_SWITCH;
MEDIA_ENT_FL_*

Do we actually have a flag here? We already have .function, so this looks like a duplicate.

Quoted 9 lines

Quoted 7 lines
drivers/media/platform/video-bus-switch.c line 351
Quoted 7 lines
drivers/media/platform/video-bus-switch.c new file +7 -0
+	if (err < 0) {
+		dev_err(&pdev->dev, "Failed to register v4l2 subdev: %d\n", err);
+		media_entity_cleanup(&pdata->subdev.entity);
+		return err;
+	}
+
+	dev_info(&pdev->dev, "video-bus-switch registered\n");
How about dev_dbg()?

Ok.

Quoted 7 lines

Quoted 5 lines
drivers/media/platform/video-bus-switch.c line 360
Quoted 5 lines
drivers/media/platform/video-bus-switch.c new file +5 -0
+static int video_bus_switch_remove(struct platform_device *pdev)
+{
+	struct vbs_data *pdata = platform_get_drvdata(pdev);
+
+	v4l2_async_notifier_unregister(&pdata->notifier);
Shouldn't you unregister the notifier in the .unregister() callback?

Ok, I guess we can do that for symetry.

Quoted 10 lines

Quoted 8 lines
include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 420
Quoted 8 lines
/* generic v4l2_device notify callback notification values */
#define V4L2_SUBDEV_IR_RX_NOTIFY _IOW('v', 0, u32)
include/media/v4l2-subdev.h +2 -0
@@ -415,6 +416,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);
This should be in a separate patch --- assuming we'll add this one.

Hmm. I believe the rest of the driver is quite useful in understanding this. Ok, lets get the discussion started.

Quoted 10 lines

Quoted 7 lines
include/uapi/linux/media.h struct media_device_info { · line 150
Quoted 7 lines
include/uapi/linux/media.h +1 -0
@@ -147,6 +147,7 @@ struct media_device_info {
  * MEDIA_ENT_F_IF_VID_DECODER and/or MEDIA_ENT_F_IF_AUD_DECODER.
  */
 #define MEDIA_ENT_F_TUNER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
+#define MEDIA_ENT_F_SWITCH		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 6)
I wonder if MEDIA_ENT_F_PROC_ would be a better prefix.
We shouldn't have new entries in MEDIA_ENT_F_OLD_SUBDEV_BASE anymore.

Ok.
Pavel

Signature · 2 lines

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

Sakari Ailus

maintainer

2017-02-05 22:45 UTC #62 lore raw

Re: [PATCH] media: Add video bus switch

Hi, Pavel!

Quoted 3 lines · Pavel Machek

Hi!
I lost my original reply... so this will be slightly brief.

:-o

Quoted 65 lines

Quoted 10 lines
Quoted 8 lines
drivers/media/platform/video-bus-switch.c line 17
Quoted 8 lines
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
drivers/media/platform/video-bus-switch.c new file +2 -0
+
+#define DEBUG
Please remove.
Ok.
Quoted 4 lines
Quoted 2 lines
drivers/media/platform/video-bus-switch.c line 23
Quoted 2 lines
drivers/media/platform/video-bus-switch.c new file +2 -0
+#include <linux/of_graph.h>
+#include <linux/gpio/consumer.h>
Alphabetical order, please.
Ok. (But let me make unhappy noise, because these rules are inconsistent across kernel.)
Quoted 9 lines
Quoted 6 lines
drivers/media/platform/video-bus-switch.c line 34
Quoted 6 lines
+ * TODO:
+ * isp_subdev_notifier_complete() calls v4l2_device_register_subdev_nodes()
+ */
drivers/media/platform/video-bus-switch.c new file +3 -0
+
+#define CSI_SWITCH_SUBDEVS 2
+#define CSI_SWITCH_PORTS 3
This could go to the enum below.
Quoted 4 lines
I guess the CSI_SWITCH_SUBDEVS could be (CSI_SWITCH_PORTS - 1).
I'd just replace CSI_SWITCH by VBS. The bus could be called differently.
Ok.
Quoted 9 lines
Quoted 7 lines
drivers/media/platform/video-bus-switch.c line 125
Quoted 7 lines
drivers/media/platform/video-bus-switch.c new file +7 -0
+static int vbs_registered(struct v4l2_subdev *sd)
+{
+	struct v4l2_device *v4l2_dev = sd->v4l2_dev;
+	struct vbs_data *pdata;
+	int err;
+
+	dev_dbg(sd->dev, "registered, init notifier...\n");
Looks like a development time debug message. :-)
ex-development message ;-).
Quoted 16 lines
Quoted 8 lines
drivers/media/platform/video-bus-switch.c line 189
Quoted 8 lines
drivers/media/platform/video-bus-switch.c new file +8 -0
+	gpiod_set_value(pdata->swgpio, local->index == CSI_SWITCH_PORT_2);
+	pdata->state = local->index;
+
+	sd = vbs_get_remote_subdev(sd);
+	if (IS_ERR(sd))
+		return PTR_ERR(sd);
+
+	pdata->subdev.ctrl_handler = sd->ctrl_handler;
This is ugly. You're exposing all the controls through another sub-device.
How does link validation work now?
I wonder if it'd be less so if you just pass through the LINK_FREQ and PIXEL_RATE controls. It'll certainly be more code though.

Quoted 8 lines

Quoted 6 lines
I think the link frequency could be something that goes to the frame descriptor as well. Then we wouldn't need to worry about the controls separately, just passing the frame descriptor would be enough.
I apologise that I don't have patches quite ready for posting to the list.
(Plus of course question is "what is link validation".)

The links along the pipeline are validated for matching width, height, media bus code and possibly other matters. The aim is to make sure that the hardware configuration is a valid one before streaming starts.

Quoted 3 lines

Ok, let me play with this one. Solution you are suggesting is to make a custom harndler that only passes certain data through, right?

That's an option. But supposing we'll add that to the frame desciptors [1], there won't be need for a custom handler either.

Quoted 33 lines

Quoted 10 lines
Quoted 6 lines
drivers/media/platform/video-bus-switch.c line 248
Quoted 6 lines
drivers/media/platform/video-bus-switch.c new file +5 -0
+		dev_dbg(pdata->subdev.dev, "create link: %s[%d] -> %s[%d])\n",
+			src->name, src_pad, sink->name, sink_pad);
+	}
+
+	return v4l2_device_register_subdev_nodes(pdata->subdev.v4l2_dev);
The ISP driver's complete() callback calls v4l2_device_register_subdev_nodes() already. Currently it cannot handle being called more than once --- that needs to be fixed.
I may have patches for that. Let me check.
Quoted 5 lines
Quoted 3 lines
drivers/media/platform/video-bus-switch.c line 93
Quoted 3 lines
drivers/media/platform/video-bus-switch.c new file +3 -0
+}
+
+
I'd say that's an extra newline.
Not any more.
Quoted 6 lines
Quoted 4 lines
drivers/media/platform/video-bus-switch.c line 330
Quoted 4 lines
drivers/media/platform/video-bus-switch.c new file +4 -0
+	v4l2_subdev_init(&pdata->subdev, &vbs_ops);
+	pdata->subdev.dev = &pdev->dev;
+	pdata->subdev.owner = pdev->dev.driver->owner;
+	strncpy(pdata->subdev.name, dev_name(&pdev->dev), V4L2_SUBDEV_NAME_SIZE);
How about sizeof(pdata->subdev.name) ?
Ok.
Quoted 3 lines
I'm not sure I like V4L2_SUBDEV_NAME_SIZE in general. It could be even removed. But not by this patch. :-)

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
drivers/media/platform/video-bus-switch.c line 333
Quoted 3 lines
drivers/media/platform/video-bus-switch.c new file +3 -0
+	v4l2_set_subdevdata(&pdata->subdev, pdata);
+	pdata->subdev.entity.function = MEDIA_ENT_F_SWITCH;
+	pdata->subdev.entity.flags |= MEDIA_ENT_F_SWITCH;
MEDIA_ENT_FL_*
Do we actually have a flag here? We already have .function, so this looks like a duplicate.

You can skip setting this. We only have flags for DEFAULT and CONNECTOR and neither is relevant here.

Quoted 22 lines

Quoted 11 lines
Quoted 9 lines
drivers/media/platform/video-bus-switch.c line 351
Quoted 9 lines
drivers/media/platform/video-bus-switch.c new file +7 -0
+	if (err < 0) {
+		dev_err(&pdev->dev, "Failed to register v4l2 subdev: %d\n", err);
+		media_entity_cleanup(&pdata->subdev.entity);
+		return err;
+	}
+
+	dev_info(&pdev->dev, "video-bus-switch registered\n");
How about dev_dbg()?
Ok.
Quoted 7 lines
Quoted 5 lines
drivers/media/platform/video-bus-switch.c line 360
Quoted 5 lines
drivers/media/platform/video-bus-switch.c new file +5 -0
+static int video_bus_switch_remove(struct platform_device *pdev)
+{
+	struct vbs_data *pdata = platform_get_drvdata(pdev);
+
+	v4l2_async_notifier_unregister(&pdata->notifier);
Shouldn't you unregister the notifier in the .unregister() callback?

Quoted 1 line

Ok, I guess we can do that for symetry.

The sub-device may be bound and unbound without the driver being probed or removed. That's why the notifier unregistration should take place in the .unregister callback.

Quoted 12 lines

Quoted 11 lines
Quoted 9 lines
include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 420
Quoted 9 lines
/* generic v4l2_device notify callback notification values */
#define V4L2_SUBDEV_IR_RX_NOTIFY _IOW('v', 0, u32)
include/media/v4l2-subdev.h +2 -0
@@ -415,6 +416,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);
This should be in a separate patch --- assuming we'll add this one.

Quoted 2 lines

Hmm. I believe the rest of the driver is quite useful in understanding this. Ok, lets get the discussion started.

Please add field documentation as well in the comment above.

Quoted 15 lines

Quoted 11 lines
Quoted 8 lines
include/uapi/linux/media.h struct media_device_info { · line 150
Quoted 8 lines
include/uapi/linux/media.h +1 -0
@@ -147,6 +147,7 @@ struct media_device_info {
  * MEDIA_ENT_F_IF_VID_DECODER and/or MEDIA_ENT_F_IF_AUD_DECODER.
  */
 #define MEDIA_ENT_F_TUNER		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
+#define MEDIA_ENT_F_SWITCH		(MEDIA_ENT_F_OLD_SUBDEV_BASE + 6)
I wonder if MEDIA_ENT_F_PROC_ would be a better prefix.
We shouldn't have new entries in MEDIA_ENT_F_OLD_SUBDEV_BASE anymore.
Ok.
Pavel

Signature · 4 lines

Kind regards,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]

Sebastian Reichel

2017-02-05 23:40 UTC #70 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Pavel Machek

Hi,

Quoted 30 lines · Pavel Machek

Quoted 15 lines
Quoted 15 lines
9) Highly reconfigurable hardware - Julien Beraud
- 44 sub-devices connected with an interconnect.
- As long as formats match, any sub-device could be connected to any
- other sub-device through a link.
- The result is 44 * 44 links at worst.
- A switch sub-device proposed as the solution to model the
- interconnect. The sub-devices are connected to the switch
- sub-devices through the hardware links that connect to the
- interconnect.
- The switch would be controlled through new IOCTLs S_ROUTING and
- G_ROUTING.
- Patches available:
http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip
Quoted 5 lines
Quoted 1 line
but the patches are from 2005. So I guess I'll need some guidance here...
Yeah, that's where it began (2015?), but right now I can only suggest to wait until there's more. My estimate is within next couple of weeks / months. But it won't be years.
Ok, week or two would be ok, couple of months is not. And all I need is single hook in common structure.
So if g_endpoint_config hook looks sane to _you_, I suggest we simply proceed. Now, maybe Mauro Carvalho Chehab <[email protected]> or Laurent or Julien will want a different solution, but
then... they'll have to suggest something doable now, not in couple of months.

Quoted 3 lines

Does that sound like a plan?
Mauro added to cc list, so we can get some input.

side note: It's an kernel-internal API only used by the media subsystem. Code can be easily updated to use an improved API at any time.

-- Sebastian

Pavel Machek

author

2017-02-06 09:37 UTC #71 lore raw

PATCH 1/1 media: add operation to get configuration of "the other side" of the link

Normally, link configuration can be determined at probe time... but Nokia N900 has two cameras, and can switch between them at runtime, so that mechanism is not suitable here.

Add a hook that tells us link configuration.

Signed-off-by Pavel Machek <[email protected]>

include/media/v4l2-subdev.h +5 -0

index cf778c5..74148b9 100644
@@ -25,6 +25,7 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-mediabus.h>
+#include <media/v4l2-of.h>
 
 /* generic v4l2_device notify callback notification values */
 #define V4L2_SUBDEV_IR_RX_NOTIFY		_IOW('v', 0, u32)

@@ -383,6 +384,8 @@ struct v4l2_mbus_frame_desc {
  * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
  *	can adjust @size to a lower value and must not write more data to the
  *	buffer starting at @data than the original value of @size.
+ *
+ * @g_endpoint_config: get link configuration required by this device.
  */
 struct v4l2_subdev_video_ops {
 	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);

@@ -415,6 +418,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);
 };
 
 /**

Signature · 2 lines

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

Rob Herring

maintainer

2017-02-08 21:36 UTC #80 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Pavel Machek

Quoted 18 lines · Pavel Machek

N900 contains front and back camera, with a switch between the two. This adds support for the switch component, and it is now possible to select between front and back cameras during runtime.
This adds documentation for the devicetree binding.
Signed-off-by Sebastian Reichel <[email protected]>
Signed-off-by Ivaylo Dimitrov <[email protected]>
Signed-off-by Pavel Machek <[email protected]>
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +0 -0
new file mode 100644
index 0000000..1b9f8e0

Documentation/devicetree/bindings/media/video-bus-switch.txt line 2

Quoted 2 lines

Documentation/devicetree/bindings/media/video-bus-switch.txt new file +2 -0
@@ -0,0 +1,63 @@
+Video Bus Switch Binding
+========================

I'd call it a mux rather than switch.

BTW, there's a new mux-controller binding under review you might look at. It would only be needed here if the mux ctrl also controls other things.

Documentation/devicetree/bindings/media/video-bus-switch.txt line 11

Quoted 9 lines

Documentation/devicetree/bindings/media/video-bus-switch.txt new file +9 -0

+
+This is a binding for a gpio controlled switch for camera interfaces. Such a
+device is used on some embedded devices to connect two cameras to the same
+interface of a image signal processor.
+
+Required properties
+===================
+
+compatible	: must contain "video-bus-switch"

video-bus-gpio-mux

Documentation/devicetree/bindings/media/video-bus-switch.txt line 12

Quoted 1 line

Documentation/devicetree/bindings/media/video-bus-switch.txt new file +1 -0
+switch-gpios	: GPIO specifier for the gpio, which can toggle the

mux-gpios to align with existing GPIO controlled muxes.

Quoted 4 lines

Documentation/devicetree/bindings/media/video-bus-switch.txt new file +4 -0
+		  selected camera. The GPIO should be configured, so
+		  that a disabled GPIO means, that the first port is
+		  selected.
+

Documentation/devicetree/bindings/media/video-bus-switch.txt line 26

Quoted 10 lines

Documentation/devicetree/bindings/media/video-bus-switch.txt new file +10 -0

+Required Port nodes
+===================
+
+More documentation on these bindings is available in
+video-interfaces.txt in the same directory.
+
+reg		: The interface:
+		  0 - port for image signal processor
+		  1 - port for first camera sensor
+		  2 - port for second camera sensor

This could be used for display side as well. So describe these just as inputs and outputs.

Rob

Pavel Machek

author

2017-02-08 22:30 UTC #81 lore raw

Re: [PATCH] devicetree: Add video bus switch

Quoted 13 lines · Rob Herring

Quoted 12 lines · Pavel Machek
N900 contains front and back camera, with a switch between the two. This adds support for the switch component, and it is now possible to select between front and back cameras during runtime.
This adds documentation for the devicetree binding.
Signed-off-by Sebastian Reichel <[email protected]>
Signed-off-by Ivaylo Dimitrov <[email protected]>
Signed-off-by Pavel Machek <[email protected]>

Quoted 10 lines

Documentation/devicetree/bindings/media/video-bus-switch.txt line 2
Quoted 8 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +2 -0
new file mode 100644
index 0000000..1b9f8e0
@@ -0,0 +1,63 @@
+Video Bus Switch Binding
+========================
I'd call it a mux rather than switch.

It is a switch, not a multiplexor (
https://en.wikipedia.org/wiki/Multiplexing ). Only one camera can operate at a time.

Quoted 3 lines

BTW, there's a new mux-controller binding under review you might look at. It would only be needed here if the mux ctrl also controls other things.

Do you have a pointer?

Quoted 5 lines

Quoted 5 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +5 -0
+Required Port nodes
+===================
+
+More documentation on these bindings is available in
+video-interfaces.txt in the same directory.

Quoted 8 lines

Documentation/devicetree/bindings/media/video-bus-switch.txt line 26
Quoted 5 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +5 -0
+
+reg		: The interface:
+		  0 - port for image signal processor
+		  1 - port for first camera sensor
+		  2 - port for second camera sensor
This could be used for display side as well. So describe these just as inputs and outputs.

I'd prefer not to confuse people. I guess that would be 0 -- output port, 1, 2 -- input ports... But this is media data, are you sure it is good idea to change this?

Pavel

Signature · 2 lines

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

Pavel Machek

author

2017-02-08 22:34 UTC #89 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Rob Herring

Quoted 11 lines

Documentation/devicetree/bindings/media/video-bus-switch.txt line 11
Quoted 9 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +9 -0
+
+This is a binding for a gpio controlled switch for camera interfaces. Such a
+device is used on some embedded devices to connect two cameras to the same
+interface of a image signal processor.
+
+Required properties
+===================
+
+compatible	: must contain "video-bus-switch"
video-bus-gpio-mux

Sakari already asked for rename here. I believe I waited reasonable time, but got no input from you, so I did rename it. Now you decide on different name.

Can we either get timely reactions or less bikeshedding?

Thanks,

Pavel

Signature · 2 lines

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

Rob Herring

maintainer

2017-02-09 22:58 UTC #90 lore raw

Re: [PATCH] devicetree: Add video bus switch

Quoted 12 lines · Pavel Machek

Quoted 11 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt line 11
Quoted 9 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +9 -0
+
+This is a binding for a gpio controlled switch for camera interfaces. Such a
+device is used on some embedded devices to connect two cameras to the same
+interface of a image signal processor.
+
+Required properties
+===================
+
+compatible : must contain "video-bus-switch"
video-bus-gpio-mux

Quoted 5 lines

Sakari already asked for rename here. I believe I waited reasonable time, but got no input from you, so I did rename it. Now you decide on different name.
Can we either get timely reactions or less bikeshedding?

You mean less than 5 days because I don't see any other version of this? But in short, no, you can't.

Rob

Rob Herring

maintainer

2017-02-09 23:02 UTC #82 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Pavel Machek

Quoted 25 lines · Pavel Machek

Quoted 13 lines · Rob Herring
Quoted 12 lines · Pavel Machek
N900 contains front and back camera, with a switch between the two. This adds support for the switch component, and it is now possible to select between front and back cameras during runtime.
This adds documentation for the devicetree binding.
Signed-off-by Sebastian Reichel <[email protected]>
Signed-off-by Ivaylo Dimitrov <[email protected]>
Signed-off-by Pavel Machek <[email protected]>
Quoted 10 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt line 2
Quoted 8 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +2 -0
new file mode 100644
index 0000000..1b9f8e0
@@ -0,0 +1,63 @@
+Video Bus Switch Binding
+========================
I'd call it a mux rather than switch.

Quoted 3 lines

It is a switch, not a multiplexor (
https://en.wikipedia.org/wiki/Multiplexing ). Only one camera can operate at a time.

It's no different than an i2c mux. It's one at a time.

Quoted 6 lines

Quoted 4 lines
BTW, there's a new mux-controller binding under review you might look at. It would only be needed here if the mux ctrl also controls other things.
Do you have a pointer?

Let me Google that for you:

Quoted 15 lines

Quoted 14 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt line 26
Quoted 11 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +10 -0
+Required Port nodes
+===================
+
+More documentation on these bindings is available in
+video-interfaces.txt in the same directory.
+
+reg                : The interface:
+             0 - port for image signal processor
+             1 - port for first camera sensor
+             2 - port for second camera sensor
This could be used for display side as well. So describe these just as inputs and outputs.

Quoted 3 lines

I'd prefer not to confuse people. I guess that would be 0 -- output port, 1, 2 -- input ports... But this is media data, are you sure it is good idea to change this?

And I'd prefer something that can be reused by others.

Quoted 5 lines

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

Rob Herring

maintainer

2017-02-09 23:03 UTC #83 lore raw

Re: [PATCH] devicetree: Add video bus switch

Quoted 33 lines · Rob Herring

Quoted 25 lines · Pavel Machek
Quoted 13 lines · Rob Herring
Quoted 12 lines · Pavel Machek
N900 contains front and back camera, with a switch between the two. This adds support for the switch component, and it is now possible to select between front and back cameras during runtime.
This adds documentation for the devicetree binding.
Signed-off-by Sebastian Reichel <[email protected]>
Signed-off-by Ivaylo Dimitrov <[email protected]>
Signed-off-by Pavel Machek <[email protected]>
Quoted 10 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt line 2
Quoted 8 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +2 -0
new file mode 100644
index 0000000..1b9f8e0
@@ -0,0 +1,63 @@
+Video Bus Switch Binding
+========================
I'd call it a mux rather than switch.
Quoted 3 lines
It is a switch, not a multiplexor (
https://en.wikipedia.org/wiki/Multiplexing ). Only one camera can operate at a time.
It's no different than an i2c mux. It's one at a time.
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
BTW, there's a new mux-controller binding under review you might look at. It would only be needed here if the mux ctrl also controls other things.
Do you have a pointer?
Let me Google that for you:

https://lwn.net/Articles/713971/

Quoted 27 lines

Quoted 16 lines
Quoted 15 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt line 26
Quoted 12 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +10 -0
+Required Port nodes
+===================
+
+More documentation on these bindings is available in
+video-interfaces.txt in the same directory.
+
+reg                : The interface:
+             0 - port for image signal processor
+             1 - port for first camera sensor
+             2 - port for second camera sensor
This could be used for display side as well. So describe these just as inputs and outputs.
Quoted 3 lines
I'd prefer not to confuse people. I guess that would be 0 -- output port, 1, 2 -- input ports... But this is media data, are you sure it is good idea to change this?
And I'd prefer something that can be reused by others.
Quoted 5 lines
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Pavel Machek

author

2017-02-10 19:54 UTC #84 lore raw

Re: [PATCH] devicetree: Add video bus switch

Quoted 12 lines

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt line 2
Quoted 9 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +2 -0
new file mode 100644
index 0000000..1b9f8e0
@@ -0,0 +1,63 @@
+Video Bus Switch Binding
+========================
I'd call it a mux rather than switch.

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
It is a switch, not a multiplexor (
https://en.wikipedia.org/wiki/Multiplexing ). Only one camera can operate at a time.
It's no different than an i2c mux. It's one at a time.

Take a look at the wikipedia. If you do "one at a time" at 100Hz, you can claim it is time-domain multiplex. But we are plain switching the cameras. It takes second (or so) to setup the pipeline.

This is not multiplex.

Pavel

Signature · 2 lines

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

Pavel Machek

author

2017-02-10 21:17 UTC #91 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Rob Herring

Quoted 13 lines · Rob Herring

Quoted 12 lines · Pavel Machek
Quoted 11 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt line 11
Quoted 9 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +9 -0
+
+This is a binding for a gpio controlled switch for camera interfaces. Such a
+device is used on some embedded devices to connect two cameras to the same
+interface of a image signal processor.
+
+Required properties
+===================
+
+compatible : must contain "video-bus-switch"
video-bus-gpio-mux

Quoted 8 lines

Quoted 5 lines
Sakari already asked for rename here. I believe I waited reasonable time, but got no input from you, so I did rename it. Now you decide on different name.
Can we either get timely reactions or less bikeshedding?
You mean less than 5 days because I don't see any other version of this? But in short, no, you can't.

Could we switch device tree bindings from "cc: subsystem, to: device tree" to "to: subsystem, cc: device tree" mode? Currently it takes more effort to merge the device tree parts than the relevant driver, and that is not quite good.

Best regards,
Pavel

Signature · 2 lines

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

Sakari Ailus

maintainer

2017-02-10 22:17 UTC #85 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Pavel Machek

Hi Pavel,

Quoted 16 lines · Pavel Machek

Quoted 16 lines
Quoted 16 lines
Quoted 15 lines
Quoted 11 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt line 2
Quoted 9 lines
Documentation/devicetree/bindings/media/video-bus-switch.txt new file +2 -0
new file mode 100644
index 0000000..1b9f8e0
@@ -0,0 +1,63 @@
+Video Bus Switch Binding
+========================
I'd call it a mux rather than switch.
It is a switch, not a multiplexor (
https://en.wikipedia.org/wiki/Multiplexing ). Only one camera can operate at a time.

Quoted 7 lines

Quoted 1 line
Quoted 1 line
It's no different than an i2c mux. It's one at a time.
Take a look at the wikipedia. If you do "one at a time" at 100Hz, you can claim it is time-domain multiplex. But we are plain switching the cameras. It takes second (or so) to setup the pipeline.
This is not multiplex.

The functionality is still the same, isn't it? Does it change what it is if the frequency might be 100 Hz or 0,01 Hz?

I was a bit annoyed for having to have two drivers for switching the source (one for GPIO, another for syscon / register), where both of the drivers would be essentially the same with the minor exception of having a slightly different means to toggle the mux setting.

The MUX framework adds an API for controlling the MUX. Thus we'll need only a single driver that uses the MUX framework API for V4L2. As an added bonus, V4L2 would be in line with the rest of the MUX usage in the kernel.

The set appears to already contain a GPIO MUX. What's needed would be to use the MUX API instead of direct GPIOs usage.

Signature · 8 lines

Kind regards,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pavel Machek

author

2017-02-13 09:54 UTC #86 lore raw

Re: [PATCH] devicetree: Add video bus switch

Hi!

Quoted 8 lines

Quoted 5 lines
Take a look at the wikipedia. If you do "one at a time" at 100Hz, you can claim it is time-domain multiplex. But we are plain switching the cameras. It takes second (or so) to setup the pipeline.
This is not multiplex.
The functionality is still the same, isn't it? Does it change what it is if the frequency might be 100 Hz or 0,01 Hz?

Well. In your living your you can have a switch, which is switch at much less than 0.01Hz. You can also have a dimmer, which is a PWM, which is switch at 100Hz or so. So yes, I'd say switch and mux are different things.

Quoted 4 lines

I was a bit annoyed for having to have two drivers for switching the source (one for GPIO, another for syscon / register), where both of the drivers would be essentially the same with the minor exception of having a slightly different means to toggle the mux setting.

Well, most of the video-bus-switch is the video4linux glue. Actual switching is very very small part. So.. where is the other driver? Looks like we have the same problem.

Quoted 6 lines

The MUX framework adds an API for controlling the MUX. Thus we'll need only a single driver that uses the MUX framework API for V4L2. As an added bonus, V4L2 would be in line with the rest of the MUX usage in the kernel.
The set appears to already contain a GPIO MUX. What's needed would be to use the MUX API instead of direct GPIOs usage.

If there's a driver that already does switching for video4linux devices? Do you have a pointer?

Pavel

Signature · 2 lines

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

Sakari Ailus

maintainer

2017-02-13 10:20 UTC #87 lore raw

Re: [PATCH] devicetree: Add video bus switch

Hi Pavel,

Quoted 8 lines · Pavel Machek

Hi!
Quoted 6 lines
Quoted 5 lines
Take a look at the wikipedia. If you do "one at a time" at 100Hz, you can claim it is time-domain multiplex. But we are plain switching the cameras. It takes second (or so) to setup the pipeline.
This is not multiplex.

Quoted 7 lines

Quoted 2 lines
The functionality is still the same, isn't it? Does it change what it is if the frequency might be 100 Hz or 0,01 Hz?
Well. In your living your you can have a switch, which is switch at much less than 0.01Hz. You can also have a dimmer, which is a PWM, which is switch at 100Hz or so. So yes, I'd say switch and mux are different things.

Light switches are mostly on/off switches. It'd be interesting to have a light switch that you could use to light either of the light bulbs in a room but not to switch both of them on at the same time. Or off... :-)

I wonder if everyone would be happy with a statement saying that it's a on / on switch which is used to implement a multiplexer?

Quoted 9 lines

Quoted 5 lines
I was a bit annoyed for having to have two drivers for switching the source (one for GPIO, another for syscon / register), where both of the drivers would be essentially the same with the minor exception of having a slightly different means to toggle the mux setting.
Well, most of the video-bus-switch is the video4linux glue. Actual switching is very very small part. So.. where is the other driver? Looks like we have the same problem.

It's here, slightly hidden in plain sight in the same patch with the MUX framework:

<URL:https://www.mail-archive.com/[email protected]/msg1328763.html>

Quoted 10 lines

Quoted 7 lines
The MUX framework adds an API for controlling the MUX. Thus we'll need only a single driver that uses the MUX framework API for V4L2. As an added bonus, V4L2 would be in line with the rest of the MUX usage in the kernel.
The set appears to already contain a GPIO MUX. What's needed would be to use the MUX API instead of direct GPIOs usage.
If there's a driver that already does switching for video4linux devices? Do you have a pointer?

I don't think there's one. But with MUX API, we'll be fine using a single driver instead of two (other one for syscon on iMX).

Cc Steve and Philipp.

Signature · 8 lines

Kind regards,

Sakari Ailus
e-mail: [email protected]	XMPP: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pavel Machek

author

2017-03-02 08:54 UTC #88 lore raw

Re: [PATCH] devicetree: Add video bus switch

Quoted 19 lines · Sakari Ailus

Hi Pavel,
Quoted 8 lines · Pavel Machek
Hi!
Quoted 6 lines
Quoted 5 lines
Take a look at the wikipedia. If you do "one at a time" at 100Hz, you can claim it is time-domain multiplex. But we are plain switching the cameras. It takes second (or so) to setup the pipeline.
This is not multiplex.
Quoted 7 lines
Quoted 2 lines
The functionality is still the same, isn't it? Does it change what it is if the frequency might be 100 Hz or 0,01 Hz?
Well. In your living your you can have a switch, which is switch at much less than 0.01Hz. You can also have a dimmer, which is a PWM, which is switch at 100Hz or so. So yes, I'd say switch and mux are different things.

Quoted 6 lines

Light switches are mostly on/off switches. It'd be interesting to have a light switch that you could use to light either of the light bulbs in a room but not to switch both of them on at the same time. Or off... :-)
I wonder if everyone would be happy with a statement saying that it's a on / on switch which is used to implement a multiplexer?

I believe the difference is the timescale. If it switches "slow" it is a switch. If it switches fast, it is a dimmer, mux, or something....

Anyway, someone else was faster, so they get to name their creation...

Pavel

Signature · 2 lines

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

Sakari Ailus

maintainer

2017-12-19 15:43 UTC #72 lore raw

Re: [PATCH] media: add operation to get configuration of "the other side" of the link

In reply to Pavel Machek

Hi Pavel,

Quoted 35 lines · Pavel Machek

Normally, link configuration can be determined at probe time... but Nokia N900 has two cameras, and can switch between them at runtime, so that mechanism is not suitable here.

Add a hook that tells us link configuration.

Signed-off-by Pavel Machek <[email protected]>

include/media/v4l2-subdev.h +3 -0

index cf778c5..74148b9 100644
@@ -25,6 +25,7 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-mediabus.h>
+#include <media/v4l2-of.h>
 
 /* generic v4l2_device notify callback notification values */
 #define V4L2_SUBDEV_IR_RX_NOTIFY		_IOW('v', 0, u32)

@@ -383,6 +384,8 @@ struct v4l2_mbus_frame_desc {
  * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev
  *	can adjust @size to a lower value and must not write more data to the
  *	buffer starting at @data than the original value of @size.
+ *
+ * @g_endpoint_config: get link configuration required by this device.
  */
 struct v4l2_subdev_video_ops {
 	int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config);

include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 425

Quoted 8 lines

include/media/v4l2-subdev.h +2 -0

@@ -415,6 +418,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);
 };
 
 /**

I think Laurent has a board that has a similar issue.

I'd like to address such issues in conjunction with the CSI-2 virtual channel and data type support, with the patches in the vc branch here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=vc>

V4L2 OF (or fwnode) endpoint alone doesn't contain all the related information, and it'd be nice if the solution was indeed independent of OF (or fwnode).

Niklas has been working on more driver support for this so we're getting closer to having these merged.

Signature · 4 lines

Kind regards,

Sakari Ailus
e-mail: [email protected]

Laurent Pinchart

maintainer

2017-12-20 17:54 UTC #73 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Pavel Machek

Hi Pavel,

Quoted 51 lines · On Saturday, 4 February 2017 23:56:10 EET Pavel Machek wrote:

Hi!
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+Required properties
+===================
+
+compatible	: must contain "video-bus-switch"
How generic is this? Should we have e.g. nokia,video-bus-switch? And if so, change the file name accordingly.
Quoted 4 lines
Quoted 2 lines
Generic for "single GPIO controls the switch", AFAICT. But that should be common enough...
Um, yes. Then... how about: video-bus-switch-gpio? No Nokia prefix.
Ok, done. I also fixed the english a bit.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+reg		: The interface:
+		  0 - port for image signal processor
+		  1 - port for first camera sensor
+		  2 - port for second camera sensor
I'd say this must be pretty much specific to the one in N900. You could have more ports. Or you could say that ports beyond 0 are camera sensors. I guess this is good enough for now though, it can be changed later on with the source if a need arises.
Quoted 8 lines
Quoted 3 lines
Well, I'd say that selecting between two sensors is going to be the common case. If someone needs more than two, it will no longer be simple GPIO, so we'll have some fixing to do.
It could be two GPIOs --- that's how the GPIO I2C mux works.
But I'd be surprised if someone ever uses something like that again. ;-)
I'd say.. lets handle that when we see hardware like that.
Quoted 13 lines
Quoted 9 lines
Quoted 4 lines
Btw. was it still considered a problem that the endpoint properties for the sensors can be different? With the g_routing() pad op which is to be added, the ISP driver (should actually go to a framework somewhere) could parse the graph and find the proper endpoint there.
I don't know about g_routing. I added g_endpoint_config method that passes the configuration, and that seems to work for me.
I don't see g_routing in next-20170201 . Is there place to look?
I think there was a patch by Laurent to LMML quite some time ago. I suppose that set will be repicked soonish.

Quoted 8 lines

Quoted 3 lines
I don't really object using g_endpoint_config() as a temporary solution; I'd like to have Laurent's opinion on that though. Another option is to wait, but we've already waited a looong time (as in total).
Laurent, do you have some input here? We have simple "2 cameras connected to one signal processor" situation here. We need some way of passing endpoint configuration from the sensors through the switch. I did this:

Could you give me a bit more information about the platform you're targeting:
how the switch is connected, what kind of switch it is, and what endpoint configuration data you need ?

Quoted 24 lines

Quoted 6 lines
include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 420
Quoted 6 lines
include/media/v4l2-subdev.h +2 -0
@@ -415,6 +416,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);
Google of g_routing tells me:
9) Highly reconfigurable hardware - Julien Beraud
- 44 sub-devices connected with an interconnect.
- As long as formats match, any sub-device could be connected to any
- other sub-device through a link.
- The result is 44 * 44 links at worst.
- A switch sub-device proposed as the solution to model the
- interconnect. The sub-devices are connected to the switch
- sub-devices through the hardware links that connect to the
- interconnect.
- The switch would be controlled through new IOCTLs S_ROUTING and
- G_ROUTING.
- Patches available:
http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip

Quoted 1 line

but the patches are from 2005. So I guess I'll need some guidance here...

You made me feel very old for a moment. The patches are from 2015 :-)

Quoted 1 line

Quoted 1 line
I'll reply to the other patch containing the code.

Signature · 3 lines

Regards,

Laurent Pinchart

Sakari Ailus

maintainer

2017-12-21 09:05 UTC #74 lore raw

Re: [PATCH] devicetree: Add video bus switch

Quoted 91 lines · Laurent Pinchart

Hi Pavel,
Quoted 51 lines · On Saturday, 4 February 2017 23:56:10 EET Pavel Machek wrote:
Hi!
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+Required properties
+===================
+
+compatible	: must contain "video-bus-switch"
How generic is this? Should we have e.g. nokia,video-bus-switch? And if so, change the file name accordingly.
Quoted 4 lines
Quoted 2 lines
Generic for "single GPIO controls the switch", AFAICT. But that should be common enough...
Um, yes. Then... how about: video-bus-switch-gpio? No Nokia prefix.
Ok, done. I also fixed the english a bit.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+reg		: The interface:
+		  0 - port for image signal processor
+		  1 - port for first camera sensor
+		  2 - port for second camera sensor
I'd say this must be pretty much specific to the one in N900. You could have more ports. Or you could say that ports beyond 0 are camera sensors. I guess this is good enough for now though, it can be changed later on with the source if a need arises.
Quoted 8 lines
Quoted 3 lines
Well, I'd say that selecting between two sensors is going to be the common case. If someone needs more than two, it will no longer be simple GPIO, so we'll have some fixing to do.
It could be two GPIOs --- that's how the GPIO I2C mux works.
But I'd be surprised if someone ever uses something like that again. ;-)
I'd say.. lets handle that when we see hardware like that.
Quoted 13 lines
Quoted 9 lines
Quoted 4 lines
Btw. was it still considered a problem that the endpoint properties for the sensors can be different? With the g_routing() pad op which is to be added, the ISP driver (should actually go to a framework somewhere) could parse the graph and find the proper endpoint there.
I don't know about g_routing. I added g_endpoint_config method that passes the configuration, and that seems to work for me.
I don't see g_routing in next-20170201 . Is there place to look?
I think there was a patch by Laurent to LMML quite some time ago. I suppose that set will be repicked soonish.
Quoted 8 lines
Quoted 3 lines
I don't really object using g_endpoint_config() as a temporary solution; I'd like to have Laurent's opinion on that though. Another option is to wait, but we've already waited a looong time (as in total).
Laurent, do you have some input here? We have simple "2 cameras connected to one signal processor" situation here. We need some way of passing endpoint configuration from the sensors through the switch. I did this:
Could you give me a bit more information about the platform you're targeting:
how the switch is connected, what kind of switch it is, and what endpoint configuration data you need ?
Quoted 24 lines
Quoted 6 lines
include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 420
Quoted 6 lines
include/media/v4l2-subdev.h +2 -0
@@ -415,6 +416,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);
Google of g_routing tells me:
9) Highly reconfigurable hardware - Julien Beraud
- 44 sub-devices connected with an interconnect.
- As long as formats match, any sub-device could be connected to any
- other sub-device through a link.
- The result is 44 * 44 links at worst.
- A switch sub-device proposed as the solution to model the
- interconnect. The sub-devices are connected to the switch
- sub-devices through the hardware links that connect to the
- interconnect.
- The switch would be controlled through new IOCTLs S_ROUTING and
- G_ROUTING.
- Patches available:
http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip

Quoted 3 lines

Quoted 1 line
but the patches are from 2005. So I guess I'll need some guidance here...
You made me feel very old for a moment. The patches are from 2015 :-)

There are up-to-date patches here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=vc>

Signature · 6 lines

Sakari Ailus
e-mail: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ivaylo Dimitrov

2017-12-21 16:36 UTC #75 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Laurent Pinchart

Hi,

Quoted 58 lines · Laurent Pinchart

Hi Pavel,
Quoted 55 lines · On Saturday, 4 February 2017 23:56:10 EET Pavel Machek wrote:
Hi!
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+Required properties
+===================
+
+compatible	: must contain "video-bus-switch"
How generic is this? Should we have e.g. nokia,video-bus-switch? And if so, change the file name accordingly.
Quoted 4 lines
Quoted 2 lines
Generic for "single GPIO controls the switch", AFAICT. But that should be common enough...
Um, yes. Then... how about: video-bus-switch-gpio? No Nokia prefix.
Ok, done. I also fixed the english a bit.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+reg		: The interface:
+		  0 - port for image signal processor
+		  1 - port for first camera sensor
+		  2 - port for second camera sensor
I'd say this must be pretty much specific to the one in N900. You could have more ports. Or you could say that ports beyond 0 are camera sensors. I guess this is good enough for now though, it can be changed later on with the source if a need arises.
Quoted 8 lines
Quoted 3 lines
Well, I'd say that selecting between two sensors is going to be the common case. If someone needs more than two, it will no longer be simple GPIO, so we'll have some fixing to do.
It could be two GPIOs --- that's how the GPIO I2C mux works.
But I'd be surprised if someone ever uses something like that again. ;-)
I'd say.. lets handle that when we see hardware like that.
Quoted 16 lines
Quoted 9 lines
Quoted 4 lines
Btw. was it still considered a problem that the endpoint properties for the sensors can be different? With the g_routing() pad op which is to be added, the ISP driver (should actually go to a framework somewhere) could parse the graph and find the proper endpoint there.
I don't know about g_routing. I added g_endpoint_config method that passes the configuration, and that seems to work for me.
I don't see g_routing in next-20170201 . Is there place to look?
I think there was a patch by Laurent to LMML quite some time ago. I suppose that set will be repicked soonish.
I don't really object using g_endpoint_config() as a temporary solution; I'd like to have Laurent's opinion on that though. Another option is to wait, but we've already waited a looong time (as in total).

Quoted 7 lines

Quoted 4 lines
Laurent, do you have some input here? We have simple "2 cameras connected to one signal processor" situation here. We need some way of passing endpoint configuration from the sensors through the switch. I did this:
Could you give me a bit more information about the platform you're targeting:
how the switch is connected, what kind of switch it is, and what endpoint

http://plan9.stanleylieber.com/hardware/n900/n900.schematics.pdf, on page 2, see N5801 and N5802.

Quoted 26 lines

configuration data you need ?
Quoted 24 lines
Quoted 6 lines
include/media/v4l2-subdev.h struct v4l2_subdev_video_ops { · line 420
Quoted 6 lines
include/media/v4l2-subdev.h +2 -0
@@ -415,6 +416,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);
Google of g_routing tells me:
9) Highly reconfigurable hardware - Julien Beraud
- 44 sub-devices connected with an interconnect.
- As long as formats match, any sub-device could be connected to any
- other sub-device through a link.
- The result is 44 * 44 links at worst.
- A switch sub-device proposed as the solution to model the
- interconnect. The sub-devices are connected to the switch
- sub-devices through the hardware links that connect to the
- interconnect.
- The switch would be controlled through new IOCTLs S_ROUTING and
- G_ROUTING.
- Patches available:
http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip

Quoted 6 lines

Quoted 1 line
but the patches are from 2005. So I guess I'll need some guidance here...
You made me feel very old for a moment. The patches are from 2015 :-)
Quoted 1 line
Quoted 1 line
I'll reply to the other patch containing the code.

Regards,
Ivo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Pavel Machek

author

2017-12-22 09:24 UTC #76 lore raw

Re: [PATCH] devicetree: Add video bus switch

In reply to Laurent Pinchart

Hi!

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
I don't really object using g_endpoint_config() as a temporary solution; I'd like to have Laurent's opinion on that though. Another option is to wait, but we've already waited a looong time (as in total).

Quoted 8 lines

Quoted 4 lines
Laurent, do you have some input here? We have simple "2 cameras connected to one signal processor" situation here. We need some way of passing endpoint configuration from the sensors through the switch. I did this:
Could you give me a bit more information about the platform you're targeting:
how the switch is connected, what kind of switch it is, and what endpoint configuration data you need ?

Platform is Nokia N900, Ivaylo already gave pointer to schematics.

Switch is controlled using GPIO, and basically there's CSI configuration that would normally be in the device tree, but now we have two CSI configurations to select from...

Quoted 15 lines

Quoted 15 lines
9) Highly reconfigurable hardware - Julien Beraud
- 44 sub-devices connected with an interconnect.
- As long as formats match, any sub-device could be connected to any
- other sub-device through a link.
- The result is 44 * 44 links at worst.
- A switch sub-device proposed as the solution to model the
- interconnect. The sub-devices are connected to the switch
- sub-devices through the hardware links that connect to the
- interconnect.
- The switch would be controlled through new IOCTLs S_ROUTING and
- G_ROUTING.
- Patches available:
http://git.linuxtv.org/cgit.cgi/pinchartl/media.git/log/?h=xilinx-wip

Quoted 3 lines

Quoted 1 line
but the patches are from 2005. So I guess I'll need some guidance here...
You made me feel very old for a moment. The patches are from 2015 :-)

Sorry about that :-).
Pavel

Signature · 2 lines

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