On Fri, 2017-01-06 at 18:11 -0800, Steve Longerbeam wrote:

This is a media entity subdevice for the i.MX Camera
Serial Interface module.

s/Serial/Sensor/

s/CAMERA/CSI/ ?

+	tristate "i.MX5/6 Camera driver"

i.MX5/6 Camera Sensor Interface driver

src_sd is not used except that its presence marks an enabled input link.
-> could be changed to bool.

+	struct v4l2_ctrl_handler ctrl_hdlr;
+	struct imx_media_fim *fim;
+
+	/* the attached sensor at stream on */
+	struct imx_media_subdev *sensor;
+};
+
+static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev)
+{
+	return container_of(sdev, struct csi_priv, sd);
+}
+
+/* Update the CSI whole sensor and active windows */
+static int csi_setup(struct csi_priv *priv)
+{
+	struct v4l2_mbus_framefmt infmt;
+
+	ipu_csi_set_window(priv->csi, &priv->crop);
+
+	/*
+	 * the ipu-csi doesn't understand ALTERNATE, but it only
+	 * needs to know whether the stream is interlaced, so set
+	 * to INTERLACED if infmt field is ALTERNATE.
+	 */
+	infmt = priv->format_mbus[priv->input_pad];
+	if (infmt.field == V4L2_FIELD_ALTERNATE)
+		infmt.field = V4L2_FIELD_INTERLACED;

That should be SEQ_TB/BT depending on video standard.

+	ipu_csi_init_interface(priv->csi, &priv->sensor_mbus_cfg, &infmt);
+
+	ipu_csi_set_dest(priv->csi, priv->dest);
+
+	ipu_csi_dump(priv->csi);
+
+	return 0;
+}
+
+static int csi_start(struct csi_priv *priv)
+{
+	int ret;
+
+	if (!priv->sensor) {
+		v4l2_err(&priv->sd, "no sensor attached\n");
+		return -EINVAL;
+	}
+
+	ret = csi_setup(priv);
+	if (ret)
+		return ret;
+
+	/* start the frame interval monitor */
+	if (priv->fim) {
+		ret = imx_media_fim_set_stream(priv->fim, priv->sensor, true);
+		if (ret)
+			return ret;
+	}
+
+	ret = ipu_csi_enable(priv->csi);
+	if (ret) {
+		v4l2_err(&priv->sd, "CSI enable error: %d\n", ret);
+		goto fim_off;
+	}
+
+	return 0;
+
+fim_off:
+	if (priv->fim)
+		imx_media_fim_set_stream(priv->fim, priv->sensor, false);
+	return ret;
+}
+
+static void csi_stop(struct csi_priv *priv)
+{
+	/* stop the frame interval monitor */
+	if (priv->fim)
+		imx_media_fim_set_stream(priv->fim, priv->sensor, false);
+
+	ipu_csi_disable(priv->csi);
+}
+
+static int csi_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	int ret = 0;
+
+	if (!priv->src_sd || !priv->sink_sd)
+		return -EPIPE;
+
+	v4l2_info(sd, "stream %s\n", enable ? "ON" : "OFF");

These could be silenced a bit.

[...]

+static int csi_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	int ret = 0;
+
+	v4l2_info(sd, "power %s\n", on ? "ON" : "OFF");
+
+	if (priv->fim && on != priv->power_on)
+		ret = imx_media_fim_set_power(priv->fim, on);
+
+	if (!ret)
+		priv->power_on = on;
+	return ret;
+}

Is this called multiple times? I'd expect a poweron during open and a
poweroff during close, so no need for priv->power_on.

+static int csi_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 csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct v4l2_subdev *remote_sd;
+
+	dev_dbg(priv->dev, "link setup %s -> %s", remote->entity->name,
+		local->entity->name);
+
+	remote_sd = media_entity_to_v4l2_subdev(remote->entity);
+
+	if (local->flags & MEDIA_PAD_FL_SINK) {
+		if (flags & MEDIA_LNK_FL_ENABLED) {
+			if (priv->src_sd)
+				return -EBUSY;
+			priv->src_sd = remote_sd;
+		} else {
+			priv->src_sd = NULL;
+		}
+
+		return 0;
+	}
+
+	if (flags & MEDIA_LNK_FL_ENABLED) {
+		if (priv->sink_sd)
+			return -EBUSY;
+		priv->sink_sd = remote_sd;
+	} else {
+		priv->sink_sd = NULL;
+		return 0;
+	}
+
+	/* set CSI destination */
+	switch (remote_sd->grp_id) {
+	case IMX_MEDIA_GRP_ID_SMFC0:
+	case IMX_MEDIA_GRP_ID_SMFC1:
+	case IMX_MEDIA_GRP_ID_SMFC2:
+	case IMX_MEDIA_GRP_ID_SMFC3:

With removal of the SMFC entities, CSI0 could be fixed to SMFC0 and CSI1
to the SMFC2 channel.

[...]

+static int csi_set_fmt(struct v4l2_subdev *sd,
+		       struct v4l2_subdev_pad_config *cfg,
+		       struct v4l2_subdev_format *sdformat)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct v4l2_mbus_framefmt *infmt, *outfmt;
+	struct v4l2_rect crop;
+	int ret;
+
+	if (sdformat->pad >= CSI_NUM_PADS)
+		return -EINVAL;
+
+	if (priv->stream_on)
+		return -EBUSY;
+
+	infmt = &priv->format_mbus[priv->input_pad];
+	outfmt = &priv->format_mbus[priv->output_pad];
+
+	if (sdformat->pad == priv->output_pad) {
+		sdformat->format.code = infmt->code;
+		sdformat->format.field = infmt->field;
+		crop.left = priv->crop.left;
+		crop.top = priv->crop.top;
+		crop.width = sdformat->format.width;
+		crop.height = sdformat->format.height;
+		ret = csi_try_crop(priv, &crop);
+		if (ret)
+			return ret;
+		sdformat->format.width = crop.width;
+		sdformat->format.height = crop.height;
+	}
+
+	if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {

Should there be some limitations on the format here?

regards
Philipp