Linux kernel and device drivers for NXP i.MX platforms
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 6.17] media: nxp: imx8-isi: Fix streaming cleanup on release
       [not found] <20251025160905.3857885-1-sashal@kernel.org>
@ 2025-10-25 15:53 ` Sasha Levin
  2025-10-25 15:54 ` [PATCH AUTOSEL 6.17-6.12] PCI: imx6: Enable the Vaux supply if available Sasha Levin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2025-10-25 15:53 UTC (permalink / raw)
  To: patches, stable
  Cc: Richard Leitner, Laurent Pinchart, Hans Verkuil, Sasha Levin,
	shawnguo, linux-media, imx, linux-arm-kernel

From: Richard Leitner <richard.leitner@linux.dev>

[ Upstream commit 47773031a148ad7973b809cc7723cba77eda2b42 ]

The current implementation unconditionally calls
mxc_isi_video_cleanup_streaming() in mxc_isi_video_release(). This can
lead to situations where any release call (like from a simple
"v4l2-ctl -l") may release a currently streaming queue when called on
such a device.

This is reproducible on an i.MX8MP board by streaming from an ISI
capture device using gstreamer:

	gst-launch-1.0 -v v4l2src device=/dev/videoX ! \
	    video/x-raw,format=GRAY8,width=1280,height=800,framerate=1/120 ! \
	    fakesink

While this stream is running, querying the caps of the same device
provokes the error state:

	v4l2-ctl -l -d /dev/videoX

This results in the following trace:

[  155.452152] ------------[ cut here ]------------
[  155.452163] WARNING: CPU: 0 PID: 1708 at drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c:713 mxc_isi_pipe_irq_handler+0x19c/0x1b0 [imx8_isi]
[  157.004248] Modules linked in: cfg80211 rpmsg_ctrl rpmsg_char rpmsg_tty virtio_rpmsg_bus rpmsg_ns rpmsg_core rfkill nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables mcp251x6
[  157.053499] CPU: 0 UID: 0 PID: 1708 Comm: python3 Not tainted 6.15.4-00114-g1f61ca5cad76 #1 PREEMPT
[  157.064369] Hardware name: imx8mp_board_01 (DT)
[  157.068205] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  157.075169] pc : mxc_isi_pipe_irq_handler+0x19c/0x1b0 [imx8_isi]
[  157.081195] lr : mxc_isi_pipe_irq_handler+0x38/0x1b0 [imx8_isi]
[  157.087126] sp : ffff800080003ee0
[  157.090438] x29: ffff800080003ee0 x28: ffff0000c3688000 x27: 0000000000000000
[  157.097580] x26: 0000000000000000 x25: ffff0000c1e7ac00 x24: ffff800081b5ad50
[  157.104723] x23: 00000000000000d1 x22: 0000000000000000 x21: ffff0000c25e4000
[  157.111866] x20: 0000000060000200 x19: ffff80007a0608d0 x18: 0000000000000000
[  157.119008] x17: ffff80006a4e3000 x16: ffff800080000000 x15: 0000000000000000
[  157.126146] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
[  157.133287] x11: 0000000000000040 x10: ffff0000c01445f0 x9 : ffff80007a053a38
[  157.140425] x8 : ffff0000c04004b8 x7 : 0000000000000000 x6 : 0000000000000000
[  157.147567] x5 : ffff0000c0400490 x4 : ffff80006a4e3000 x3 : ffff0000c25e4000
[  157.154706] x2 : 0000000000000000 x1 : ffff8000825c0014 x0 : 0000000060000200
[  157.161850] Call trace:
[  157.164296]  mxc_isi_pipe_irq_handler+0x19c/0x1b0 [imx8_isi] (P)
[  157.170319]  __handle_irq_event_percpu+0x58/0x218
[  157.175029]  handle_irq_event+0x54/0xb8
[  157.178867]  handle_fasteoi_irq+0xac/0x248
[  157.182968]  handle_irq_desc+0x48/0x68
[  157.186723]  generic_handle_domain_irq+0x24/0x38
[  157.191346]  gic_handle_irq+0x54/0x120
[  157.195098]  call_on_irq_stack+0x24/0x30
[  157.199027]  do_interrupt_handler+0x88/0x98
[  157.203212]  el0_interrupt+0x44/0xc0
[  157.206792]  __el0_irq_handler_common+0x18/0x28
[  157.211328]  el0t_64_irq_handler+0x10/0x20
[  157.215429]  el0t_64_irq+0x198/0x1a0
[  157.219009] ---[ end trace 0000000000000000 ]---

