Re: [PATCH v5 04/11] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface

Hi Jean-Michel,

here are just some nits

Am 08.02.22 um 16:50 schrieb Jean-Michel Hautbois:

...

+
+static int unicam_sd_enable_streams(struct v4l2_subdev *sd, u32 pad,
+				    u64 streams_mask)
+{
+	struct unicam_device *unicam = sd_to_unicam_device(sd);
+	struct v4l2_subdev_state *state;
+	u32 other_pad, other_stream;
+	int ret;
+
+	if (WARN_ON(streams_mask != 1))
+		return -EINVAL;

Can we print some error message additionally to the stacktrace?

+
+	state = v4l2_subdev_lock_active_state(sd);
+
+	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
+						    &other_pad, &other_stream);
+
+	v4l2_subdev_unlock_state(state);
+
+	if (ret)
+		return ret;
+
+	unicam->sequence = 0;
+
+	dev_dbg(unicam->dev, "Running with %u data lanes\n",
+		unicam->active_data_lanes);
+
+	ret = clk_set_min_rate(unicam->vpu_clock, UNICAM_MIN_VPU_CLOCK_RATE);
+	if (ret) {
+		dev_err(unicam->dev, "failed to set up VPU clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(unicam->vpu_clock);
+	if (ret) {
+		dev_err(unicam->dev, "Failed to enable VPU clock: %d\n", ret);
+		goto err_vpu_clock;
+	}
+
+	ret = clk_set_rate(unicam->clock, 100 * 1000 * 1000);
+	if (ret) {
+		dev_err(unicam->dev, "failed to set up CSI clock\n");
+		goto err_vpu_prepare;
+	}
+
+	ret = clk_prepare_enable(unicam->clock);
+	if (ret) {
+		dev_err(unicam->dev, "Failed to enable CSI clock: %d\n", ret);
+		goto err_vpu_prepare;
+	}
+
+	ret = v4l2_subdev_call(unicam->sensor.subdev, video, enable_streams,
+			       unicam->sensor.pad->index, BIT(other_stream));
+
+	if (ret && ret == -ENOIOCTLCMD)
+		ret = v4l2_subdev_call(unicam->sensor.subdev, video, s_stream, 1);
+
+	if (ret) {
+		dev_err(unicam->dev, "stream on failed in subdev\n");
+		goto err_enable_stream;
+	}
+
+	unicam->subdev.streaming = true;
+
+	return 0;
+
+err_enable_stream:
+	clk_disable_unprepare(unicam->clock);
+err_vpu_prepare:
+	clk_disable_unprepare(unicam->vpu_clock);
+err_vpu_clock:
+	if (clk_set_min_rate(unicam->vpu_clock, 0))
+		dev_err(unicam->dev, "failed to reset the VPU clock\n");
+	return ret;
+}
+
+static int unicam_sd_disable_streams(struct v4l2_subdev *sd, u32 pad,
+				     u64 streams_mask)
+{
+	struct unicam_device *unicam = sd_to_unicam_device(sd);
+	struct v4l2_subdev_state *state;
+	u32 other_pad, other_stream;
+	int ret;
+
+	if (WARN_ON(streams_mask != 1))
+		return -EINVAL;

Can we print some error message additionally to the stacktrace?

+
+	state = v4l2_subdev_lock_active_state(sd);
+
+	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
+						    &other_pad, &other_stream);
+
+	v4l2_subdev_unlock_state(state);
+
+	if (ret) {
+		dev_err(unicam->dev, "disable streams failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = v4l2_subdev_call(unicam->sensor.subdev, video, disable_streams,
+			       unicam->sensor.pad->index, BIT(other_stream));
+
+	if (ret && ret == -ENOIOCTLCMD) {
+		/* The sensor does not support disable streams. */
+		if (unicam->node[UNICAM_IMAGE_NODE].streaming &&
+		    other_pad == unicam->node[UNICAM_METADATA_NODE].pad.index) {
+			/* We won't stop the sensor yet. */
+			unicam->subdev.streaming = false;
+			return 0;
+		}
+		/* There is no more the metadata node, we can stop. */
+		ret = v4l2_subdev_call(unicam->sensor.subdev, video, s_stream, 0);
+	}
+
+	clk_disable_unprepare(unicam->clock);
+	if (clk_set_min_rate(unicam->vpu_clock, 0))
+		dev_err(unicam->dev, "failed to reset the VPU clock\n");
+	clk_disable_unprepare(unicam->vpu_clock);
+
+	unicam->subdev.streaming = false;
+
+	return 0;
+}
+
+static int unicam_subdev_set_pad_format(struct v4l2_subdev *sd,
+					struct v4l2_subdev_state *state,
+					struct v4l2_subdev_format *format)
+{
+	struct unicam_device *unicam = sd_to_unicam_device(sd);
+	struct v4l2_mbus_framefmt *sink_format, *source_format;
+	const struct unicam_fmt *fmtinfo;
+	int ret = 0;
+
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && unicam->subdev.streaming)
+		return -EBUSY;
+
+	/* No transcoding, source and sink formats must match. */
+	if (unicam_sd_pad_is_source(format->pad))
+		return v4l2_subdev_get_fmt(sd, state, format);
+
+	fmtinfo = unicam_find_format_by_code(format->format.code, format->pad);
+	if (!fmtinfo)
+		fmtinfo = &unicam_image_formats[0];
+
+	if (format->pad == UNICAM_SD_PAD_SOURCE_IMAGE) {
+		format->format.width = clamp_t(unsigned int,
+					       format->format.width,
+					       UNICAM_MIN_WIDTH,
+					       UNICAM_MAX_WIDTH);
+		format->format.height = clamp_t(unsigned int,
+						format->format.height,
+						UNICAM_MIN_HEIGHT,
+						UNICAM_MAX_HEIGHT);
+	}
+	format->format.field = V4L2_FIELD_NONE;
+
+	v4l2_subdev_lock_state(state);
+
+	sink_format = v4l2_state_get_stream_format(state, format->pad,
+						   format->stream);
+	source_format = v4l2_subdev_state_get_opposite_stream_format(state,
+								     format->pad,
+								     format->stream);
+	if (!sink_format || !source_format) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	*sink_format = format->format;
+	*source_format = format->format;
+out:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
+static int unicam_subdev_enum_frame_size(struct v4l2_subdev *sd,
+					 struct v4l2_subdev_state *state,
+					 struct v4l2_subdev_frame_size_enum *fse)
+{
+	const struct unicam_fmt *fmtinfo;
+	u32 pad, stream;
+	int ret = -EINVAL;
+
+	if (fse->index > 0)
+		return ret;
+
+	v4l2_subdev_lock_state(state);
+
+	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, fse->pad,
+						    fse->stream, &pad,
+						    &stream);
+	if (ret)
+		goto out;
+
+	if (unicam_sd_pad_is_source(fse->pad)) {
+		/* No transcoding, source and sink formats must match. */
+		struct v4l2_mbus_framefmt *fmt;
+
+		fmt = v4l2_state_get_stream_format(state, pad, stream);
+		if (!fmt)
+			goto out;
+
+		if (fse->code != fmt->code)
+			goto out;
+
+		fse->min_width = fmt->width;
+		fse->max_width = fmt->width;
+		fse->min_height = fmt->height;
+		fse->max_height = fmt->height;
+	} else {
+		fmtinfo = unicam_find_format_by_code(fse->code, pad);
+		if (!fmtinfo)
+			goto out;
+
+		fse->min_width = UNICAM_MIN_WIDTH;
+		fse->max_width = UNICAM_MAX_WIDTH;
+		fse->min_height = UNICAM_MIN_HEIGHT;
+		fse->max_height = UNICAM_MAX_HEIGHT;
+	}
+
+out:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
+}
+
+static const struct v4l2_subdev_video_ops unicam_subdev_video_ops = {
+	.enable_streams	= unicam_sd_enable_streams,
+	.disable_streams = unicam_sd_disable_streams,
+};
+
+static const struct v4l2_subdev_pad_ops unicam_subdev_pad_ops = {
+	.init_cfg		= unicam_subdev_init_cfg,
+	.enum_mbus_code		= unicam_subdev_enum_mbus_code,
+	.get_fmt		= v4l2_subdev_get_fmt,
+	.set_fmt		= unicam_subdev_set_pad_format,
+	.set_routing		= unicam_subdev_set_routing,
+	.enum_frame_size	= unicam_subdev_enum_frame_size,
+};
+
+static const struct v4l2_subdev_ops unicam_subdev_ops = {
+	.video		= &unicam_subdev_video_ops,
+	.pad		= &unicam_subdev_pad_ops,
+};
+
+static const struct media_entity_operations unicam_subdev_media_ops = {
+	.link_validate = v4l2_subdev_link_validate,
+	.has_route = v4l2_subdev_has_route,
+};
+
+static int unicam_init_and_register_subdev(struct unicam_device *unicam)
+{
+	struct v4l2_subdev *sd = &unicam->subdev.sd;
+	int ret;
+
+	v4l2_subdev_init(sd, &unicam_subdev_ops);
+	v4l2_set_subdevdata(sd, unicam);
+	unicam->subdev.sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+	unicam->subdev.sd.entity.ops = &unicam_subdev_media_ops;
+	unicam->subdev.sd.dev = unicam->dev;
+	unicam->subdev.sd.owner = THIS_MODULE;
+	unicam->subdev.sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_MULTIPLEXED;
+	snprintf(unicam->subdev.sd.name, sizeof(unicam->subdev.sd.name), "unicam-subdev");
+
+	unicam->subdev.pads[UNICAM_SD_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+
+	unicam->subdev.pads[UNICAM_SD_PAD_SOURCE_IMAGE].flags = MEDIA_PAD_FL_SOURCE;
+	unicam->subdev.pads[UNICAM_SD_PAD_SOURCE_METADATA].flags = MEDIA_PAD_FL_SOURCE;
+
+	ret = media_entity_pads_init(&sd->entity,
+				     ARRAY_SIZE(unicam->subdev.pads), unicam->subdev.pads);
+	if (ret) {
+		dev_err(unicam->dev, "Could not init media entity");
+		return ret;
+	}
+
+	ret = v4l2_subdev_init_finalize(sd);
+	if (ret) {
+		dev_err(unicam->dev, "Could not finalize init for subdev");
+		return ret;
+	}
+
+	return v4l2_device_register_subdev(&unicam->v4l2_dev, sd);
+}
+
+/* -----------------------------------------------------------------------------
+ * Videobuf2 Queue Operations
+ */
+
+static int unicam_queue_setup(struct vb2_queue *vq,
+			      unsigned int *nbuffers,
+			      unsigned int *nplanes,
+			      unsigned int sizes[],
+			      struct device *alloc_devs[])
+{
+	struct unicam_node *node = vb2_get_drv_priv(vq);
+	struct unicam_device *unicam = node->dev;
+	unsigned int size = is_image_node(node) ?
+				node->v_fmt.fmt.pix.sizeimage :
+				node->v_fmt.fmt.meta.buffersize;
+
+	if (vq->num_buffers + *nbuffers < 3)
+		*nbuffers = 3 - vq->num_buffers;
+
+	if (*nplanes) {
+		if (sizes[0] < size) {
+			dev_dbg(unicam->dev, "sizes[0] %i < size %u\n", sizes[0],
+				size);
+			return -EINVAL;
+		}
+		size = sizes[0];
+	}
+
+	*nplanes = 1;
+	sizes[0] = size;
+
+	return 0;
+}
+
+static int unicam_buffer_prepare(struct vb2_buffer *vb)
+{
+	struct unicam_node *node = vb2_get_drv_priv(vb->vb2_queue);
+	struct unicam_device *unicam = node->dev;
+	struct unicam_buffer *buf = to_unicam_buffer(vb);
+	unsigned long size;
+
+	if (WARN_ON(!node->fmt))
+		return -EINVAL;

Can we print some error message additionally to the stacktrace?

+
+	size = is_image_node(node) ? node->v_fmt.fmt.pix.sizeimage :
+				     node->v_fmt.fmt.meta.buffersize;
+	if (vb2_plane_size(vb, 0) < size) {
+		dev_dbg(unicam->dev, "data will not fit into plane (%lu < %lu)\n",
+			vb2_plane_size(vb, 0), size);
+		return -EINVAL;
+	}
+
+	vb2_set_plane_payload(&buf->vb.vb2_buf, 0, size);
+	return 0;
+}
+

...

+
+static int unicam_connect_of_subdevs(struct unicam_device *unicam)
+{
+	struct v4l2_fwnode_endpoint ep = { };
+	struct fwnode_handle *ep_handle;
+	struct v4l2_async_subdev *asd;
+	unsigned int lane;
+	int ret = -EINVAL;
+
+	if (of_property_read_u32(unicam->dev->of_node, "brcm,num-data-lanes",
+				 &unicam->max_data_lanes) < 0) {
+		dev_err(unicam->dev, "DT property %s not set\n",
+			"brcm,num-data-lanes");
+		return -EINVAL;
+	}
+
+	/* Get the local endpoint and remote device. */
+	ep_handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(unicam->dev),
+						    0, 0,
+						    FWNODE_GRAPH_ENDPOINT_NEXT);
+	if (!ep_handle) {
+		dev_err(unicam->dev, "No endpoint\n");
+		return -ENODEV;
+	}
+
+	/* Parse the local endpoint and validate its configuration. */
+	if (v4l2_fwnode_endpoint_alloc_parse(ep_handle, &ep)) {
+		dev_err(unicam->dev, "could not parse endpoint\n");
+		goto cleanup_exit;
+	}
+
+	dev_dbg(unicam->dev, "parsed local endpoint, bus_type %u\n",
+		ep.bus_type);
+
+	unicam->bus_type = ep.bus_type;
+
+	switch (ep.bus_type) {
+	case V4L2_MBUS_CSI2_DPHY:
+		switch (ep.bus.mipi_csi2.num_data_lanes) {
+		case 1:
+		case 2:
+		case 4:
+			break;
+
+		default:
+			dev_err(unicam->dev, "%u data lanes not supported\n",
+				ep.bus.mipi_csi2.num_data_lanes);
+			goto cleanup_exit;
+		}
+
+		for (lane = 0; lane < ep.bus.mipi_csi2.num_data_lanes; lane++) {
+			if (ep.bus.mipi_csi2.data_lanes[lane] != lane + 1) {
+				dev_err(unicam->dev, "data lanes reordering not supported\n");
+				goto cleanup_exit;
+			}
+		}
+
+		if (ep.bus.mipi_csi2.num_data_lanes > unicam->max_data_lanes) {
+			dev_err(unicam->dev, "endpoint requires %u data lanes when %u are supported\n",
+				ep.bus.mipi_csi2.num_data_lanes,
+				unicam->max_data_lanes);
+		}
+
+		unicam->active_data_lanes = ep.bus.mipi_csi2.num_data_lanes;
+		unicam->bus_flags = ep.bus.mipi_csi2.flags;
+
+		break;
+
+	case V4L2_MBUS_CCP2:
+		if (ep.bus.mipi_csi1.clock_lane != 0 ||
+		    ep.bus.mipi_csi1.data_lane != 1) {
+			dev_err(unicam->dev, "unsupported lanes configuration\n");
+			goto cleanup_exit;
+		}
+
+		unicam->max_data_lanes = 1;
+		unicam->active_data_lanes = 1;
+		unicam->bus_flags = ep.bus.mipi_csi1.strobe;
+		break;
+
+	default:
+		/* Unsupported bus type */
+		dev_err(unicam->dev, "unsupported bus type %u\n",
+			ep.bus_type);
+		goto cleanup_exit;
+	}
+
+	dev_dbg(unicam->dev, "%s bus, %u data lanes, flags=0x%08x\n",
+		unicam->bus_type == V4L2_MBUS_CSI2_DPHY ? "CSI-2" : "CCP2",
+		unicam->active_data_lanes, unicam->bus_flags);
+
+	/* Initialize and register the async notifier. */
+	v4l2_async_nf_init(&unicam->notifier);
+
+	asd = v4l2_async_nf_add_fwnode_remote(&unicam->notifier, ep_handle,
+					      struct v4l2_async_subdev);
+
+	fwnode_handle_put(ep_handle);
+	ep_handle = NULL;
+
+	if (IS_ERR(asd)) {

please provide a log entry here

+		ret = PTR_ERR(asd);
+		goto cleanup_exit;
+	}
+
+	unicam->notifier.ops = &unicam_async_ops;
+
+	ret = v4l2_async_nf_register(&unicam->v4l2_dev, &unicam->notifier);
+	if (ret) {
+		dev_err(unicam->dev, "Error registering device notifier: %d\n", ret);
+		goto cleanup_exit;
+	}
+
+	return 0;
+
+cleanup_exit:
+	v4l2_fwnode_endpoint_free(&ep);
+	fwnode_handle_put(ep_handle);
+
+	return ret;
+}
+
+static int unicam_media_dev_init(struct unicam_device *unicam)
+{
+	int ret = 0;

no need to init ret here

+
+	unicam->mdev.dev = unicam->dev;
+	strscpy(unicam->mdev.model, UNICAM_MODULE_NAME,
+		sizeof(unicam->mdev.model));
+	strscpy(unicam->mdev.serial, "", sizeof(unicam->mdev.serial));
+	snprintf(unicam->mdev.bus_info, sizeof(unicam->mdev.bus_info),
+		 "platform:%s", dev_name(unicam->dev));
+	unicam->mdev.hw_revision = 0;
+
+	media_device_init(&unicam->mdev);
+
+	unicam->v4l2_dev.mdev = &unicam->mdev;
+
+	ret = v4l2_device_register(unicam->dev, &unicam->v4l2_dev);
+	if (ret < 0) {
+		dev_err(unicam->dev,
+			"Unable to register v4l2 device\n");
+		return ret;
+	}
+
+	ret = media_device_register(&unicam->mdev);
+	if (ret < 0) {
+		dev_err(unicam->dev,
+			"Unable to register media-controller device\n");
+		goto err_v4l2_unregister;
+	}
+
+	return 0;
+
+err_v4l2_unregister:
+	v4l2_device_unregister(&unicam->v4l2_dev);
+	return ret;
+}
+
+static int unicam_probe(struct platform_device *pdev)
+{
+	struct unicam_device *unicam;
+	int ret = 0;
+
+	unicam = kzalloc(sizeof(*unicam), GFP_KERNEL);
+	if (!unicam)
+		return -ENOMEM;
+
+	kref_init(&unicam->kref);
+	unicam->dev = &pdev->dev;
+	/* set the driver data in platform device */
+	platform_set_drvdata(pdev, unicam);
+
+	unicam->base = devm_platform_ioremap_resource_byname(pdev, "unicam");
+	if (IS_ERR(unicam->base)) {
+		ret = PTR_ERR(unicam->base);
+		goto err_unicam_put;
+	}
+
+	unicam->clk_gate_base = devm_platform_ioremap_resource_byname(pdev, "cmi");
+	if (IS_ERR(unicam->clk_gate_base)) {
+		ret = PTR_ERR(unicam->clk_gate_base);
+		goto err_unicam_put;
+	}
+
+	unicam->clock = devm_clk_get(&pdev->dev, "lp");
+	if (IS_ERR(unicam->clock)) {
+		dev_err(unicam->dev, "Failed to get lp clock\n");
+		ret = PTR_ERR(unicam->clock);
+		goto err_unicam_put;
+	}
+
+	unicam->vpu_clock = devm_clk_get(&pdev->dev, "vpu");
+	if (IS_ERR(unicam->vpu_clock)) {
+		dev_err(unicam->dev, "Failed to get vpu clock\n");
+		ret = PTR_ERR(unicam->vpu_clock);
+		goto err_unicam_put;
+	}
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret <= 0) {
+		dev_err(&pdev->dev, "No IRQ resource\n");
+		ret = -EINVAL;
+		goto err_unicam_put;
+	}
+
+	ret = devm_request_irq(&pdev->dev, ret, unicam_isr, 0,
+			       "unicam_capture0", unicam);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to request interrupt\n");
+		ret = -EINVAL;
+		goto err_unicam_put;
+	}
+
+	ret = unicam_media_dev_init(unicam);
+	if (ret) {
+		dev_err(unicam->dev,
+			"Unable to initialize media device\n");
+		goto err_unicam_put;
+	}
+
+	ret = unicam_init_and_register_subdev(unicam);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register internal subdev\n");

