* [PATCH v6 1/9] media: mc-entity: Store parsed V4L2 fwnode endpoint in media_pad
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
2026-06-24 20:56 ` sashiko-bot
2026-06-24 20:37 ` [PATCH v6 2/9] media: subdev: Add set_pad_by_ep() callback to internal ops Frank.Li
` (7 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel
From: Frank Li <Frank.Li@nxp.com>
Each media pad is associated with a firmware node endpoint. Capture the
parsed V4L2 fwnode endpoint information in struct media_pad so it can be
reused by consumers.
This avoids reparsing firmware node endpoint data every time the endpoint
configuration is needed, reduces duplicate code, and provides a common
place to store endpoint properties associated with a pad.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Assume 1 to 1 map between dt's endpoint to medie pad.
Change in v6
- new patch
---
include/media/media-entity.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index d9b72cd87d524..4a3785cd9f370 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -20,6 +20,8 @@
#include <linux/minmax.h>
#include <linux/types.h>
+#include <media/v4l2-fwnode.h>
+
/* Enums used internally at the media controller to represent graphs */
/**
@@ -230,6 +232,7 @@ enum media_pad_signal_type {
* @flags: Pad flags, as defined in
* :ref:`include/uapi/linux/media.h <media_header>`
* (seek for ``MEDIA_PAD_FL_*``)
+ * @vep: associated fwnode endpoint information
* @pipe: Pipeline this pad belongs to. Use media_entity_pipeline() to
* access this field.
*/
@@ -240,7 +243,7 @@ struct media_pad {
u16 num_links;
enum media_pad_signal_type sig_type;
unsigned long flags;
-
+ struct v4l2_fwnode_endpoint vep;
/*
* The fields below are private, and should only be accessed via
* appropriate functions.
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 1/9] media: mc-entity: Store parsed V4L2 fwnode endpoint in media_pad
2026-06-24 20:37 ` [PATCH v6 1/9] media: mc-entity: Store parsed V4L2 fwnode endpoint in media_pad Frank.Li
@ 2026-06-24 20:56 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-06-24 20:56 UTC (permalink / raw)
To: Frank.Li; +Cc: Frank.Li, imx, conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Captured fwnode endpoint contains a dangling pointer to `local_fwnode` if the caller drops its reference after parsing.
- [Medium] Embedding a large, V4L2-specific `struct v4l2_fwnode_endpoint` into the generic Media Controller `struct media_pad` breaks subsystem abstraction and unconditionally bloats memory.
--
commit 7acfb56349845d6638c963825f0f6b087ebaa54d
Author: Frank Li <Frank.Li@nxp.com>
media: mc-entity: Store parsed V4L2 fwnode endpoint in media_pad
This commit captures the parsed v4l2 fwnode endpoint information directly
within struct media_pad to allow reuse by consumers without reparsing.
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index d9b72cd87d524..4a3785cd9f370 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -20,6 +20,8 @@
> #include <linux/minmax.h>
> #include <linux/types.h>
>
> +#include <media/v4l2-fwnode.h>
> +
[Severity: Medium]
Does this include break subsystem isolation? struct media_pad is a generic
structure used across several subsystems like alsa, dvb, drm, and iio.
Including a v4l2-specific header here appears to tie the generic media
controller to v4l2.
[ ... ]
> @@ -240,7 +243,7 @@ struct media_pad {
> u16 num_links;
> enum media_pad_signal_type sig_type;
> unsigned long flags;
> -
> + struct v4l2_fwnode_endpoint vep;
[Severity: Medium]
Is it expected to embed this v4l2-specific structure into every generic
media_pad? The v4l2_fwnode_endpoint struct is quite large, and
unconditionally embedding it adds memory overhead for all non-v4l2
entities that do not use fwnode endpoints.
[Severity: Critical]
Can this cause a use-after-free regression with the fwnode pointers?
When the endpoint is parsed, v4l2_fwnode_endpoint_parse() assigns the
fwnode to vep->base.local_fwnode without taking a reference count. This
was safe when vep was a transient stack variable, but now that it is
stored persistently in the media_pad, the caller might drop the
reference.
For example, if the caller iterates using a scoped iterator:
drivers/media/v4l2-core/v4l2-fwnode.c:v4l2_async_nf_parse_fwnode() {
...
fwnode_graph_for_each_endpoint_scoped(dev_fwnode(dev), ep) {
ret = v4l2_fwnode_endpoint_parse(ep, &pad->vep);
...
}
The scoped iterator automatically drops the reference to ep at the end
of the iteration, leaving pad->vep.base.local_fwnode as a dangling
pointer.
Later access or pointer comparisons, like in v4l2_common_notifier_bound(),
might dereference freed memory if the fwnode is deleted.
> /*
> * The fields below are private, and should only be accessed via
> * appropriate functions.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 2/9] media: subdev: Add set_pad_by_ep() callback to internal ops
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
2026-06-24 20:37 ` [PATCH v6 1/9] media: mc-entity: Store parsed V4L2 fwnode endpoint in media_pad Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
2026-06-24 20:50 ` sashiko-bot
2026-06-24 20:37 ` [PATCH v6 3/9] media: subdev: Add media_async_register_subdev() helper Frank.Li
` (6 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel
From: Frank Li <Frank.Li@nxp.com>
Add a set_pad_by_ep() callback to struct v4l2_subdev_internal_ops. The
callback is invoked while parsing firmware node endpoints and allows
subdevice drivers to configure media pad properties based on endpoint
information.
Typical uses include setting media pad flags according to the endpoint's
port number or type, and validating that the endpoint configuration is
supported by the underlying hardware. This provides a common mechanism
for endpoint-aware pad initialization during graph construction.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v6
- new patch
---
include/media/v4l2-subdev.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d256b7ec8f848..eb652eb76d33f 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -935,6 +935,10 @@ struct v4l2_subdev_ops {
* the v4l2_subdev structure. It is almost certainly required for any
* sub-device that sets the V4L2_SUBDEV_FL_HAS_DEVNODE flag.
*
+ * @set_pad_by_ep: Set pad informaiton by fwnode endpoint, parsed fwnode already
+ * saved into pad->vep. return < 0 means can't support this type
+ * endpoint. Set pad->flags according to pad->vep information.
+ *
* .. note::
* Never call this from drivers, only the v4l2 framework can call
* these ops.
@@ -947,6 +951,7 @@ struct v4l2_subdev_internal_ops {
int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
void (*release)(struct v4l2_subdev *sd);
+ int (*set_pad_by_ep)(struct v4l2_subdev *sd, struct media_pad *pad);
};
/* Set this flag if this subdev is a i2c device. */
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 2/9] media: subdev: Add set_pad_by_ep() callback to internal ops
2026-06-24 20:37 ` [PATCH v6 2/9] media: subdev: Add set_pad_by_ep() callback to internal ops Frank.Li
@ 2026-06-24 20:50 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-06-24 20:50 UTC (permalink / raw)
To: Frank.Li; +Cc: imx, Frank.Li, devicetree, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Severe layering violation by tightly coupling the core Media Controller framework with V4L2-specific firmware parsing.
- [Low] Typo in the callback documentation.
--
commit 3e6e9fb5a2a23ceb5cd559adeba28603dd1f9e0f
Author: Frank Li <Frank.Li@nxp.com>
media: subdev: Add set_pad_by_ep() callback to internal ops
This commit adds a set_pad_by_ep() callback to v4l2_subdev_internal_ops.
The callback is invoked while parsing firmware node endpoints and allows
subdevice drivers to configure media pad properties based on endpoint
information.
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d256b7ec8f848..eb652eb76d33f 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -935,6 +935,10 @@ struct v4l2_subdev_ops {
> * the v4l2_subdev structure. It is almost certainly required for any
> * sub-device that sets the V4L2_SUBDEV_FL_HAS_DEVNODE flag.
> *
> + * @set_pad_by_ep: Set pad informaiton by fwnode endpoint, parsed fwnode already
[Severity: Low]
This isn't a bug, but there is a typo in information (informaiton).
> + * saved into pad->vep. return < 0 means can't support this type
> + * endpoint. Set pad->flags according to pad->vep information.
> + *
> * .. note::
> * Never call this from drivers, only the v4l2 framework can call
> * these ops.
> @@ -947,6 +951,7 @@ struct v4l2_subdev_internal_ops {
> int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
> int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
> void (*release)(struct v4l2_subdev *sd);
> + int (*set_pad_by_ep)(struct v4l2_subdev *sd, struct media_pad *pad);
[Severity: Medium]
Does this callback signature enforce an architectural layering violation?
By omitting the vep argument, the callback design expects the parsed V4L2
endpoint configuration to be passed via pad->vep. struct media_pad is part of
the generic Media Controller API, which is shared across various subsystems
like ALSA and DVB.
Embedding the V4L2-specific struct v4l2_fwnode_endpoint directly into the
generic struct media_pad creates an inappropriate subsystem dependency and
wastes memory for all non-V4L2 media pads.
Could the V4L2-specific endpoint be passed directly as an argument to the
callback, such as:
int (*set_pad_by_ep)(struct v4l2_subdev *sd, struct media_pad *pad,
struct v4l2_fwnode_endpoint *vep);
> };
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 3/9] media: subdev: Add media_async_register_subdev() helper
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
2026-06-24 20:37 ` [PATCH v6 1/9] media: mc-entity: Store parsed V4L2 fwnode endpoint in media_pad Frank.Li
2026-06-24 20:37 ` [PATCH v6 2/9] media: subdev: Add set_pad_by_ep() callback to internal ops Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
2026-06-24 21:00 ` sashiko-bot
2026-06-24 20:37 ` [PATCH v6 4/9] media: synopsys: Use v4l2_subdev_get_frame_desc_passthrough() Frank.Li
` (5 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel
From: Frank Li <Frank.Li@nxp.com>
Add media_async_register_subdev(), a helper to register a V4L2 sub-device
with the asynchronous sub-device framework.
The helper assumes a 1:1 mapping between firmware endpoints and media pads.
During registration it parses the firmware graph, creates media pads for
all endpoints, and registers common asynchronous notifiers for sink
endpoints. These notifiers automatically create media links when the
corresponding remote source devices become available.
The set_pad_by_ep() callback allows drivers to determine the media pad
associated with a firmware endpoint and identify whether the endpoint
represents a sink pad.
By centralizing firmware graph parsing, media pad creation, notifier
registration, and link creation, this helper reduces duplicated code and
simplifies error handling in V4L2 sub-device drivers.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v6
- new patch
---
drivers/media/v4l2-core/v4l2-fwnode.c | 155 ++++++++++++++++++++++++++++++++++
include/media/v4l2-async.h | 39 +++++++++
2 files changed, 194 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 62a3a452f7884..169059654478f 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -26,6 +26,7 @@
#include <media/v4l2-async.h>
#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mc.h>
#include <media/v4l2-subdev.h>
#include "v4l2-subdev-priv.h"
@@ -1302,6 +1303,160 @@ int __v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *m
}
EXPORT_SYMBOL_GPL(__v4l2_async_register_subdev_sensor);
+static int v4l2_common_notifier_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *sd,
+ struct v4l2_async_connection *asd)
+{
+ struct media_pad *pad = NULL;
+ int ret;
+
+ if (asd->match.type != V4L2_ASYNC_MATCH_TYPE_FWNODE)
+ return -EINVAL;
+
+ if (!asd->match.fwnode)
+ return -EINVAL;
+
+ struct fwnode_handle *remote __free(fwnode_handle) =
+ fwnode_graph_get_remote_endpoint(asd->match.fwnode);
+
+ for (int i = 0; i < notifier->sd->entity.num_pads; i++) {
+ if (notifier->sd->entity.pads[i].vep.base.local_fwnode == remote) {
+ pad = ¬ifier->sd->entity.pads[i];
+ break;
+ }
+ }
+
+ if (!pad) {
+ dev_err(notifier->sd->dev, "failed to find sink pad\n");
+ return -EINVAL;
+ }
+
+ ret = v4l2_create_fwnode_links_to_pad(sd, pad, MEDIA_LNK_FL_ENABLED);
+ if (ret) {
+ dev_err(sd->dev, "failed to link source pad\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct v4l2_async_notifier_operations v4l2_common_notifier_ops = {
+ .bound = v4l2_common_notifier_bound,
+};
+
+static int
+v4l2_async_nf_parse_fwnode(struct device *dev, struct media_pad *pads,
+ struct v4l2_async_notifier *notifier)
+{
+ struct v4l2_subdev *sd = notifier->sd;
+ struct v4l2_async_connection *asd;
+ struct media_pad *pad;
+ int ret;
+
+ if (!sd->internal_ops->set_pad_by_ep)
+ return dev_err_probe(dev, -EINVAL,
+ "Missed valiate_endpoint() callback\n");
+ pad = pads;
+
+ fwnode_graph_for_each_endpoint_scoped(dev_fwnode(dev), ep) {
+ u32 flags;
+
+ ret = v4l2_fwnode_endpoint_parse(ep, &pad->vep);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to parse endpoint\n");
+
+ ret = sd->internal_ops->set_pad_by_ep(sd, pad);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Can support endponit\n");
+
+ flags = pad->flags;
+
+ pad++;
+
+ if (flags & MEDIA_PAD_FL_SOURCE)
+ continue; /* Bypass source port */
+
+ notifier->ops = &v4l2_common_notifier_ops;
+
+ asd = v4l2_async_nf_add_fwnode_remote(notifier, ep,
+ struct v4l2_async_connection);
+ if (IS_ERR(asd))
+ return dev_err_probe(dev, PTR_ERR(asd),
+ "failed to add notifier\n");
+ }
+
+ return 0;
+}
+
+void media_async_subdev_cleanup(struct v4l2_subdev *sd)
+{
+ v4l2_async_unregister_subdev(sd);
+ v4l2_subdev_cleanup(sd);
+ media_entity_cleanup(&sd->entity);
+ v4l2_async_nf_unregister(sd->subdev_notifier);
+ v4l2_async_nf_cleanup(sd->subdev_notifier);
+ kfree(sd->entity.pads);
+}
+EXPORT_SYMBOL_GPL(media_async_subdev_cleanup);
+
+int __media_async_register_subdev(struct v4l2_subdev *sd, struct module *module)
+{
+ struct device *dev = sd->dev;
+ u32 ep_count;
+ int ret;
+
+ if (WARN_ON(!sd->dev))
+ return -ENODEV;
+
+ struct v4l2_async_notifier *notifier __free(kfree) = kzalloc_obj(*notifier);
+ if (!notifier)
+ return -ENOMEM;
+
+ v4l2_async_subdev_nf_init(notifier, sd);
+
+ ep_count = fwnode_graph_get_endpoint_count(dev_fwnode(dev), 0);
+ if (!ep_count)
+ return dev_err_probe(dev, -EINVAL, "No connected endpoints\n");
+
+ struct media_pad *pads __free(kfree) = kzalloc_objs(struct media_pad, ep_count);
+ if (!pads)
+ return -ENOMEM;
+
+ ret = v4l2_async_nf_parse_fwnode(dev, pads, notifier);
+ if (ret < 0)
+ return ret;
+
+ ret = media_entity_pads_init(&sd->entity, ep_count, pads);
+ if (ret)
+ goto out_cleanup;
+
+ ret = v4l2_async_nf_register(notifier);
+ if (ret < 0)
+ goto out_cleanup;
+
+ ret = v4l2_subdev_init_finalize(sd);
+ if (ret)
+ goto out_unregister;
+
+ ret = __v4l2_async_register_subdev(sd, module);
+ if (ret < 0)
+ goto out_unregister;
+
+ sd->subdev_notifier = no_free_ptr(notifier);
+ retain_and_null_ptr(pads);
+
+ return 0;
+
+out_unregister:
+ v4l2_async_nf_unregister(notifier);
+
+out_cleanup:
+ v4l2_async_nf_cleanup(notifier);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(__media_async_register_subdev);
+
MODULE_DESCRIPTION("V4L2 fwnode binding parsing library");
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 54a2d9620ed5b..ca41820f776c5 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -345,4 +345,43 @@ __v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *modul
* @sd: pointer to &struct v4l2_subdev
*/
void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
+
+enum v4l2_subdev_1to1_pads {
+ V4L2_SUBDEV_1TO1_PADS_SINK,
+ V4L2_SUBDEV_1TO1_PADS_SOURCE,
+ V4L2_SUBDEV_1TO1_PADS_TOTAL,
+};
+
+/**
+ * media_async_register_subdev - registers a sub-device to the asynchronous
+ * sub-device framework and parse set up common
+ * related devices
+ *
+ * @sd: pointer to struct &v4l2_subdev
+ *
+ * Register a V4L2 sub-device with the asynchronous sub-device framework.
+ * In addition to v4l2_async_register_subdev(), this function parses the
+ * firmware graph, creates media pads for the endpoints, and registers common
+ * notifiers to create media links between connected devices.
+ *
+ * This function also init media_pads.
+ *
+ * The sub-device is unregistered and cleanup by media_async_subdev_cleanup()
+ *
+ * While registered, the subdev module is marked as in-use.
+ *
+ * An error is returned if the module is no longer loaded on any attempts
+ * to register it.
+ */
+#define media_async_register_subdev(sd_1to1) \
+ __media_async_register_subdev(sd_1to1, THIS_MODULE)
+
+int __media_async_register_subdev(struct v4l2_subdev *sd_1to1, struct module *module);
+
+/**
+ * media_async_subdev_cleanup - unregistered and cleanup subdev and media pads
+ * @sd_1to1: pointer to struct &v4l2_subdev_1to1
+ */
+void media_async_subdev_cleanup(struct v4l2_subdev *sd_1to1);
+
#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 3/9] media: subdev: Add media_async_register_subdev() helper
2026-06-24 20:37 ` [PATCH v6 3/9] media: subdev: Add media_async_register_subdev() helper Frank.Li
@ 2026-06-24 21:00 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-06-24 21:00 UTC (permalink / raw)
To: Frank.Li; +Cc: conor+dt, Frank.Li, robh, imx, devicetree
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Heap Buffer Overflow during endpoint parsing
- [High] NULL Pointer Dereference of `sd->internal_ops`
- [Medium] Resource Leak / Subsystem Guideline Violation due to goto and scope-based cleanup mixing
- [Low] Code Quality / Misspelled diagnostic messages
--
commit 050e10af8571659357b0d6bb808eab4e58871636
Author: Frank Li <Frank.Li@nxp.com>
media: subdev: Add media_async_register_subdev() helper
This commit introduces media_async_register_subdev() to register a V4L2
sub-device with the asynchronous framework. It parses the firmware graph
and sets up common notifiers and media pads. This aims to reduce duplicated
code and simplify error handling in V4L2 sub-device drivers.
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 62a3a452f7884..169059654478f 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
[ ... ]
> @@ -1302,6 +1303,160 @@ int __v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *m
[ ... ]
> +static int
> +v4l2_async_nf_parse_fwnode(struct device *dev, struct media_pad *pads,
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_subdev *sd = notifier->sd;
> + struct v4l2_async_connection *asd;
> + struct media_pad *pad;
> + int ret;
> +
> + if (!sd->internal_ops->set_pad_by_ep)
[Severity: High]
If a driver passes a subdevice without internal_ops initialized, will this
dereference a NULL pointer?
> + return dev_err_probe(dev, -EINVAL,
> + "Missed valiate_endpoint() callback\n");
[Severity: Low]
This isn't a bug, but there's a typo in "valiate" and it references the wrong
callback name (set_pad_by_ep).
> + pad = pads;
> +
> + fwnode_graph_for_each_endpoint_scoped(dev_fwnode(dev), ep) {
> + u32 flags;
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &pad->vep);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to parse endpoint\n");
> +
> + ret = sd->internal_ops->set_pad_by_ep(sd, pad);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Can support endponit\n");
[Severity: Low]
This isn't a bug, but "endponit" is misspelled, and the logic seems inverted
since this prints on an error path where the endpoint cannot be supported.
> +
> + flags = pad->flags;
> +
> + pad++;
[Severity: High]
Could this loop write out of bounds of the pads array?
In __media_async_register_subdev(), ep_count is calculated with:
ep_count = fwnode_graph_get_endpoint_count(dev_fwnode(dev), 0);
which only counts endpoints that have a remote connection, and the pads
array is allocated to exactly that size.
Since fwnode_graph_for_each_endpoint_scoped() unconditionally iterates over
all endpoints, if the firmware graph contains any unconnected endpoints, won't
this execute more times than ep_count and write into adjacent heap memory?
> +
> + if (flags & MEDIA_PAD_FL_SOURCE)
> + continue; /* Bypass source port */
> +
> + notifier->ops = &v4l2_common_notifier_ops;
> +
> + asd = v4l2_async_nf_add_fwnode_remote(notifier, ep,
> + struct v4l2_async_connection);
> + if (IS_ERR(asd))
> + return dev_err_probe(dev, PTR_ERR(asd),
> + "failed to add notifier\n");
> + }
> +
> + return 0;
> +}
[ ... ]
> +int __media_async_register_subdev(struct v4l2_subdev *sd, struct module *module)
> +{
> + struct device *dev = sd->dev;
> + u32 ep_count;
> + int ret;
> +
> + if (WARN_ON(!sd->dev))
> + return -ENODEV;
> +
> + struct v4l2_async_notifier *notifier __free(kfree) = kzalloc_obj(*notifier);
> + if (!notifier)
> + return -ENOMEM;
> +
> + v4l2_async_subdev_nf_init(notifier, sd);
> +
> + ep_count = fwnode_graph_get_endpoint_count(dev_fwnode(dev), 0);
> + if (!ep_count)
> + return dev_err_probe(dev, -EINVAL, "No connected endpoints\n");
> +
> + struct media_pad *pads __free(kfree) = kzalloc_objs(struct media_pad, ep_count);
> + if (!pads)
> + return -ENOMEM;
> +
> + ret = v4l2_async_nf_parse_fwnode(dev, pads, notifier);
> + if (ret < 0)
> + return ret;
[Severity: Medium]
If v4l2_async_nf_parse_fwnode() fails, does returning directly bypass
the v4l2_async_nf_cleanup() call that would normally happen via the
out_cleanup label?
The notifier itself is freed via cleanup helpers, but it seems the
v4l2_async_connection structures allocated in the parse loop and their fwnode
references will be leaked if we don't explicitly call v4l2_async_nf_cleanup().
> +
> + ret = media_entity_pads_init(&sd->entity, ep_count, pads);
> + if (ret)
> + goto out_cleanup;
> +
> + ret = v4l2_async_nf_register(notifier);
> + if (ret < 0)
> + goto out_cleanup;
> +
> + ret = v4l2_subdev_init_finalize(sd);
> + if (ret)
> + goto out_unregister;
> +
> + ret = __v4l2_async_register_subdev(sd, module);
> + if (ret < 0)
> + goto out_unregister;
[Severity: Medium]
If __v4l2_async_register_subdev() fails, we jump to out_unregister, which
cleans up the notifier and returns.
Does this leak the active_state allocated earlier by
v4l2_subdev_init_finalize(sd) since v4l2_subdev_cleanup() is bypassed?
> +
> + sd->subdev_notifier = no_free_ptr(notifier);
> + retain_and_null_ptr(pads);
> +
> + return 0;
> +
> +out_unregister:
> + v4l2_async_nf_unregister(notifier);
> +
> +out_cleanup:
> + v4l2_async_nf_cleanup(notifier);
> +
> + return ret;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 4/9] media: synopsys: Use v4l2_subdev_get_frame_desc_passthrough()
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
` (2 preceding siblings ...)
2026-06-24 20:37 ` [PATCH v6 3/9] media: subdev: Add media_async_register_subdev() helper Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
2026-06-24 20:37 ` [PATCH v6 5/9] media: synopsys: Use media_async_register_subdev() to simplify code Frank.Li
` (4 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel
From: Frank Li <Frank.Li@nxp.com>
Replace the local frame descriptor callback implementation with
v4l2_subdev_get_frame_desc_passthrough().
This helper provides the same functionality while avoiding duplicate
code and simplifying the driver implementation.
Reviewed-by: Guoniu Zhou <guoniu.zhou@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v6
- collect gouniu's review by
change in v5
- new patch
---
drivers/media/platform/synopsys/dw-mipi-csi2rx.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
index 41e48365167e5..f51367409ff46 100644
--- a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
+++ b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
@@ -630,31 +630,11 @@ static int dw_mipi_csi2rx_disable_streams(struct v4l2_subdev *sd,
return ret;
}
-static int
-dw_mipi_csi2rx_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
- struct v4l2_mbus_frame_desc *fd)
-{
- struct dw_mipi_csi2rx_device *csi2 = to_csi2(sd);
- struct v4l2_subdev *remote_sd;
- struct media_pad *remote_pad;
-
- remote_pad = media_pad_remote_pad_unique(&csi2->pads[DW_MIPI_CSI2RX_PAD_SINK]);
- if (IS_ERR(remote_pad)) {
- dev_err(csi2->dev, "can't get remote source pad\n");
- return PTR_ERR(remote_pad);
- }
-
- remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
-
- return v4l2_subdev_call(remote_sd, pad, get_frame_desc,
- remote_pad->index, fd);
-}
-
static const struct v4l2_subdev_pad_ops dw_mipi_csi2rx_pad_ops = {
.enum_mbus_code = dw_mipi_csi2rx_enum_mbus_code,
.get_fmt = v4l2_subdev_get_fmt,
.set_fmt = dw_mipi_csi2rx_set_fmt,
- .get_frame_desc = dw_mipi_csi2rx_get_frame_desc,
+ .get_frame_desc = v4l2_subdev_get_frame_desc_passthrough,
.set_routing = dw_mipi_csi2rx_set_routing,
.enable_streams = dw_mipi_csi2rx_enable_streams,
.disable_streams = dw_mipi_csi2rx_disable_streams,
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v6 5/9] media: synopsys: Use media_async_register_subdev() to simplify code
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
` (3 preceding siblings ...)
2026-06-24 20:37 ` [PATCH v6 4/9] media: synopsys: Use v4l2_subdev_get_frame_desc_passthrough() Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
2026-06-24 21:07 ` sashiko-bot
2026-06-24 20:37 ` [PATCH v6 6/9] dt-bindings: media: add i.MX parallel CPI support Frank.Li
` (3 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel
From: Frank Li <Frank.Li@nxp.com>
Use the media_async_register_subdev() to simplify the driver.
Replace the local subdev registration and media pad setup code with
media_async_register_subdev(). Reduce boilerplate code and aligns the
driver with the common pattern used by simple subdevices that each media
pad has one endpoint in fwnode.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change in v6
- use media_async_register_subdev()
- remove sd_1to1.
change in v5
new patch
previous method:
https://lore.kernel.org/imx/20260226-v4l2_init_register-v2-2-902d7140f9fa@nxp.com/
---
drivers/media/platform/synopsys/dw-mipi-csi2rx.c | 178 +++++------------------
1 file changed, 40 insertions(+), 138 deletions(-)
diff --git a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
index f51367409ff46..0fabc89a49b80 100644
--- a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
+++ b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
@@ -78,12 +78,6 @@ enum dw_mipi_csi2rx_regs_index {
DW_MIPI_CSI2RX_MAX,
};
-enum {
- DW_MIPI_CSI2RX_PAD_SINK,
- DW_MIPI_CSI2RX_PAD_SRC,
- DW_MIPI_CSI2RX_PAD_MAX,
-};
-
struct dw_mipi_csi2rx_device;
struct dw_mipi_csi2rx_drvdata {
@@ -112,12 +106,8 @@ struct dw_mipi_csi2rx_device {
const struct dw_mipi_csi2rx_format *formats;
unsigned int formats_num;
- struct media_pad pads[DW_MIPI_CSI2RX_PAD_MAX];
- struct v4l2_async_notifier notifier;
struct v4l2_subdev sd;
- enum v4l2_mbus_type bus_type;
- u32 lanes_num;
u64 enabled_streams;
const struct dw_mipi_csi2rx_drvdata *drvdata;
@@ -360,9 +350,10 @@ dw_mipi_csi2rx_find_format(struct dw_mipi_csi2rx_device *csi2, u32 mbus_code)
static int dw_mipi_csi2rx_start(struct dw_mipi_csi2rx_device *csi2)
{
+ struct media_pad *sink_pad = &csi2->sd.entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
+ u32 lanes = sink_pad->vep.bus.mipi_csi2.num_data_lanes;
struct media_pad *source_pad;
union phy_configure_opts opts;
- u32 lanes = csi2->lanes_num;
u32 control = 0;
s64 link_freq;
int ret;
@@ -370,8 +361,7 @@ static int dw_mipi_csi2rx_start(struct dw_mipi_csi2rx_device *csi2)
if (lanes < 1 || lanes > 4)
return -EINVAL;
- source_pad = media_pad_remote_pad_unique(
- &csi2->pads[DW_MIPI_CSI2RX_PAD_SINK]);
+ source_pad = media_pad_remote_pad_unique(sink_pad);
if (IS_ERR(source_pad))
return PTR_ERR(source_pad);
@@ -380,7 +370,7 @@ static int dw_mipi_csi2rx_start(struct dw_mipi_csi2rx_device *csi2)
if (link_freq < 0)
return link_freq;
- switch (csi2->bus_type) {
+ switch (sink_pad->vep.bus_type) {
case V4L2_MBUS_CSI2_DPHY:
ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq * 2,
lanes, &opts.mipi_dphy);
@@ -458,16 +448,16 @@ dw_mipi_csi2rx_enum_mbus_code(struct v4l2_subdev *sd,
struct dw_mipi_csi2rx_device *csi2 = to_csi2(sd);
switch (code->pad) {
- case DW_MIPI_CSI2RX_PAD_SRC:
+ case V4L2_SUBDEV_1TO1_PADS_SOURCE:
if (code->index)
return -EINVAL;
code->code =
v4l2_subdev_state_get_format(sd_state,
- DW_MIPI_CSI2RX_PAD_SINK)->code;
+ V4L2_SUBDEV_1TO1_PADS_SINK)->code;
return 0;
- case DW_MIPI_CSI2RX_PAD_SINK:
+ case V4L2_SUBDEV_1TO1_PADS_SINK:
if (code->index >= csi2->formats_num)
return -EINVAL;
@@ -487,7 +477,7 @@ static int dw_mipi_csi2rx_set_fmt(struct v4l2_subdev *sd,
struct v4l2_mbus_framefmt *sink, *src;
/* the format on the source pad always matches the sink pad */
- if (format->pad == DW_MIPI_CSI2RX_PAD_SRC)
+ if (format->pad == V4L2_SUBDEV_1TO1_PADS_SOURCE)
return v4l2_subdev_get_fmt(sd, state, format);
sink = v4l2_subdev_state_get_format(state, format->pad, format->stream);
@@ -549,12 +539,12 @@ static int dw_mipi_csi2rx_enable_streams(struct v4l2_subdev *sd,
u64 mask;
int ret;
- sink_pad = &sd->entity.pads[DW_MIPI_CSI2RX_PAD_SINK];
+ sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
remote_pad = media_pad_remote_pad_first(sink_pad);
remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
- mask = v4l2_subdev_state_xlate_streams(state, DW_MIPI_CSI2RX_PAD_SINK,
- DW_MIPI_CSI2RX_PAD_SRC,
+ mask = v4l2_subdev_state_xlate_streams(state, V4L2_SUBDEV_1TO1_PADS_SINK,
+ V4L2_SUBDEV_1TO1_PADS_SOURCE,
&streams_mask);
if (!csi2->enabled_streams) {
@@ -608,12 +598,12 @@ static int dw_mipi_csi2rx_disable_streams(struct v4l2_subdev *sd,
u64 mask;
int ret;
- sink_pad = &sd->entity.pads[DW_MIPI_CSI2RX_PAD_SINK];
+ sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
remote_pad = media_pad_remote_pad_first(sink_pad);
remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
- mask = v4l2_subdev_state_xlate_streams(state, DW_MIPI_CSI2RX_PAD_SINK,
- DW_MIPI_CSI2RX_PAD_SRC,
+ mask = v4l2_subdev_state_xlate_streams(state, V4L2_SUBDEV_1TO1_PADS_SINK,
+ V4L2_SUBDEV_1TO1_PADS_SOURCE,
&streams_mask);
ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask);
@@ -649,9 +639,9 @@ static int dw_mipi_csi2rx_init_state(struct v4l2_subdev *sd,
{
struct v4l2_subdev_route routes[] = {
{
- .sink_pad = DW_MIPI_CSI2RX_PAD_SINK,
+ .sink_pad = V4L2_SUBDEV_1TO1_PADS_SINK,
.sink_stream = 0,
- .source_pad = DW_MIPI_CSI2RX_PAD_SRC,
+ .source_pad = V4L2_SUBDEV_1TO1_PADS_SOURCE,
.source_stream = 0,
.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
},
@@ -666,95 +656,38 @@ static int dw_mipi_csi2rx_init_state(struct v4l2_subdev *sd,
&default_format);
}
-static const struct v4l2_subdev_internal_ops dw_mipi_csi2rx_internal_ops = {
- .init_state = dw_mipi_csi2rx_init_state,
-};
-
-static int dw_mipi_csi2rx_notifier_bound(struct v4l2_async_notifier *notifier,
- struct v4l2_subdev *sd,
- struct v4l2_async_connection *asd)
+static int dw_mipi_set_pad_by_ep(struct v4l2_subdev *sd, struct media_pad *pad)
{
- struct dw_mipi_csi2rx_device *csi2 =
- container_of(notifier, struct dw_mipi_csi2rx_device, notifier);
- struct media_pad *sink_pad = &csi2->pads[DW_MIPI_CSI2RX_PAD_SINK];
- int ret;
-
- ret = v4l2_create_fwnode_links_to_pad(sd, sink_pad,
- MEDIA_LNK_FL_ENABLED);
- if (ret) {
- dev_err(csi2->dev, "failed to link source pad of %s\n",
- sd->name);
- return ret;
- }
-
- return 0;
-}
+ struct v4l2_fwnode_endpoint *vep = &pad->vep;
-static const struct v4l2_async_notifier_operations dw_mipi_csi2rx_notifier_ops = {
- .bound = dw_mipi_csi2rx_notifier_bound,
-};
-
-static int dw_mipi_csi2rx_register_notifier(struct dw_mipi_csi2rx_device *csi2)
-{
- struct v4l2_async_connection *asd;
- struct v4l2_async_notifier *ntf = &csi2->notifier;
- struct v4l2_fwnode_endpoint vep;
- struct v4l2_subdev *sd = &csi2->sd;
- struct device *dev = csi2->dev;
- int ret;
-
- struct fwnode_handle *ep __free(fwnode_handle) =
- fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
- if (!ep)
- return dev_err_probe(dev, -ENODEV, "failed to get endpoint\n");
-
- vep.bus_type = V4L2_MBUS_UNKNOWN;
- ret = v4l2_fwnode_endpoint_parse(ep, &vep);
- if (ret)
- return dev_err_probe(dev, ret, "failed to parse endpoint\n");
-
- if (vep.bus_type != V4L2_MBUS_CSI2_DPHY &&
- vep.bus_type != V4L2_MBUS_CSI2_CPHY)
- return dev_err_probe(dev, -EINVAL,
- "invalid bus type of endpoint\n");
-
- csi2->bus_type = vep.bus_type;
- csi2->lanes_num = vep.bus.mipi_csi2.num_data_lanes;
+ if (vep->base.port == V4L2_SUBDEV_1TO1_PADS_SINK) {
+ if (vep->bus_type != V4L2_MBUS_CSI2_DPHY &&
+ vep->bus_type != V4L2_MBUS_CSI2_CPHY)
+ return -EINVAL;
- v4l2_async_subdev_nf_init(ntf, sd);
- ntf->ops = &dw_mipi_csi2rx_notifier_ops;
+ pad->flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
- asd = v4l2_async_nf_add_fwnode_remote(ntf, ep,
- struct v4l2_async_connection);
- if (IS_ERR(asd)) {
- ret = PTR_ERR(asd);
- goto err_nf_cleanup;
+ return 0;
}
- ret = v4l2_async_nf_register(ntf);
- if (ret) {
- ret = dev_err_probe(dev, ret, "failed to register notifier\n");
- goto err_nf_cleanup;
+ if (vep->base.port == V4L2_SUBDEV_1TO1_PADS_SOURCE) {
+ pad->flags = MEDIA_PAD_FL_SOURCE | MEDIA_PAD_FL_MUST_CONNECT;
+ return 0;
}
- return 0;
-
-err_nf_cleanup:
- v4l2_async_nf_cleanup(ntf);
-
- return ret;
+ return -EINVAL;
}
+static const struct v4l2_subdev_internal_ops dw_mipi_csi2rx_internal_ops = {
+ .init_state = dw_mipi_csi2rx_init_state,
+ .set_pad_by_ep = dw_mipi_set_pad_by_ep,
+};
+
static int dw_mipi_csi2rx_register(struct dw_mipi_csi2rx_device *csi2)
{
- struct media_pad *pads = csi2->pads;
struct v4l2_subdev *sd = &csi2->sd;
int ret;
- ret = dw_mipi_csi2rx_register_notifier(csi2);
- if (ret)
- goto err;
-
v4l2_subdev_init(sd, &dw_mipi_csi2rx_ops);
sd->dev = csi2->dev;
sd->entity.ops = &dw_mipi_csi2rx_media_ops;
@@ -764,45 +697,12 @@ static int dw_mipi_csi2rx_register(struct dw_mipi_csi2rx_device *csi2)
snprintf(sd->name, sizeof(sd->name), "dw-mipi-csi2rx %s",
dev_name(csi2->dev));
- pads[DW_MIPI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK |
- MEDIA_PAD_FL_MUST_CONNECT;
- pads[DW_MIPI_CSI2RX_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE;
- ret = media_entity_pads_init(&sd->entity, DW_MIPI_CSI2RX_PAD_MAX, pads);
+ ret = media_async_register_subdev(&csi2->sd);
if (ret)
- goto err_notifier_unregister;
-
- ret = v4l2_subdev_init_finalize(sd);
- if (ret)
- goto err_entity_cleanup;
-
- ret = v4l2_async_register_subdev(sd);
- if (ret) {
- dev_err(sd->dev, "failed to register CSI-2 subdev\n");
- goto err_subdev_cleanup;
- }
+ return dev_err_probe(sd->dev, ret,
+ "failed to register CSI-2 subdev\n");
return 0;
-
-err_subdev_cleanup:
- v4l2_subdev_cleanup(sd);
-err_entity_cleanup:
- media_entity_cleanup(&sd->entity);
-err_notifier_unregister:
- v4l2_async_nf_unregister(&csi2->notifier);
- v4l2_async_nf_cleanup(&csi2->notifier);
-err:
- return ret;
-}
-
-static void dw_mipi_csi2rx_unregister(struct dw_mipi_csi2rx_device *csi2)
-{
- struct v4l2_subdev *sd = &csi2->sd;
-
- v4l2_async_unregister_subdev(sd);
- v4l2_subdev_cleanup(sd);
- media_entity_cleanup(&sd->entity);
- v4l2_async_nf_unregister(&csi2->notifier);
- v4l2_async_nf_cleanup(&csi2->notifier);
}
static void imx93_csi2rx_dphy_assert_reset(struct dw_mipi_csi2rx_device *csi2)
@@ -879,12 +779,14 @@ static void imx93_csi2rx_dphy_ipi_enable(struct dw_mipi_csi2rx_device *csi2)
static int imx93_csi2rx_wait_for_phy_stopstate(struct dw_mipi_csi2rx_device *csi2)
{
+ struct media_pad *sink_pad = &csi2->sd.entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
+ u32 num_lanes = sink_pad->vep.bus.mipi_csi2.num_data_lanes;
struct device *dev = csi2->dev;
u32 stopstate_mask;
u32 val;
int ret;
- stopstate_mask = DPHY_STOPSTATE_CLK_LANE | GENMASK(csi2->lanes_num - 1, 0);
+ stopstate_mask = DPHY_STOPSTATE_CLK_LANE | GENMASK(num_lanes - 1, 0);
ret = read_poll_timeout(dw_mipi_csi2rx_read, val,
(val & stopstate_mask) == stopstate_mask,
@@ -993,7 +895,7 @@ static void dw_mipi_csi2rx_remove(struct platform_device *pdev)
{
struct dw_mipi_csi2rx_device *csi2 = platform_get_drvdata(pdev);
- dw_mipi_csi2rx_unregister(csi2);
+ media_async_subdev_cleanup(&csi2->sd);
phy_exit(csi2->phy);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 5/9] media: synopsys: Use media_async_register_subdev() to simplify code
2026-06-24 20:37 ` [PATCH v6 5/9] media: synopsys: Use media_async_register_subdev() to simplify code Frank.Li
@ 2026-06-24 21:07 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-06-24 21:07 UTC (permalink / raw)
To: Frank.Li; +Cc: devicetree, Frank.Li, robh, imx, conor+dt
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
New issues:
- [Critical] Critical out-of-bounds heap write in v4l2_async_nf_parse_fwnode() due to mismatched endpoint counting.
- [High] Mixed goto-based error handling and __free() cleanup violates subsystem rules and leaves a dangling pointer.
- [High] Hardcoded array indexing for pads mismatches the dynamic Device Tree enumeration order.
- [High] Heap out-of-bounds access and routing validation failure on single-endpoint Device Trees.
- [High] Memory leak of `sd->active_state` on subdev registration failure.
- [High] Unconditional memory leak of `sd->subdev_notifier` during driver removal.
- [High] Leaked async connections and fwnode references on error path.
Pre-existing issues:
- [High] Lockless traversal of the media entity links list causes a race condition.
--
commit 892732bd4672cbe12751cfc2979a3f508c5cde6d
Author: Frank Li <Frank.Li@nxp.com>
Subject: media: synopsys: Use media_async_register_subdev() to simplify code
Use the media_async_register_subdev() to simplify the driver.
Replace the local subdev registration and media pad setup code with
media_async_register_subdev(). Reduce boilerplate code and aligns the
driver with the common pattern used by simple subdevices that each media
pad has one endpoint in fwnode.
> diff --git a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> index f51367409ff46..0fabc89a49b80 100644
> --- a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> +++ b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
[ ... ]
> @@ -360,9 +350,10 @@ dw_mipi_csi2rx_find_format(struct dw_mipi_csi2rx_device *csi2, u32 mbus_code)
>
> static int dw_mipi_csi2rx_start(struct dw_mipi_csi2rx_device *csi2)
> {
> + struct media_pad *sink_pad = &csi2->sd.entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
[Severity: High]
Does this hardcoded index match the dynamic pad allocation from the Device
Tree?
Because v4l2_async_nf_parse_fwnode() populates the pads array strictly in the
order endpoints appear in the Device Tree, accessing index
V4L2_SUBDEV_1TO1_PADS_SINK unconditionally here assumes the sink pad is always
at index 0.
If a platform's Device Tree lists the source endpoint before the sink
endpoint, will the driver access the wrong pad and potentially read incorrect
hardware parameters (like lane counts)?
> + u32 lanes = sink_pad->vep.bus.mipi_csi2.num_data_lanes;
> struct media_pad *source_pad;
[ ... ]
> @@ -549,12 +539,12 @@ static int dw_mipi_csi2rx_enable_streams(struct v4l2_subdev *sd,
> u64 mask;
> int ret;
>
> - sink_pad = &sd->entity.pads[DW_MIPI_CSI2RX_PAD_SINK];
> + sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
> remote_pad = media_pad_remote_pad_first(sink_pad);
[Severity: High]
This isn't a bug introduced by this patch, but does this lockless list
traversal race with userspace ioctls?
Calling media_pad_remote_pad_first() iterates the media entity links list:
drivers/media/mc/mc-entity.c:media_pad_remote_pad_first() {
...
for_each_media_entity_data_link(pad->entity, link) {
...
}
Since v4l2_subdev_enable_streams() does not acquire the global
mdev->graph_mutex, could this traversal race with userspace ioctls like
MEDIA_IOC_SETUP_LINK that mutate the graph structure, leading to linked list
corruption or a use-after-free?
> remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
[ ... ]
> @@ -649,9 +639,9 @@ static int dw_mipi_csi2rx_init_state(struct v4l2_subdev *sd,
> {
> struct v4l2_subdev_route routes[] = {
> {
> - .sink_pad = DW_MIPI_CSI2RX_PAD_SINK,
> + .sink_pad = V4L2_SUBDEV_1TO1_PADS_SINK,
> .sink_stream = 0,
> - .source_pad = DW_MIPI_CSI2RX_PAD_SRC,
> + .source_pad = V4L2_SUBDEV_1TO1_PADS_SOURCE,
[Severity: High]
Could this cause an out-of-bounds heap access if the Device Tree only defines
a single endpoint?
When __media_async_register_subdev() dynamically sizes the pads array based
on ep_count, a Device Tree with only a sink endpoint will result in an array
of size 1.
Since the driver unconditionally sets up active routes referring to
V4L2_SUBDEV_1TO1_PADS_SOURCE (index 1), won't this attempt to access pads[1]
on a size-1 array?
> .source_stream = 0,
> .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> },
[ ... ]
> @@ -764,45 +697,12 @@ static int dw_mipi_csi2rx_register(struct dw_mipi_csi2rx_device *csi2)
> snprintf(sd->name, sizeof(sd->name), "dw-mipi-csi2rx %s",
> dev_name(csi2->dev));
>
> - pads[DW_MIPI_CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK |
> - MEDIA_PAD_FL_MUST_CONNECT;
> - pads[DW_MIPI_CSI2RX_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE;
> - ret = media_entity_pads_init(&sd->entity, DW_MIPI_CSI2RX_PAD_MAX, pads);
> + ret = media_async_register_subdev(&csi2->sd);
[Severity: High]
Does calling media_async_register_subdev() here expose a dangling pointer due
to mixed goto and __free() usage in __media_async_register_subdev()?
In drivers/media/v4l2-core/v4l2-fwnode.c:__media_async_register_subdev(), the
pads array is bound to scope-based cleanup and assigned to sd->entity.pads:
struct media_pad *pads __free(kfree) = kzalloc_objs(struct media_pad,
ep_count);
...
sd->entity.pads = pads;
If __v4l2_async_register_subdev() fails, the code jumps to out_unregister:
out_unregister:
v4l2_async_nf_unregister(notifier);
Because the function returns, pads is automatically freed, but sd->entity.pads
is not set to NULL. Does this leave a dangling pointer in the subdevice?
[Severity: High]
Could media_async_register_subdev() leak the active state if registration
fails?
In __media_async_register_subdev(), v4l2_subdev_init_finalize() dynamically
allocates sd->active_state. If the subsequent call to
__v4l2_async_register_subdev() fails:
ret = v4l2_subdev_init_finalize(sd);
if (ret)
goto out_unregister;
ret = __v4l2_async_register_subdev(sd, module);
if (ret < 0)
goto out_unregister;
The error path jumps to out_unregister and returns without calling
v4l2_subdev_cleanup(). Does this bypass freeing the active state?
[Severity: Critical]
Can this call trigger a heap out-of-bounds write if the Device Tree contains
disabled or disconnected endpoints?
In __media_async_register_subdev(), the pads array is allocated based on
ep_count, which only counts available connected endpoints (flags=0):
struct media_pad *pads __free(kfree) = kzalloc_objs(struct media_pad,
ep_count);
However, v4l2_async_nf_parse_fwnode() iterates over all endpoints without
filtering:
fwnode_graph_for_each_endpoint_scoped(dev_fwnode(dev), ep) {
...
ret = v4l2_fwnode_endpoint_parse(ep, &pad->vep);
If there are disconnected endpoints in the Device Tree, the loop will iterate
more times than ep_count. Will this increment the pad pointer beyond the
allocated array and overwrite adjacent memory?
[Severity: High]
Does media_async_register_subdev() leak async connections and fwnode
references if an error occurs during endpoint parsing?
In v4l2_async_nf_parse_fwnode(), newly allocated async connections are added
to the notifier. If an error occurs in a subsequent loop iteration:
if (IS_ERR(asd))
return dev_err_probe(dev, PTR_ERR(asd), "failed to add notifier\n");
The function returns directly. Then in __media_async_register_subdev():
ret = v4l2_async_nf_parse_fwnode(dev, pads, notifier);
if (ret < 0)
return ret;
This bypasses the out_cleanup label and fails to call v4l2_async_nf_cleanup(),
which seems to leak everything added to the notifier so far.
> if (ret)
> - goto err_notifier_unregister;
[ ... ]
> @@ -993,7 +895,7 @@ static void dw_mipi_csi2rx_remove(struct platform_device *pdev)
> {
> struct dw_mipi_csi2rx_device *csi2 = platform_get_drvdata(pdev);
>
> - dw_mipi_csi2rx_unregister(csi2);
> + media_async_subdev_cleanup(&csi2->sd);
[Severity: High]
Does media_async_subdev_cleanup() unconditionally leak the subdevice notifier?
The function __media_async_register_subdev() dynamically allocates the
notifier and assigns it to sd->subdev_notifier. During teardown here,
media_async_subdev_cleanup() calls v4l2_async_nf_cleanup() to clean the lists:
drivers/media/v4l2-core/v4l2-fwnode.c:media_async_subdev_cleanup() {
...
v4l2_async_nf_cleanup(sd->subdev_notifier);
...
}
However, it does not appear to call kfree(sd->subdev_notifier). Will this
leak the notifier structure every time the module is unbound or removed?
> phy_exit(csi2->phy);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=5
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 6/9] dt-bindings: media: add i.MX parallel CPI support
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
` (4 preceding siblings ...)
2026-06-24 20:37 ` [PATCH v6 5/9] media: synopsys: Use media_async_register_subdev() to simplify code Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
2026-06-24 20:57 ` sashiko-bot
2026-06-24 20:37 ` [PATCH v6 7/9] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI) Frank.Li
` (2 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel, Alice Yuan, Krzysztof Kozlowski
From: Alice Yuan <alice.yuan@nxp.com>
Document the binding for parallel CPI controller found in i.MX8QXP, i.MX93
and i.MX91 SoCs.
Signed-off-by: Alice Yuan <alice.yuan@nxp.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Chagnes in v4
- add Laurent Pinchart's review by tag
- fix $ref: /schemas/graph.yaml#/$defs/port-base, original is
$ref: /schemas/graph.yaml#/properties/port-base
Change in v3:
- use enum at compatible string
- add ref to video-interfaces.yaml#
- use cpi as node name in examples.
- replace csi (Camera Serial Interface) with CPI (Camera Parallel Interface)
in commit message.
Change in v2:
- use pcif surfix as Laurent Pinchart's suggest.
- put power-domains into required list
---
.../devicetree/bindings/media/fsl,imx93-pcif.yaml | 126 +++++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 127 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/fsl,imx93-pcif.yaml b/Documentation/devicetree/bindings/media/fsl,imx93-pcif.yaml
new file mode 100644
index 0000000000000..9dd0331f6ef75
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/fsl,imx93-pcif.yaml
@@ -0,0 +1,126 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/fsl,imx93-pcif.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: i.MX8/9 Parallel Camera Interface
+
+maintainers:
+ - Frank Li <Frank.Li@nxp.com>
+
+description: |
+ This is device node for the Parallel Camera Interface which enables the
+ chip to connect directly to external Parallel CMOS image sensors.
+ Supports up to 80MHz input clock from sensor.
+ Supports the following input data formats
+ - 8-bit/10-bit Camera Sensor Interface (CSI)
+ - 8-bit data port for RGB, YCbCr, and YUV data input
+ - 8-bit/10-bit data ports for Bayer data input
+ Parallel Camera Interface is hooked to the Imaging subsystem via the
+ Pixel Link.
+
+properties:
+ compatible:
+ oneOf:
+ - enum:
+ - fsl,imx8qxp-pcif
+ - fsl,imx93-pcif
+ - items:
+ - enum:
+ - fsl,imx91-pcif
+ - const: fsl,imx93-pcif
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 2
+
+ clock-names:
+ items:
+ - const: pixel
+ - const: ipg
+
+ power-domains:
+ maxItems: 1
+
+ ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+
+ properties:
+ port@0:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description: Input port node.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ bus-type:
+ const: 5
+
+ port@1:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description: Output port node.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ bus-type:
+ const: 5
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - power-domains
+ - ports
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/imx93-clock.h>
+ #include <dt-bindings/power/fsl,imx93-power.h>
+
+ cpi@4ac10070 {
+ compatible = "fsl,imx93-pcif";
+ reg = <0x4ac10070 0x10>;
+ clocks = <&clk IMX93_CLK_MIPI_CSI_GATE>,
+ <&clk IMX93_CLK_MEDIA_APB>;
+ clock-names = "pixel", "ipg";
+ assigned-clocks = <&clk IMX93_CLK_CAM_PIX>;
+ assigned-clock-parents = <&clk IMX93_CLK_VIDEO_PLL>;
+ assigned-clock-rates = <140000000>;
+ power-domains = <&media_blk_ctrl IMX93_MEDIABLK_PD_MIPI_CSI>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ endpoint {
+ remote-endpoint = <&mt9m114_ep>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+ endpoint {
+ remote-endpoint = <&isi_in>;
+ };
+ };
+ };
+ };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 895a87b571c35..51d5c62e3fdea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16258,6 +16258,7 @@ L: linux-media@vger.kernel.org
S: Maintained
T: git git://linuxtv.org/media.git
F: Documentation/admin-guide/media/imx7.rst
+F: Documentation/devicetree/bindings/media/fsl,imx93-pcif.yaml
F: Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
F: Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
F: Documentation/devicetree/bindings/media/nxp,imx8mq-mipi-csi2.yaml
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 6/9] dt-bindings: media: add i.MX parallel CPI support
2026-06-24 20:37 ` [PATCH v6 6/9] dt-bindings: media: add i.MX parallel CPI support Frank.Li
@ 2026-06-24 20:57 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-06-24 20:57 UTC (permalink / raw)
To: Frank.Li; +Cc: robh, Frank.Li, conor+dt, imx, devicetree
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unhandled resource leaks (memory and fwnode references) and double-free risk in `__media_async_register_subdev` and its cleanup path.
--
commit 050e10af8571659357b0d6bb808eab4e58871636
Author: Frank Li <Frank.Li@nxp.com>
media: subdev: Add media_async_register_subdev() helper
This commit adds media_async_register_subdev(), a helper to register a V4L2
sub-device with the asynchronous sub-device framework. It parses the firmware
graph, creates media pads for endpoints, and registers asynchronous notifiers.
This centralizes common initialization and link creation to reduce boilerplate.
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 62a3a452f7884..169059654478f 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -1302,6 +1303,160 @@ int __v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd, struct module *m
> }
> EXPORT_SYMBOL_GPL(__v4l2_async_register_subdev_sensor);
>
[ ... ]
> +void media_async_subdev_cleanup(struct v4l2_subdev *sd)
> +{
> + v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> + media_entity_cleanup(&sd->entity);
> + v4l2_async_nf_unregister(sd->subdev_notifier);
> + v4l2_async_nf_cleanup(sd->subdev_notifier);
> + kfree(sd->entity.pads);
> +}
> +EXPORT_SYMBOL_GPL(media_async_subdev_cleanup);
[Severity: High]
Does this cleanup function leak fwnode references?
v4l2_async_nf_parse_fwnode() parses endpoints directly into pad->vep, taking
an fwnode reference via fwnode_handle_get(). It appears this cleanup function
fails to iterate over the pads to call v4l2_fwnode_endpoint_free(&pad->vep) to
release these references.
Also, if a driver explicitly invokes this cleanup helper to recover after
__media_async_register_subdev() fails, will kfree(sd->entity.pads) cause a
double-free panic?
> +
> +int __media_async_register_subdev(struct v4l2_subdev *sd, struct module *module)
> +{
> + struct device *dev = sd->dev;
> + u32 ep_count;
> + int ret;
> +
[ ... ]
> + struct media_pad *pads __free(kfree) = kzalloc_objs(struct media_pad, ep_count);
> + if (!pads)
> + return -ENOMEM;
> +
> + ret = v4l2_async_nf_parse_fwnode(dev, pads, notifier);
> + if (ret < 0)
> + return ret;
> +
> + ret = media_entity_pads_init(&sd->entity, ep_count, pads);
> + if (ret)
> + goto out_cleanup;
> +
> + ret = v4l2_async_nf_register(notifier);
> + if (ret < 0)
> + goto out_cleanup;
> +
> + ret = v4l2_subdev_init_finalize(sd);
> + if (ret)
> + goto out_unregister;
> +
> + ret = __v4l2_async_register_subdev(sd, module);
> + if (ret < 0)
> + goto out_unregister;
> +
> + sd->subdev_notifier = no_free_ptr(notifier);
> + retain_and_null_ptr(pads);
> +
> + return 0;
> +
> +out_unregister:
> + v4l2_async_nf_unregister(notifier);
> +
> +out_cleanup:
> + v4l2_async_nf_cleanup(notifier);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(__media_async_register_subdev);
[Severity: High]
If __v4l2_async_register_subdev() fails, the error path returns without
calling v4l2_subdev_cleanup(sd) to free the active state allocated by
v4l2_subdev_init_finalize(). Can this leak sd->active_state?
Additionally, pads is allocated with __free(kfree) and linked to
sd->entity.pads. On a registration failure, pads is automatically freed when
the function returns, but sd->entity.pads is not cleared.
Could this leave a dangling pointer that causes a double-free if error
recovery is attempted?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=6
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 7/9] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
` (5 preceding siblings ...)
2026-06-24 20:37 ` [PATCH v6 6/9] dt-bindings: media: add i.MX parallel CPI support Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
2026-06-24 21:03 ` sashiko-bot
2026-06-24 20:37 ` [PATCH v6 8/9] arm64: dts: imx8: add camera parallel interface (CPI) node Frank.Li
2026-06-24 20:37 ` [PATCH v6 9/9] arm64: dts: imx8qxp-mek: add parallel ov5640 camera support Frank.Li
8 siblings, 1 reply; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel, Alice Yuan, Robert Chiras, Zhipeng Wang
From: Alice Yuan <alice.yuan@nxp.com>
Add a V4L2 sub-device driver for the CPI controller found on i.MX8QXP,
i.MX8QM, and i.MX93 SoCs. This controller supports parallel camera sensors
and enables image data capture through a parallel interface.
Signed-off-by: Alice Yuan <alice.yuan@nxp.com>
Signed-off-by: Robert Chiras <robert.chiras@nxp.com>
Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change in v6
- Don't use subdev_1to1
- use new api media_async_register_subdev()
- remove used switch -case
Change in v5
- Use subdev_1to1 register function
- Use v4l2_subdev_get_frame_desc_passthrough
- Use dwc csi2 similar logic enable/disable stream
- Add route settup at imx_cpi_init_state()
- Remove V2 register layout support, add it later
change in v4
- remove unnecesary header file.
- use devm_bulk_clk_get().
- update kConfig i.MX8/i.MX9
- Remove define IMX_CPI_DEF_PIX_WIDTH ..., which used once only
- drop get_interface_ctrl_reg1_param
- drop uv-swap
- drop imx_cpi_link_setup by use immutable link.
- use enable/disable_stream() replace depericated .s_stream.
- remove dbg print and reg dump functions.
- use goto/.remove() to do manual cleanup.
- remove imx93 support. Add it later.
change in v3
- replace csi with cpi
- use __free(fwnode_handle) to simpilfy code
- remove imx91 driver data, which is the same as imx93
change in v2
- remove MODULE_ALIAS
- use devm_pm_runtime_enable() and cleanup remove function
- change output format to 1x16. controller convert 2x8 to 1x16 format
---
MAINTAINERS | 1 +
drivers/media/platform/nxp/Kconfig | 12 +
drivers/media/platform/nxp/Makefile | 1 +
drivers/media/platform/nxp/imx-parallel-cpi.c | 629 ++++++++++++++++++++++++++
4 files changed, 643 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 51d5c62e3fdea..045a06d0bb216 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16263,6 +16263,7 @@ F: Documentation/devicetree/bindings/media/nxp,imx-mipi-csi2.yaml
F: Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
F: Documentation/devicetree/bindings/media/nxp,imx8mq-mipi-csi2.yaml
F: drivers/media/platform/nxp/imx-mipi-csis.c
+F: drivers/media/platform/nxp/imx-parallel-cpi.c
F: drivers/media/platform/nxp/imx7-media-csi.c
F: drivers/media/platform/nxp/imx8mq-mipi-csi2.c
diff --git a/drivers/media/platform/nxp/Kconfig b/drivers/media/platform/nxp/Kconfig
index 40e3436669e21..90f7c792003f2 100644
--- a/drivers/media/platform/nxp/Kconfig
+++ b/drivers/media/platform/nxp/Kconfig
@@ -39,6 +39,18 @@ config VIDEO_IMX_MIPI_CSIS
Video4Linux2 sub-device driver for the MIPI CSI-2 CSIS receiver
v3.3/v3.6.3 found on some i.MX7 and i.MX8 SoCs.
+config VIDEO_IMX_PARALLEL_CPI
+ tristate "NXP i.MX8/i.MX9 Parallel CPI Driver"
+ depends on ARCH_MXC || COMPILE_TEST
+ depends on VIDEO_DEV
+ select MEDIA_CONTROLLER
+ select V4L2_1TO1
+ select V4L2_FWNODE
+ select VIDEO_V4L2_SUBDEV_API
+ help
+ Video4Linux2 sub-device driver for PARALLEL CPI receiver found
+ on some iMX8 and iMX9 SoCs.
+
source "drivers/media/platform/nxp/imx8-isi/Kconfig"
# mem2mem drivers
diff --git a/drivers/media/platform/nxp/Makefile b/drivers/media/platform/nxp/Makefile
index 4d90eb7136525..5346919d2f108 100644
--- a/drivers/media/platform/nxp/Makefile
+++ b/drivers/media/platform/nxp/Makefile
@@ -7,5 +7,6 @@ obj-y += imx8-isi/
obj-$(CONFIG_VIDEO_IMX7_CSI) += imx7-media-csi.o
obj-$(CONFIG_VIDEO_IMX8MQ_MIPI_CSI2) += imx8mq-mipi-csi2.o
obj-$(CONFIG_VIDEO_IMX_MIPI_CSIS) += imx-mipi-csis.o
+obj-$(CONFIG_VIDEO_IMX_PARALLEL_CPI) += imx-parallel-cpi.o
obj-$(CONFIG_VIDEO_IMX_PXP) += imx-pxp.o
obj-$(CONFIG_VIDEO_MX2_EMMAPRP) += mx2_emmaprp.o
diff --git a/drivers/media/platform/nxp/imx-parallel-cpi.c b/drivers/media/platform/nxp/imx-parallel-cpi.c
new file mode 100644
index 0000000000000..0f74b51608715
--- /dev/null
+++ b/drivers/media/platform/nxp/imx-parallel-cpi.c
@@ -0,0 +1,629 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * i.MX Parallel CPI receiver driver.
+ *
+ * Copyright 2019-2025 NXP
+ *
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#include <media/v4l2-common.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-mc.h>
+#include <media/v4l2-subdev.h>
+
+/* CI_PI INTERFACE CONTROL */
+#define IF_CTRL_REG_PL_ENABLE BIT(0)
+#define IF_CTRL_REG_PL_VALID BIT(1)
+#define IF_CTRL_REG_DATA_TYPE_SEL BIT(8)
+#define IF_CTRL_REG_DATA_TYPE(x) FIELD_PREP(GENMASK(13, 9), (x))
+
+#define DATA_TYPE_OUT_NULL 0x00
+#define DATA_TYPE_OUT_RGB 0x04
+#define DATA_TYPE_OUT_YUV444 0x08
+#define DATA_TYPE_OUT_YYU420_ODD 0x10
+#define DATA_TYPE_OUT_YYU420_EVEN 0x12
+#define DATA_TYPE_OUT_YYY_ODD 0x18
+#define DATA_TYPE_OUT_UYVY_EVEN 0x1a
+#define DATA_TYPE_OUT_RAW 0x1c
+
+#define IF_CTRL_REG_IF_FORCE_HSYNV_OVERRIDE 0x4
+#define IF_CTRL_REG_IF_FORCE_VSYNV_OVERRIDE 0x2
+#define IF_CTRL_REG_IF_FORCE_DATA_ENABLE_OVERRIDE 0x1
+
+/* CPI INTERFACE CONTROL REG */
+#define CPI_CTRL_REG_CPI_EN BIT(0)
+#define CPI_CTRL_REG_PIXEL_CLK_POL BIT(1)
+#define CPI_CTRL_REG_HSYNC_POL BIT(2)
+#define CPI_CTRL_REG_VSYNC_POL BIT(3)
+#define CPI_CTRL_REG_DE_POL BIT(4)
+#define CPI_CTRL_REG_PIXEL_DATA_POL BIT(5)
+#define CPI_CTRL_REG_CCIR_EXT_VSYNC_EN BIT(6)
+#define CPI_CTRL_REG_CCIR_EN BIT(7)
+#define CPI_CTRL_REG_CCIR_VIDEO_MODE BIT(8)
+#define CPI_CTRL_REG_CCIR_NTSC_EN BIT(9)
+#define CPI_CTRL_REG_CCIR_VSYNC_RESET_EN BIT(10)
+#define CPI_CTRL_REG_CCIR_ECC_ERR_CORRECT_EN BIT(11)
+#define CPI_CTRL_REG_HSYNC_FORCE_EN BIT(12)
+#define CPI_CTRL_REG_VSYNC_FORCE_EN BIT(13)
+#define CPI_CTRL_REG_GCLK_MODE_EN BIT(14)
+#define CPI_CTRL_REG_VALID_SEL BIT(15)
+#define CPI_CTRL_REG_RAW_OUT_SEL BIT(16)
+#define CPI_CTRL_REG_HSYNC_OUT_SEL BIT(17)
+#define CPI_CTRL_REG_HSYNC_PULSE(x) FIELD_PREP(GENMASK(21, 19), (x))
+#define CPI_CTRL_REG_UV_SWAP_EN BIT(22)
+#define CPI_CTRL_REG_DATA_TYPE_IN(x) FIELD_PREP(GENMASK(26, 23), (x))
+#define CPI_CTRL_REG_MASK_VSYNC_COUNTER(x) FIELD_PREP(GENMASK(28, 27), (x))
+#define CPI_CTRL_REG_SOFTRST BIT(31)
+
+/* CPI INTERFACE STATUS */
+#define CPI_STATUS_FIELD_TOGGLE BIT(0)
+#define CPI_STATUS_ECC_ERROR BIT(1)
+
+/* CPI INTERFACE CONTROL REG1 */
+#define CPI_CTRL_REG1_PIXEL_WIDTH(v) FIELD_PREP(GENMASK(15, 0), (v))
+#define CPI_CTRL_REG1_VSYNC_PULSE(v) FIELD_PREP(GENMASK(31, 16), (v))
+
+#define CPI_CTRL_V2_REG1_PIXEL_WIDTH(v) FIELD_PREP(GENMASK(16, 0), (v))
+#define CPI_CTRL_V2_REG1_VSYNC_PULSE(v) FIELD_PREP(GENMASK(31, 16), (v))
+
+/* Need match field DATA_TYPE_IN definition at CPI CTRL register */
+enum cpi_in_data_type {
+ CPI_IN_DT_UYVY_BT656_8 = 0x0,
+ CPI_IN_DT_UYVY_BT656_10,
+ CPI_IN_DT_RGB_8,
+ CPI_IN_DT_BGR_8,
+ CPI_IN_DT_YVYU_8 = 0x5,
+ CPI_IN_DT_YUV_8,
+ CPI_IN_DT_RAW_8 = 0x9,
+ CPI_IN_DT_RAW_10,
+};
+
+enum {
+ PI_GATE_CLOCK_MODE,
+ PI_CCIR_MODE,
+};
+
+enum {
+ PI_V1,
+};
+
+struct imx_cpi_plat_data {
+ u32 version;
+ u32 if_ctrl_reg;
+ u32 interface_status;
+ u32 interface_ctrl_reg;
+ u32 interface_ctrl_reg1;
+};
+
+struct imx_cpi_device {
+ struct device *dev;
+ void __iomem *regs;
+ struct clk_bulk_data *clks;
+ int num_clks;
+
+ struct v4l2_subdev sd;
+
+ const struct imx_cpi_plat_data *pdata;
+
+ u32 enabled_streams;
+ u8 mode;
+};
+
+struct imx_cpi_pix_format {
+ u32 code;
+ u32 output;
+ u32 data_type;
+ u8 width;
+};
+
+static const struct imx_cpi_pix_format imx_cpi_formats[] = {
+ /* YUV formats. */
+ {
+ .code = MEDIA_BUS_FMT_UYVY8_2X8,
+ .output = MEDIA_BUS_FMT_UYVY8_1X16,
+ .data_type = CPI_IN_DT_UYVY_BT656_8,
+ .width = 16,
+ }, {
+ .code = MEDIA_BUS_FMT_YUYV8_2X8,
+ .output = MEDIA_BUS_FMT_YUYV8_1X16,
+ .data_type = CPI_IN_DT_YVYU_8,
+ .width = 16,
+ },
+};
+
+static const struct imx_cpi_plat_data imx8qxp_pdata = {
+ .version = PI_V1,
+ .if_ctrl_reg = 0x0,
+ .interface_status = 0x20,
+ .interface_ctrl_reg = 0x10,
+ .interface_ctrl_reg1 = 0x30,
+};
+
+static const struct imx_cpi_pix_format *find_imx_cpi_format(u32 code)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(imx_cpi_formats); i++)
+ if (code == imx_cpi_formats[i].code)
+ return &imx_cpi_formats[i];
+
+ return NULL;
+}
+
+static void imx_cpi_sw_reset(struct imx_cpi_device *pcpidev)
+{
+ const struct imx_cpi_plat_data *pdata = pcpidev->pdata;
+ u32 val;
+
+ /* Softwaret Reset */
+ val = readl(pcpidev->regs + pdata->interface_ctrl_reg);
+ val |= CPI_CTRL_REG_SOFTRST;
+ writel(val, pcpidev->regs + pdata->interface_ctrl_reg);
+
+ fsleep(500);
+ val = readl(pcpidev->regs + pdata->interface_ctrl_reg);
+ val &= ~CPI_CTRL_REG_SOFTRST;
+ writel(val, pcpidev->regs + pdata->interface_ctrl_reg);
+}
+
+static void imx_cpi_hw_config(struct imx_cpi_device *pcpidev,
+ const struct imx_cpi_pix_format *pcpidev_fmt)
+{
+ u32 flags = pcpidev->sd.entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK].vep.bus.parallel.flags;
+ const struct imx_cpi_plat_data *pdata = pcpidev->pdata;
+ bool hsync_pol = flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH;
+ bool vsync_pol = flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH;
+ u32 val;
+
+ /* Software Reset */
+ imx_cpi_sw_reset(pcpidev);
+
+ /* Config PL Data Type */
+ val = IF_CTRL_REG_DATA_TYPE(DATA_TYPE_OUT_YUV444);
+ val |= IF_CTRL_REG_PL_ENABLE | IF_CTRL_REG_PL_VALID;
+ writel(val, pcpidev->regs + pdata->if_ctrl_reg);
+
+ /* Config CTRL REG */
+ val = CPI_CTRL_REG_HSYNC_FORCE_EN | CPI_CTRL_REG_VSYNC_FORCE_EN;
+
+ val |= CPI_CTRL_REG_DATA_TYPE_IN(pcpidev_fmt->data_type) |
+ FIELD_PREP(CPI_CTRL_REG_HSYNC_POL, hsync_pol) |
+ FIELD_PREP(CPI_CTRL_REG_VSYNC_POL, vsync_pol) |
+ FIELD_PREP(CPI_CTRL_REG_PIXEL_CLK_POL, 0) |
+ CPI_CTRL_REG_MASK_VSYNC_COUNTER(3) |
+ CPI_CTRL_REG_HSYNC_PULSE(2);
+
+ if (pcpidev_fmt->code == MEDIA_BUS_FMT_YUYV8_2X8 ||
+ pcpidev_fmt->code == MEDIA_BUS_FMT_UYVY8_2X8)
+ val |= CPI_CTRL_REG_UV_SWAP_EN;
+
+ if (pcpidev->mode == PI_GATE_CLOCK_MODE) {
+ val |= CPI_CTRL_REG_GCLK_MODE_EN;
+ } else if (pcpidev->mode == PI_CCIR_MODE) {
+ val |= (CPI_CTRL_REG_CCIR_EN |
+ CPI_CTRL_REG_CCIR_VSYNC_RESET_EN |
+ CPI_CTRL_REG_CCIR_EXT_VSYNC_EN |
+ CPI_CTRL_REG_CCIR_ECC_ERR_CORRECT_EN);
+ }
+
+ writel(val, pcpidev->regs + pdata->interface_ctrl_reg);
+}
+
+static void imx_cpi_config_ctrl_reg1(struct imx_cpi_device *pcpidev,
+ const struct v4l2_mbus_framefmt *format)
+{
+ const struct imx_cpi_plat_data *pdata = pcpidev->pdata;
+ u32 pixel_width;
+ u32 vsync_pulse;
+ u32 val;
+
+ pixel_width = format->width - 1;
+ vsync_pulse = format->width << 1;
+
+ val = CPI_CTRL_REG1_PIXEL_WIDTH(pixel_width) |
+ CPI_CTRL_REG1_VSYNC_PULSE(vsync_pulse);
+ writel(val, pcpidev->regs + pdata->interface_ctrl_reg1);
+}
+
+static void imx_cpi_enable(struct imx_cpi_device *pcpidev)
+{
+ const struct imx_cpi_plat_data *pdata = pcpidev->pdata;
+ u32 val;
+
+ /* Enable CPI */
+ val = readl(pcpidev->regs + pdata->interface_ctrl_reg);
+ val |= CPI_CTRL_REG_CPI_EN;
+ writel(val, pcpidev->regs + pdata->interface_ctrl_reg);
+
+ /* Disable SYNC Force */
+ val = readl(pcpidev->regs + pdata->interface_ctrl_reg);
+ val &= ~(CPI_CTRL_REG_HSYNC_FORCE_EN | CPI_CTRL_REG_VSYNC_FORCE_EN);
+ writel(val, pcpidev->regs + pdata->interface_ctrl_reg);
+}
+
+static void imx_cpi_disable(struct imx_cpi_device *pcpidev)
+{
+ const struct imx_cpi_plat_data *pdata = pcpidev->pdata;
+ u32 val;
+
+ /* Enable Sync Force */
+ val = readl(pcpidev->regs + pdata->interface_ctrl_reg);
+ val |= CPI_CTRL_REG_HSYNC_FORCE_EN | CPI_CTRL_REG_VSYNC_FORCE_EN;
+ writel(val, pcpidev->regs + pdata->interface_ctrl_reg);
+
+ /* Disable CPI */
+ val = readl(pcpidev->regs + pdata->interface_ctrl_reg);
+ val &= ~CPI_CTRL_REG_CPI_EN;
+ writel(val, pcpidev->regs + pdata->interface_ctrl_reg);
+
+ /* Disable Pixel Link */
+ val = readl(pcpidev->regs + pdata->if_ctrl_reg);
+ val &= ~(IF_CTRL_REG_PL_VALID | IF_CTRL_REG_PL_ENABLE);
+ writel(val, pcpidev->regs + pdata->if_ctrl_reg);
+}
+
+static struct imx_cpi_device *sd_to_imx_cpi_device(struct v4l2_subdev *sdev)
+{
+ return container_of(sdev, struct imx_cpi_device, sd);
+}
+
+static const struct media_entity_operations imx_cpi_entity_ops = {
+ .link_validate = v4l2_subdev_link_validate,
+ .get_fwnode_pad = v4l2_subdev_get_fwnode_pad_1_to_1,
+};
+
+static int imx_cpi_set_fmt(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *sdformat)
+{
+ struct imx_cpi_pix_format const *pcpidev_fmt;
+ struct v4l2_mbus_framefmt *fmt;
+
+ /*
+ * The Parallel cpi can't transcode in any way, the source format
+ * can't be modified.
+ */
+ if (sdformat->pad == V4L2_SUBDEV_1TO1_PADS_SOURCE)
+ return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
+
+ pcpidev_fmt = find_imx_cpi_format(sdformat->format.code);
+ if (!pcpidev_fmt)
+ pcpidev_fmt = &imx_cpi_formats[0];
+
+ fmt = v4l2_subdev_state_get_format(sd_state, sdformat->pad);
+
+ fmt->code = pcpidev_fmt->code;
+ fmt->width = sdformat->format.width;
+ fmt->height = sdformat->format.height;
+ fmt->field = V4L2_FIELD_NONE;
+ fmt->colorspace = sdformat->format.colorspace;
+ fmt->quantization = sdformat->format.quantization;
+ fmt->xfer_func = sdformat->format.xfer_func;
+ fmt->ycbcr_enc = sdformat->format.ycbcr_enc;
+
+ sdformat->format = *fmt;
+
+ /* Propagate the format from sink to source. */
+ fmt = v4l2_subdev_state_get_format(sd_state, V4L2_SUBDEV_1TO1_PADS_SOURCE);
+ *fmt = sdformat->format;
+
+ /* The format on the source pad might change due to unpacking. */
+ fmt->code = pcpidev_fmt->output;
+
+ return 0;
+}
+
+static int imx_cpi_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state)
+{
+ struct v4l2_subdev_route routes[] = {
+ {
+ .sink_pad = V4L2_SUBDEV_1TO1_PADS_SINK,
+ .sink_stream = 0,
+ .source_pad = V4L2_SUBDEV_1TO1_PADS_SOURCE,
+ .source_stream = 0,
+ .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+ },
+ };
+ struct v4l2_subdev_krouting routing = {
+ .len_routes = ARRAY_SIZE(routes),
+ .num_routes = ARRAY_SIZE(routes),
+ .routes = routes,
+ };
+ struct v4l2_mbus_framefmt *fmt;
+
+ fmt = v4l2_subdev_state_get_format(state, 0);
+
+ fmt->code = imx_cpi_formats[0].code;
+ fmt->width = 1920;
+ fmt->height = 1080;
+
+ fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
+ fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+ fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
+ fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(false,
+ fmt->colorspace,
+ fmt->ycbcr_enc);
+
+ return v4l2_subdev_set_routing_with_fmt(sd, state, &routing, fmt);
+}
+
+static int imx_cpi_disable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state, u32 pad,
+ u64 streams_mask)
+{
+ struct imx_cpi_device *pcpidev = sd_to_imx_cpi_device(sd);
+ struct media_pad *sink_pad, *remote_pad;
+ struct device *dev = pcpidev->dev;
+ struct v4l2_subdev *remote_sd;
+ u64 mask;
+ int ret;
+
+ sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
+ remote_pad = media_pad_remote_pad_first(sink_pad);
+ remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
+
+ mask = v4l2_subdev_state_xlate_streams(state, V4L2_SUBDEV_1TO1_PADS_SINK,
+ V4L2_SUBDEV_1TO1_PADS_SOURCE,
+ &streams_mask);
+
+ ret = v4l2_subdev_disable_streams(remote_sd, remote_pad->index, mask);
+ if (ret)
+ dev_err(dev, "failed to disable streams on remote subdev: %d\n", ret);
+
+ pcpidev->enabled_streams &= ~streams_mask;
+
+ if (!pcpidev->enabled_streams) {
+ imx_cpi_disable(pcpidev);
+ pm_runtime_put_autosuspend(dev);
+ }
+
+ return 0;
+}
+
+static int imx_cpi_enable_streams(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *state, u32 pad,
+ u64 streams_mask)
+{
+ struct imx_cpi_device *pcpidev = sd_to_imx_cpi_device(sd);
+ const struct imx_cpi_pix_format *pcpidev_fmt;
+ const struct v4l2_mbus_framefmt *format;
+ struct media_pad *sink_pad, *remote_pad;
+ struct device *dev = pcpidev->dev;
+ struct v4l2_subdev *remote_sd;
+ u64 mask;
+ int ret;
+
+ sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
+ remote_pad = media_pad_remote_pad_first(sink_pad);
+ remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
+
+ mask = v4l2_subdev_state_xlate_streams(state, V4L2_SUBDEV_1TO1_PADS_SINK,
+ V4L2_SUBDEV_1TO1_PADS_SOURCE,
+ &streams_mask);
+
+ format = v4l2_subdev_state_get_format(state, V4L2_SUBDEV_1TO1_PADS_SINK);
+ pcpidev_fmt = find_imx_cpi_format(format->code);
+
+ if (!pcpidev->enabled_streams) {
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret)
+ return ret;
+
+ imx_cpi_hw_config(pcpidev, pcpidev_fmt);
+ imx_cpi_config_ctrl_reg1(pcpidev, format);
+ imx_cpi_enable(pcpidev);
+ }
+
+ ret = v4l2_subdev_enable_streams(remote_sd, remote_pad->index, mask);
+ if (ret)
+ goto err_cpi_stop;
+
+ pcpidev->enabled_streams |= streams_mask;
+
+ return 0;
+
+err_cpi_stop:
+ /* Stop CSI hardware if no streams are enabled */
+ if (!pcpidev->enabled_streams)
+ imx_cpi_disable(pcpidev);
+
+ pm_runtime_put_autosuspend(dev);
+
+ return ret;
+}
+
+static int imx_cpi_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ /*
+ * The PARALLEL CPI can't transcode in any way, the source format
+ * is identical to the sink format.
+ */
+ if (code->pad == V4L2_SUBDEV_1TO1_PADS_SOURCE) {
+ struct v4l2_mbus_framefmt *fmt;
+
+ if (code->index > 0)
+ return -EINVAL;
+
+ fmt = v4l2_subdev_state_get_format(sd_state, code->pad);
+ code->code = fmt->code;
+ return 0;
+ }
+
+ if (code->pad != V4L2_SUBDEV_1TO1_PADS_SINK)
+ return -EINVAL;
+
+ if (code->index >= ARRAY_SIZE(imx_cpi_formats))
+ return -EINVAL;
+
+ code->code = imx_cpi_formats[code->index].code;
+
+ return 0;
+}
+
+static int
+imx_cpi_set_pad_by_ep(struct v4l2_subdev *sd, struct media_pad *pad)
+{
+ struct v4l2_fwnode_endpoint *vep = &pad->vep;
+
+ if (vep->base.port == V4L2_SUBDEV_1TO1_PADS_SINK) {
+ /* Sink port */
+
+ if (vep->bus_type != V4L2_MBUS_PARALLEL)
+ return -EINVAL;
+
+ pad->flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT;
+
+ return 0;
+ }
+
+ if (vep->base.port == V4L2_SUBDEV_1TO1_PADS_SOURCE) {
+ /* Source port */
+ pad->flags = MEDIA_PAD_FL_SOURCE | MEDIA_PAD_FL_MUST_CONNECT;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static const struct v4l2_subdev_video_ops imx_cpi_video_ops = {
+ .s_stream = v4l2_subdev_s_stream_helper,
+};
+
+static const struct v4l2_subdev_pad_ops imx_cpi_pad_ops = {
+ .enum_mbus_code = imx_cpi_enum_mbus_code,
+ .get_fmt = v4l2_subdev_get_fmt,
+ .set_fmt = imx_cpi_set_fmt,
+ .get_frame_desc = v4l2_subdev_get_frame_desc_passthrough,
+ .enable_streams = imx_cpi_enable_streams,
+ .disable_streams = imx_cpi_disable_streams,
+};
+
+static const struct v4l2_subdev_ops imx_cpi_subdev_ops = {
+ .pad = &imx_cpi_pad_ops,
+ .video = &imx_cpi_video_ops,
+};
+
+static const struct v4l2_subdev_internal_ops imx_cpi_internal_ops = {
+ .init_state = imx_cpi_init_state,
+ .set_pad_by_ep = imx_cpi_set_pad_by_ep,
+};
+
+/* ----------------------------------------------------------------------
+ * Suspend/resume
+ */
+
+static int imx_cpi_runtime_suspend(struct device *dev)
+{
+ struct imx_cpi_device *pcpidev = dev_get_drvdata(dev);
+
+ clk_bulk_disable_unprepare(pcpidev->num_clks, pcpidev->clks);
+
+ return 0;
+}
+
+static int imx_cpi_runtime_resume(struct device *dev)
+{
+ struct imx_cpi_device *pcpidev = dev_get_drvdata(dev);
+
+ return clk_bulk_prepare_enable(pcpidev->num_clks, pcpidev->clks);
+}
+
+static const struct dev_pm_ops imx_cpi_pm_ops = {
+ RUNTIME_PM_OPS(imx_cpi_runtime_suspend, imx_cpi_runtime_resume, NULL)
+};
+
+static void imx_cpi_remove(struct platform_device *pdev)
+{
+ struct imx_cpi_device *pcpidev = platform_get_drvdata(pdev);
+
+ media_async_subdev_cleanup(&pcpidev->sd);
+}
+
+static int imx_cpi_probe(struct platform_device *pdev)
+{
+ struct imx_cpi_device *pcpidev;
+ struct device *dev = &pdev->dev;
+ struct v4l2_subdev *sd;
+ int ret;
+
+ pcpidev = devm_kzalloc(dev, sizeof(*pcpidev), GFP_KERNEL);
+ if (!pcpidev)
+ return -ENOMEM;
+
+ pcpidev->dev = dev;
+ platform_set_drvdata(pdev, pcpidev);
+
+ pcpidev->pdata = of_device_get_match_data(dev);
+ pcpidev->mode = PI_GATE_CLOCK_MODE;
+
+ pcpidev->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(pcpidev->regs))
+ return dev_err_probe(dev, PTR_ERR(pcpidev->regs),
+ "Failed to get regs\n");
+
+ pcpidev->num_clks = devm_clk_bulk_get_all(dev, &pcpidev->clks);
+ if (pcpidev->num_clks < 0)
+ return pcpidev->num_clks;
+
+ sd = &pcpidev->sd;
+
+ v4l2_subdev_init(sd, &imx_cpi_subdev_ops);
+
+ sd->internal_ops = &imx_cpi_internal_ops;
+ snprintf(sd->name, sizeof(sd->name), "parallel-%s",
+ dev_name(pcpidev->dev));
+
+ sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+ sd->entity.ops = &imx_cpi_entity_ops;
+
+ sd->dev = pcpidev->dev;
+
+ pm_runtime_use_autosuspend(dev);
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return ret;
+
+ return media_async_register_subdev(sd);
+}
+
+static const struct of_device_id imx_cpi_of_match[] = {
+ { .compatible = "fsl,imx8qxp-pcif", .data = &imx8qxp_pdata },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, imx_cpi_of_match);
+
+static struct platform_driver imx_cpi_driver = {
+ .probe = imx_cpi_probe,
+ .remove = imx_cpi_remove,
+ .driver = {
+ .of_match_table = imx_cpi_of_match,
+ .name = "imx-parallel-cpi",
+ .pm = pm_ptr(&imx_cpi_pm_ops),
+ },
+};
+
+module_platform_driver(imx_cpi_driver);
+
+MODULE_DESCRIPTION("i.MX9 Parallel CPI receiver driver");
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 7/9] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)
2026-06-24 20:37 ` [PATCH v6 7/9] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI) Frank.Li
@ 2026-06-24 21:03 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-06-24 21:03 UTC (permalink / raw)
To: Frank.Li; +Cc: conor+dt, devicetree, imx, robh, Frank.Li
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Hardcoded pad indices lead to a NULL pointer dereference in `imx_cpi_set_fmt()` if the device tree specifies fewer than 2 endpoints.
- [High] Hardcoded pad indices cause memory corruption and use of uninitialized flags if device tree endpoints are reordered.
- [High] Pending asynchronous PM runtime suspend is cancelled during device removal, permanently leaking hardware clocks.
- [Medium] The Kconfig symbol `VIDEO_IMX_PARALLEL_CPI` selects a non-existent symbol `V4L2_1TO1`.
- [Medium] Unchecked return value from `of_device_get_match_data()` leads to a NULL pointer dereference if bound via sysfs.
- [Medium] Incorrect pad arguments passed to `v4l2_subdev_state_xlate_streams()`.
- [Low] The V2 register macros for `CPI_CTRL_V2_REG1` overlap at bit 16 and exceed the 32-bit register width.
--
commit 547923ad3a1ab4157dbe156e0b9151600e5bb65f
Author: Alice Yuan <alice.yuan@nxp.com>
media: nxp: add V4L2 subdev driver for camera parallel interface (CPI)
This commit adds a V4L2 sub-device driver for the CPI controller found
on i.MX8QXP, i.MX8QM, and i.MX93 SoCs. It supports parallel camera
sensors and enables image data capture through a parallel interface.
> diff --git a/drivers/media/platform/nxp/Kconfig b/drivers/media/platform/nxp/Kconfig
> index 40e3436669e21..90f7c792003f2 100644
> --- a/drivers/media/platform/nxp/Kconfig
> +++ b/drivers/media/platform/nxp/Kconfig
> @@ -39,6 +39,18 @@ config VIDEO_IMX_MIPI_CSIS
[ ... ]
> +config VIDEO_IMX_PARALLEL_CPI
> + tristate "NXP i.MX8/i.MX9 Parallel CPI Driver"
> + depends on ARCH_MXC || COMPILE_TEST
> + depends on VIDEO_DEV
> + select MEDIA_CONTROLLER
> + select V4L2_1TO1
[Severity: Medium]
Is this selecting a non-existent Kconfig symbol? V4L2_1TO1 does not appear
to exist in the kernel tree. Selecting a missing symbol can cause unmet
dependency warnings or silent build failures.
> + select V4L2_FWNODE
> + select VIDEO_V4L2_SUBDEV_API
> + help
> + Video4Linux2 sub-device driver for PARALLEL CPI receiver found
> + on some iMX8 and iMX9 SoCs.
> diff --git a/drivers/media/platform/nxp/imx-parallel-cpi.c b/drivers/media/platform/nxp/imx-parallel-cpi.c
> new file mode 100644
> index 0000000000000..0f74b51608715
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx-parallel-cpi.c
> @@ -0,0 +1,629 @@
[ ... ]
> +/* CPI INTERFACE CONTROL REG1 */
> +#define CPI_CTRL_REG1_PIXEL_WIDTH(v) FIELD_PREP(GENMASK(15, 0), (v))
> +#define CPI_CTRL_REG1_VSYNC_PULSE(v) FIELD_PREP(GENMASK(31, 16), (v))
> +
> +#define CPI_CTRL_V2_REG1_PIXEL_WIDTH(v) FIELD_PREP(GENMASK(16, 0), (v))
> +#define CPI_CTRL_V2_REG1_VSYNC_PULSE(v) FIELD_PREP(GENMASK(31, 16), (v))
[Severity: Low]
Do these V2 register macros overlap? The GENMASK(16, 0) and GENMASK(31, 16)
overlap at bit 16, resulting in a 33-bit span for what appears to be a
32-bit register. While currently unused, could this corrupt register
values if utilized in the future?
> +
> +/* Need match field DATA_TYPE_IN definition at CPI CTRL register */
[ ... ]
> +static void imx_cpi_hw_config(struct imx_cpi_device *pcpidev,
> + const struct imx_cpi_pix_format *pcpidev_fmt)
> +{
> + u32 flags = pcpidev->sd.entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK].vep.bus.parallel.flags;
[Severity: High]
Can hardcoding the pad index lead to reading uninitialized memory here?
The pads array is populated based on the order endpoints appear in the
Device Tree. If a device tree defines the source endpoint before the
sink endpoint, pads[0] will correspond to the source pad instead of
the sink pad.
> + const struct imx_cpi_plat_data *pdata = pcpidev->pdata;
> + bool hsync_pol = flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH;
[ ... ]
> +static int imx_cpi_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *sdformat)
> +{
> + struct imx_cpi_pix_format const *pcpidev_fmt;
> + struct v4l2_mbus_framefmt *fmt;
[ ... ]
> + /* Propagate the format from sink to source. */
> + fmt = v4l2_subdev_state_get_format(sd_state, V4L2_SUBDEV_1TO1_PADS_SOURCE);
> + *fmt = sdformat->format;
[Severity: High]
Can this result in a NULL pointer dereference?
If a misconfigured device tree only specifies a single endpoint,
media_async_register_subdev() will allocate exactly one pad, meaning
sd->entity.num_pads = 1.
When v4l2_subdev_state_get_format() is explicitly called with
V4L2_SUBDEV_1TO1_PADS_SOURCE (index 1), it will return NULL since the
index is out of bounds. The pointer is then blindly dereferenced.
> +
> + /* The format on the source pad might change due to unpacking. */
> + fmt->code = pcpidev_fmt->output;
> +
> + return 0;
> +}
[ ... ]
> +static int imx_cpi_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct imx_cpi_device *pcpidev = sd_to_imx_cpi_device(sd);
> + const struct imx_cpi_pix_format *pcpidev_fmt;
> + const struct v4l2_mbus_framefmt *format;
> + struct media_pad *sink_pad, *remote_pad;
> + struct device *dev = pcpidev->dev;
> + struct v4l2_subdev *remote_sd;
> + u64 mask;
> + int ret;
> +
> + sink_pad = &sd->entity.pads[V4L2_SUBDEV_1TO1_PADS_SINK];
> + remote_pad = media_pad_remote_pad_first(sink_pad);
> + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
[Severity: High]
If the device tree endpoints are reordered as mentioned previously, can
this cast the wrong entity type to a v4l2_subdev?
If pads[0] corresponds to the source pad, its remote entity will be a
video_device, not a v4l2_subdev. Calling media_entity_to_v4l2_subdev()
on a video_device produces an invalid pointer that will crash the kernel
when dereferenced.
> +
> + mask = v4l2_subdev_state_xlate_streams(state, V4L2_SUBDEV_1TO1_PADS_SINK,
> + V4L2_SUBDEV_1TO1_PADS_SOURCE,
> + &streams_mask);
[Severity: Medium]
Are the pad arguments reversed here?
Since v4l2_subdev_enable_streams() is called on the source pad,
streams_mask represents streams on the source pad. Passing
V4L2_SUBDEV_1TO1_PADS_SINK as the first argument informs the helper that
streams_mask belongs to the sink pad. Could this silently break routing
logic if asymmetrical stream IDs are ever used?
> +
> + format = v4l2_subdev_state_get_format(state, V4L2_SUBDEV_1TO1_PADS_SINK);
[ ... ]
> +static void imx_cpi_remove(struct platform_device *pdev)
> +{
> + struct imx_cpi_device *pcpidev = platform_get_drvdata(pdev);
> +
> + media_async_subdev_cleanup(&pcpidev->sd);
> +}
[Severity: High]
Could this leak hardware clocks if an asynchronous suspend is pending?
When streams are disabled, pm_runtime_put_autosuspend() is called to
queue an async suspend. If the driver is unbound before the async work
executes, imx_cpi_remove() will run.
Since the driver relies on devm_pm_runtime_enable() for PM cleanup,
pm_runtime_disable() will flush and cancel the pending async suspend,
which leaves the device permanently powered on. Should this call
pm_runtime_put_sync() or explicitly suspend the device if an async put
was previously used?
> +
> +static int imx_cpi_probe(struct platform_device *pdev)
> +{
> + struct imx_cpi_device *pcpidev;
> + struct device *dev = &pdev->dev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + pcpidev = devm_kzalloc(dev, sizeof(*pcpidev), GFP_KERNEL);
> + if (!pcpidev)
> + return -ENOMEM;
> +
> + pcpidev->dev = dev;
> + platform_set_drvdata(pdev, pcpidev);
> +
> + pcpidev->pdata = of_device_get_match_data(dev);
[Severity: Medium]
Does this missing NULL check lead to a crash when binding via sysfs?
If an administrator manually binds the driver to a platform device without
an Open Firmware node via sysfs, of_device_get_match_data() returns NULL.
The driver will later unconditionally dereference pcpidev->pdata (such as
in imx_cpi_sw_reset()), leading to a kernel panic instead of gracefully
failing.
> + pcpidev->mode = PI_GATE_CLOCK_MODE;
> +
> + pcpidev->regs = devm_platform_ioremap_resource(pdev, 0);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=7
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 8/9] arm64: dts: imx8: add camera parallel interface (CPI) node
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
` (6 preceding siblings ...)
2026-06-24 20:37 ` [PATCH v6 7/9] media: nxp: add V4L2 subdev driver for camera parallel interface (CPI) Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
2026-06-24 21:00 ` sashiko-bot
2026-06-24 20:37 ` [PATCH v6 9/9] arm64: dts: imx8qxp-mek: add parallel ov5640 camera support Frank.Li
8 siblings, 1 reply; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel
From: Frank Li <Frank.Li@nxp.com>
Add camera parallel interface (CPI) node.
Reviewed-by: Guoniu Zhou <guoniu.zhou@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
changes in v6
- add Guoniu Zhou's review by
changes in v4
- none
changes in v3
- replace csi with cpi.
changes in v2
- update compatible string to match binding's change
---
arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi | 13 +++++++++++
arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi | 27 +++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi b/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
index a72b2f1c4a1b2..b504f99f6acdb 100644
--- a/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
@@ -222,6 +222,19 @@ irqsteer_parallel: irqsteer@58260000 {
status = "disabled";
};
+ parallel_cpi: cpi@58261000 {
+ compatible = "fsl,imx8qxp-pcif";
+ reg = <0x58261000 0x1000>;
+ clocks = <&pi0_pxl_lpcg IMX_LPCG_CLK_0>,
+ <&pi0_ipg_lpcg IMX_LPCG_CLK_4>;
+ clock-names = "pixel", "ipg";
+ assigned-clocks = <&clk IMX_SC_R_PI_0 IMX_SC_PM_CLK_PER>;
+ assigned-clock-parents = <&clk IMX_SC_R_PI_0_PLL IMX_SC_PM_CLK_PLL>;
+ assigned-clock-rates = <160000000>;
+ power-domains = <&pd IMX_SC_R_PI_0>;
+ status = "disabled";
+ };
+
pi0_ipg_lpcg: clock-controller@58263004 {
compatible = "fsl,imx8qxp-lpcg";
reg = <0x58263004 0x4>;
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
index 232cf25dadfcd..5aae15540d6cb 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
@@ -62,6 +62,14 @@ isi_in_2: endpoint {
remote-endpoint = <&mipi_csi0_out>;
};
};
+
+ port@4 {
+ reg = <4>;
+
+ isi_in_4: endpoint {
+ remote-endpoint = <¶llel_cpi_out>;
+ };
+ };
};
};
@@ -95,3 +103,22 @@ &jpegenc {
&mipi_csi_1 {
status = "disabled";
};
+
+¶llel_cpi {
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ };
+
+ port@1 {
+ reg = <1>;
+
+ parallel_cpi_out: endpoint {
+ remote-endpoint = <&isi_in_4>;
+ };
+ };
+ };
+};
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v6 8/9] arm64: dts: imx8: add camera parallel interface (CPI) node
2026-06-24 20:37 ` [PATCH v6 8/9] arm64: dts: imx8: add camera parallel interface (CPI) node Frank.Li
@ 2026-06-24 21:00 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-06-24 21:00 UTC (permalink / raw)
To: Frank.Li; +Cc: conor+dt, imx, robh, devicetree, Frank.Li
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Mismatch between DTS port assignment (`port@4`) and the dt-binding documentation which specifies `port@6` for the Parallel RX.
- [Low] Devicetree schema validation error: 'ports' is a strictly required property but is missing from the base `parallel_cpi` node.
--
commit 80a1591fa539edb7320ce7a2c3b3f4dda7b4baa1
Author: Frank Li <Frank.Li@nxp.com>
arm64: dts: imx8: add camera parallel interface (CPI) node
This commit adds the camera parallel interface (CPI) node for imx8. It
defines the base cpi node in imx8-ss-img.dtsi and configures the endpoint
connections in imx8qxp-ss-img.dtsi.
> diff --git a/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi b/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
> --- a/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8-ss-img.dtsi
[ ... ]
> @@ -222,6 +222,19 @@ irqsteer_parallel: irqsteer@58260000 {
> status = "disabled";
> };
>
> + parallel_cpi: cpi@58261000 {
> + compatible = "fsl,imx8qxp-pcif";
> + reg = <0x58261000 0x1000>;
[Severity: Low]
Does this base node need a ports property to pass dtbs_check validation?
The fsl,imx93-pcif.yaml schema strictly requires the ports property, and
dt-validate processes nodes regardless of whether status is disabled.
Will building devicetrees that include imx8-ss-img.dtsi without extending
this node trigger validation errors?
> + clocks = <&pi0_pxl_lpcg IMX_LPCG_CLK_0>,
> + <&pi0_ipg_lpcg IMX_LPCG_CLK_4>;
> + clock-names = "pixel", "ipg";
> + assigned-clocks = <&clk IMX_SC_R_PI_0 IMX_SC_PM_CLK_PER>;
> + assigned-clock-parents = <&clk IMX_SC_R_PI_0_PLL IMX_SC_PM_CLK_PLL>;
> + assigned-clock-rates = <160000000>;
> + power-domains = <&pd IMX_SC_R_PI_0>;
> + status = "disabled";
> + };
> diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi b/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
> --- a/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8qxp-ss-img.dtsi
[ ... ]
> @@ -62,6 +62,14 @@ isi_in_2: endpoint {
> remote-endpoint = <&mipi_csi0_out>;
> };
> };
> +
> + port@4 {
> + reg = <4>;
> +
> + isi_in_4: endpoint {
> + remote-endpoint = <¶llel_cpi_out>;
> + };
> + };
[Severity: Low]
Does port@4 match the devicetree binding documentation?
The Documentation/devicetree/bindings/media/fsl,imx8qxp-isi.yaml
documentation explicitly specifies port@6 as the CSI-2 Parallel RX.
Even though the driver configures the hardware correctly with num_ports = 5,
does adding port@4 here introduce an undocumented port and create a schema
mismatch?
> };
> };
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624-imx8qxp_pcam-v6-0-4b3f45920d2f@nxp.com?part=8
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 9/9] arm64: dts: imx8qxp-mek: add parallel ov5640 camera support
2026-06-24 20:37 [PATCH v6 0/9] media: add new API simple 1to1 subdev register and add imx parallel camera support Frank.Li
` (7 preceding siblings ...)
2026-06-24 20:37 ` [PATCH v6 8/9] arm64: dts: imx8: add camera parallel interface (CPI) node Frank.Li
@ 2026-06-24 20:37 ` Frank.Li
8 siblings, 0 replies; 17+ messages in thread
From: Frank.Li @ 2026-06-24 20:37 UTC (permalink / raw)
To: Sakari Ailus, Mauro Carvalho Chehab, Michael Riesch,
Laurent Pinchart, Frank Li, Martin Kepplinger-Novakovic,
Rui Miguel Silva, Purism Kernel Team, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam
Cc: linux-media, linux-kernel, imx, Guoniu Zhou, devicetree,
linux-arm-kernel
From: Frank Li <Frank.Li@nxp.com>
Add parallel ov5640 nodes in imx8qxp-mek and create overlay file to enable
it because it can work at two mode: MIPI CSI and parallel mode.
Reviewed-by: Guoniu Zhou <guoniu.zhou@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
changes in v6
- add Guoniu's reviewed-by tags
changes in v4
- add hsync-active = <1>
changes in v3
- replace csi with cpi.
- use imx8qxp-mek-ov5640-cpi.dtso since csi use imx8qxp-mek-ov5640-csi.dtso
change in v2
- move ov5640 part to overlay file
- rename to imx8qxp-mek-ov5640-parallel.dtso
- remove data-lanes
---
arch/arm64/boot/dts/freescale/Makefile | 3 +
.../boot/dts/freescale/imx8qxp-mek-ov5640-cpi.dtso | 83 ++++++++++++++++++++++
2 files changed, 86 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 001ca3a12c0ae..3b9e9844f11ef 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -554,6 +554,9 @@ dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek-pcie-ep.dtb
imx8qxp-mek-ov5640-csi-dtbs := imx8qxp-mek.dtb imx8qxp-mek-ov5640-csi.dtbo
dtb-${CONFIG_ARCH_MXC} += imx8qxp-mek-ov5640-csi.dtb
+imx8qxp-mek-ov5640-cpi-dtbs := imx8qxp-mek.dtb imx8qxp-mek-ov5640-cpi.dtbo
+dtb-${CONFIG_ARCH_MXC} += imx8qxp-mek-ov5640-cpi.dtb
+
dtb-$(CONFIG_ARCH_MXC) += imx8qxp-tqma8xqp-mba8xx.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8qxp-tqma8xqps-mb-smarc-2.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8ulp-9x9-evk.dtb
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-mek-ov5640-cpi.dtso b/arch/arm64/boot/dts/freescale/imx8qxp-mek-ov5640-cpi.dtso
new file mode 100644
index 0000000000000..9fbdd798f17d6
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8qxp-mek-ov5640-cpi.dtso
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2025 NXP
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include <dt-bindings/clock/imx8-lpcg.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/media/video-interfaces.h>
+#include <dt-bindings/pinctrl/pads-imx8qxp.h>
+
+&cm40_i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ov5640_pi: camera@3c {
+ compatible = "ovti,ov5640";
+ reg = <0x3c>;
+ clocks = <&pi0_misc_lpcg IMX_LPCG_CLK_0>;
+ clock-names = "xclk";
+ assigned-clocks = <&pi0_misc_lpcg IMX_LPCG_CLK_0>;
+ assigned-clock-rates = <24000000>;
+ AVDD-supply = <®_2v8>;
+ DOVDD-supply = <®_1v8>;
+ DVDD-supply = <®_1v5>;
+ pinctrl-0 = <&pinctrl_parallel_cpi>;
+ pinctrl-names = "default";
+ powerdown-gpios = <&lsio_gpio3 2 GPIO_ACTIVE_HIGH>;
+ reset-gpios = <&lsio_gpio3 3 GPIO_ACTIVE_LOW>;
+
+ port {
+ ov5640_pi_ep: endpoint {
+ bus-type = <MEDIA_BUS_TYPE_PARALLEL>;
+ bus-width = <8>;
+ hsync-active = <1>;
+ pclk-sample = <1>;
+ remote-endpoint = <¶llel_cpi_in>;
+ vsync-active = <0>;
+ };
+ };
+ };
+};
+
+&iomuxc {
+ pinctrl_parallel_cpi: parallelcpigrp {
+ fsl,pins = <
+ IMX8QXP_CSI_D00_CI_PI_D02 0xc0000041
+ IMX8QXP_CSI_D01_CI_PI_D03 0xc0000041
+ IMX8QXP_CSI_D02_CI_PI_D04 0xc0000041
+ IMX8QXP_CSI_D03_CI_PI_D05 0xc0000041
+ IMX8QXP_CSI_D04_CI_PI_D06 0xc0000041
+ IMX8QXP_CSI_D05_CI_PI_D07 0xc0000041
+ IMX8QXP_CSI_D06_CI_PI_D08 0xc0000041
+ IMX8QXP_CSI_D07_CI_PI_D09 0xc0000041
+
+ IMX8QXP_CSI_MCLK_CI_PI_MCLK 0xc0000041
+ IMX8QXP_CSI_PCLK_CI_PI_PCLK 0xc0000041
+ IMX8QXP_CSI_HSYNC_CI_PI_HSYNC 0xc0000041
+ IMX8QXP_CSI_VSYNC_CI_PI_VSYNC 0xc0000041
+ IMX8QXP_CSI_EN_LSIO_GPIO3_IO02 0xc0000041
+ IMX8QXP_CSI_RESET_LSIO_GPIO3_IO03 0xc0000041
+ >;
+ };
+};
+
+&isi {
+ status = "okay";
+};
+
+¶llel_cpi {
+ status = "okay";
+
+ ports {
+ port@0 {
+ parallel_cpi_in: endpoint {
+ hsync-active = <1>;
+ remote-endpoint = <&ov5640_pi_ep>;
+ };
+ };
+ };
+};
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread