Message-ID: <aTB2NNYVSm6POk9y@lizhi-Precision-Tower-5810>
Date: Wed, 3 Dec 2025 12:41:08 -0500
From: Frank Li <Frank.li@....com>
To: Guoniu Zhou <guoniu.zhou@....nxp.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	imx@...ts.linux.dev, linux-media@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, Guoniu Zhou <guoniu.zhou@....com>
Subject: Re: [PATCH 2/2] media: nxp: Add i.MX9 CSI pixel formatter v4l2
 drivery

On Wed, Dec 03, 2025 at 02:30:03PM +0800, Guoniu Zhou wrote:
> From: Guoniu Zhou <guoniu.zhou@....com>
>
> The CSI pixel formatter is a module found on i.MX95 used to reformat
> packet info, pixel and non-pixel data from CSI-2 host controller to
> match Pixel Link(PL) definition.
>
> This driver adds its data formatting support.

Needn't words "this driver", just

   Add data formatting support.

>
> Signed-off-by: Guoniu Zhou <guoniu.zhou@....com>
> ---
>  MAINTAINERS                                     |   8 +
>  drivers/media/platform/nxp/Kconfig              |  14 +
>  drivers/media/platform/nxp/Makefile             |   1 +
>  drivers/media/platform/nxp/imx9-csi-formatter.c | 894 ++++++++++++++++++++++++
>  4 files changed, 917 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4205ca007e076f869613032b51e8cc5bff06b98e..24681a787818c3e69f93dcc220ee9838cef99886 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18626,6 +18626,14 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/media/nxp,imx8-jpeg.yaml
>  F:	drivers/media/platform/nxp/imx-jpeg
>
> +NXP i.MX 9 CSI PIXEL FORMATTER V4L2 DRIVER
> +M:	Guoniu Zhou <guoniu.zhou@....com>
> +L:	imx@...ts.linux.dev
> +L:	linux-media@...r.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/media/nxp,imx9-csi-formatter.yaml
> +F:	drivers/media/platform/nxp/imx9-csi-formatter.c
> +

...
> +
> +#define CSI_FORMATTER_PAD_SINK			0
> +#define CSI_FORMATTER_PAD_SOURCE		1
> +#define CSI_FORMATTER_PAD_NUM			2
> +
> +#define CSI_FORMATTER_DEF_MBUS_CODE		MEDIA_BUS_FMT_UYVY8_1X16
> +#define CSI_FORMATTER_DEF_PIX_WIDTH		1920U
> +#define CSI_FORMATTER_DEF_PIX_HEIGHT		1080U
> +#define CSI_FORMATTER_MAX_PIX_WIDTH		0xffff
> +#define CSI_FORMATTER_MAX_PIX_HEIGHT		0xffff

These macro only use once, you put value direct at formatter_default_fmt.

