Re: [PATCH v3 6/8] media: platform: Add NXP Neoisp Image Signal Processor

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Jun 12, 2026 at 03:20:37PM +0200, Antoine Bouyer wrote:
> First NXP neoisp driver version with the following contents:
>
> This driver was initially inspired from raspberrypi pisp_be driver. It
> reuses same approach for ISP job scheduling.
>
> The Neoisp driver supports:
> * 8, 10, 12, 14 and 16-bits RAW Bayer images input.
> * Monochrome sensors input.
> * RGB/YUV, IR and Greyscale output formats.
>
> The neoisp features are:
> * Provides single context to limit amount of v4l2 devices.
> * Supports M2M operations.
> * Support SDR and HDR modes.
> * Supports generic v4l2-isp framework for extensible Parameters and
> Statistics buffers.
> * Provides a `core_media_register` API to register neoisp's media entities
> into another media graph.
> * A module parameter to run in standalone mode with its own media device.
>
> Co-developed-by: Alexi Birlinger <alexi.birlinger@nxp.com>
> Signed-off-by: Alexi Birlinger <alexi.birlinger@nxp.com>
> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
> ---
...
>
> +MEDIA DRIVERS FOR NXP NEOISP
> +M:	Antoine Bouyer <antoine.bouyer@nxp.com>
> +S:	Maintained
> +F:	Documentation/admin-guide/media/nxp-neoisp*
> +F:	Documentation/devicetree/bindings/media/nxp,imx95-neoisp.yaml
> +F:  Documentation/userspace-api/media/v4l/metafmt-nxp-neoisp.rst

Use tab.

> +F:	drivers/media/platform/nxp/neoisp/*
> +F:	include/uapi/linux/media/nxp/nxp_neoisp.h
> +
...
> +#define NEOISP_MIN_H			64U
> +#define NEOISP_MAX_W			4096U
> +#define NEOISP_MAX_H			4096U
> +#define NEOISP_MAX_BPP			4U
> +#define NEOISP_ALIGN_W			3
> +#define NEOISP_ALIGN_H			3
> +#define NEOISP_DEF_W			640U
> +#define NEOISP_DEF_H			480U

look like needn't "U" for these const.

> +
> +#define NEOISP_SUSPEND_TIMEOUT_MS	500
> +
...
> +
> +/*
> + * Extract offset and size in bytes from memory region map
> + */
> +static inline void get_offsize(enum isp_block_map_e map, u32 *offset, u32 *size)

Needn't inline. check others, compiler will auto do it and also find unused
function if no inline.

> +{
> +	*offset = ISP_GET_OFF(map);
> +	*size = ISP_GET_SZ(map);
> +}
> +
...
> +
> +static const struct v4l2_frmsize_stepwise neoisp_frmsize_stepwise = {
> +	.min_width = NEOISP_MIN_W,
> +	.min_height = NEOISP_MIN_H,
> +	.max_width = NEOISP_MAX_W,
> +	.max_height = NEOISP_MAX_H,
> +	.step_width = 1UL << NEOISP_ALIGN_W,
> +	.step_height = 1UL << NEOISP_ALIGN_H,
> +};

why put this into neoisp_fmt.h? if two c file include it, these data will
be duplicated

...
> +		bit = NEO_PIPE_CONF_SOFT_RESET_HARD_RESET;
> +
> +	neoisp_wr(neoispd, NEO_PIPE_CONF_SOFT_RESET, bit);
> +
> +	/* Wait for auto-clear */
> +	do {
> +		usleep_range(1, 2);
> +		val = neoisp_rd(neoispd, NEO_PIPE_CONF_SOFT_RESET);
> +		count--;
> +	} while ((val & bit) && count);

Use read_poll_timeout()

> +
> +	if (val & bit)
> +		dev_warn(neoispd->dev, "%s reset incomplete\n",
> +			 is_hw ? "hw" : "sw");
> +}
...
> +		/*
> +		 * Take a copy of streaming_map: nodes activated after this
> +		 * point are ignored when preparing this job
> +		 */
> +		streaming_map = neoispd->streaming_map;
> +	}
> +
> +	job = kzalloc(sizeof(*job), GFP_KERNEL);

use kzalloc_obj()

