Hi Steve,

On 01/03/2017 10:57 PM, Steve Longerbeam wrote:

Add the core media driver for i.MX SOC.

Signed-off-by: Steve Longerbeam <redacted>
---
 Documentation/devicetree/bindings/media/imx.txt   | 205 +++++

v2 was sent before getting Rob's review comments, but still they
should be addressed in v3.

Also I would suggest to separate device tree binding documentation
change and place it as the first patch in the series, this should
make the following DTS changes valid.

 Documentation/media/v4l-drivers/imx.rst           | 430 ++++++++++
 drivers/staging/media/Kconfig                     |   2 +
 drivers/staging/media/Makefile                    |   1 +
 drivers/staging/media/imx/Kconfig                 |   8 +
 drivers/staging/media/imx/Makefile                |   6 +
 drivers/staging/media/imx/TODO                    |  18 +
 drivers/staging/media/imx/imx-media-common.c      | 985 ++++++++++++++++++++++
 drivers/staging/media/imx/imx-media-dev.c         | 479 +++++++++++
 drivers/staging/media/imx/imx-media-fim.c         | 509 +++++++++++
 drivers/staging/media/imx/imx-media-internal-sd.c | 457 ++++++++++
 drivers/staging/media/imx/imx-media-of.c          | 291 +++++++
 drivers/staging/media/imx/imx-media-of.h          |  25 +
 drivers/staging/media/imx/imx-media.h             | 299 +++++++
 include/media/imx.h                               |  15 +
 include/uapi/Kbuild                               |   1 +
 include/uapi/linux/v4l2-controls.h                |   4 +
 include/uapi/media/Kbuild                         |   2 +
 include/uapi/media/imx.h                          |  30 +

Probably Greg should ack the UAPI changes, you may consider
to split them into a separate patch.

 19 files changed, 3767 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/imx.txt
 create mode 100644 Documentation/media/v4l-drivers/imx.rst
 create mode 100644 drivers/staging/media/imx/Kconfig
 create mode 100644 drivers/staging/media/imx/Makefile
 create mode 100644 drivers/staging/media/imx/TODO
 create mode 100644 drivers/staging/media/imx/imx-media-common.c
 create mode 100644 drivers/staging/media/imx/imx-media-dev.c
 create mode 100644 drivers/staging/media/imx/imx-media-fim.c
 create mode 100644 drivers/staging/media/imx/imx-media-internal-sd.c
 create mode 100644 drivers/staging/media/imx/imx-media-of.c
 create mode 100644 drivers/staging/media/imx/imx-media-of.h
 create mode 100644 drivers/staging/media/imx/imx-media.h
 create mode 100644 include/media/imx.h
 create mode 100644 include/uapi/media/Kbuild
 create mode 100644 include/uapi/media/imx.h

[snip]

+
+struct imx_media_subdev *
+imx_media_find_subdev_by_sd(struct imx_media_dev *imxmd,
+			    struct v4l2_subdev *sd)
+{
+	struct imx_media_subdev *imxsd;
+	int i, ret = -ENODEV;
+
+	for (i = 0; i < imxmd->num_subdevs; i++) {
+		imxsd = &imxmd->subdev[i];
+		if (sd == imxsd->sd) {

This can be simplifed:

...

	if (sd == imxsd->sd)
		return imxsd;
}

return ERR_PTR(-ENODEV);

+			ret = 0;
+			break;
+		}
+	}
+
+	return ret ? ERR_PTR(ret) : imxsd;
+}
+EXPORT_SYMBOL_GPL(imx_media_find_subdev_by_sd);
+
+struct imx_media_subdev *
+imx_media_find_subdev_by_id(struct imx_media_dev *imxmd, u32 grp_id)
+{
+	struct imx_media_subdev *imxsd;
+	int i, ret = -ENODEV;
+
+	for (i = 0; i < imxmd->num_subdevs; i++) {
+		imxsd = &imxmd->subdev[i];
+		if (imxsd->sd && imxsd->sd->grp_id == grp_id) {
+			ret = 0;
+			break;

This can be simplifed:

...

	if (imxsd->sd && imxsd->sd->grp_id == grp_i)
		return imxsd;
}

return ERR_PTR(-ENODEV);

+		}
+	}
+
+	return ret ? ERR_PTR(ret) : imxsd;
+}
+EXPORT_SYMBOL_GPL(imx_media_find_subdev_by_id);
+