> +
> +#define CSI_FORMATTER_DRV_NAME			"csi-pixel-formatter"
> +#define CSI_FORMATTER_VC_MAX			8
> +
> +struct formatter_pix_format {
> +	u32 code;
> +	u32 data_type;
> +};
> +
...
> +
> +static int formatter_subdev_get_frame_desc(struct v4l2_subdev *sd,
> +					   unsigned int pad,
> +					   struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +	struct v4l2_mbus_frame_desc csi_fd;
> +	struct v4l2_subdev_route *route;
> +	struct v4l2_subdev_state *state;
> +	int ret;
> +
> +	if (pad != CSI_FORMATTER_PAD_SOURCE)
> +		return -EINVAL;
> +
> +	ret = v4l2_subdev_call(formatter->csi_sd, pad, get_frame_desc,
> +			       formatter->remote_pad, &csi_fd);
> +	if (ret)
> +		return ret;
> +
> +	if (csi_fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> +		dev_err(formatter->dev,
> +			"Frame descriptor does not describe CSI-2 link\n");
> +		return -EINVAL;
> +	}
> +
> +	memset(fd, 0, sizeof(*fd));
> +
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> +	for_each_active_route(&state->routing, route) {
> +		struct v4l2_mbus_frame_desc_entry *entry = NULL;
> +		unsigned int i;
> +
> +		if (route->source_pad != pad)
> +			continue;
> +
> +		for (i = 0; i < csi_fd.num_entries; ++i) {
> +			if (csi_fd.entry[i].stream == route->sink_stream) {
> +				entry = &csi_fd.entry[i];
> +				break;
> +			}
> +		}
> +
> +		if (!entry) {
> +			dev_err(formatter->dev,
> +				"Failed to find stream from source frames desc\n");
> +			ret = -EPIPE;
> +			goto out_unlock;

may 'break' here is better?

> +		}
> +
> +		fd->entry[fd->num_entries].stream = route->source_stream;
> +		fd->entry[fd->num_entries].flags = entry->flags;
> +		fd->entry[fd->num_entries].length = entry->length;
> +		fd->entry[fd->num_entries].pixelcode = entry->pixelcode;
> +		fd->entry[fd->num_entries].bus.csi2.vc = entry->bus.csi2.vc;
> +		fd->entry[fd->num_entries].bus.csi2.dt = entry->bus.csi2.dt;
> +
> +		fd->num_entries++;
> +	}
> +
> +out_unlock:
> +	v4l2_subdev_unlock_state(state);
> +	return ret;
> +}
> +
...
> +
> +static int formatter_subdev_enable_streams(struct v4l2_subdev *sd,
> +					   struct v4l2_subdev_state *state,
> +					   u32 pad, u64 streams_mask)
> +{
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +	struct device *dev = formatter->dev;
> +	u64 sink_streams;
> +	int ret;
> +
> +	if (!formatter->csi_sd) {
> +		dev_err(dev, "CSI controller don't link with formatter\n");
> +		return -EPIPE;
> +	}
> +
> +	if (!formatter->enabled_streams) {
> +		ret = pm_runtime_resume_and_get(formatter->dev);
> +		if (ret < 0) {
> +			dev_err(dev, "Formatter runtime get fail\n");
> +			return ret;
> +		}
> +	}
> +
> +	ret = csi_formatter_start_stream(formatter, streams_mask);
> +	if (ret)
> +		goto runtime_put;

if enable_streams is not 0, and goto runtime_put, it will cause runtime
refer count miss match.

I think you can call runtime_get uncondtional.

	PM_RUNTIME_ACQUIRE(xspi->dev, pm);
	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
	if (ret)
		return ret;

PM_RUNTIME_ACQUIRE already in linux-next, it should be safe to use for
next version.


> +
> +	sink_streams = v4l2_subdev_state_xlate_streams(state,
> +						       CSI_FORMATTER_PAD_SOURCE,
> +						       CSI_FORMATTER_PAD_SINK,
> +						       &streams_mask);
> +
> +	dev_dbg(dev, "remote sd: %s pad: %u, sink_stream:0x%llx\n",
> +		formatter->csi_sd->name, formatter->remote_pad, sink_streams);
> +
> +	ret = v4l2_subdev_enable_streams(formatter->csi_sd, formatter->remote_pad,
> +					 sink_streams);
> +	if (ret)
> +		goto runtime_put;
> +
> +	formatter->enabled_streams |= streams_mask;
> +
> +	return 0;
> +
> +runtime_put:
> +	pm_runtime_put(formatter->dev);
> +	return ret;
> +}
> +
> +static int csi_formatter_stop_stream(struct csi_formatter *formatter,
> +				     u64 stream_mask)
> +{
> +	unsigned int i;
> +	int vc;
> +
> +	for (i = 0; i < V4L2_FRAME_DESC_ENTRY_MAX; ++i) {
> +		if (stream_mask & BIT(i))
> +			break;
> +	}
> +
> +	if (i == V4L2_FRAME_DESC_ENTRY_MAX) {
> +		dev_err(formatter->dev, "Stream ID out of range\n");
> +		return -EINVAL;
> +	}
> +
> +	vc = get_vc(formatter, i);
> +
> +	if (vc < 0 || vc > CSI_FORMATTER_VC_MAX) {
> +		dev_err(formatter->dev, "Invalid virtual channel(%d)\n", vc);
> +		return -EINVAL;
> +	}
> +
> +	formatter_write(formatter, CSI_VCx_PIXEL_DATA_TYPE(vc), 0);
> +
> +	return 0;
> +}
> +
> +static int formatter_subdev_disable_streams(struct v4l2_subdev *sd,
> +					    struct v4l2_subdev_state *state,
> +					    u32 pad, u64 streams_mask)
> +{
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +	u64 sink_streams;
> +	int ret;
> +
> +	sink_streams = v4l2_subdev_state_xlate_streams(state,
> +						       CSI_FORMATTER_PAD_SOURCE,
> +						       CSI_FORMATTER_PAD_SINK,
> +						       &streams_mask);
> +
> +	ret = v4l2_subdev_disable_streams(formatter->csi_sd, formatter->remote_pad,
> +					  sink_streams);
> +	if (ret)
> +		return ret;
> +
> +	ret = csi_formatter_stop_stream(formatter, streams_mask);
> +	if (ret)
> +		return ret;
> +
> +	formatter->enabled_streams &= ~streams_mask;
> +
> +	if (!formatter->enabled_streams)
> +		pm_runtime_put(formatter->dev);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops formatter_subdev_pad_ops = {
> +	.enum_mbus_code		= formatter_subdev_enum_mbus_code,
> +	.get_fmt		= v4l2_subdev_get_fmt,
> +	.set_fmt		= formatter_subdev_set_fmt,
> +	.get_frame_desc		= formatter_subdev_get_frame_desc,
> +	.set_routing		= formatter_subdev_set_routing,
> +	.enable_streams		= formatter_subdev_enable_streams,
> +	.disable_streams	= formatter_subdev_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_ops formatter_subdev_ops = {
> +	.pad = &formatter_subdev_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops formatter_internal_ops = {
> +	.init_state = formatter_subdev_init_state,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Media entity operations
> + */
> +
> +static const struct media_entity_operations formatter_entity_ops = {
> +	.link_validate	= v4l2_subdev_link_validate,
> +	.get_fwnode_pad = v4l2_subdev_get_fwnode_pad_1_to_1,
> +};
> +
> +static int csi_formatter_subdev_init(struct csi_formatter *formatter)
> +{
> +	struct v4l2_subdev *sd = &formatter->sd;
> +	int ret;
> +
> +	v4l2_subdev_init(sd, &formatter_subdev_ops);
> +
> +	snprintf(sd->name, sizeof(sd->name), "%s", dev_name(formatter->dev));
> +	sd->internal_ops = &formatter_internal_ops;
> +
> +	sd->owner = THIS_MODULE;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +		     V4L2_SUBDEV_FL_HAS_EVENTS |
> +		     V4L2_SUBDEV_FL_STREAMS;
> +	sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> +	sd->entity.ops = &formatter_entity_ops;
> +	sd->dev = formatter->dev;
> +
> +	formatter->pads[CSI_FORMATTER_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +	formatter->pads[CSI_FORMATTER_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&sd->entity, CSI_FORMATTER_PAD_NUM,
> +				     formatter->pads);
> +	if (ret) {
> +		dev_err(formatter->dev, "Failed to init pads\n");
> +		return ret;
> +	}
> +
> +	ret = v4l2_subdev_init_finalize(sd);
> +	if (ret)
> +		media_entity_cleanup(&sd->entity);
> +
> +	return ret;
> +}
> +
> +static inline struct csi_formatter *
> +notifier_to_formatter(struct v4l2_async_notifier *n)
> +{
> +	return container_of(n, struct csi_formatter, notifier);
> +}
> +
> +static int csi_formatter_notify_bound(struct v4l2_async_notifier *notifier,
> +				      struct v4l2_subdev *sd,
> +				      struct v4l2_async_connection *asc)
> +{
> +	const unsigned int link_flags = MEDIA_LNK_FL_IMMUTABLE
> +				      | MEDIA_LNK_FL_ENABLED;
> +	struct csi_formatter *formatter = notifier_to_formatter(notifier);
> +	struct v4l2_subdev *sdev = &formatter->sd;
> +	struct media_pad *sink = &sdev->entity.pads[CSI_FORMATTER_PAD_SINK];
> +	struct media_pad *remote_pad;
> +	int ret;
> +
> +	formatter->csi_sd = sd;
> +
> +	dev_dbg(formatter->dev, "Bound subdev: %s pad\n", sd->name);
> +
> +	ret = v4l2_create_fwnode_links_to_pad(sd, sink, link_flags);
> +	if (ret < 0)
> +		return ret;
> +
> +	remote_pad = media_pad_remote_pad_first(sink);
> +	if (!remote_pad) {
> +		dev_err(formatter->dev, "Pipe not setup correctly\n");
> +		return -EPIPE;
> +	}
> +	formatter->remote_pad = remote_pad->index;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations formatter_notify_ops = {
> +	.bound = csi_formatter_notify_bound,
> +};
> +
> +static int csi_formatter_async_register(struct csi_formatter *formatter)
> +{
> +	struct device *dev = formatter->dev;
> +	struct v4l2_async_connection *asc;
> +	struct fwnode_handle *ep;

struct fwnode_handle *ep __free(fwnode_handle) = fwnode_graph_get_endpoint_by_id()

https://lore.kernel.org/all/CAHk-=whPZoi03ZwphxiW6cuWPtC3nyKYS8_BThgztCdgPWP1WA@mail.gmail.com/

Laurent Pinchart:

	look like prefer this way, let me know if I am wrong.

> +	int ret;
> +
> +	v4l2_async_subdev_nf_init(&formatter->notifier, &formatter->sd);
> +
> +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> +					     FWNODE_GRAPH_ENDPOINT_NEXT);
> +	if (!ep)
> +		return -ENOTCONN;
> +
> +	asc = v4l2_async_nf_add_fwnode_remote(&formatter->notifier, ep,
> +					      struct v4l2_async_connection);
> +	if (IS_ERR(asc)) {
> +		fwnode_handle_put(ep);
> +		return PTR_ERR(asc);

missed fwnode_handle_put(ep);

> +	}
> +
> +	fwnode_handle_put(ep);
> +
> +	formatter->notifier.ops = &formatter_notify_ops;
> +
> +	ret = v4l2_async_nf_register(&formatter->notifier);
> +	if (ret)
> +		return ret;
> +
> +	return v4l2_async_register_subdev(&formatter->sd);
> +}
> +
> +/* -----------------------------------------------------------------------------
> + * Suspend/resume
> + */
> +
> +static int csi_formatter_system_suspend(struct device *dev)
> +{
> +	return pm_runtime_force_suspend(dev);
> +}
> +
> +static int csi_formatter_system_resume(struct device *dev)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "force resume %s failed!\n", dev_name(dev));
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int csi_formatter_runtime_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +
> +	clk_disable_unprepare(formatter->clk);
> +
> +	return 0;
> +}
> +
> +static int csi_formatter_runtime_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +
> +	return clk_prepare_enable(formatter->clk);
> +}
> +
> +static const struct dev_pm_ops csi_formatter_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(csi_formatter_system_suspend,
> +				csi_formatter_system_resume)
> +	SET_RUNTIME_PM_OPS(csi_formatter_runtime_suspend,
> +			   csi_formatter_runtime_resume,
> +			   NULL)


use modern:
SYSTEM_SLEEP_PM_OPS
RUNTIME_PM_OPS

ref
https://lore.kernel.org/imx/20250411085932.1902662-1-arnd@kernel.org/

> +};
> +
> +static int csi_formatter_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct csi_formatter *formatter;
> +	u32 val;
> +	int ret;
> +
> +	formatter = devm_kzalloc(dev, sizeof(*formatter), GFP_KERNEL);
> +	if (!formatter)
> +		return -ENOMEM;
> +
> +	formatter->dev = dev;
> +
> +	formatter->regs = syscon_node_to_regmap(dev->parent->of_node);
> +	if (IS_ERR(formatter->regs)) {
> +		dev_err(dev, "Failed to get csi formatter regmap\n");
> +		return -ENODEV;

return dev_err_probe(), check others

> +	}
> +
> +	ret = of_property_read_u32(dev->of_node, "reg", &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to get csi formatter reg property\n");
> +		return ret;
> +	}
> +	formatter->reg_offset = val;
> +
> +	formatter->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(formatter->clk)) {
> +		dev_err(dev, "Failed to get pixel clock\n");
> +		return PTR_ERR(formatter->clk);
> +	}
> +
> +	ret = csi_formatter_subdev_init(formatter);
> +	if (ret < 0) {
> +		dev_err(dev, "formatter subdev init fail\n");
> +		return ret;
> +	}
> +
> +	/* Initialize formatter pixel format */
> +	formatter->fmt = find_csi_format(formatter_default_fmt.code);
> +
> +	ret = csi_formatter_async_register(formatter);
> +	if (ret < 0) {
> +		v4l2_subdev_cleanup(&formatter->sd);
> +		dev_err(dev, "Async register failed\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, &formatter->sd);
> +
> +	/* Enable runtime PM. */
> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +}
> +
> +static void csi_formatter_remove(struct platform_device *pdev)
> +{
> +	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> +	struct csi_formatter *formatter = sd_to_formatter(sd);
> +
> +	v4l2_async_nf_unregister(&formatter->notifier);
> +	v4l2_async_nf_cleanup(&formatter->notifier);
> +	v4l2_async_unregister_subdev(&formatter->sd);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	media_entity_cleanup(&formatter->sd.entity);
> +	pm_runtime_set_suspended(&pdev->dev);
> +}
> +
> +static const struct of_device_id csi_formatter_of_match[] = {
> +	{ .compatible = "fsl,imx9-csi-formatter" },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, csi_formatter_of_match);
> +
> +static struct platform_driver csi_formatter_device_driver = {
> +	.driver = {
> +		.owner          = THIS_MODULE,
> +		.name           = CSI_FORMATTER_DRV_NAME,
> +		.of_match_table = csi_formatter_of_match,
> +		.pm             = &csi_formatter_pm_ops,

pm_ptr()

> +	},
> +	.probe  = csi_formatter_probe,
> +	.remove = csi_formatter_remove,
> +};
> +
> +module_platform_driver(csi_formatter_device_driver);
> +
> +MODULE_ALIAS("platform:" CSI_FORMATTER_DRV_NAME);

drop this one.

Frank
> +MODULE_AUTHOR("NXP Semiconductor, Inc.");
> +MODULE_DESCRIPTION("NXP i.MX9 CSI Pixel Formatter driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.34.1
>

Powered by blists - more mailing lists