> +	if (!job)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < NEOISP_NODES_COUNT; i++) {
...
> +static irqreturn_t neoisp_irq_handler(int irq, void *dev_id)
> +{
> +	struct neoisp_dev_s *neoispd = (struct neoisp_dev_s *)dev_id;
> +	struct neoisp_buffer_s **buf = neoispd->queued_job.buf;
> +	u64 ts = ktime_get_ns();
> +	u32 irq_status = 0;
> +	u32 irq_clear = 0;
> +	bool done = false;
> +	int i;
> +
> +	irq_status = neoisp_rd(neoispd, NEO_PIPE_CONF_INT_STAT0);
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_FS1) {
> +		dev_dbg(neoispd->dev, "Neo IRQ FS1 !\n");
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_FS1;
> +		done = false;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_FS2) {
> +		dev_dbg(neoispd->dev, "Neo IRQ FS2 !\n");
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_FS2;
> +		done = false;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_FD1) {
> +		dev_dbg(neoispd->dev, "Neo IRQ FD1 !\n");
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_FD1;
> +		done = false;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_STATD) {
> +		dev_dbg(neoispd->dev, "Neo IRQ STATD !\n");
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_STATD;
> +		done = false;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_DRCD) {
> +		dev_dbg(neoispd->dev, "Neo IRQ DRCD !\n");
> +		neoisp_ctx_get_stats(neoispd, buf[NEOISP_STATS_NODE]);
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_DRCD;
> +		done = false;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_BUS_ERR) {
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_BUS_ERR;
> +		dev_err(neoispd->dev, "Neo IRQ BUS ERR!\n");
> +		done = true;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_TRIG_ERR) {
> +		dev_err(neoispd->dev, "Neo IRQ TRIG ERR !\n");
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_TRIG_ERR;
> +		done = true;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_CSI_TERR) {
> +		dev_err(neoispd->dev, "Neo IRQ TRIG CSI Trigger ERR !\n");
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_CSI_TERR;
> +		done = true;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_S_FD2) {
> +		dev_dbg(neoispd->dev, "Neo IRQ FD2 !\n");
> +		irq_clear |= NEO_PIPE_CONF_INT_STAT0_S_FD2;
> +		done = true;
> +	}
> +
> +	if (irq_status & NEO_PIPE_CONF_INT_STAT0_BUSY)
> +		dev_err(neoispd->dev, "Neo is busy !\n");
> +
> +	neoisp_wr(neoispd, NEO_PIPE_CONF_INT_STAT0, irq_clear);

Did you share irq with other device?

you can use directly
	neoisp_wr(neoispd, NEO_PIPE_CONF_INT_STAT0, irq_status);

and if there are unexpected irq happen, which is not in your check list,
it will cause irq storm.

	so need more if block

	just

	if (irq_status & (NEO_PIPE_CONF_INT_STAT0_S_BUS_ER | irq_status & NEO_PIPE_CONF_INT_STAT0_S_BUS_ER ...)
		done = true;

default done is falue.

> +
> +	if (done) {
> +		for (i = 0; i < NEOISP_NODES_COUNT; i++) {
> +			if (!buf[i])
> +				continue;
> +
> +			buf[i]->vb.sequence = neoispd->frame_sequence;
> +			buf[i]->vb.vb2_buf.timestamp = ts;
> +			vb2_buffer_done(&buf[i]->vb.vb2_buf, VB2_BUF_STATE_DONE);
> +
> +			/* To prevent double buffer handling in case of spurius interrupt */
> +			buf[i] = NULL;
> +		}
> +		/* Update frame_sequence */
> +		neoispd->frame_sequence++;
> +		/* Check if there's more to do before going to sleep */
> +		neoisp_schedule(neoispd, true);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
...
> +static int neoisp_init_node(struct neoisp_dev_s *neoispd, u32 id)
> +{
> +	bool output = node_desc_is_output(&node_desc[id]);
> +	struct neoisp_node_s *node = &neoispd->node[id];
> +	struct media_entity *entity = &node->vfd.entity;
> +	struct media_pad *mpad;
> +	struct video_device *vdev = &node->vfd;
> +	struct vb2_queue *q = &node->queue;
> +	int ret;

please try keep revise christmas tree order for nice look.

> +
> +	node->id = id;
> +	node->neoisp = neoispd;
> +	node->buf_type = node_desc[id].buf_type;
> +
...
> +{
> +	struct v4l2_device *v4l2_dev = &neoispd->v4l2_dev;
> +	u32 num_registered = 0;
> +	int ret;
> +
> +	mutex_init(&neoispd->queue_lock);

suppose this function call from probe, so you can use devm_mutex_init()

> +
> +	/* Register v4l2_device and media_device */
> +	v4l2_dev->mdev = mdev;
> +	strscpy(v4l2_dev->name, NEOISP_NAME, sizeof(v4l2_dev->name));
...
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to request irq: %d\n", ret);
> +		goto err_irq;
> +	}
> +
> +	pm_runtime_set_autosuspend_delay(dev, NEOISP_SUSPEND_TIMEOUT_MS);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_enable(dev);

devm_pm_runtime_enable();

> +	ret = pm_runtime_resume_and_get(dev);

you can use below code now, needn't goto branch

	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
		return -ENXIO;


> +	if (ret < 0) {
> +		dev_err(dev, "Unable to resume the device: %d\n", ret);
> +		goto err_pm_runtime_disable;
> +	}
> +
> +	ret = neoisp_init_devices(neoispd);
> +	if (ret)
> +		goto err_pm_runtime_suspend;
> +
> +	spin_lock_init(&neoispd->hw_lock);
> +	neoisp_init_hw(neoispd);
> +	neoisp_set_default_context(neoispd);
> +
> +	pm_runtime_mark_last_busy(dev);

Needn't call this now

> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +
> +err_pm_runtime_suspend:
> +	pm_runtime_put(dev);
> +err_pm_runtime_disable:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
> +err_irq:
> +	dev_err(dev, "probe: error %d\n", ret);
> +	return ret;
> +}
> +
> +static void neoisp_remove(struct platform_device *pdev)
> +{
> +	struct neoisp_dev_s *neoispd = platform_get_drvdata(pdev);
> +
> +	neoisp_destroy_devices(neoispd);
> +
> +	if (standalone_mdev)
> +		media_device_cleanup(&neoispd->mdev);
> +
> +	pm_runtime_dont_use_autosuspend(neoispd->dev);
> +	pm_runtime_disable(neoispd->dev);
> +}
> +
> +static int __maybe_unused neoisp_runtime_suspend(struct device *dev)

Needn't __maybe now

> +{
> +	struct neoisp_dev_s *neoispd = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(neoispd->num_clks, neoispd->clks);
> +
> +	return 0;
> +}
> +
...
> +
> +static const struct dev_pm_ops neoisp_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(neoisp_pm_suspend, neoisp_pm_resume)
> +	SET_RUNTIME_PM_OPS(neoisp_runtime_suspend, neoisp_runtime_resume, NULL)

Use new macro
RUNTIME_PM_OPS

> +};
> +
...
> +
> +static struct platform_driver neoisp_driver = {
> +	.probe  = neoisp_probe,
> +	.remove = neoisp_remove,
> +	.driver = {
> +		.name = NEOISP_NAME,
> +		.pm = &neoisp_pm,

pm_ptr(&neoisp_pm)

> +		.of_match_table = neoisp_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(neoisp_driver);
> +
> +MODULE_DESCRIPTION("NXP NEOISP Hardware");
> +MODULE_AUTHOR("Antoine Bouyer <antoine.bouyer@nxp.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/nxp/neoisp/neoisp_nodes.h b/drivers/media/platform/nxp/neoisp/neoisp_nodes.h
> new file mode 100644
> index 000000000000..54986fe13b33
> --- /dev/null
> +++ b/drivers/media/platform/nxp/neoisp/neoisp_nodes.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * NEOISP nodes description
> + *
> + * Copyright 2023-2026 NXP
> + */
> +
> +#ifndef __NXP_NEOISP_NODES_H
> +#define __NXP_NEOISP_NODES_H
> +
> +#include <linux/videodev2.h>
> +
> +#include "neoisp.h"
> +
> +static const struct neoisp_node_desc_s node_desc[NEOISP_NODES_COUNT] = {

These const data should not appear in header files.

Frank
>