please drop this unnecessary log entry, because the called function
already provide more helpful log entries.

Best regards

+		goto err_media_unregister;
+	}
+
+	ret = unicam_connect_of_subdevs(unicam);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to connect subdevs\n");
+		goto err_subdev_unregister;
+	}
+
+	/* Enable the block power domain */
+	pm_runtime_enable(&pdev->dev);
+
+	return 0;
+
+err_subdev_unregister:
+	v4l2_subdev_cleanup(&unicam->subdev.sd);
+err_media_unregister:
+	media_device_unregister(&unicam->mdev);
+err_unicam_put:
+	unicam_put(unicam);
+
+	return ret;
+}
+
+static int unicam_remove(struct platform_device *pdev)
+{
+	struct unicam_device *unicam = platform_get_drvdata(pdev);
+
+	unicam_unregister_nodes(unicam);
+	v4l2_device_unregister(&unicam->v4l2_dev);
+	media_device_unregister(&unicam->mdev);
+	v4l2_async_nf_unregister(&unicam->notifier);
+	unicam_put(unicam);
+
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id unicam_of_match[] = {
+	{ .compatible = "brcm,bcm2835-unicam", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, unicam_of_match);
+
+static struct platform_driver unicam_driver = {
+	.probe		= unicam_probe,
+	.remove		= unicam_remove,
+	.driver = {
+		.name	= UNICAM_MODULE_NAME,
+		.of_match_table = of_match_ptr(unicam_of_match),
+	},
+};
+
+module_platform_driver(unicam_driver);
+
+MODULE_AUTHOR("Dave Stevenson [off-list ref]");
+MODULE_DESCRIPTION("BCM2835 Unicam driver");
+MODULE_LICENSE("GPL");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel