On 04/04/2017 05:47 AM, Sakari Ailus wrote:
Hi Steve, Philipp and Pavel,
On Mon, Mar 27, 2017 at 05:40:34PM -0700, Steve Longerbeam wrote:
From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
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.
The MUX framework is already in linux-next. Could you use that instead of
adding new driver + bindings that are not compliant with the MUX framework?
I don't think it'd be much of a change in terms of code, using the MUX
framework appears quite simple.
I would prefer to wait on this, and get what we have merged now so I can
unload all these patches first.
Also this is Philipp's driver, so again I would prefer to get this
merged as-is and then Philipp can address these issues in a future
patch. But I will add my comments below...
In general the driver looks pretty good, especially regarding the user space
API implementation which is important for use with other drivers.
I have some more detailed comments below.
<snip>
+
+struct vidsw {
+ struct v4l2_subdev subdev;
+ unsigned int num_pads;
You could use subdev.entity.num_pads instead of caching the value locally.
Agreed.
+ struct media_pad *pads;
+ struct v4l2_mbus_framefmt *format_mbus;
+ 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);
I wouldn't implicitly disable a link, even if only one can be active at a
given time. No other drivers do that either.
Perhaps returning an error might be a better thing to do: if you're
reconfiguring the pipeline anyway, there are likely issues elsewhere in it.
We could also change the behaviour later to allow implicit changes but we
can't later on go the other way without breaking the user space.
I think this whole if (vidsw->active >= 0) { ... } block should be
removed. This is left-over from the first implementation that tried
to propagate link setup upstream. This is not working yet, so for now
I think this should be removed.
<snip>
+
+static bool vidsw_endpoint_disabled(struct device_node *ep)
+{
+ struct device_node *rpp;
+
+ if (!of_device_is_available(ep))
ep here is the endpoint, whereas the argument to of_device_is_available()
should correspond to the actual device.
Agreed, I think this if statement should be removed, and...
+ return true;
+
+ rpp = of_graph_get_remote_port_parent(ep);
+ if (!rpp)
+ return true;
this if statement can also be removed, since that is
handled automatically by of_device_is_available() below.
+
+ return !of_device_is_available(rpp);
+}
+
+static int vidsw_async_init(struct vidsw *vidsw, struct device_node *node)
I think I'd arrange this closer to probe as it's related to probe directly.
Up to you.
+{
+ 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_VID_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)
We should get rid of g_mbus_config() in the long run, but as we don't have
the alternative (frame descriptors) isn't up to the job yet I guess it's ok.
I don't think we'll have too many users for the video switch right now.
+{
+ 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;
+}
+
+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);
Now that we'll have more than two drivers involved in the same pipeline it
becomes necessary to define the behaviour of s_stream() throughout the
pipeline --- i.e. whose responsibility is it to call s_stream() on the
sub-devices in the pipeline?
In the case of imx-media, the capture device calls set stream on the
whole pipeline in the start_streaming() callback. This subdev call is
actually a NOOP for imx-media, because the upstream entity has already
started streaming. Again I think this should be removed. It also
enforces a stream order that some MC drivers may have a problem with.
For the remaining comments I'll let Philipp respond.
Steve
I can submit a patch for that. I think the way you do it here is good, as it
enables the caller to choose the appropriate behaviour, i.e. start the local
device before or after the upstream sub-device.
+}
+
+static const struct v4l2_subdev_video_ops vidsw_subdev_video_ops = {
+ .g_mbus_config = vidsw_g_mbus_config,
+ .s_stream = vidsw_s_stream,
+};
+
+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;
This check is already performed in v4l2-subdev.c.
+
+ 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 */
Source and sink pads.
+ if (sdformat->pad == (vidsw->num_pads - 1) && vidsw->active >= 0)
I think it'd be cleaner to test for the pad flag instead of the number. Or,
add a macro to obtain the source pad number.
+ 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;
I think the above would look nice in a MUX driver. :-)
+
+ 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;
You can initialise num_pads in variable declaration.
+ for_each_endpoint_of_node(np, ep) {
+ of_graph_parse_endpoint(ep, &endpoint);
+ num_pads = max(num_pads, endpoint.port + 1);
Port numbers come directly from DT.
Shouldn't num_pads be only the number of pads that have links with actual
physical connections? I.e. if a device is disabled, it shouldn't be
counted here.
+ }
+
+ 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);
+
+ return 0;
+}
+
+static const struct of_device_id vidsw_dt_ids[] = {
+ { .compatible = "video-multiplexer", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, vidsw_dt_ids);
+
+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");
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