Address this issue by moving the streaming preparation and cleanup to
the vb2 .prepare_streaming() and .unprepare_streaming() operations. This
also simplifies the driver by allowing direct usage of the
vb2_ioctl_streamon() and vb2_ioctl_streamoff() helpers, and removal of
the manual cleanup from mxc_isi_video_release().

Link: https://lore.kernel.org/r/20250813212451.22140-2-laurent.pinchart@ideasonboard.com
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Richard Leitner <richard.leitner@linux.dev> # i.MX8MP
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

YES

- Fixes a real, user-facing bug: closing any file descriptor on the same
  node could tear down an active stream (e.g., “v4l2-ctl -l”), leading
  to WARNs in the IRQ handler. The commit moves streaming lifetime
  management into vb2 so only the streaming filehandle controls
  teardown.
- Moves pipeline start/stop and discard buffer lifetime from ad-hoc
  ioctl/release paths into vb2’s streaming lifecycle:
  - New prepare path acquires and starts the pipeline, validates format,
    and allocates discard buffers:
    - Acquire/start: drivers/media/platform/nxp/imx8-isi/imx8-isi-
      video.c:949, 957
    - Graph mutex protection (scoped guard):
      drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:948
    - Format validation: drivers/media/platform/nxp/imx8-isi/imx8-isi-
      video.c:962
    - Discard buffers allocation:
      drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:967
    - All in mxc_isi_vb2_prepare_streaming():
      drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:940
  - New unprepare path frees discard buffers, stops the pipeline, and
    releases it:
    - Free/stop/release: drivers/media/platform/nxp/imx8-isi/imx8-isi-
      video.c:1033, 1034, 1035
    - In mxc_isi_vb2_unprepare_streaming():
      drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:1029
- Switches to standard vb2 stream ioctl helpers, aligning with core
  expectations and ensuring (un)prepare hooks are used:
  - .vidioc_streamon/off → vb2 helpers:
    drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:1255, 1256
  - Registers vb2 ops including prepare/unprepare:
    drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:1038
- Removes unconditional teardown from release, preventing non-streaming
  FDs from stopping an active stream:
  - mxc_isi_video_release() now relies on vb2_fop_release() and no
    manual cleanup: drivers/media/platform/nxp/imx8-isi/imx8-isi-
    video.c:1286-1296
- Streaming start/stop remains confined and symmetric:
  - Start: channel init, queue setup, enable:
    drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:981-1011
  - Stop: pipe disable, channel put, return buffers:
    drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:1019-1027

Why this fits stable backport criteria
- Important bugfix with clear user impact (streams can be disrupted by
  unrelated handle close; WARN in IRQ handler).
- Contained to the imx8-isi driver; no architectural changes to core
  subsystems.
- Uses established vb2 mechanisms to correctly bind resource lifetime to
  the streaming filehandle, minimizing regression risk.
- Simplifies code by deferring to vb2 helpers and lifecycle callbacks.

Compatibility notes for stable maintainers
- Requires vb2 .prepare_streaming/.unprepare_streaming support (present
  in modern stable series such as 6.1.y and 6.6.y).
- If targeting older stables lacking these vb2 ops, an equivalent fix
  must avoid unconditional release-time cleanup and keep pipeline
  (un)prepare tied to STREAMON/STREAMOFF (i.e., adapt without the new
  callbacks).
- The scoped_guard pattern
  (drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c:948) can be
  replaced with explicit mutex_lock/unlock for older trees if needed.

Overall, this is a focused, low-risk fix for a real streaming lifecycle
bug and is suitable for stable backporting.

 .../platform/nxp/imx8-isi/imx8-isi-video.c    | 156 +++++++-----------
 1 file changed, 58 insertions(+), 98 deletions(-)

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
index 8654150728a86..042b554d2775a 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
@@ -937,6 +937,49 @@ static void mxc_isi_video_init_channel(struct mxc_isi_video *video)
 	mxc_isi_channel_set_output_format(pipe, video->fmtinfo, &video->pix);
 }
 
