media: Add video bus switch

Message ID 20161224152031.GA8420@amd (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Pavel Machek Dec. 24, 2016, 3:20 p.m. UTC
  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 <sre@kernel.org>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
  

Comments

kernel test robot Dec. 24, 2016, 6:35 p.m. UTC | #1
Hi Pavel,

[auto build test WARNING on v4.9-rc8]
[also build test WARNING on next-20161224]
[cannot apply to linuxtv-media/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Pavel-Machek/media-Add-video-bus-switch/20161225-003239
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   make[3]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.
   include/linux/init.h:1: warning: no structured comments found
   include/linux/workqueue.h:392: warning: No description found for parameter '...'
   include/linux/workqueue.h:392: warning: Excess function parameter 'args' description in 'alloc_workqueue'
   include/linux/workqueue.h:413: warning: No description found for parameter '...'
   include/linux/workqueue.h:413: warning: Excess function parameter 'args' description in 'alloc_ordered_workqueue'
   include/linux/kthread.h:26: warning: No description found for parameter '...'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/linux/fence-array.h:61: warning: No description found for parameter 'fence'
   include/sound/core.h:324: warning: No description found for parameter '...'
   include/sound/core.h:335: warning: No description found for parameter '...'
   include/sound/core.h:388: warning: No description found for parameter '...'
   include/media/media-entity.h:1054: warning: No description found for parameter '...'
>> include/media/v4l2-subdev.h:421: warning: No description found for parameter 'g_endpoint_config'
   include/net/mac80211.h:3207: ERROR: Unexpected indentation.
   include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:3212: ERROR: Unexpected indentation.
   include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:1772: ERROR: Unexpected indentation.
   include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1240: ERROR: Unexpected indentation.
   kernel/time/timer.c:1242: ERROR: Unexpected indentation.
   kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:124: ERROR: Unexpected indentation.
   include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
   drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent.
   drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without a blank line; unexpected unindent.
   include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
   WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting
   Documentation/output/media.h.rst:6: WARNING: undefined label: media-ent-f-switch (if the link has no caption the label must precede a section header)

vim +/g_endpoint_config +421 include/media/v4l2-subdev.h

35c3017a Laurent Pinchart       2010-05-05  405  	int (*s_frame_interval)(struct v4l2_subdev *sd,
35c3017a Laurent Pinchart       2010-05-05  406  				struct v4l2_subdev_frame_interval *interval);
b6456c0c Muralidharan Karicheri 2009-11-19  407  	int (*s_dv_timings)(struct v4l2_subdev *sd,
b6456c0c Muralidharan Karicheri 2009-11-19  408  			struct v4l2_dv_timings *timings);
b6456c0c Muralidharan Karicheri 2009-11-19  409  	int (*g_dv_timings)(struct v4l2_subdev *sd,
b6456c0c Muralidharan Karicheri 2009-11-19  410  			struct v4l2_dv_timings *timings);
5d7758ee Hans Verkuil           2012-05-15  411  	int (*query_dv_timings)(struct v4l2_subdev *sd,
5d7758ee Hans Verkuil           2012-05-15  412  			struct v4l2_dv_timings *timings);
91c79530 Guennadi Liakhovetski  2011-07-01  413  	int (*g_mbus_config)(struct v4l2_subdev *sd,
91c79530 Guennadi Liakhovetski  2011-07-01  414  			     struct v4l2_mbus_config *cfg);
91c79530 Guennadi Liakhovetski  2011-07-01  415  	int (*s_mbus_config)(struct v4l2_subdev *sd,
91c79530 Guennadi Liakhovetski  2011-07-01  416  			     const struct v4l2_mbus_config *cfg);
a375e1df Sylwester Nawrocki     2012-09-13  417  	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
a375e1df Sylwester Nawrocki     2012-09-13  418  			   unsigned int *size);
d19af828 Pavel Machek           2016-12-24  419  	int (*g_endpoint_config)(struct v4l2_subdev *sd,
d19af828 Pavel Machek           2016-12-24  420  			    struct v4l2_of_endpoint *cfg);
2a1fcdf0 Hans Verkuil           2008-11-29 @421  };
2a1fcdf0 Hans Verkuil           2008-11-29  422  
5c662984 Mauro Carvalho Chehab  2015-08-22  423  /**
5c662984 Mauro Carvalho Chehab  2015-08-22  424   * struct v4l2_subdev_vbi_ops - Callbacks used when v4l device was opened
5c662984 Mauro Carvalho Chehab  2015-08-22  425   *				  in video mode via the vbi device node.
5c662984 Mauro Carvalho Chehab  2015-08-22  426   *
5c662984 Mauro Carvalho Chehab  2015-08-22  427   *  @decode_vbi_line: video decoders that support sliced VBI need to implement
9ef0b3f3 Mauro Carvalho Chehab  2016-09-09  428   *	this ioctl. Field p of the &struct v4l2_decode_vbi_line is set to the
5c662984 Mauro Carvalho Chehab  2015-08-22  429   *	start of the VBI data that was generated by the decoder. The driver

:::::: The code at line 421 was first introduced by commit
:::::: 2a1fcdf08230522bd5024f91da24aaa6e8d81f59 V4L/DVB (9820): v4l2: add v4l2_device and v4l2_subdev structs to the v4l2 framework.

:::::: TO: Hans Verkuil <hverkuil@xs4all.nl>
:::::: CC: Mauro Carvalho Chehab <mchehab@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
  
Pavel Machek Jan. 12, 2017, 11:17 a.m. UTC | #2
On Sat 2016-12-24 16:20:31, 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.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

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

> diff --git a/Documentation/devicetree/bindings/media/video-bus-switch.txt b/Documentation/devicetree/bindings/media/video-bus-switch.txt
> new file mode 100644
> index 0000000..1b9f8e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-bus-switch.txt
> @@ -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>;
> +			};
> +		};
> +	};
> +};
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index ce4a96f..a4b509e 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -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
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 40b18d1..8eafc27 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -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/
> diff --git a/drivers/media/platform/video-bus-switch.c b/drivers/media/platform/video-bus-switch.c
> new file mode 100644
> index 0000000..6400cfc
> --- /dev/null
> +++ b/drivers/media/platform/video-bus-switch.c
> @@ -0,0 +1,387 @@
> +/*
> + * Generic driver for video bus switches
> + *
> + * Copyright (C) 2015 Sebastian Reichel <sre@kernel.org>
> + * Copyright (C) 2016 Pavel Machek <pavel@ucw.cz>
> + *
> + * 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.
> + *
> + * 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
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/gpio/consumer.h>
> +#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
> +
> +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;
> +}
> +
> +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");
> +
> +	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);
> +
> +	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;
> +
> +	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);
> +		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);
> +}
> +
> +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)
> +{
> +	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;
> +}
> +
> +
> +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 */
> +	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);
> +	v4l2_set_subdevdata(&pdata->subdev, pdata);
> +	pdata->subdev.entity.function = MEDIA_ENT_F_SWITCH;
> +	pdata->subdev.entity.flags |= MEDIA_ENT_F_SWITCH;
> +	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 */
> +	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");
> +
> +	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);
> +	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 <sre@kernel.org>");
> +MODULE_DESCRIPTION("Video Bus Switch");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:video-bus-switch");
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index cf778c5..448dbb5 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -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)
> @@ -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);
>  };
>  
>  /**
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 4890787..94648ab 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -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)
>  
>  #define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_F_OLD_SUBDEV_BASE
>  
> 
>
  
Sakari Ailus Feb. 3, 2017, 10:25 p.m. UTC | #3
Hi Pavel,

On Thu, Jan 12, 2017 at 12:17:31PM +0100, Pavel Machek wrote:
> On Sat 2016-12-24 16:20:31, 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.
> > 
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> 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
> 
> > diff --git a/Documentation/devicetree/bindings/media/video-bus-switch.txt b/Documentation/devicetree/bindings/media/video-bus-switch.txt
> > new file mode 100644
> > index 0000000..1b9f8e0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/video-bus-switch.txt
> > @@ -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>;
> > +			};
> > +		};
> > +	};
> > +};
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index ce4a96f..a4b509e 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -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
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index 40b18d1..8eafc27 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -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/
> > diff --git a/drivers/media/platform/video-bus-switch.c b/drivers/media/platform/video-bus-switch.c
> > new file mode 100644
> > index 0000000..6400cfc
> > --- /dev/null
> > +++ b/drivers/media/platform/video-bus-switch.c
> > @@ -0,0 +1,387 @@
> > +/*
> > + * Generic driver for video bus switches
> > + *
> > + * Copyright (C) 2015 Sebastian Reichel <sre@kernel.org>
> > + * Copyright (C) 2016 Pavel Machek <pavel@ucw.cz>
> > + *
> > + * 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.
> > + *
> > + * 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.

> > +
> > +#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.

> > +#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.

> > +
> > +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;
> > +}
> > +
> > +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. :-)

> > +
> > +	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);
> > +
> > +	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.

> > +
> > +	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);
> > +		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.

> > +}
> > +
> > +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)
> > +{
> > +	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.

> > +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 */
> > +	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. :-)

