Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1563615 > unrolled thread

Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-01-20 15:10 +0100
    Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice  driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-01-24 13:10 +0100
      Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-01-25 03:10 +0100

#1563615 — Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> This driver can handle SoC internal and external video bus multiplexers,
> controlled either by register bit fields or by a GPIO. The subdevice
> passes through frame interval and mbus configuration of the active input
> to the output side.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> --
> 
> - fixed a cut&paste error in vidsw_remove(): v4l2_async_register_subdev()
>   should be unregister.
> 
> - added media_entity_cleanup() and v4l2_device_unregister_subdev()
>   to vidsw_remove().
> 
> - there was a line left over from a previous iteration that negated
>   the new way of determining the pad count just before it which
>   has been removed (num_pads = of_get_child_count(np)).
> 
> - Philipp Zabel has developed a set of patches that allow adding
>   to the subdev async notifier waiting list using a chaining method
>   from the async registered callbacks (v4l2_of_subdev_registered()
>   and the prep patches for that). For now, I've removed the use of
>   v4l2_of_subdev_registered() for the vidmux driver's registered
>   callback. This doesn't affect the functionality of this driver,
>   but allows for it to be merged now, before adding the chaining
>   support.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  .../bindings/media/video-multiplexer.txt           |  59 +++
>  drivers/media/platform/Kconfig                     |   8 +
>  drivers/media/platform/Makefile                    |   2 +
>  drivers/media/platform/video-multiplexer.c         | 472 +++++++++++++++++++++
>  4 files changed, 541 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/video-multiplexer.txt
>  create mode 100644 drivers/media/platform/video-multiplexer.c
> 
> diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> new file mode 100644
> index 0000000..9d133d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> @@ -0,0 +1,59 @@
> +Video Multiplexer
> +=================
> +
> +Video multiplexers allow to select between multiple input ports. Video received
> +on the active input port is passed through to the output port. Muxes described
> +by this binding may be controlled by a syscon register bitfield or by a GPIO.
> +
> +Required properties:
> +- compatible : should be "video-multiplexer"
> +- reg: should be register base of the register containing the control bitfield
> +- bit-mask: bitmask of the control bitfield in the control register
> +- bit-shift: bit offset of the control bitfield in the control register
> +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO phandle
> +  may be given to switch between two inputs
> +- #address-cells: should be <1>
> +- #size-cells: should be <0>
> +- port@*: at least three port nodes containing endpoints connecting to the
> +  source and sink devices according to of_graph bindings. The last port is
> +  the output port, all others are inputs.
> +
> +Example:
> +
> +syscon {
> +	compatible = "syscon", "simple-mfd";
> +
> +	mux {
> +		compatible = "video-multiplexer";
> +		/* Single bit (1 << 19) in syscon register 0x04: */
> +		reg = <0x04>;
> +		bit-mask = <1>;
> +		bit-shift = <19>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			mux_in0: endpoint {
> +				remote-endpoint = <&video_source0_out>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			mux_in1: endpoint {
> +				remote-endpoint = <&video_source1_out>;
> +			};
> +		};
> +
> +		port@2 {
> +			reg = <2>;
> +
> +			mux_out: endpoint {
> +				remote-endpoint = <&capture_interface_in>;
> +			};
> +		};
> +	};
> +};
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index d944421..65614b5 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,14 @@ config VIDEO_M32R_AR_M64278
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called arv.
>  
> +config VIDEO_MULTIPLEXER
> +	tristate "Video Multiplexer"
> +	depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
> +	help
> +	  This driver provides support for SoC internal N:1 video bus
> +	  multiplexers controlled by register bitfields as well as external
> +	  2:1 video multiplexers controlled by a single GPIO.
> +
>  config VIDEO_OMAP3
>  	tristate "OMAP 3 Camera support"
>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 5b3cb27..7cf0ee5 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
>  
>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
>  
> +obj-$(CONFIG_VIDEO_MULTIPLEXER)		+= video-multiplexer.o
> +
>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
> diff --git a/drivers/media/platform/video-multiplexer.c b/drivers/media/platform/video-multiplexer.c
> new file mode 100644
> index 0000000..48980c4
> --- /dev/null
> +++ b/drivers/media/platform/video-multiplexer.c
> @@ -0,0 +1,472 @@
> +/*
> + * video stream multiplexer controlled via gpio or syscon
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * 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.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +
> +struct vidsw {
> +	struct v4l2_subdev subdev;
> +	unsigned int num_pads;
> +	struct media_pad *pads;
> +	struct v4l2_mbus_framefmt *format_mbus;
> +	struct v4l2_fract timeperframe;
> +	struct v4l2_of_endpoint *endpoint;
> +	struct regmap_field *field;
> +	struct gpio_desc *gpio;
> +	int active;
> +};
> +
> +static inline struct vidsw *v4l2_subdev_to_vidsw(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct vidsw, subdev);
> +}
> +
> +static void vidsw_set_active(struct vidsw *vidsw, int active)
> +{
> +	vidsw->active = active;
> +	if (active < 0)
> +		return;
> +
> +	dev_dbg(vidsw->subdev.dev, "setting %d active\n", active);
> +
> +	if (vidsw->field)
> +		regmap_field_write(vidsw->field, active);
> +	else if (vidsw->gpio)
> +		gpiod_set_value(vidsw->gpio, active);
> +}
> +
> +static int vidsw_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 vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> +
> +	/* We have no limitations on enabling or disabling our output link */
> +	if (local->index == vidsw->num_pads - 1)
> +		return 0;
> +
> +	dev_dbg(sd->dev, "link setup %s -> %s", remote->entity->name,
> +		local->entity->name);
> +
> +	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> +		if (local->index == vidsw->active) {
> +			dev_dbg(sd->dev, "going inactive\n");
> +			vidsw->active = -1;
> +		}
> +		return 0;
> +	}
> +
> +	if (vidsw->active >= 0) {
> +		struct media_pad *pad;
> +
> +		if (vidsw->active == local->index)
> +			return 0;
> +
> +		pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
> +		if (pad) {
> +			struct media_link *link;
> +			int ret;
> +
> +			link = media_entity_find_link(pad,
> +						&vidsw->pads[vidsw->active]);
> +			if (link) {
> +				ret = __media_entity_setup_link(link, 0);
> +				if (ret)
> +					return ret;
> +			}
> +		}
> +	}
> +
> +	vidsw_set_active(vidsw, local->index);
> +
> +	return 0;
> +}
> +
> +static struct media_entity_operations vidsw_ops = {
> +	.link_setup = vidsw_link_setup,
> +};
> +
> +static bool vidsw_endpoint_disabled(struct device_node *ep)
> +{
> +	struct device_node *rpp;
> +
> +	if (!of_device_is_available(ep))
> +		return true;
> +
> +	rpp = of_graph_get_remote_port_parent(ep);
> +	if (!rpp)
> +		return true;
> +
> +	return !of_device_is_available(rpp);
> +}
> +
> +static int vidsw_async_init(struct vidsw *vidsw, struct device_node *node)
> +{
> +	struct device_node *ep;
> +	u32 portno;
> +	int numports;
> +	int ret;
> +	int i;
> +	bool active_link = false;
> +
> +	numports = vidsw->num_pads;
> +
> +	for (i = 0; i < numports - 1; i++)
> +		vidsw->pads[i].flags = MEDIA_PAD_FL_SINK;
> +	vidsw->pads[numports - 1].flags = MEDIA_PAD_FL_SOURCE;
> +
> +	vidsw->subdev.entity.function = MEDIA_ENT_F_MUX;
> +	ret = media_entity_pads_init(&vidsw->subdev.entity, numports,
> +				     vidsw->pads);
> +	if (ret < 0)
> +		return ret;
> +
> +	vidsw->subdev.entity.ops = &vidsw_ops;
> +
> +	for_each_endpoint_of_node(node, ep) {
> +		struct v4l2_of_endpoint endpoint;
> +
> +		v4l2_of_parse_endpoint(ep, &endpoint);
> +
> +		portno = endpoint.base.port;
> +		if (portno >= numports - 1)
> +			continue;
> +
> +		if (vidsw_endpoint_disabled(ep)) {
> +			dev_dbg(vidsw->subdev.dev, "port %d disabled\n", portno);
> +			continue;
> +		}
> +
> +		vidsw->endpoint[portno] = endpoint;
> +
> +		if (portno == vidsw->active)
> +			active_link = true;
> +	}
> +
> +	for (portno = 0; portno < numports - 1; portno++) {
> +		if (!vidsw->endpoint[portno].base.local_node)
> +			continue;
> +
> +		/* If the active input is not connected, use another */
> +		if (!active_link) {
> +			vidsw_set_active(vidsw, portno);
> +			active_link = true;
> +		}
> +	}
> +
> +	return v4l2_async_register_subdev(&vidsw->subdev);
> +}
> +
> +int vidsw_g_mbus_config(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg)
> +{
> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> +	struct media_pad *pad;
> +	int ret;
> +
> +	if (vidsw->active == -1) {
> +		dev_err(sd->dev, "no configuration for inactive mux\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Retrieve media bus configuration from the entity connected to the
> +	 * active input
> +	 */
> +	pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
> +	if (pad) {
> +		sd = media_entity_to_v4l2_subdev(pad->entity);
> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> +		if (ret == -ENOIOCTLCMD)
> +			pad = NULL;
> +		else if (ret < 0) {
> +			dev_err(sd->dev, "failed to get source configuration\n");
> +			return ret;
> +		}
> +	}
> +	if (!pad) {
> +		/* Mirror the input side on the output side */
> +		cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
> +		    cfg->type == V4L2_MBUS_BT656)
> +			cfg->flags = vidsw->endpoint[vidsw->active].bus.parallel.flags;
> +	}
> +
> +	return 0;
> +}

I am not certain this op is needed at all. In the current kernel this op is only
used by soc_camera, pxa_camera and omap3isp (somewhat dubious). Normally this
information should come from the device tree and there should be no need for this op.

My (tentative) long-term plan was to get rid of this op.

If you don't need it, then I recommend it is removed.

> +
> +static int vidsw_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> +	struct v4l2_subdev *upstream_sd;
> +	struct media_pad *pad;
> +
> +	if (vidsw->active == -1) {
> +		dev_err(sd->dev, "Can not start streaming on inactive mux\n");
> +		return -EINVAL;
> +	}
> +
> +	pad = media_entity_remote_pad(&sd->entity.pads[vidsw->active]);
> +	if (!pad) {
> +		dev_err(sd->dev, "Failed to find remote source pad\n");
> +		return -ENOLINK;
> +	}
> +
> +	if (!is_media_entity_v4l2_subdev(pad->entity)) {
> +		dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
> +		return -ENODEV;
> +	}
> +
> +	upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> +	return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
> +}
> +
> +static int vidsw_g_frame_interval(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> +
> +	fi->interval = vidsw->timeperframe;
> +
> +	return 0;
> +}
> +
> +static int vidsw_s_frame_interval(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> +
> +	vidsw->timeperframe = fi->interval;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops vidsw_subdev_video_ops = {
> +	.g_mbus_config = vidsw_g_mbus_config,
> +	.s_stream = vidsw_s_stream,
> +	.g_frame_interval = vidsw_g_frame_interval,
> +	.s_frame_interval = vidsw_s_frame_interval,
> +};
> +
> +static struct v4l2_mbus_framefmt *
> +__vidsw_get_pad_format(struct v4l2_subdev *sd,
> +		       struct v4l2_subdev_pad_config *cfg,
> +		       unsigned int pad, u32 which)
> +{
> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> +
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_format(sd, cfg, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &vidsw->format_mbus[pad];
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static int vidsw_get_format(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_pad_config *cfg,
> +			    struct v4l2_subdev_format *sdformat)
> +{
> +	sdformat->format = *__vidsw_get_pad_format(sd, cfg, sdformat->pad,
> +						   sdformat->which);
> +	return 0;
> +}
> +
> +static int vidsw_set_format(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_pad_config *cfg,
> +			    struct v4l2_subdev_format *sdformat)
> +{
> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> +	struct v4l2_mbus_framefmt *mbusformat;
> +
> +	if (sdformat->pad >= vidsw->num_pads)
> +		return -EINVAL;
> +
> +	mbusformat = __vidsw_get_pad_format(sd, cfg, sdformat->pad,
> +					    sdformat->which);
> +	if (!mbusformat)
> +		return -EINVAL;
> +
> +	/* Output pad mirrors active input pad, no limitations on input pads */
> +	if (sdformat->pad == (vidsw->num_pads - 1) && vidsw->active >= 0)
> +		sdformat->format = vidsw->format_mbus[vidsw->active];
> +
> +	*mbusformat = sdformat->format;
> +
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_pad_ops vidsw_pad_ops = {
> +	.get_fmt = vidsw_get_format,
> +	.set_fmt = vidsw_set_format,
> +};
> +
> +static struct v4l2_subdev_ops vidsw_subdev_ops = {
> +	.pad = &vidsw_pad_ops,
> +	.video = &vidsw_subdev_video_ops,
> +};
> +
> +static int of_get_reg_field(struct device_node *node, struct reg_field *field)
> +{
> +	u32 bit_mask;
> +	int ret;
> +
> +	ret = of_property_read_u32(node, "reg", &field->reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = of_property_read_u32(node, "bit-mask", &bit_mask);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = of_property_read_u32(node, "bit-shift", &field->lsb);
> +	if (ret < 0)
> +		return ret;
> +
> +	field->msb = field->lsb + fls(bit_mask) - 1;
> +
> +	return 0;
> +}
> +
> +static int vidsw_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct of_endpoint endpoint;
> +	struct device_node *ep;
> +	struct reg_field field;
> +	struct vidsw *vidsw;
> +	struct regmap *map;
> +	unsigned int num_pads;
> +	int ret;
> +
> +	vidsw = devm_kzalloc(&pdev->dev, sizeof(*vidsw), GFP_KERNEL);
> +	if (!vidsw)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, vidsw);
> +
> +	v4l2_subdev_init(&vidsw->subdev, &vidsw_subdev_ops);
> +	snprintf(vidsw->subdev.name, sizeof(vidsw->subdev.name), "%s",
> +			np->name);
> +	vidsw->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	vidsw->subdev.dev = &pdev->dev;
> +
> +	/*
> +	 * The largest numbered port is the output port. It determines
> +	 * total number of pads
> +	 */
> +	num_pads = 0;
> +	for_each_endpoint_of_node(np, ep) {
> +		of_graph_parse_endpoint(ep, &endpoint);
> +		num_pads = max(num_pads, endpoint.port + 1);
> +	}
> +
> +	if (num_pads < 2) {
> +		dev_err(&pdev->dev, "Not enough ports %d\n", num_pads);
> +		return -EINVAL;
> +	}
> +
> +	ret = of_get_reg_field(np, &field);
> +	if (ret == 0) {
> +		map = syscon_node_to_regmap(np->parent);
> +		if (!map) {
> +			dev_err(&pdev->dev, "Failed to get syscon register map\n");
> +			return PTR_ERR(map);
> +		}
> +
> +		vidsw->field = devm_regmap_field_alloc(&pdev->dev, map, field);
> +		if (IS_ERR(vidsw->field)) {
> +			dev_err(&pdev->dev, "Failed to allocate regmap field\n");
> +			return PTR_ERR(vidsw->field);
> +		}
> +
> +		regmap_field_read(vidsw->field, &vidsw->active);
> +	} else {
> +		if (num_pads > 3) {
> +			dev_err(&pdev->dev, "Too many ports %d\n", num_pads);
> +			return -EINVAL;
> +		}
> +
> +		vidsw->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
> +		if (IS_ERR(vidsw->gpio)) {
> +			dev_warn(&pdev->dev,
> +				 "could not request control gpio: %d\n", ret);
> +			vidsw->gpio = NULL;
> +		}
> +
> +		vidsw->active = gpiod_get_value(vidsw->gpio) ? 1 : 0;
> +	}
> +
> +	vidsw->num_pads = num_pads;
> +	vidsw->pads = devm_kzalloc(&pdev->dev, sizeof(*vidsw->pads) * num_pads,
> +			GFP_KERNEL);
> +	vidsw->format_mbus = devm_kzalloc(&pdev->dev,
> +			sizeof(*vidsw->format_mbus) * num_pads, GFP_KERNEL);
> +	vidsw->endpoint = devm_kzalloc(&pdev->dev,
> +			sizeof(*vidsw->endpoint) * (num_pads - 1), GFP_KERNEL);
> +
> +	ret = vidsw_async_init(vidsw, np);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int vidsw_remove(struct platform_device *pdev)
> +{
> +	struct vidsw *vidsw = platform_get_drvdata(pdev);
> +	struct v4l2_subdev *sd = &vidsw->subdev;
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_device_unregister_subdev(sd);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id vidsw_dt_ids[] = {
> +	{ .compatible = "video-multiplexer", },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver vidsw_driver = {
> +	.probe		= vidsw_probe,
> +	.remove		= vidsw_remove,
> +	.driver		= {
> +		.of_match_table = vidsw_dt_ids,
> +		.name = "video-multiplexer",
> +	},
> +};
> +
> +module_platform_driver(vidsw_driver);
> +
> +MODULE_DESCRIPTION("video stream multiplexer");
> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
> +MODULE_LICENSE("GPL");
> 

Regards,

	Hans

[toc] | [next] | [standalone]

#1565805 — Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

Hi Hans,

On Fri, 2017-01-20 at 15:03 +0100, Hans Verkuil wrote:
> On 01/07/2017 03:11 AM, Steve Longerbeam wrote:
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > This driver can handle SoC internal and external video bus multiplexers,
> > controlled either by register bit fields or by a GPIO. The subdevice
> > passes through frame interval and mbus configuration of the active input
> > to the output side.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > --
> > 
> > - fixed a cut&paste error in vidsw_remove(): v4l2_async_register_subdev()
> >   should be unregister.
> > 
> > - added media_entity_cleanup() and v4l2_device_unregister_subdev()
> >   to vidsw_remove().
> > 
> > - there was a line left over from a previous iteration that negated
> >   the new way of determining the pad count just before it which
> >   has been removed (num_pads = of_get_child_count(np)).
> > 
> > - Philipp Zabel has developed a set of patches that allow adding
> >   to the subdev async notifier waiting list using a chaining method
> >   from the async registered callbacks (v4l2_of_subdev_registered()
> >   and the prep patches for that). For now, I've removed the use of
> >   v4l2_of_subdev_registered() for the vidmux driver's registered
> >   callback. This doesn't affect the functionality of this driver,
> >   but allows for it to be merged now, before adding the chaining
> >   support.
> > 
> > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > ---
> >  .../bindings/media/video-multiplexer.txt           |  59 +++
> >  drivers/media/platform/Kconfig                     |   8 +
> >  drivers/media/platform/Makefile                    |   2 +
> >  drivers/media/platform/video-multiplexer.c         | 472 +++++++++++++++++++++
> >  4 files changed, 541 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/video-multiplexer.txt
> >  create mode 100644 drivers/media/platform/video-multiplexer.c
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > new file mode 100644
> > index 0000000..9d133d9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > @@ -0,0 +1,59 @@
> > +Video Multiplexer
> > +=================
> > +
> > +Video multiplexers allow to select between multiple input ports. Video received
> > +on the active input port is passed through to the output port. Muxes described
> > +by this binding may be controlled by a syscon register bitfield or by a GPIO.
> > +
> > +Required properties:
> > +- compatible : should be "video-multiplexer"
> > +- reg: should be register base of the register containing the control bitfield
> > +- bit-mask: bitmask of the control bitfield in the control register
> > +- bit-shift: bit offset of the control bitfield in the control register
> > +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO phandle
> > +  may be given to switch between two inputs
> > +- #address-cells: should be <1>
> > +- #size-cells: should be <0>
> > +- port@*: at least three port nodes containing endpoints connecting to the
> > +  source and sink devices according to of_graph bindings. The last port is
> > +  the output port, all others are inputs.
> > +
> > +Example:
> > +
> > +syscon {
> > +	compatible = "syscon", "simple-mfd";
> > +
> > +	mux {
> > +		compatible = "video-multiplexer";
> > +		/* Single bit (1 << 19) in syscon register 0x04: */
> > +		reg = <0x04>;
> > +		bit-mask = <1>;
> > +		bit-shift = <19>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +
> > +			mux_in0: endpoint {
> > +				remote-endpoint = <&video_source0_out>;
> > +			};
> > +		};
> > +
> > +		port@1 {
> > +			reg = <1>;
> > +
> > +			mux_in1: endpoint {
> > +				remote-endpoint = <&video_source1_out>;
> > +			};
> > +		};
> > +
> > +		port@2 {
> > +			reg = <2>;
> > +
> > +			mux_out: endpoint {
> > +				remote-endpoint = <&capture_interface_in>;
> > +			};
> > +		};
> > +	};
> > +};
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index d944421..65614b5 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -74,6 +74,14 @@ config VIDEO_M32R_AR_M64278
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called arv.
> >  
> > +config VIDEO_MULTIPLEXER
> > +	tristate "Video Multiplexer"
> > +	depends on VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER
> > +	help
> > +	  This driver provides support for SoC internal N:1 video bus
> > +	  multiplexers controlled by register bitfields as well as external
> > +	  2:1 video multiplexers controlled by a single GPIO.
> > +
> >  config VIDEO_OMAP3
> >  	tristate "OMAP 3 Camera support"
> >  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index 5b3cb27..7cf0ee5 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
> >  
> >  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
> >  
> > +obj-$(CONFIG_VIDEO_MULTIPLEXER)		+= video-multiplexer.o
> > +
> >  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
> >  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
> >  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
> > diff --git a/drivers/media/platform/video-multiplexer.c b/drivers/media/platform/video-multiplexer.c
> > new file mode 100644
> > index 0000000..48980c4
> > --- /dev/null
> > +++ b/drivers/media/platform/video-multiplexer.c
> > @@ -0,0 +1,472 @@
> > +/*
> > + * video stream multiplexer controlled via gpio or syscon
> > + *
> > + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> > + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + * 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.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <media/v4l2-async.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-subdev.h>
> > +#include <media/v4l2-of.h>
> > +
> > +struct vidsw {
> > +	struct v4l2_subdev subdev;
> > +	unsigned int num_pads;
> > +	struct media_pad *pads;
> > +	struct v4l2_mbus_framefmt *format_mbus;
> > +	struct v4l2_fract timeperframe;
> > +	struct v4l2_of_endpoint *endpoint;
> > +	struct regmap_field *field;
> > +	struct gpio_desc *gpio;
> > +	int active;
> > +};
> > +
> > +static inline struct vidsw *v4l2_subdev_to_vidsw(struct v4l2_subdev *sd)
> > +{
> > +	return container_of(sd, struct vidsw, subdev);
> > +}
> > +
> > +static void vidsw_set_active(struct vidsw *vidsw, int active)
> > +{
> > +	vidsw->active = active;
> > +	if (active < 0)
> > +		return;
> > +
> > +	dev_dbg(vidsw->subdev.dev, "setting %d active\n", active);
> > +
> > +	if (vidsw->field)
> > +		regmap_field_write(vidsw->field, active);
> > +	else if (vidsw->gpio)
> > +		gpiod_set_value(vidsw->gpio, active);
> > +}
> > +
> > +static int vidsw_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 vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> > +
> > +	/* We have no limitations on enabling or disabling our output link */
> > +	if (local->index == vidsw->num_pads - 1)
> > +		return 0;
> > +
> > +	dev_dbg(sd->dev, "link setup %s -> %s", remote->entity->name,
> > +		local->entity->name);
> > +
> > +	if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> > +		if (local->index == vidsw->active) {
> > +			dev_dbg(sd->dev, "going inactive\n");
> > +			vidsw->active = -1;
> > +		}
> > +		return 0;
> > +	}
> > +
> > +	if (vidsw->active >= 0) {
> > +		struct media_pad *pad;
> > +
> > +		if (vidsw->active == local->index)
> > +			return 0;
> > +
> > +		pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
> > +		if (pad) {
> > +			struct media_link *link;
> > +			int ret;
> > +
> > +			link = media_entity_find_link(pad,
> > +						&vidsw->pads[vidsw->active]);
> > +			if (link) {
> > +				ret = __media_entity_setup_link(link, 0);
> > +				if (ret)
> > +					return ret;
> > +			}
> > +		}
> > +	}
> > +
> > +	vidsw_set_active(vidsw, local->index);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct media_entity_operations vidsw_ops = {
> > +	.link_setup = vidsw_link_setup,
> > +};
> > +
> > +static bool vidsw_endpoint_disabled(struct device_node *ep)
> > +{
> > +	struct device_node *rpp;
> > +
> > +	if (!of_device_is_available(ep))
> > +		return true;
> > +
> > +	rpp = of_graph_get_remote_port_parent(ep);
> > +	if (!rpp)
> > +		return true;
> > +
> > +	return !of_device_is_available(rpp);
> > +}
> > +
> > +static int vidsw_async_init(struct vidsw *vidsw, struct device_node *node)
> > +{
> > +	struct device_node *ep;
> > +	u32 portno;
> > +	int numports;
> > +	int ret;
> > +	int i;
> > +	bool active_link = false;
> > +
> > +	numports = vidsw->num_pads;
> > +
> > +	for (i = 0; i < numports - 1; i++)
> > +		vidsw->pads[i].flags = MEDIA_PAD_FL_SINK;
> > +	vidsw->pads[numports - 1].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > +	vidsw->subdev.entity.function = MEDIA_ENT_F_MUX;
> > +	ret = media_entity_pads_init(&vidsw->subdev.entity, numports,
> > +				     vidsw->pads);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	vidsw->subdev.entity.ops = &vidsw_ops;
> > +
> > +	for_each_endpoint_of_node(node, ep) {
> > +		struct v4l2_of_endpoint endpoint;
> > +
> > +		v4l2_of_parse_endpoint(ep, &endpoint);
> > +
> > +		portno = endpoint.base.port;
> > +		if (portno >= numports - 1)
> > +			continue;
> > +
> > +		if (vidsw_endpoint_disabled(ep)) {
> > +			dev_dbg(vidsw->subdev.dev, "port %d disabled\n", portno);
> > +			continue;
> > +		}
> > +
> > +		vidsw->endpoint[portno] = endpoint;
> > +
> > +		if (portno == vidsw->active)
> > +			active_link = true;
> > +	}
> > +
> > +	for (portno = 0; portno < numports - 1; portno++) {
> > +		if (!vidsw->endpoint[portno].base.local_node)
> > +			continue;
> > +
> > +		/* If the active input is not connected, use another */
> > +		if (!active_link) {
> > +			vidsw_set_active(vidsw, portno);
> > +			active_link = true;
> > +		}
> > +	}
> > +
> > +	return v4l2_async_register_subdev(&vidsw->subdev);
> > +}
> > +
> > +int vidsw_g_mbus_config(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg)
> > +{
> > +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> > +	struct media_pad *pad;
> > +	int ret;
> > +
> > +	if (vidsw->active == -1) {
> > +		dev_err(sd->dev, "no configuration for inactive mux\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * Retrieve media bus configuration from the entity connected to the
> > +	 * active input
> > +	 */
> > +	pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
> > +	if (pad) {
> > +		sd = media_entity_to_v4l2_subdev(pad->entity);
> > +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> > +		if (ret == -ENOIOCTLCMD)
> > +			pad = NULL;
> > +		else if (ret < 0) {
> > +			dev_err(sd->dev, "failed to get source configuration\n");
> > +			return ret;
> > +		}
> > +	}
> > +	if (!pad) {
> > +		/* Mirror the input side on the output side */
> > +		cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> > +		if (cfg->type == V4L2_MBUS_PARALLEL ||
> > +		    cfg->type == V4L2_MBUS_BT656)
> > +			cfg->flags = vidsw->endpoint[vidsw->active].bus.parallel.flags;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> I am not certain this op is needed at all. In the current kernel this op is only
> used by soc_camera, pxa_camera and omap3isp (somewhat dubious). Normally this
> information should come from the device tree and there should be no need for this op.
> 
> My (tentative) long-term plan was to get rid of this op.
> 
> If you don't need it, then I recommend it is removed.

We currently use this to make the CSI capture interface understand
whether its source from the MIPI CSI-2 or from the parallel bus. That is
probably something that should be fixed, but I'm not quite sure how.

The Synopsys DesignWare MIPI CSI-2 reciever turns the incoming MIPI
CSI-2 signal into a 32-bit parallel pixel bus plus some signals for the
MIPI specific metadata (virtual channel, data type).

Then the CSI2IPU gasket turns this input bus into four separate parallel
16-bit pixel buses plus an 8-bit "mct_di" bus for each of them, that
carries the MIPI metadata. The incoming data is split into the four
outputs according to the MIPI virtual channel.

Two of these 16-bit + 8-bit parallel buses are routed through a
multiplexer before finally arriving at the CSI on the other side.

We need to configure the CSI to either use or ignore the data from the
8-bit mct_di bus depending on whether the source of the mux is
configured to the MIPI CSI-2 receiver / CSI2IPU gasket, or to a parallel
input.

Currently we let g_mbus_config pretend that even the internal 32-bit +
metadata and 16-bit + 8-bit metadata parallel buses are of type
V4L2_MBUS_CSI so that the CSI can ask the mux, which propagates to the
CSI-2 receiver, if connected.

Without g_mbus_config we'd need to get that information from somewhere
else. One possibility would be to extend MEDIA_BUS formats to describe
these "parallelized MIPI data" buses separately.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1566275

On 01/24/2017 04:02 AM, Philipp Zabel wrote:
> Hi Hans,
>
> On Fri, 2017-01-20 at 15:03 +0100, Hans Verkuil wrote:
>> <snip>
>>> +
>>> +int vidsw_g_mbus_config(struct v4l2_subdev *sd, struct v4l2_mbus_config *cfg)
>>> +{
>>> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
>>> +	struct media_pad *pad;
>>> +	int ret;
>>> +
>>> +	if (vidsw->active == -1) {
>>> +		dev_err(sd->dev, "no configuration for inactive mux\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Retrieve media bus configuration from the entity connected to the
>>> +	 * active input
>>> +	 */
>>> +	pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
>>> +	if (pad) {
>>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
>>> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
>>> +		if (ret == -ENOIOCTLCMD)
>>> +			pad = NULL;
>>> +		else if (ret < 0) {
>>> +			dev_err(sd->dev, "failed to get source configuration\n");
>>> +			return ret;
>>> +		}
>>> +	}
>>> +	if (!pad) {
>>> +		/* Mirror the input side on the output side */
>>> +		cfg->type = vidsw->endpoint[vidsw->active].bus_type;
>>> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
>>> +		    cfg->type == V4L2_MBUS_BT656)
>>> +			cfg->flags = vidsw->endpoint[vidsw->active].bus.parallel.flags;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>> I am not certain this op is needed at all. In the current kernel this op is only
>> used by soc_camera, pxa_camera and omap3isp (somewhat dubious). Normally this
>> information should come from the device tree and there should be no need for this op.
>>
>> My (tentative) long-term plan was to get rid of this op.
>>
>> If you don't need it, then I recommend it is removed.

Hi Hans, the imx-media driver was only calling g_mbus_config to the camera
sensor, and it was doing that to determine the sensor's bus type. This info
was already available from parsing a v4l2_of_endpoint from the sensor node.
So it was simple to remove the g_mbus_config calls, and instead rely on the
parsed sensor v4l2_of_endpoint.

> We currently use this to make the CSI capture interface understand
> whether its source from the MIPI CSI-2 or from the parallel bus. That is
> probably something that should be fixed, but I'm not quite sure how.
>
> The Synopsys DesignWare MIPI CSI-2 reciever turns the incoming MIPI
> CSI-2 signal into a 32-bit parallel pixel bus plus some signals for the
> MIPI specific metadata (virtual channel, data type).
>
> Then the CSI2IPU gasket turns this input bus into four separate parallel
> 16-bit pixel buses plus an 8-bit "mct_di" bus for each of them, that
> carries the MIPI metadata. The incoming data is split into the four
> outputs according to the MIPI virtual channel.
>
> Two of these 16-bit + 8-bit parallel buses are routed through a
> multiplexer before finally arriving at the CSI on the other side.
>
> We need to configure the CSI to either use or ignore the data from the
> 8-bit mct_di bus depending on whether the source of the mux is
> configured to the MIPI CSI-2 receiver / CSI2IPU gasket, or to a parallel
> input.

Philipp, from my experience, the CSI_MIPI_DI register (configured
by ipu_csi_set_mipi_datatype()) can only be given a virtual channel 0,
otherwise no data is received from the MIPI CSI-2 sensor, regardless
of the virtual channel the sensor is transmitting over.

So it seems the info on the 8-bit mct_di buses generated by the CSI2IPU
gasket are ignored by the CSI's, at least the virtual channel number is
ignored.

For example, if the sensor is transmitting on vc 1, the gasket routes
the sensor data to the parallel bus to CSI1. But if CSI_MIPI_DI on CSI1
is written with vc 1, no data is received.

Steve

>
> Currently we let g_mbus_config pretend that even the internal 32-bit +
> metadata and 16-bit + 8-bit metadata parallel buses are of type
> V4L2_MBUS_CSI so that the CSI can ask the mux, which propagates to the
> CSI-2 receiver, if connected.
>
> Without g_mbus_config we'd need to get that information from somewhere
> else. One possibility would be to extend MEDIA_BUS formats to describe
> these "parallelized MIPI data" buses separately.
>
> regards
> Philipp
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web