+static int mxc_isi_vb2_prepare_streaming(struct vb2_queue *q)
+{
+	struct mxc_isi_video *video = vb2_get_drv_priv(q);
+	struct media_device *mdev = &video->pipe->isi->media_dev;
+	struct media_pipeline *pipe;
+	int ret;
+
+	/* Get a pipeline for the video node and start it. */
+	scoped_guard(mutex, &mdev->graph_mutex) {
+		ret = mxc_isi_pipe_acquire(video->pipe,
+					   &mxc_isi_video_frame_write_done);
+		if (ret)
+			return ret;
+
+		pipe = media_entity_pipeline(&video->vdev.entity)
+		     ? : &video->pipe->pipe;
+
+		ret = __video_device_pipeline_start(&video->vdev, pipe);
+		if (ret)
+			goto err_release;
+	}
+
+	/* Verify that the video format matches the output of the subdev. */
+	ret = mxc_isi_video_validate_format(video);
+	if (ret)
+		goto err_stop;
+
+	/* Allocate buffers for discard operation. */
+	ret = mxc_isi_video_alloc_discard_buffers(video);
+	if (ret)
+		goto err_stop;
+
+	video->is_streaming = true;
+
+	return 0;
+
+err_stop:
+	video_device_pipeline_stop(&video->vdev);
+err_release:
+	mxc_isi_pipe_release(video->pipe);
+	return ret;
+}
+
 static int mxc_isi_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct mxc_isi_video *video = vb2_get_drv_priv(q);
@@ -985,13 +1028,26 @@ static void mxc_isi_vb2_stop_streaming(struct vb2_queue *q)
 	mxc_isi_video_return_buffers(video, VB2_BUF_STATE_ERROR);
 }
 