[snip]

Please sort out the list of headers alphabetically.

+#include <video/imx-ipu-v3.h>
+#include <media/imx.h>
+#include "imx-media.h"
+#include "imx-media-of.h"
+
+#define DEVICE_NAME "imx-media"

I suppose you don't need this macro.

[snip]

+ */
+static int imx_media_create_links(struct imx_media_dev *imxmd)
+{
+	struct imx_media_subdev *local_sd;
+	struct imx_media_subdev *remote_sd;
+	struct v4l2_subdev *source, *sink;
+	struct imx_media_link *link;
+	struct imx_media_pad *pad;
+	u16 source_pad, sink_pad;
+	int num_pads, i, j, k;
+	int ret = 0;
+
+	for (i = 0; i < imxmd->num_subdevs; i++) {
+		local_sd = &imxmd->subdev[i];
+		num_pads = local_sd->num_sink_pads + local_sd->num_src_pads;
+
+		for (j = 0; j < num_pads; j++) {
+			pad = &local_sd->pad[j];
+
+			for (k = 0; k < pad->num_links; k++) {
+				link = &pad->link[k];
+
+				remote_sd = imx_media_find_async_subdev(
+					imxmd, link->remote_sd_node,
+					link->remote_devname);
+				if (!remote_sd) {
+					v4l2_warn(&imxmd->v4l2_dev,
+						  "%s: no remote for %s:%d\n",
+						  __func__, local_sd->sd->name,
+						  link->local_pad);
+					continue;
+				}
+
+				/* only create the source->sink links */
+				if (pad->pad.flags & MEDIA_PAD_FL_SINK)
+					continue;
+
+				source = local_sd->sd;
+				sink = remote_sd->sd;
+				source_pad = link->local_pad;
+				sink_pad = link->remote_pad;
+
+				v4l2_info(&imxmd->v4l2_dev,
+					  "%s: %s:%d -> %s:%d\n", __func__,
+					  source->name, source_pad,
+					  sink->name, sink_pad);
+
+				ret = media_create_pad_link(&source->entity,
+							    source_pad,
+							    &sink->entity,
+							    sink_pad,
+							    0);
+				if (ret) {
+					v4l2_err(&imxmd->v4l2_dev,
+						 "create_pad_link failed: %d\n",
+						 ret);
+					goto out;

Indentation depth is quite terrific.

+				}
+			}
+		}
+	}
+
+out:
+	return ret;

[snip]

+
+static const struct of_device_id imx_media_dt_ids[] = {
+	{ .compatible = "fsl,imx-media" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_media_dt_ids);
+
+static struct platform_driver imx_media_pdrv = {
+	.probe		= imx_media_probe,
+	.remove		= imx_media_remove,
+	.driver		= {
+		.name	= DEVICE_NAME,
+		.owner	= THIS_MODULE,

Setting of .owner is not needed nowadays IIRC.

This looks clumsy. Include it unconditionally, if needed
do #ifdef's inside the header file.

+#include <media/v4l2-subdev.h>
+#include <media/v4l2-of.h>
+#include <media/v4l2-ctrls.h>

Please sort out the list alphabetically.

+#include <media/imx.h>
+#include "imx-media.h"
+

[snip]

+
+static int of_parse_fim(struct imx_media_fim *fim, struct device_node *np)
+{
+	struct device_node *fim_np;
+	u32 val, tol[2], icap[2];
+	int ret;
+
+	fim_np = of_get_child_by_name(np, "fim");
+	if (!fim_np) {
+		/* set to the default defaults */
+		fim->of_defaults[FIM_CL_ENABLE] = FIM_CL_ENABLE_DEF;
+		fim->of_defaults[FIM_CL_NUM] = FIM_CL_NUM_DEF;
+		fim->of_defaults[FIM_CL_NUM_SKIP] = FIM_CL_NUM_SKIP_DEF;
+		fim->of_defaults[FIM_CL_TOLERANCE_MIN] =
+			FIM_CL_TOLERANCE_MIN_DEF;
+		fim->of_defaults[FIM_CL_TOLERANCE_MAX] =
+			FIM_CL_TOLERANCE_MAX_DEF;
+		fim->icap_channel = -1;
+		return 0;
+	}
+
+	ret = of_property_read_u32(fim_np, "enable", &val);
+	if (ret)
+		val = FIM_CL_ENABLE_DEF;
+	fim->of_defaults[FIM_CL_ENABLE] = val;
+
+	ret = of_property_read_u32(fim_np, "num-avg", &val);
+	if (ret)
+		val = FIM_CL_NUM_DEF;
+	fim->of_defaults[FIM_CL_NUM] = val;
+
+	ret = of_property_read_u32(fim_np, "num-skip", &val);
+	if (ret)
+		val = FIM_CL_NUM_SKIP_DEF;
+	fim->of_defaults[FIM_CL_NUM_SKIP] = val;
+
+	ret = of_property_read_u32_array(fim_np, "tolerance-range", tol, 2);
+	if (ret) {
+		tol[0] = FIM_CL_TOLERANCE_MIN_DEF;
+		tol[1] = FIM_CL_TOLERANCE_MAX_DEF;
+	}
+	fim->of_defaults[FIM_CL_TOLERANCE_MIN] = tol[0];
+	fim->of_defaults[FIM_CL_TOLERANCE_MAX] = tol[1];
+
+	fim->icap_channel = -1;
+	if (IS_ENABLED(CONFIG_IMX_GPT_ICAP)) {
+		ret = of_property_read_u32_array(fim_np,
+						 "input-capture-channel",
+						 icap, 2);
+		if (!ret) {
+			fim->icap_channel = icap[0];
+			fim->icap_flags = icap[1];
+		}

Should you return error otherwise?

+	}
+
+	of_node_put(fim_np);
+	return 0;
+}

[snip]

Please sort out the list alphabetically.

+#include <video/imx-ipu-v3.h>
+#include "imx-media.h"
+
+static int of_add_pad_link(struct imx_media_dev *imxmd,
+			   struct imx_media_pad *pad,
+			   struct device_node *local_sd_node,
+			   struct device_node *remote_sd_node,
+			   int local_pad, int remote_pad)
+{
+	dev_dbg(imxmd->dev, "%s: adding %s:%d -> %s:%d\n", __func__,
+		local_sd_node->name, local_pad,
+		remote_sd_node->name, remote_pad);
+
+	return imx_media_add_pad_link(imxmd, pad, remote_sd_node, NULL,
+				      local_pad, remote_pad);
+}
+
+/* parse inputs property from a sensor node */
+static void of_parse_sensor_inputs(struct imx_media_dev *imxmd,
+				   struct imx_media_subdev *sensor,
+				   struct device_node *sensor_np)
+{
+	struct imx_media_sensor_input *sinput = &sensor->input;
+	int ret, i;
+
+	for (i = 0; i < IMX_MEDIA_MAX_SENSOR_INPUTS; i++) {
+		const char *input_name;
+		u32 val;
+
+		ret = of_property_read_u32_index(sensor_np, "inputs", i, &val);
+		if (ret)
+			break;
+
+		sinput->value[i] = val;
+
+		ret = of_property_read_string_index(sensor_np, "input-names",
+						    i, &input_name);
+		/*
+		 * if input-names not provided, they will be set using
+		 * the subdev name once the sensor is known during
+		 * async bind
+		 */
+		if (!ret)
+			strncpy(sinput->name[i], input_name,
+				sizeof(sinput->name[i]));
+	}
+
+	sinput->num = i;
+
+	/* if no inputs provided just assume a single input */
+	if (sinput->num == 0)
+		sinput->num = 1;
+}
+
+static void of_parse_sensor(struct imx_media_dev *imxmd,
+			    struct imx_media_subdev *sensor,
+			    struct device_node *sensor_np)
+{
+	struct device_node *endpoint;
+
+	of_parse_sensor_inputs(imxmd, sensor, sensor_np);
+
+	endpoint = of_graph_get_next_endpoint(sensor_np, NULL);
+	if (endpoint) {
+		v4l2_of_parse_endpoint(endpoint, &sensor->sensor_ep);
+		of_node_put(endpoint);
+	}
+}
+
+static int of_get_port_count(const struct device_node *np)
+{
+	struct device_node *child;
+	int num = 0;
+
+	/* if this node is itself a port, return 1 */
+	if (of_node_cmp(np->name, "port") == 0)
+		return 1;
+
+	for_each_child_of_node(np, child) {
+		if (of_node_cmp(child->name, "port") == 0)
+			num++;
+	}

Unneeded bracers.

+	return num;
+}
+
+/*
+ * find the remote device node and remote port id (remote pad #)
+ * given local endpoint node
+ */
+static void of_get_remote_pad(struct device_node *epnode,
+			      struct device_node **remote_node,
+			      int *remote_pad)
+{
+	struct device_node *rp, *rpp;
+	struct device_node *remote;
+
+	rp = of_graph_get_remote_port(epnode);
+	rpp = of_graph_get_remote_port_parent(epnode);
+
+	if (of_device_is_compatible(rpp, "fsl,imx6q-ipu")) {
+		/* the remote is one of the CSI ports */
+		remote = rp;
+		*remote_pad = 0;
+		of_node_put(rpp);
+	} else {
+		remote = rpp;
+		of_property_read_u32(rp, "reg", remote_pad);
+		of_node_put(rp);
+	}
+
+	if (!remote || !of_device_is_available(remote)) {
+		of_node_put(remote);
+		*remote_node = NULL;
+	} else {
+		*remote_node = remote;
+	}
+}
+
+static struct imx_media_subdev *
+of_parse_subdev(struct imx_media_dev *imxmd, struct device_node *sd_np,
+		bool is_csi_port)
+{
+	struct imx_media_subdev *imxsd;
+	int i, num_pads, ret;
+
+	if (!of_device_is_available(sd_np)) {
+		dev_dbg(imxmd->dev, "%s: %s not enabled\n", __func__,
+			sd_np->name);
+		return NULL;
+	}
+
+	/* register this subdev with async notifier */
+	imxsd = imx_media_add_async_subdev(imxmd, sd_np, NULL);
+	if (!imxsd)
+		return NULL;
+	if (IS_ERR(imxsd))
+		return imxsd;

if (IS_ERR_OR_NULL(imxsd))
	return imxsd;

+
+	if (is_csi_port) {
+		/*
+		 * the ipu-csi has one sink port and one source port.
+		 * The source port is not represented in the device tree,
+		 * but is described by the internal pads and links later.
+		 */
+		num_pads = 2;
+		imxsd->num_sink_pads = 1;
+	} else if (of_device_is_compatible(sd_np, "fsl,imx-mipi-csi2")) {
+		num_pads = of_get_port_count(sd_np);
+		/* the mipi csi2 receiver has only one sink port */
+		imxsd->num_sink_pads = 1;
+	} else if (of_device_is_compatible(sd_np, "imx-video-mux")) {
+		num_pads = of_get_port_count(sd_np);
+		/* for the video mux, all but the last port are sinks */
+		imxsd->num_sink_pads = num_pads - 1;
+	} else {
+		/* must be a sensor */
+		num_pads = 1;
+		imxsd->num_sink_pads = 0;
+	}
+
+	if (imxsd->num_sink_pads >= num_pads)
+		return ERR_PTR(-EINVAL);
+
+	imxsd->num_src_pads = num_pads - imxsd->num_sink_pads;
+
+	dev_dbg(imxmd->dev, "%s: %s has %d pads (%d sink, %d src)\n",
+		__func__, sd_np->name, num_pads,
+		imxsd->num_sink_pads, imxsd->num_src_pads);
+
+	if (imxsd->num_sink_pads == 0) {
+		/* this might be a sensor */
+		of_parse_sensor(imxmd, imxsd, sd_np);
+	}

Unneeded bracers.

+
+	for (i = 0; i < num_pads; i++) {
+		struct device_node *epnode = NULL, *port, *remote_np;
+		struct imx_media_subdev *remote_imxsd;
+		struct imx_media_pad *pad;
+		int remote_pad;

Too deep indentation, may be move the cycle body into a separate function?

+
+		/* init this pad */
+		pad = &imxsd->pad[i];
+		pad->pad.flags = (i < imxsd->num_sink_pads) ?
+			MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
+
+		if (is_csi_port)
+			port = (i < imxsd->num_sink_pads) ? sd_np : NULL;
+		else
+			port = of_graph_get_port_by_id(sd_np, i);
+		if (!port)
+			continue;
+
+		while ((epnode = of_get_next_child(port, epnode))) {

Please reuse for_each_child_of_node() here.

+			of_get_remote_pad(epnode, &remote_np, &remote_pad);
+			if (!remote_np) {
+				of_node_put(epnode);

Please remove of_node_put() here, of_get_next_child() does it.

+				continue;
+			}
+
+			ret = of_add_pad_link(imxmd, pad, sd_np, remote_np,
+					      i, remote_pad);
+			if (ret) {
+				imxsd = ERR_PTR(ret);
+				break;
+			}
+
+			if (i < imxsd->num_sink_pads) {
+				/* follow sink endpoints upstream */
+				remote_imxsd = of_parse_subdev(imxmd,
+							       remote_np,
+							       false);
+				if (IS_ERR(remote_imxsd)) {
+					imxsd = remote_imxsd;
+					break;
+				}
+			}
+
+			of_node_put(remote_np);
+			of_node_put(epnode);
+		}
+
+		if (port != sd_np)
+			of_node_put(port);
+		if (IS_ERR(imxsd)) {
+			of_node_put(remote_np);
+			of_node_put(epnode);
+			break;
+		}
+	}
+
+	return imxsd;
+}
+
+int imx_media_of_parse(struct imx_media_dev *imxmd,
+		       struct imx_media_subdev *(*csi)[4],
+		       struct device_node *np)
+{
+	struct device_node *csi_np;
+	struct imx_media_subdev *lcsi;

Please swap two lines above to get the reverse christmas tree ordering.

+	u32 ipu_id, csi_id;
+	int i, ret;
+
+	for (i = 0; ; i++) {
+		csi_np = of_parse_phandle(np, "ports", i);
+		if (!csi_np)
+			break;
+
+		lcsi = of_parse_subdev(imxmd, csi_np, true);
+		if (IS_ERR(lcsi)) {
+			ret = PTR_ERR(lcsi);
+			goto err_put;
+		}
+
+		of_property_read_u32(csi_np, "reg", &csi_id);

Not sure if it is safe enough to ignore return value and potentially
left csi_id uninitialized.

+		ipu_id = of_alias_get_id(csi_np->parent, "ipu");
+
+		if (ipu_id > 1 || csi_id > 1) {
+			dev_err(imxmd->dev, "%s: invalid ipu/csi id (%u/%u)\n",
+				__func__, ipu_id, csi_id);
+			ret = -EINVAL;
+			goto err_put;
+		}
+
+		of_node_put(csi_np);

You can put the node right after of_alias_get_id() call, then in case
of error return right from the if block and remove the goto label.

I do believe you should include some headers or add declarations
of "struct imx_media_dev", "struct imx_media_subdev", "struct device_node".

Please insert here an empty line to improve readability.

+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-of.h>

Please sort out the list alphabetically.

+#include <media/videobuf2-dma-contig.h>
+#include <video/imx-ipu-v3.h>
+
+/*
+ * This is somewhat arbitrary, but we need at least:
+ * - 2 camera interface subdevs
+ * - 3 IC subdevs
+ * - 2 CSI subdevs
+ * - 1 mipi-csi2 receiver subdev
+ * - 2 video-mux subdevs
+ * - 3 camera sensor subdevs (2 parallel, 1 mipi-csi2)
+ *
+ * And double the above numbers for quad i.mx!
+ */

[snip]

--
With best wishes,
Vladimir