> > +	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_*

> > +	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 */
> > +	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()?

> > +
> > +	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?

> > +	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 <sre@kernel.org>");
> > +MODULE_DESCRIPTION("Video Bus Switch");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:video-bus-switch");
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index cf778c5..448dbb5 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -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)
> > @@ -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.

> >  };
> >  
> >  /**
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index 4890787..94648ab 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -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.

> >  
> >  #define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_F_OLD_SUBDEV_BASE
> >  
> > 
> > 
>
  
Pavel Machek Feb. 5, 2017, 10:16 p.m. UTC | #4
Hi!

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

> > > + * 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.

Ok.

> > > +#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.)

> > > + * 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.

Ok.

> > > +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 ;-).

> > > +	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.

(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?

> > > +		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.

> > > +}
> > > +
> > > +
> 
> I'd say that's an extra newline.

Not any more.

> > > +	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.

> I'm not sure I like V4L2_SUBDEV_NAME_SIZE in general. It could be even
> removed. But not by this patch. :-)
> 
> > > +	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.


> > > +	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.

> > > +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.

> > >  /* 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.

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

> > > --- a/include/uapi/linux/media.h
> > > +++ b/include/uapi/linux/media.h
> > > @@ -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
  
Sakari Ailus Feb. 5, 2017, 10:44 p.m. UTC | #5
Hi, Pavel!

On Sun, Feb 05, 2017 at 11:16:22PM +0100, Pavel Machek wrote:
> Hi!
> 
> I lost my original reply... so this will be slightly brief.

:-o

> 
> > > > + * 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.
> 
> Ok.
> 
> > > > +#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.)
> 
> > > > + * 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.
> 
> Ok.
> 
> > > > +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 ;-).
> 
> > > > +	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.
> 
> (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.

> 
> 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.

> 
> > > > +		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.
> 
> > > > +}
> > > > +
> > > > +
> > 
> > I'd say that's an extra newline.
> 
> Not any more.
> 
> > > > +	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.
> 
> > I'm not sure I like V4L2_SUBDEV_NAME_SIZE in general. It could be even
> > removed. But not by this patch. :-)
> > 
> > > > +	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.

> 
> 
> > > > +	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.
> 
> > > > +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.

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.

> 
> > > >  /* 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.
> 
> 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.

> 
> > > > --- a/include/uapi/linux/media.h
> > > > +++ b/include/uapi/linux/media.h
> > > > @@ -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
>
  

Patch

diff --git a/Documentation/devicetree/bindings/media/video-bus-switch.txt b/Documentation/devicetree/bindings/media/video-bus-switch.txt
new file mode 100644
index 0000000..1b9f8e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/video-bus-switch.txt
@@ -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>;
+			};
+		};
+	};
+};
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index ce4a96f..a4b509e 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -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
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 40b18d1..8eafc27 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -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/
diff --git a/drivers/media/platform/video-bus-switch.c b/drivers/media/platform/video-bus-switch.c
new file mode 100644
index 0000000..6400cfc
--- /dev/null
+++ b/drivers/media/platform/video-bus-switch.c
@@ -0,0 +1,387 @@ 
+/*
+ * Generic driver for video bus switches
+ *
+ * Copyright (C) 2015 Sebastian Reichel <sre@kernel.org>
+ * Copyright (C) 2016 Pavel Machek <pavel@ucw.cz>
+ *
+ * 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.
+ *
+ * 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
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/gpio/consumer.h>
+#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
+
+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;
+}
+
+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");
+
+	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);
+
+	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;
+
+	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);
+		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);
+}
+
+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)
+{
+	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;
+}
+
+
+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 */
+	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);
+	v4l2_set_subdevdata(&pdata->subdev, pdata);
+	pdata->subdev.entity.function = MEDIA_ENT_F_SWITCH;
+	pdata->subdev.entity.flags |= MEDIA_ENT_F_SWITCH;
+	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 */
+	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");
+
+	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);
+	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 <sre@kernel.org>");
+MODULE_DESCRIPTION("Video Bus Switch");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:video-bus-switch");
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index cf778c5..448dbb5 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -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)
@@ -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);
 };
 
 /**
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 4890787..94648ab 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -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)
 
 #define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN	MEDIA_ENT_F_OLD_SUBDEV_BASE