+static void mxc_isi_vb2_unprepare_streaming(struct vb2_queue *q)
+{
+	struct mxc_isi_video *video = vb2_get_drv_priv(q);
+
+	mxc_isi_video_free_discard_buffers(video);
+	video_device_pipeline_stop(&video->vdev);
+	mxc_isi_pipe_release(video->pipe);
+
+	video->is_streaming = false;
+}
+
 static const struct vb2_ops mxc_isi_vb2_qops = {
 	.queue_setup		= mxc_isi_vb2_queue_setup,
 	.buf_init		= mxc_isi_vb2_buffer_init,
 	.buf_prepare		= mxc_isi_vb2_buffer_prepare,
 	.buf_queue		= mxc_isi_vb2_buffer_queue,
+	.prepare_streaming	= mxc_isi_vb2_prepare_streaming,
 	.start_streaming	= mxc_isi_vb2_start_streaming,
 	.stop_streaming		= mxc_isi_vb2_stop_streaming,
+	.unprepare_streaming	= mxc_isi_vb2_unprepare_streaming,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1145,97 +1201,6 @@ static int mxc_isi_video_s_fmt(struct file *file, void *priv,
 	return 0;
 }
 
-static int mxc_isi_video_streamon(struct file *file, void *priv,
-				  enum v4l2_buf_type type)
-{
-	struct mxc_isi_video *video = video_drvdata(file);
-	struct media_device *mdev = &video->pipe->isi->media_dev;
-	struct media_pipeline *pipe;
-	int ret;
-
-	if (vb2_queue_is_busy(&video->vb2_q, file))
-		return -EBUSY;
-
-	/*
-	 * Get a pipeline for the video node and start it. This must be done
-	 * here and not in the queue .start_streaming() handler, so that
-	 * pipeline start errors can be reported from VIDIOC_STREAMON and not
-	 * delayed until subsequent VIDIOC_QBUF calls.
-	 */
-	mutex_lock(&mdev->graph_mutex);
-
-	ret = mxc_isi_pipe_acquire(video->pipe, &mxc_isi_video_frame_write_done);
-	if (ret) {
-		mutex_unlock(&mdev->graph_mutex);
-		return ret;
-	}
-
-	pipe = media_entity_pipeline(&video->vdev.entity) ? : &video->pipe->pipe;
-
-	ret = __video_device_pipeline_start(&video->vdev, pipe);
-	if (ret) {
-		mutex_unlock(&mdev->graph_mutex);
-		goto err_release;
-	}
-
-	mutex_unlock(&mdev->graph_mutex);
-
-	/* Verify that the video format matches the output of the subdev. */
-	ret = mxc_isi_video_validate_format(video);
-	if (ret)
-		goto err_stop;
-
-	/* Allocate buffers for discard operation. */
-	ret = mxc_isi_video_alloc_discard_buffers(video);
-	if (ret)
-		goto err_stop;
-
-	ret = vb2_streamon(&video->vb2_q, type);
-	if (ret)
-		goto err_free;
-
-	video->is_streaming = true;
-
-	return 0;
-
-err_free:
-	mxc_isi_video_free_discard_buffers(video);
-err_stop:
-	video_device_pipeline_stop(&video->vdev);
-err_release:
-	mxc_isi_pipe_release(video->pipe);
-	return ret;
-}
-
-static void mxc_isi_video_cleanup_streaming(struct mxc_isi_video *video)
-{
-	lockdep_assert_held(&video->lock);
-
-	if (!video->is_streaming)
-		return;
-
-	mxc_isi_video_free_discard_buffers(video);
-	video_device_pipeline_stop(&video->vdev);
-	mxc_isi_pipe_release(video->pipe);
-
-	video->is_streaming = false;
-}
-
-static int mxc_isi_video_streamoff(struct file *file, void *priv,
-				   enum v4l2_buf_type type)
-{
-	struct mxc_isi_video *video = video_drvdata(file);
-	int ret;
-
-	ret = vb2_ioctl_streamoff(file, priv, type);
-	if (ret)
-		return ret;
-
-	mxc_isi_video_cleanup_streaming(video);
-
-	return 0;
-}
-
 static int mxc_isi_video_enum_framesizes(struct file *file, void *priv,
 					 struct v4l2_frmsizeenum *fsize)
 {
@@ -1291,9 +1256,8 @@ static const struct v4l2_ioctl_ops mxc_isi_video_ioctl_ops = {
 	.vidioc_expbuf			= vb2_ioctl_expbuf,
 	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
 	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
-
-	.vidioc_streamon		= mxc_isi_video_streamon,
-	.vidioc_streamoff		= mxc_isi_video_streamoff,
+	.vidioc_streamon		= vb2_ioctl_streamon,
+	.vidioc_streamoff		= vb2_ioctl_streamoff,
 
 	.vidioc_enum_framesizes		= mxc_isi_video_enum_framesizes,
 
@@ -1332,10 +1296,6 @@ static int mxc_isi_video_release(struct file *file)
 	if (ret)
 		dev_err(video->pipe->isi->dev, "%s fail\n", __func__);
 
-	mutex_lock(&video->lock);
-	mxc_isi_video_cleanup_streaming(video);
-	mutex_unlock(&video->lock);
-
 	pm_runtime_put(video->pipe->isi->dev);
 	return ret;
 }
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH AUTOSEL 6.17-6.12] PCI: imx6: Enable the Vaux supply if available
       [not found] <20251025160905.3857885-1-sashal@kernel.org>
  2025-10-25 15:53 ` [PATCH AUTOSEL 6.17] media: nxp: imx8-isi: Fix streaming cleanup on release Sasha Levin
@ 2025-10-25 15:54 ` Sasha Levin
  2025-10-25 15:55 ` [PATCH AUTOSEL 6.17-6.6] iio: adc: imx93_adc: load calibrated values even calibration failed Sasha Levin
  2025-10-25 15:56 ` [PATCH AUTOSEL 6.17] media: imx-mipi-csis: Only set clock rate when specified in DT Sasha Levin
  3 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2025-10-25 15:54 UTC (permalink / raw)
  To: patches, stable
  Cc: Richard Zhu, Manivannan Sadhasivam, Frank Li, Sasha Levin,
	l.stach, shawnguo, linux-pci, linux-arm-kernel, imx

From: Richard Zhu <hongxing.zhu@nxp.com>

[ Upstream commit c221cbf8dc547eb8489152ac62ef103fede99545 ]

When the 3.3Vaux supply is present, fetch it at the probe time and keep it
enabled for the entire PCIe controller lifecycle so that the link can enter
L2 state and the devices can signal wakeup using either Beacon or WAKE#
mechanisms.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
[mani: reworded the subject, description and error message]
Signed-off-by: Manivannan Sadhasivam <mani@kernel.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Link: https://patch.msgid.link/20250820022328.2143374-1-hongxing.zhu@nxp.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

YES

- The change enables an optional 3.3V auxiliary PCIe supply early in
  probe and keeps it on for the controller’s lifetime via devm, which
  directly addresses link low‑power (L2) entry and wake signaling
  reliability. The new call
  `devm_regulator_get_enable_optional(&pdev->dev, "vpcie3v3aux")` is
  added in `drivers/pci/controller/dwc/pci-imx6.c:1744`. Errors other
  than “not present” are surfaced using `dev_err_probe()`
  (`drivers/pci/controller/dwc/pci-imx6.c:1745`), ensuring a clear,
  fail‑fast behavior if hardware provides the supply but it cannot be
  enabled.

- The helper used is a standard devres API that both acquires and
  enables the regulator for the device lifetime, and automatically
  disables it on device teardown. See the declaration in
  `include/linux/regulator/consumer.h:166` and implementation in
  `drivers/regulator/devres.c:110`. This matches the commit’s intent to
  “keep it enabled for the entire PCIe controller lifecycle.”

- This is a contained, minimal change within the i.MX DesignWare PCIe
  host driver probe path. It does not alter broader PCIe core behavior,
  call flows, or add architectural changes. It only:
  - Enables `vpcie3v3aux` if present (`drivers/pci/controller/dwc/pci-
    imx6.c:1744`).
  - Leaves existing supply handling intact for `vpcie` and `vph`
    (`drivers/pci/controller/dwc/pci-imx6.c:1748` and
    `drivers/pci/controller/dwc/pci-imx6.c:1755`).
  - Keeps `vpcie` enable/disable at host init/exit unchanged
    (`drivers/pci/controller/dwc/pci-imx6.c:1205`,
    `drivers/pci/controller/dwc/pci-imx6.c:1280`,
    `drivers/pci/controller/dwc/pci-imx6.c:1297`).

- The functional impact is to enable proper L2 and wake signaling
  (Beacon or WAKE#) on boards that wire up 3.3Vaux. The driver already
  carries context that AUX power matters; for example, i.MX95 has an
  erratum requiring AUX power detect handling to exit L23 Ready
  (`drivers/pci/controller/dwc/pci-imx6.c:245` comment explains AUX
  power implications). Turning on AUX power when available is therefore
  a correctness fix, not a feature.

- Risk/regression assessment:
  - If the supply is not defined, nothing changes (uses “optional” API
    and ignores `-ENODEV`).
  - If the supply is defined but cannot be enabled, probe now fails
    loudly; this surfaces real hardware/regulator issues instead of
    running with broken low‑power/wake behavior.
  - The pattern matches existing PCIe controller drivers that enable
    optional PCIe supplies at probe with the same helper (e.g.,
    `drivers/pci/controller/pcie-rcar-host.c:954`), indicating
    established practice across subsystems.
  - Binding-wise, the i.MX PCIe common binding allows additional
    properties (`additionalProperties: true` in
    `Documentation/devicetree/bindings/pci/fsl,imx6q-pcie-
    common.yaml:246`), so using `vpcie3v3aux-supply` is non‑disruptive
    for DT validation. DT updates are optional and can follow
    separately.

- Stable criteria fit:
  - Fixes a real user-visible issue (L2 entry and wake signaling fail
    without AUX).
  - Small and self-contained change in a single driver.
  - No architectural refactor or feature addition beyond enabling an
    optional, already-described hardware supply.
  - Uses existing, widely deployed APIs with minimal regression risk.

Given the clear bugfix nature, minimal scope, and alignment with
established patterns, this is a good candidate for stable backport.

 drivers/pci/controller/dwc/pci-imx6.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 80e48746bbaf6..db51e382a7cf3 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -1745,6 +1745,10 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	pci->max_link_speed = 1;
 	of_property_read_u32(node, "fsl,max-link-speed", &pci->max_link_speed);
 
+	ret = devm_regulator_get_enable_optional(&pdev->dev, "vpcie3v3aux");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to enable Vaux supply\n");
+
 	imx_pcie->vpcie = devm_regulator_get_optional(&pdev->dev, "vpcie");
 	if (IS_ERR(imx_pcie->vpcie)) {
 		if (PTR_ERR(imx_pcie->vpcie) != -ENODEV)
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH AUTOSEL 6.17-6.6] iio: adc: imx93_adc: load calibrated values even calibration failed
       [not found] <20251025160905.3857885-1-sashal@kernel.org>
  2025-10-25 15:53 ` [PATCH AUTOSEL 6.17] media: nxp: imx8-isi: Fix streaming cleanup on release Sasha Levin
  2025-10-25 15:54 ` [PATCH AUTOSEL 6.17-6.12] PCI: imx6: Enable the Vaux supply if available Sasha Levin
@ 2025-10-25 15:55 ` Sasha Levin
  2025-10-25 15:56 ` [PATCH AUTOSEL 6.17] media: imx-mipi-csis: Only set clock rate when specified in DT Sasha Levin
  3 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2025-10-25 15:55 UTC (permalink / raw)
  To: patches, stable
  Cc: Haibo Chen, Frank Li, Primoz Fiser, Jonathan Cameron, Sasha Levin,
	shawnguo, linux-iio, imx, linux-arm-kernel

From: Haibo Chen <haibo.chen@nxp.com>

[ Upstream commit 12c9b09e981ab14ebec8e4eefa946cbd26dd306b ]

ADC calibration might fail because of the noise on reference voltage.
To avoid calibration fail, need to meet the following requirement:
    ADC reference voltage Noise < 1.8V * 1/2^ENOB

For the case which the ADC reference voltage on board do not meet
the requirement, still load the calibrated values, so ADC can also
work but maybe not that accurate.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Primoz Fiser <primoz.fiser@norik.com>
Link: https://patch.msgid.link/20250812-adc-v2-2-0260833f13b8@nxp.com
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

YES

Explanation
- Problem addressed: On some boards the i.MX93 ADC calibration fails due
  to reference voltage noise (commit message). In the current code, a
  calibration failure aborts driver probe, leaving the ADC unusable for
  users.
- Current failure path: `imx93_adc_calibration()` returns `-EAGAIN` on
  calibration failure, explicitly powering down the ADC and aborting
  probe:
  - Failure check/return: drivers/iio/adc/imx93_adc.c:181 (checks
    `IMX93_ADC_MSR_CALFAIL_MASK`) and drivers/iio/adc/imx93_adc.c:185
    (returns `-EAGAIN`).
  - Probe abort on error: drivers/iio/adc/imx93_adc.c:367 (calibration
    call) and drivers/iio/adc/imx93_adc.c:368–396 (error unwinding).
- What the patch changes:
  - Adds `IMX93_ADC_CALCFG0` (0x3A0) and `IMX93_ADC_CALCFG0_LDFAIL_MASK`
    (BIT(4)) so the driver can instruct hardware to load calibrated
    values even if calibration “fails”.
  - In `imx93_adc_calibration()` (drivers/iio/adc/imx93_adc.c:146),
    before starting calibration, it writes
    `IMX93_ADC_CALCFG0_LDFAIL_MASK` to `IMX93_ADC_CALCFG0` to enable
    “load-on-fail”.
  - It changes the failure handling on `CALFAIL`: instead of returning
    `-EAGAIN`, it logs a warning and continues, allowing the driver to
    register and the ADC to function, albeit with potentially reduced
    accuracy.
  - The timeout path remains unchanged and still returns an error if
    calibration never completes (drivers/iio/adc/imx93_adc.c:171–178),
    preserving safety for a hard failure.
- User impact: This is a practical fix for real boards where Vref noise
  is above the stated threshold; without it, the ADC never comes up.
  With it, the ADC works (possibly with lower accuracy), which is
  typically preferable to complete unavailability.
- Scope and risk:
  - Small, contained change in a single driver
    (`drivers/iio/adc/imx93_adc.c`) with no ABI or framework changes.
  - No architectural refactoring; only adds a register define and a
    single bit write plus relaxed error handling.
  - Timeout/hard-error behavior is unchanged; only soft failure
    (CALFAIL) behavior is relaxed.
  - The driver matches only `nxp,imx93-adc`
    (drivers/iio/adc/imx93_adc.c:465–469), so the change is isolated to
    this hardware.
- Stable criteria:
  - Fixes a user-visible bug (driver failing to probe on noisy Vref
    boards).
  - Minimal and low risk; confined to probe/calibration logic.
  - No new features; behavior change is a robustness fix with guarded
    warning.
  - No broader side effects beyond this ADC device.

Given these points, this is a solid candidate for backporting to any
stable trees that contain the i.MX93 ADC driver and its current fail-
hard calibration path.

 drivers/iio/adc/imx93_adc.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/imx93_adc.c b/drivers/iio/adc/imx93_adc.c
index 7feaafd2316f2..9f1546c3d39d5 100644
--- a/drivers/iio/adc/imx93_adc.c
+++ b/drivers/iio/adc/imx93_adc.c
@@ -38,6 +38,7 @@
 #define IMX93_ADC_PCDR6		0x118
 #define IMX93_ADC_PCDR7		0x11c
 #define IMX93_ADC_CALSTAT	0x39C
+#define IMX93_ADC_CALCFG0	0x3A0
 
 /* ADC bit shift */
 #define IMX93_ADC_MCR_MODE_MASK			BIT(29)
@@ -58,6 +59,8 @@
 #define IMX93_ADC_IMR_ECH_MASK			BIT(0)
 #define IMX93_ADC_PCDR_CDATA_MASK		GENMASK(11, 0)
 
+#define IMX93_ADC_CALCFG0_LDFAIL_MASK		BIT(4)
+
 /* ADC status */
 #define IMX93_ADC_MSR_ADCSTATUS_IDLE			0
 #define IMX93_ADC_MSR_ADCSTATUS_POWER_DOWN		1
@@ -145,7 +148,7 @@ static void imx93_adc_config_ad_clk(struct imx93_adc *adc)
 
 static int imx93_adc_calibration(struct imx93_adc *adc)
 {
-	u32 mcr, msr;
+	u32 mcr, msr, calcfg;
 	int ret;
 
 	/* make sure ADC in power down mode */
@@ -158,6 +161,11 @@ static int imx93_adc_calibration(struct imx93_adc *adc)
 
 	imx93_adc_power_up(adc);
 
+	/* Enable loading of calibrated values even in fail condition */
+	calcfg = readl(adc->regs + IMX93_ADC_CALCFG0);
+	calcfg |= IMX93_ADC_CALCFG0_LDFAIL_MASK;
+	writel(calcfg, adc->regs + IMX93_ADC_CALCFG0);
+
 	/*
 	 * TODO: we use the default TSAMP/NRSMPL/AVGEN in MCR,
 	 * can add the setting of these bit if need in future.
@@ -180,9 +188,13 @@ static int imx93_adc_calibration(struct imx93_adc *adc)
 	/* check whether calbration is success or not */
 	msr = readl(adc->regs + IMX93_ADC_MSR);
 	if (msr & IMX93_ADC_MSR_CALFAIL_MASK) {
+		/*
+		 * Only give warning here, this means the noise of the
+		 * reference voltage do not meet the requirement:
+		 *     ADC reference voltage Noise < 1.8V * 1/2^ENOB
+		 * And the resault of ADC is not that accurate.
+		 */
 		dev_warn(adc->dev, "ADC calibration failed!\n");
-		imx93_adc_power_down(adc);
-		return -EAGAIN;
 	}
 
 	return 0;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH AUTOSEL 6.17] media: imx-mipi-csis: Only set clock rate when specified in DT
       [not found] <20251025160905.3857885-1-sashal@kernel.org>
                   ` (2 preceding siblings ...)
  2025-10-25 15:55 ` [PATCH AUTOSEL 6.17-6.6] iio: adc: imx93_adc: load calibrated values even calibration failed Sasha Levin
@ 2025-10-25 15:56 ` Sasha Levin
  3 siblings, 0 replies; 4+ messages in thread
From: Sasha Levin @ 2025-10-25 15:56 UTC (permalink / raw)
  To: patches, stable
  Cc: Laurent Pinchart, Frank Li, Hans Verkuil, Sasha Levin, rmfrfs,
	shawnguo, linux-media, imx, linux-arm-kernel

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[ Upstream commit 65673c6e33cf46f220cc5774166b373b3c087739 ]

The imx-mipi-csis driver sets the rate of the wrap clock to the value
specified in the device tree's "clock-frequency" property, and defaults
to 166 MHz otherwise. This is a historical mistake, as clock rate
selection should have been left to the assigned-clock-rates property.

Honouring the clock-frequency property can't be removed without breaking
backwards compatibility, and the corresponding code isn't very
intrusive. The 166 MHz default, on the other hand, prevents
configuration of the clock rate through assigned-clock-rates, as the
driver immediately overwrites the rate. This behaviour is confusing and
has cost debugging time.

There is little value in a 166 MHz default. All mainline device tree
sources that enable the CSIS specify a clock-frequency explicitly, and
the default wrap clock configuration on supported platforms is at least
as high as 166 MHz. Drop the default, and only set the clock rate
manually when the clock-frequency property is specified.

Link: https://lore.kernel.org/r/20250822002734.23516-10-laurent.pinchart@ideasonboard.com
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

YES

Why this is a bug fix
- The driver unconditionally forced the wrap clock to 166 MHz when the
  DT lacked a clock-frequency property, which overrides DT-assigned
  clock rates. That breaks the expected DT model where `assigned-clock-
  rates` controls rates, leading to misconfiguration and hard-to-debug
  behavior.
- This change stops overriding the clock unless the DT explicitly
  requests it, restoring correct DT semantics.

What changed (code references)
- Set rate only when explicitly requested:
  - `drivers/media/platform/nxp/imx-mipi-csis.c:744` now guards
    `clk_set_rate()` with `if (csis->clk_frequency) { ... }`, meaning
    the driver only sets the rate when the DT provided `clock-
    frequency`.
- Drop the 166 MHz fallback:
  - `drivers/media/platform/nxp/imx-mipi-csis.c:1483` now reads `clock-
    frequency` without assigning a default if the property is absent,
    removing the prior implicit 166 MHz default.
- The removal of the default macro and fallback behavior eliminates the
  unconditional override while preserving backward compatibility for DTs
  that do specify `clock-frequency`.

Why it matters (user impact)
- Systems using `assigned-clock-rates` in DT were previously ignored by
  the driver due to the unconditional 166 MHz set, causing unexpected
  clock rates and potential functional issues.
- With this patch, DT-provided assigned rates take effect unless a
  legacy DT explicitly uses `clock-frequency`, which is retained for
  compatibility.

Risk and compatibility
- Scope is small and contained to one driver; no core or architectural
  changes.
- Backward compatibility is preserved for legacy DTs that specify
  `clock-frequency` (the driver still sets the rate in that case).
- For DTs without `clock-frequency`, the driver no longer forces 166 MHz
  and leaves the rate to the clock framework/DT assignments. The commit
  rationale notes that all mainline DTs enabling CSIS already specify
  `clock-frequency`, and default platform wrap clock configurations are
  at least as high as 166 MHz, reducing regression risk.
- The only functional behavior change is the removal of an incorrect
  default that masked DT configuration.

Stable criteria assessment
- Fixes a real misbehavior that affects users (DT `assigned-clock-rates`
  ignored).
- Minimal, well-contained change in a single driver file.
- No new features or architectural changes.
- Low regression risk with explicit consideration for legacy DT
  compatibility.
- No explicit “Cc: stable” or “Fixes” tag, but technically aligns with
  stable policy as a correctness fix that removes a problematic default
  override.

Conclusion
- This is a clear, low-risk bug fix that restores proper DT semantics
  and prevents the driver from clobbering assigned clock rates. It
  should be backported to stable.

 drivers/media/platform/nxp/imx-mipi-csis.c | 23 +++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 2beb5f43c2c01..cea017a2b14ec 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -228,8 +228,6 @@
 #define MIPI_CSIS_PKTDATA_EVEN			0x3000
 #define MIPI_CSIS_PKTDATA_SIZE			SZ_4K
 
-#define DEFAULT_SCLK_CSIS_FREQ			166000000UL
-
 struct mipi_csis_event {
 	bool debug;
 	u32 mask;
@@ -704,12 +702,17 @@ static int mipi_csis_clk_get(struct mipi_csis_device *csis)
 	if (ret < 0)
 		return ret;
 
-	/* Set clock rate */
-	ret = clk_set_rate(csis->clks[MIPI_CSIS_CLK_WRAP].clk,
-			   csis->clk_frequency);
-	if (ret < 0)
-		dev_err(csis->dev, "set rate=%d failed: %d\n",
-			csis->clk_frequency, ret);
+	if (csis->clk_frequency) {
+		/*
+		 * Set the clock rate. This is deprecated, for backward
+		 * compatibility with old device trees.
+		 */
+		ret = clk_set_rate(csis->clks[MIPI_CSIS_CLK_WRAP].clk,
+				   csis->clk_frequency);
+		if (ret < 0)
+			dev_err(csis->dev, "set rate=%d failed: %d\n",
+				csis->clk_frequency, ret);
+	}
 
 	return ret;
 }
@@ -1413,9 +1416,7 @@ static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
 {
 	struct device_node *node = csis->dev->of_node;
 
-	if (of_property_read_u32(node, "clock-frequency",
-				 &csis->clk_frequency))
-		csis->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
+	of_property_read_u32(node, "clock-frequency", &csis->clk_frequency);
 
 	return 0;
 }
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-10-25 16:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20251025160905.3857885-1-sashal@kernel.org>
2025-10-25 15:53 ` [PATCH AUTOSEL 6.17] media: nxp: imx8-isi: Fix streaming cleanup on release Sasha Levin
2025-10-25 15:54 ` [PATCH AUTOSEL 6.17-6.12] PCI: imx6: Enable the Vaux supply if available Sasha Levin
2025-10-25 15:55 ` [PATCH AUTOSEL 6.17-6.6] iio: adc: imx93_adc: load calibrated values even calibration failed Sasha Levin
2025-10-25 15:56 ` [PATCH AUTOSEL 6.17] media: imx-mipi-csis: Only set clock rate when specified in DT Sasha Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox