[PATCH v4 00/15] media: mt9m114: Changes to make it work with atomisp devices

[PATCH v4 00/15] media: mt9m114: Changes to make it work with atomisp devices

[Hans de Goede]

Hi All,

Here is v4 of my series to make the "mainline" mt9m114 driver work
on devices with an atomisp CSI2 receiver / ISP. This has been tested on
an Asus T100TA.

Changes in v4:
- Drop "media: mt9m114: Add support for clock-frequency property" this is
  not longer necessary with the new vl2_sensor_clk_get() helper
- Add "media: aptina-pll: Add comment documenting the PLL chain"
- Rework "media: mt9m114: Drop start-, stop-streaming sequence from
  initialize" moving mt9m114_initialize() to mt9m114_start_streaming() and
  dropping the config_change_pending flag
- Address comments from Laurent about commit message and comment wording
- Some small code tweaks based on suggestions from Laurent

Changes in v3:
- Document that using 768Mhz for out_clock_max does not work
- Improve "media: mt9m114: Put sensor in reset on power down" commit message
- Drop setting of the MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE bit
- Split "media: mt9m114: Fix scaler bypass mode" into multiple patches,
  addressing various review comments as part of this

Changes in v2:
- Rebase on top of sailus/media_tree.git/fixes which now has 4 of
  the patches from Mathis': "MT9M114 driver bugfix and improvements"
  series, this avoids most of the conlicts between the 2 series
- Add Laurent's Reviewed-by to some of the patches
- Add select VIDEO_APTINA_PLL to Kconfig
- Use correct aptina_pll_limits
- After setting reset high wait 20 clk cycles before disabling
  the clk and regulators
- When bypassing the scalar make ifp_get_selection() / ifp_set_selection()
  fill sel->r with a rectangle of (0,0)/wxh and return 0 instead of
  returning -EINVAL

Regards,

Hans

---
Hans de Goede (15):
      media: aptina-pll: Debug log p1 min and max values
      media: aptina-pll: Add comment documenting the PLL chain
      media: mt9m114: Use aptina-PLL helper to get PLL values
      media: mt9m114: Lower minimum vblank value
      media: mt9m114: Fix default hblank and vblank values
      media: mt9m114: Tweak default hblank and vblank for more accurate fps
      media: mt9m114: Avoid a reset low spike during probe()
      media: mt9m114: Put sensor in reset on power down
      media: mt9m114: Add and use mt9m114_ifp_get_border() helper function
      media: mt9m114: Adjust IFP selections and source format when source format changes to/from RAW10
      media: mt9m114: Update source pad selection and format when sink pad format changes
      media: mt9m114: Don't allow changing the IFP crop/compose selections when bypassing the scaler
      media: mt9m114: Drop start-, stop-streaming sequence from initialize
      media: mt9m114: Return -EPROBE_DEFER if no endpoint is found
      media: mt9m114: Add ACPI enumeration support

 drivers/media/i2c/Kconfig      |   1 +
 drivers/media/i2c/aptina-pll.c |  12 ++
 drivers/media/i2c/mt9m114.c    | 257 ++++++++++++++++++++++++++++-------------
 3 files changed, 190 insertions(+), 80 deletions(-)
---
base-commit: b70886ff5833cf499e77af77d2324ce8f68b60ce
change-id: 20251224-mt9m114-atomisp-31a4d366328a

Best regards,
-- 
Hans de Goede <johannes.goede@oss.qualcomm.com>

[PATCH v4 01/15] media: aptina-pll: Debug log p1 min and max values

[Hans de Goede]

Make aptina_pll_calculate() debug log the calculated p1 min and max values,
this makes it easier to see how the m, n and p1 values were chosen.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/media/i2c/aptina-pll.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/aptina-pll.c b/drivers/media/i2c/aptina-pll.c
index b1f89bbf9d473f6ef00ebb8250405018d07e668b..cd2ed4583c97ec87e516acfd249fdccf2f9efbb8 100644
--- a/drivers/media/i2c/aptina-pll.c
+++ b/drivers/media/i2c/aptina-pll.c
@@ -129,6 +129,8 @@ int aptina_pll_calculate(struct device *dev,
 	p1_max = min(limits->p1_max, limits->out_clock_max * div /
 		     (pll->ext_clock * pll->m));
 
+	dev_dbg(dev, "pll: p1 min %u max %u\n", p1_min, p1_max);
+
 	for (p1 = p1_max & ~1; p1 >= p1_min; p1 -= 2) {
 		unsigned int mf_inc = p1 / gcd(div, p1);
 		unsigned int mf_high;

-- 
2.52.0

[PATCH v4 02/15] media: aptina-pll: Add comment documenting the PLL chain

[Hans de Goede]

Add a code-comment documenting the PLL chain, this is a verbatim
copy of Laurent's ASCII-art PLL chain from the mailinglist.

Link: https://lore.kernel.org/linux-media/20250629204655.GA2059@pendragon.ideasonboard.com/
Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Suggested-by: should really be Co-authored-by since I just copy and
pasted Laurent's comment from the list, but that requires Laurent's S-o-B.

Laurent can you give your S-o-B for adding a Co-authored-by ?
---
Changes in v4:
- New patch in v4 of this series
---
 drivers/media/i2c/aptina-pll.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/aptina-pll.c b/drivers/media/i2c/aptina-pll.c
index cd2ed4583c97ec87e516acfd249fdccf2f9efbb8..4a519ab587ba4cfb9945a1bb05e87a3b5e6d28c9 100644
--- a/drivers/media/i2c/aptina-pll.c
+++ b/drivers/media/i2c/aptina-pll.c
@@ -12,6 +12,16 @@
 
 #include "aptina-pll.h"
 
+/*
+ * Based on the docs the PLL is believed to have the following setup:
+ *
+ *         +-----+     +-----+     +-----+     +-----+     +-----+
+ * Fin --> | / N | --> | x M | --> | x 2 | --> | / P | --> | / 2 | -->
+ *         +-----+     +-----+     +-----+     +-----+     +-----+
+ *                                         fBit       fWord       fSensor
+ * ext_clock    int_clock   out_clock                             pix_clock
+ */
+
 int aptina_pll_calculate(struct device *dev,
 			 const struct aptina_pll_limits *limits,
 			 struct aptina_pll *pll)

-- 
2.52.0

[PATCH v4 03/15] media: mt9m114: Use aptina-PLL helper to get PLL values

[Hans de Goede]

Before this change the driver used hardcoded PLL m, n and p values to
achieve a 48MHz pixclock when used with an external clock with a frequency
of 24 MHz.

Use aptina_pll_calculate() to allow the driver to work with different
external clock frequencies. The m, n, and p values will be unchanged
with a 24 MHz extclk and this has also been tested with a 19.2 MHz
clock where m gets increased from 32 to 40.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v4:
- After re-reading the docs out_clock_max should be 384MHz and P1 should
  always be 8, adjust the pll-limits accordingly and drop the comment
  about the out_clock_max from the documentation not working

Changes in v3:
- Document that using 768Mhz for out_clock_max does not work

Changes in v2:
- Add select VIDEO_APTINA_PLL to Kconfig
- Use correct aptina_pll_limits
---
 drivers/media/i2c/Kconfig   |  1 +
 drivers/media/i2c/mt9m114.c | 50 +++++++++++++++++++++++++++++++--------------
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 4b4db8c4f49657e19018535927eb41f7ad2a4f80..befea5952191184536ad7d7e5c81f567826d8aa7 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -305,6 +305,7 @@ config VIDEO_MT9M111
 config VIDEO_MT9M114
 	tristate "onsemi MT9M114 sensor support"
 	select V4L2_CCI_I2C
+	select VIDEO_APTINA_PLL
 	help
 	  This is a Video4Linux2 sensor-level driver for the onsemi MT9M114
 	  camera.
diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 51ebbe7ae996950a58f8fee30029e0a060feaf3f..d1635f49ee047ca696f6053f6c17e30d736ab795 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -32,6 +32,8 @@
 #include <media/v4l2-mediabus.h>
 #include <media/v4l2-subdev.h>
 
+#include "aptina-pll.h"
+
 /* Sysctl registers */
 #define MT9M114_CHIP_ID					CCI_REG16(0x0000)
 #define MT9M114_COMMAND_REGISTER			CCI_REG16(0x0080)
@@ -267,9 +269,9 @@
 #define MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE			BIT(0)
 #define MT9M114_CAM_SYSCTL_PLL_DISABLE_VALUE			0x00
 #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N		CCI_REG16(0xc980)
-#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(m, n)		(((n) << 8) | (m))
+#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(m, n)		((((n) - 1) << 8) | (m))
 #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P		CCI_REG16(0xc982)
-#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(p)		((p) << 8)
+#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(p)		(((p) - 1) << 8)
 #define MT9M114_CAM_PORT_OUTPUT_CONTROL			CCI_REG16(0xc984)
 #define MT9M114_CAM_PORT_PORT_SELECT_PARALLEL			(0 << 0)
 #define MT9M114_CAM_PORT_PORT_SELECT_MIPI			(1 << 0)
@@ -330,7 +332,7 @@
  * minimum values that have been seen in register lists are 303 and 38, use
  * them.
  *
- * Set the default to achieve 1280x960 at 30fps.
+ * Set the default to achieve 1280x960 at 30fps with a 48 MHz pixclock.
  */
 #define MT9M114_MIN_HBLANK				303
 #define MT9M114_MIN_VBLANK				38
@@ -340,6 +342,8 @@
 #define MT9M114_DEF_FRAME_RATE				30
 #define MT9M114_MAX_FRAME_RATE				120
 
+#define MT9M114_DEF_PIXCLOCK				48000000
+
 #define MT9M114_PIXEL_ARRAY_WIDTH			1296U
 #define MT9M114_PIXEL_ARRAY_HEIGHT			976U
 
@@ -384,11 +388,7 @@ struct mt9m114 {
 	struct v4l2_fwnode_endpoint bus_cfg;
 	bool bypass_pll;
 
-	struct {
-		unsigned int m;
-		unsigned int n;
-		unsigned int p;
-	} pll;
+	struct aptina_pll pll;
 
 	unsigned int pixrate;
 	bool streaming;
@@ -758,7 +758,7 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
 							       sensor->pll.n),
 			  &ret);
 		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P,
-			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p),
+			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p1),
 			  &ret);
 	}
 
@@ -2283,12 +2283,25 @@ static int mt9m114_verify_link_frequency(struct mt9m114 *sensor,
 
 static int mt9m114_clk_init(struct mt9m114 *sensor)
 {
+	static const struct aptina_pll_limits limits = {
+		.ext_clock_min = 6000000,
+		.ext_clock_max = 54000000,
+		/* int_clock_* limits are not documented taken from mt9p031.c */
+		.int_clock_min = 2000000,
+		.int_clock_max = 13500000,
+		/* out_clock_min is not documented, taken from mt9p031.c */
+		.out_clock_min = 180000000,
+		.out_clock_max = 384000000,
+		.pix_clock_max = 48000000,
+		.n_min = 1,
+		.n_max = 64,
+		.m_min = 16,
+		.m_max = 192,
+		.p1_min = 8,
+		.p1_max = 8,
+	};
 	unsigned int pixrate;
-
-	/* Hardcode the PLL multiplier and dividers to default settings. */
-	sensor->pll.m = 32;
-	sensor->pll.n = 1;
-	sensor->pll.p = 7;
+	int ret;
 
 	/*
 	 * Calculate the pixel rate and link frequency. The CSI-2 bus is clocked
@@ -2308,8 +2321,15 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
 	}
 
 	/* Check if the PLL configuration fits the configured link frequency. */
+	sensor->pll.ext_clock = clk_get_rate(sensor->clk);
+	sensor->pll.pix_clock = MT9M114_DEF_PIXCLOCK;
+
+	ret = aptina_pll_calculate(&sensor->client->dev, &limits, &sensor->pll);
+	if (ret)
+		return ret;
+
 	pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
-		/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
+		/ (sensor->pll.n * sensor->pll.p1);
 	if (mt9m114_verify_link_frequency(sensor, pixrate) == 0) {
 		sensor->pixrate = pixrate;
 		sensor->bypass_pll = false;

-- 
2.52.0

[PATCH v4 04/15] media: mt9m114: Lower minimum vblank value

[Hans de Goede]

As the comment above the defines says, the minimum values are undocumented
so the lowest values seen in register lists are used.

The version of the mt9m114 driver shipped together with the atomisp code
uses 21 for vblank in its register lists, lower MT9M114_MIN_VBLANK
accordingly.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/media/i2c/mt9m114.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index d1635f49ee047ca696f6053f6c17e30d736ab795..f63ab4bd040a4a62833d8bc716f94fc29aa03bd7 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -329,13 +329,13 @@
 
 /*
  * The minimum amount of horizontal and vertical blanking is undocumented. The
- * minimum values that have been seen in register lists are 303 and 38, use
+ * minimum values that have been seen in register lists are 303 and 21, use
  * them.
  *
  * Set the default to achieve 1280x960 at 30fps with a 48 MHz pixclock.
  */
 #define MT9M114_MIN_HBLANK				303
-#define MT9M114_MIN_VBLANK				38
+#define MT9M114_MIN_VBLANK				21
 #define MT9M114_DEF_HBLANK				323
 #define MT9M114_DEF_VBLANK				39
 

-- 
2.52.0

[PATCH v4 05/15] media: mt9m114: Fix default hblank and vblank values

[Hans de Goede]

The current default hblank and vblank values are based on reaching 30 fps
with the pixel-array outputting 1280x960, but the default format for
the pixel-array source pad and the isp sink pad is 1296x976, correct
the default hblank and vblank values to take this into account.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v2:
- Update comment about resolution / pixrate / FPS to:
 * Set the default to achieve full resolution (1296x976 analog crop
 * rectangle, 1280x960 output size) at 30fps with a 48 MHz pixclock.
---
 drivers/media/i2c/mt9m114.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index f63ab4bd040a4a62833d8bc716f94fc29aa03bd7..3fed509055142d7a134cae6cabadde423743178c 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -332,12 +332,13 @@
  * minimum values that have been seen in register lists are 303 and 21, use
  * them.
  *
- * Set the default to achieve 1280x960 at 30fps with a 48 MHz pixclock.
+ * Set the default to achieve full resolution (1296x976 analog crop
+ * rectangle, 1280x960 output size) at 30fps with a 48 MHz pixclock.
  */
 #define MT9M114_MIN_HBLANK				303
 #define MT9M114_MIN_VBLANK				21
-#define MT9M114_DEF_HBLANK				323
-#define MT9M114_DEF_VBLANK				39
+#define MT9M114_DEF_HBLANK				307
+#define MT9M114_DEF_VBLANK				23
 
 #define MT9M114_DEF_FRAME_RATE				30
 #define MT9M114_MAX_FRAME_RATE				120

-- 
2.52.0

[PATCH v4 06/15] media: mt9m114: Tweak default hblank and vblank for more accurate fps

[Hans de Goede]

The PLL gets programmed to achieve a 48 MHz pixelclock, with the current
vblank + hblank defaults this results in a fps of:

48000000 / ((1296 + 307) * (976 + 23) = 29.974 fps

Tweak the defaults to get closer to 30 fps:

48000000 / ((1296 + 308) * (976 + 21) = 30.015 fps

This improves things from being 0.026 fps too low to 0.015 fps too high.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/media/i2c/mt9m114.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 3fed509055142d7a134cae6cabadde423743178c..d58445826c090f377db473f7926c1b612874ff78 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -337,8 +337,8 @@
  */
 #define MT9M114_MIN_HBLANK				303
 #define MT9M114_MIN_VBLANK				21
-#define MT9M114_DEF_HBLANK				307
-#define MT9M114_DEF_VBLANK				23
+#define MT9M114_DEF_HBLANK				308
+#define MT9M114_DEF_VBLANK				21
 
 #define MT9M114_DEF_FRAME_RATE				30
 #define MT9M114_MAX_FRAME_RATE				120

-- 
2.52.0

[PATCH v4 07/15] media: mt9m114: Avoid a reset low spike during probe()

[Hans de Goede]

mt9m114_probe() requests the reset GPIO in output low state:

	sensor->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);

and then almost immediately afterwards calls mt9m114_power_on() which does:

		gpiod_set_value(sensor->reset, 1);
		fsleep(duration);
		gpiod_set_value(sensor->reset, 0);

which means that if the reset pin was high before this code runs that
it will very briefly be driven low because of passing GPIOD_OUT_LOW when
requesting the GPIO only to be driven high again possibly directly after
that. Such a very brief driving low of the reset pin may put the chip in
a confused state.

Request the GPIO in high (reset the chip) state instead to avoid this,
turning the initial gpiod_set_value() in mt9m114_power_on() into a no-op.
and the fsleep() ensures that it will stay high long enough to properly
reset the chip.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/media/i2c/mt9m114.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index d58445826c090f377db473f7926c1b612874ff78..60afc32acee4df1e7d4f820838b72f6aa3506f8d 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2455,7 +2455,7 @@ static int mt9m114_probe(struct i2c_client *client)
 		goto error_ep_free;
 	}
 
-	sensor->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	sensor->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(sensor->reset)) {
 		ret = PTR_ERR(sensor->reset);
 		dev_err_probe(dev, ret, "Failed to get reset GPIO\n");

-- 
2.52.0

[PATCH v4 08/15] media: mt9m114: Put sensor in reset on power down

[Hans de Goede]

Put the sensor back in reset on power down. Putting the sensor in reset
reduces power-consumption by putting all the data / ctrl pins in High-Z
mode. This helps save power on designs where the regulators may need to
stay on while the sensor is powered down.

This also ensures that the sensor is properly reset on power up,
since now the sensor will see a reset high to low transition after
the regulators have been turned on.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v3:
- Improve commit message

Changes in v2
- After setting reset high wait 20 clk cycles before disabling
  the clk and regulators
---
 drivers/media/i2c/mt9m114.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 60afc32acee4df1e7d4f820838b72f6aa3506f8d..39f9d20221fee4f1d12c24a031d9f33c953b9a9c 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2228,6 +2228,13 @@ static int mt9m114_power_on(struct mt9m114 *sensor)
 
 static void mt9m114_power_off(struct mt9m114 *sensor)
 {
+	unsigned int duration;
+
+	gpiod_set_value(sensor->reset, 1);
+	/* Power off takes 10 clock cycles. Double it to be safe. */
+	duration = DIV_ROUND_UP(2 * 10 * 1000000, clk_get_rate(sensor->clk));
+	fsleep(duration);
+
 	clk_disable_unprepare(sensor->clk);
 	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
 }

-- 
2.52.0

[PATCH v4 09/15] media: mt9m114: Add and use mt9m114_ifp_get_border() helper function

[Hans de Goede]

Normally the IFP removes a 4 pixel border all around its sink format
size for demosaicing. But in RAW10 mode it does not do this.

Add a new mt9m114_ifp_get_border() helper function to get the border size
(4 or 0) and use this where applicable instead of hardcoding a border
of 4 pixels everywhere.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v4:
- Address comments from Laurent about commit message and comment wording

Changes in v3:
- New patch in v3 of this patch-set
---
 drivers/media/i2c/mt9m114.c | 62 ++++++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 39f9d20221fee4f1d12c24a031d9f33c953b9a9c..c0710dbb670d4e40e13fc895cf8c84219ea43bde 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -851,6 +851,18 @@ static int mt9m114_configure_pa(struct mt9m114 *sensor,
 	return ret;
 }
 
+/*
+ * For source pad formats other then RAW10 the IFP removes a 4 pixel border from
+ * its sink pad format size for demosaicing.
+ */
+static int mt9m114_ifp_get_border(struct v4l2_subdev_state *state)
+{
+	const struct v4l2_mbus_framefmt *format =
+		v4l2_subdev_state_get_format(state, 1);
+
+	return format->code == MEDIA_BUS_FMT_SGRBG10_1X10 ? 0 : 4;
+}
+
 static int mt9m114_configure_ifp(struct mt9m114 *sensor,
 				 struct v4l2_subdev_state *state)
 {
@@ -858,6 +870,7 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
 	const struct v4l2_mbus_framefmt *format;
 	const struct v4l2_rect *crop;
 	const struct v4l2_rect *compose;
+	unsigned int border;
 	u64 output_format;
 	int ret = 0;
 
@@ -872,15 +885,18 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
 		return ret;
 
 	/*
-	 * Color pipeline (IFP) cropping and scaling. Subtract 4 from the left
-	 * and top coordinates to compensate for the lines and columns removed
-	 * by demosaicing that are taken into account in the crop rectangle but
-	 * not in the hardware.
+	 * Color pipeline (IFP) cropping and scaling. The crop window registers
+	 * apply cropping after demosaicing, which itself consumes 4 pixels on
+	 * each side of the image. The crop rectangle exposed to userspace
+	 * includes that demosaicing border, subtract it from the left and top
+	 * coordinates to configure the crop window.
 	 */
+	border = mt9m114_ifp_get_border(state);
+
 	cci_write(sensor->regmap, MT9M114_CAM_CROP_WINDOW_XOFFSET,
-		  crop->left - 4, &ret);
+		  crop->left - border, &ret);
 	cci_write(sensor->regmap, MT9M114_CAM_CROP_WINDOW_YOFFSET,
-		  crop->top - 4, &ret);
+		  crop->top - border, &ret);
 	cci_write(sensor->regmap, MT9M114_CAM_CROP_WINDOW_WIDTH,
 		  crop->width, &ret);
 	cci_write(sensor->regmap, MT9M114_CAM_CROP_WINDOW_HEIGHT,
@@ -1865,6 +1881,7 @@ static int mt9m114_ifp_get_selection(struct v4l2_subdev *sd,
 {
 	const struct v4l2_mbus_framefmt *format;
 	const struct v4l2_rect *crop;
+	unsigned int border;
 	int ret = 0;
 
 	/* Crop and compose are only supported on the sink pad. */
@@ -1879,15 +1896,17 @@ static int mt9m114_ifp_get_selection(struct v4l2_subdev *sd,
 	case V4L2_SEL_TGT_CROP_DEFAULT:
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 		/*
-		 * The crop default and bounds are equal to the sink
-		 * format size minus 4 pixels on each side for demosaicing.
+		 * Crop defaults and bounds are equal to the sink format size.
+		 * For source pad formats other then RAW10 this gets reduced
+		 * by 4 pixels on each side for demosaicing.
 		 */
 		format = v4l2_subdev_state_get_format(state, 0);
+		border = mt9m114_ifp_get_border(state);
 
-		sel->r.left = 4;
-		sel->r.top = 4;
-		sel->r.width = format->width - 8;
-		sel->r.height = format->height - 8;
+		sel->r.left = border;
+		sel->r.top = border;
+		sel->r.width = format->width - 2 * border;
+		sel->r.height = format->height - 2 * border;
 		break;
 
 	case V4L2_SEL_TGT_COMPOSE:
@@ -1922,6 +1941,7 @@ static int mt9m114_ifp_set_selection(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
 	struct v4l2_rect *compose;
+	unsigned int border;
 
 	if (sel->target != V4L2_SEL_TGT_CROP &&
 	    sel->target != V4L2_SEL_TGT_COMPOSE)
@@ -1937,21 +1957,23 @@ static int mt9m114_ifp_set_selection(struct v4l2_subdev *sd,
 
 	if (sel->target == V4L2_SEL_TGT_CROP) {
 		/*
-		 * Clamp the crop rectangle. Demosaicing removes 4 pixels on
-		 * each side of the image.
+		 * Clamp the crop rectangle. For source pad formats other then
+		 * RAW10 demosaicing removes 4 pixels on each side of the image.
 		 */
-		crop->left = clamp_t(unsigned int, ALIGN(sel->r.left, 2), 4,
-				     format->width - 4 -
+		border = mt9m114_ifp_get_border(state);
+
+		crop->left = clamp_t(unsigned int, ALIGN(sel->r.left, 2), border,
+				     format->width - border -
 				     MT9M114_SCALER_CROPPED_INPUT_WIDTH);
-		crop->top = clamp_t(unsigned int, ALIGN(sel->r.top, 2), 4,
-				    format->height - 4 -
+		crop->top = clamp_t(unsigned int, ALIGN(sel->r.top, 2), border,
+				    format->height - border -
 				    MT9M114_SCALER_CROPPED_INPUT_HEIGHT);
 		crop->width = clamp_t(unsigned int, ALIGN(sel->r.width, 2),
 				      MT9M114_SCALER_CROPPED_INPUT_WIDTH,
-				      format->width - 4 - crop->left);
+				      format->width - border - crop->left);
 		crop->height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
 				       MT9M114_SCALER_CROPPED_INPUT_HEIGHT,
-				       format->height - 4 - crop->top);
+				       format->height - border - crop->top);
 
 		sel->r = *crop;
 

-- 
2.52.0

[PATCH v4 10/15] media: mt9m114: Adjust IFP selections and source format when source format changes to/from RAW10

[Hans de Goede]

Changing the IFP source pad format to RAW10 means disabling the scaler,
which means that the crop and compose rectangles must be reset to
match the sink format size with no border.

And when changing the source pad format back from RAW10 to another format
which require demosaicing the crop and compose rectangles must be reset
to the sink format size minus a 4 pixels border all around it.

Also when changing the source pad format back from RAW10 to another format
the colorspace, ycbcr_enc and quantization need to be updated too.

Add a new mt9m114_ifp_update_sel_and_src_fmt() helper which resets all
these taking the bordersize for the new source format into account and
call this helper whenever the source pad format changes to/from RAW10.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v4:
- Address comments from Laurent about commit message and comment wording
- Use simpler code to test when source pad format changes to/from RAW10

Changes in v3:
- This is a new patch in v3 of this patch-set, which comes from splitting
  up "media: mt9m114: Fix scaler bypass mode" into multiple patches
---
 drivers/media/i2c/mt9m114.c | 53 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index c0710dbb670d4e40e13fc895cf8c84219ea43bde..a9162457bf649404ac2057328799f1be61558481 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1840,6 +1840,41 @@ static int mt9m114_ifp_enum_frameintervals(struct v4l2_subdev *sd,
 	return 0;
 }
 
+/*
+ * Helper function to update IFP crop, compose rectangles and source format
+ * when the pixel border size changes, which requires resetting these.
+ */
+static void mt9m114_ifp_update_sel_and_src_fmt(struct v4l2_subdev_state *state)
+{
+	struct v4l2_mbus_framefmt *src_format, *sink_format;
+	struct v4l2_rect *crop;
+	unsigned int border;
+
+	sink_format = v4l2_subdev_state_get_format(state, 0);
+	src_format = v4l2_subdev_state_get_format(state, 1);
+	crop = v4l2_subdev_state_get_crop(state, 0);
+	border = mt9m114_ifp_get_border(state);
+
+	crop->left = border;
+	crop->top = border;
+	crop->width = sink_format->width - 2 * border;
+	crop->height = sink_format->height - 2 * border;
+	*v4l2_subdev_state_get_compose(state, 0) = *crop;
+
+	src_format->width = crop->width;
+	src_format->height = crop->height;
+
+	if (src_format->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
+		src_format->colorspace = V4L2_COLORSPACE_RAW;
+		src_format->ycbcr_enc = V4L2_YCBCR_ENC_601;
+		src_format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	} else {
+		src_format->colorspace = V4L2_COLORSPACE_SRGB;
+		src_format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+		src_format->quantization = V4L2_QUANTIZATION_DEFAULT;
+	}
+}
+
 static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
 			       struct v4l2_subdev_state *state,
 			       struct v4l2_subdev_format *fmt)
@@ -1863,11 +1898,19 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
 		/* Only the media bus code can be changed on the source pad. */
 		info = mt9m114_format_info(sensor, 1, fmt->format.code);
 
-		format->code = info->code;
-
-		/* If the output format is RAW10, bypass the scaler. */
-		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
-			*format = *v4l2_subdev_state_get_format(state, 0);
+		/*
+		 * If the output format changes from/to RAW10 then the crop
+		 * rectangle needs to be adjusted to add / remove the 4 pixel
+		 * border used for demosaicing. And these changes then need to
+		 * be propagated to the compose rectangle and source format.
+		 */
+		if ((format->code == MEDIA_BUS_FMT_SGRBG10_1X10) !=
+		    (info->code == MEDIA_BUS_FMT_SGRBG10_1X10)) {
+			format->code = info->code;
+			mt9m114_ifp_update_sel_and_src_fmt(state);
+		} else {
+			format->code = info->code;
+		}
 	}
 
 	fmt->format = *format;

-- 
2.52.0

[PATCH v4 11/15] media: mt9m114: Update source pad selection and format when sink pad format changes

[Hans de Goede]

Call mt9m114_ifp_update_sel_and_src_fmt() on sink pad format changes to
propagate these downstream.

This is necessary in 2 different scenarios:

1. When passing through RAW10 bypassing the scaler then any sink pad format
changes must be propagated to the crop/compose selections and to the source
pad format.

2. When the scaler is active, then the crop-rectangle cannot be bigger then
the sink pad format minus a 4 pixel border all around. If the sink format
change reduces the size then things also needs to be propagated downstream.

Rather then adding extra code to check for these conditions, simply always
propagate sink pad format changes downstream.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v4:
- Address comments from Laurent about commit message and comment wording
---
 drivers/media/i2c/mt9m114.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index a9162457bf649404ac2057328799f1be61558481..8e0b5bf6db621d12b36f578b6374377c69503976 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1892,6 +1892,9 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
 		format->height = clamp(ALIGN(fmt->format.height, 8),
 				       MT9M114_PIXEL_ARRAY_MIN_OUTPUT_HEIGHT,
 				       MT9M114_PIXEL_ARRAY_HEIGHT);
+
+		/* Propagate changes downstream. */
+		mt9m114_ifp_update_sel_and_src_fmt(state);
 	} else {
 		const struct mt9m114_format_info *info;
 

-- 
2.52.0

[PATCH v4 12/15] media: mt9m114: Don't allow changing the IFP crop/compose selections when bypassing the scaler

[Hans de Goede]

The scaler is bypassed when the ISP source/output pad's pixel-format is
set to MEDIA_BUS_FMT_SGRBG10_1X10. Don't allow changing the IFP crop and/or
compose selections when in this mode.

Instead of returning -EINVAL simply return the current (noop) crop and
compose rectangles.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v4:
- Move crop = v4l2_subdev_state_get_crop(state, 0); up a couple of lines
  to avoid having to call it twice

Changes in v3:
- This is a new patch in v3 of this patch-set, which comes from splitting
  up "media: mt9m114: Fix scaler bypass mode" into multiple patches
- Add src_format local variable
---
 drivers/media/i2c/mt9m114.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 8e0b5bf6db621d12b36f578b6374377c69503976..447a5eb34a6137a8e87bd119401571b5592fc77d 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1984,7 +1984,7 @@ static int mt9m114_ifp_set_selection(struct v4l2_subdev *sd,
 				     struct v4l2_subdev_state *state,
 				     struct v4l2_subdev_selection *sel)
 {
-	struct v4l2_mbus_framefmt *format;
+	struct v4l2_mbus_framefmt *format, *src_format;
 	struct v4l2_rect *crop;
 	struct v4l2_rect *compose;
 	unsigned int border;
@@ -1997,8 +1997,16 @@ static int mt9m114_ifp_set_selection(struct v4l2_subdev *sd,
 	if (sel->pad != 0)
 		return -EINVAL;
 
-	format = v4l2_subdev_state_get_format(state, 0);
 	crop = v4l2_subdev_state_get_crop(state, 0);
+
+	/* Crop and compose cannot be changed when bypassing the scaler. */
+	src_format = v4l2_subdev_state_get_format(state, 1);
+	if (src_format->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
+		sel->r = *crop;
+		return 0;
+	}
+
+	format = v4l2_subdev_state_get_format(state, 0);
 	compose = v4l2_subdev_state_get_compose(state, 0);
 
 	if (sel->target == V4L2_SEL_TGT_CROP) {
@@ -2043,9 +2051,8 @@ static int mt9m114_ifp_set_selection(struct v4l2_subdev *sd,
 	}
 
 	/* Propagate the compose rectangle to the source format. */
-	format = v4l2_subdev_state_get_format(state, 1);
-	format->width = compose->width;
-	format->height = compose->height;
+	src_format->width = compose->width;
+	src_format->height = compose->height;
 
 	return 0;
 }

-- 
2.52.0

[PATCH v4 13/15] media: mt9m114: Drop start-, stop-streaming sequence from initialize

[Hans de Goede]

Drop the start-, stop-streaming sequence from initialize.

When streaming is started with a runtime-suspended sensor,
mt9m114_start_streaming() will runtime-resume the sensor which calls
mt9m114_initialize() immediately followed by calling
mt9m114_set_state(ENTER_CONFIG_CHANGE).

This results in the following state changes in quick succession:

mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING
mt9m114_set_state(ENTER_SUSPEND)       -> transitions to SUSPENDED
mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING

these quick state changes confuses the CSI receiver on atomisp devices
causing streaming to not work.

Drop the state changes from mt9m114_initialize() and move
the mt9m114_initialize() call to mt9m114_start_streaming()
so that only a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call
is made when streaming is started with a runtime-suspend sensor.

Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v4:
- Move the mt9m114_initialize() call to mt9m114_start_streaming()
  and drop the config_change_pending flag
---
 drivers/media/i2c/mt9m114.c | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 447a5eb34a6137a8e87bd119401571b5592fc77d..41e98f719a23045293ee47d8980675510a142afa 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -789,14 +789,6 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
 	if (ret < 0)
 		return ret;
 
-	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
-	if (ret < 0)
-		return ret;
-
-	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_SUSPEND);
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
@@ -967,6 +959,10 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
 	if (ret)
 		return ret;
 
+	ret = mt9m114_initialize(sensor);
+	if (ret)
+		goto error;
+
 	ret = mt9m114_configure_ifp(sensor, ifp_state);
 	if (ret)
 		goto error;
@@ -2318,19 +2314,8 @@ static int __maybe_unused mt9m114_runtime_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
-	int ret;
 
-	ret = mt9m114_power_on(sensor);
-	if (ret)
-		return ret;
-
-	ret = mt9m114_initialize(sensor);
-	if (ret) {
-		mt9m114_power_off(sensor);
-		return ret;
-	}
-
-	return 0;
+	return mt9m114_power_on(sensor);
 }
 
 static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
@@ -2562,8 +2547,8 @@ static int mt9m114_probe(struct i2c_client *client)
 	/*
 	 * Identify the sensor. The driver supports runtime PM, but needs to
 	 * work when runtime PM is disabled in the kernel. To that end, power
-	 * the sensor on manually here, and initialize it after identification
-	 * to reach the same state as if resumed through runtime PM.
+	 * the sensor on manually here and identify it to reach the same state
+	 * as if resumed through runtime PM.
 	 */
 	ret = mt9m114_power_on(sensor);
 	if (ret < 0) {
@@ -2575,10 +2560,6 @@ static int mt9m114_probe(struct i2c_client *client)
 	if (ret < 0)
 		goto error_power_off;
 
-	ret = mt9m114_initialize(sensor);
-	if (ret < 0)
-		goto error_power_off;
-
 	/*
 	 * Enable runtime PM with autosuspend. As the device has been powered
 	 * manually, mark it as active, and increase the usage count without

-- 
2.52.0

[PATCH v4 14/15] media: mt9m114: Return -EPROBE_DEFER if no endpoint is found

[Hans de Goede]

With IPU# bridges, endpoints may only be created when the IPU bridge is
initialized. This may happen after the sensor driver's first probe().

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
Changes in v4:
- Update comment about why EPROBE_DEFER handling is necessary
---
 drivers/media/i2c/mt9m114.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 41e98f719a23045293ee47d8980675510a142afa..9bf900095515447d22265521349cce1d7892e9b8 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2448,11 +2448,17 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
 	struct fwnode_handle *ep;
 	int ret;
 
+	/*
+	 * On ACPI systems the fwnode graph can be initialized by a bridge
+	 * driver, which may not have probed yet. Wait for this.
+	 *
+	 * TODO: Return an error once bridge driver code will have moved
+	 * to the ACPI core.
+	 */
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
-	if (!ep) {
-		dev_err(&sensor->client->dev, "No endpoint found\n");
-		return -EINVAL;
-	}
+	if (!ep)
+		return dev_err_probe(&sensor->client->dev, -EPROBE_DEFER,
+				     "waiting for fwnode graph endpoint\n");
 
 	sensor->bus_cfg.bus_type = V4L2_MBUS_UNKNOWN;
 	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &sensor->bus_cfg);

-- 
2.52.0

[PATCH v4 15/15] media: mt9m114: Add ACPI enumeration support

[Hans de Goede]

Add support for the mt9m114 sensor being enumerated through ACPI
using the INT33F0 HID as found on the Asus T100TA.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/media/i2c/mt9m114.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 9bf900095515447d22265521349cce1d7892e9b8..836ba16d1accbfc1a6b8878ba8d3806ef7a1caa1 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2640,11 +2640,18 @@ static const struct of_device_id mt9m114_of_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, mt9m114_of_ids);
 
+static const struct acpi_device_id mt9m114_acpi_ids[] = {
+	{ "INT33F0" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(acpi, mt9m114_acpi_ids);
+
 static struct i2c_driver mt9m114_driver = {
 	.driver = {
 		.name	= "mt9m114",
 		.pm	= &mt9m114_pm_ops,
 		.of_match_table = mt9m114_of_ids,
+		.acpi_match_table = mt9m114_acpi_ids,
 	},
 	.probe		= mt9m114_probe,
 	.remove		= mt9m114_remove,

-- 
2.52.0

Re: [PATCH v4 02/15] media: aptina-pll: Add comment documenting the PLL chain

[Laurent Pinchart]

On Wed, Dec 24, 2025 at 01:31:11PM +0100, Hans de Goede wrote:
> Add a code-comment documenting the PLL chain, this is a verbatim
> copy of Laurent's ASCII-art PLL chain from the mailinglist.
> 
> Link: https://lore.kernel.org/linux-media/20250629204655.GA2059@pendragon.ideasonboard.com/
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> ---
> Suggested-by: should really be Co-authored-by since I just copy and
> pasted Laurent's comment from the list, but that requires Laurent's S-o-B.
> 
> Laurent can you give your S-o-B for adding a Co-authored-by ?

If you insist,

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

but it's such a small patch that it doesn't matter much to me.

> ---
> Changes in v4:
> - New patch in v4 of this series
> ---
>  drivers/media/i2c/aptina-pll.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/media/i2c/aptina-pll.c b/drivers/media/i2c/aptina-pll.c
> index cd2ed4583c97ec87e516acfd249fdccf2f9efbb8..4a519ab587ba4cfb9945a1bb05e87a3b5e6d28c9 100644
> --- a/drivers/media/i2c/aptina-pll.c
> +++ b/drivers/media/i2c/aptina-pll.c
> @@ -12,6 +12,16 @@
>  
>  #include "aptina-pll.h"
>  
> +/*
> + * Based on the docs the PLL is believed to have the following setup:
> + *
> + *         +-----+     +-----+     +-----+     +-----+     +-----+
> + * Fin --> | / N | --> | x M | --> | x 2 | --> | / P | --> | / 2 | -->
> + *         +-----+     +-----+     +-----+     +-----+     +-----+
> + *                                         fBit       fWord       fSensor
> + * ext_clock    int_clock   out_clock                             pix_clock
> + */

I think this belongs to mt9m114.c. The other sensor that uses
aptina-pll, MT9P031, does not include the x2 multiplier or /2 divider,
and has no concept of fBit as it has a parallel output only.

Could you please also capture that the datasheet has a constraint on
fBit, which we translate to a constraint on out_clock for the PLL
calculator by dividing it by 2 ?

> +
>  int aptina_pll_calculate(struct device *dev,
>  			 const struct aptina_pll_limits *limits,
>  			 struct aptina_pll *pll)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 03/15] media: mt9m114: Use aptina-PLL helper to get PLL values

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Wed, Dec 24, 2025 at 01:31:12PM +0100, Hans de Goede wrote:
> Before this change the driver used hardcoded PLL m, n and p values to
> achieve a 48MHz pixclock when used with an external clock with a frequency
> of 24 MHz.
> 
> Use aptina_pll_calculate() to allow the driver to work with different
> external clock frequencies. The m, n, and p values will be unchanged
> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
> clock where m gets increased from 32 to 40.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> ---
> Changes in v4:
> - After re-reading the docs out_clock_max should be 384MHz and P1 should
>   always be 8, adjust the pll-limits accordingly and drop the comment
>   about the out_clock_max from the documentation not working
> 
> Changes in v3:
> - Document that using 768Mhz for out_clock_max does not work
> 
> Changes in v2:
> - Add select VIDEO_APTINA_PLL to Kconfig
> - Use correct aptina_pll_limits
> ---
>  drivers/media/i2c/Kconfig   |  1 +
>  drivers/media/i2c/mt9m114.c | 50 +++++++++++++++++++++++++++++++--------------
>  2 files changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 4b4db8c4f49657e19018535927eb41f7ad2a4f80..befea5952191184536ad7d7e5c81f567826d8aa7 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -305,6 +305,7 @@ config VIDEO_MT9M111
>  config VIDEO_MT9M114
>  	tristate "onsemi MT9M114 sensor support"
>  	select V4L2_CCI_I2C
> +	select VIDEO_APTINA_PLL
>  	help
>  	  This is a Video4Linux2 sensor-level driver for the onsemi MT9M114
>  	  camera.
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 51ebbe7ae996950a58f8fee30029e0a060feaf3f..d1635f49ee047ca696f6053f6c17e30d736ab795 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -32,6 +32,8 @@
>  #include <media/v4l2-mediabus.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "aptina-pll.h"
> +
>  /* Sysctl registers */
>  #define MT9M114_CHIP_ID					CCI_REG16(0x0000)
>  #define MT9M114_COMMAND_REGISTER			CCI_REG16(0x0080)
> @@ -267,9 +269,9 @@
>  #define MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE			BIT(0)
>  #define MT9M114_CAM_SYSCTL_PLL_DISABLE_VALUE			0x00
>  #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N		CCI_REG16(0xc980)
> -#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(m, n)		(((n) << 8) | (m))
> +#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(m, n)		((((n) - 1) << 8) | (m))
>  #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P		CCI_REG16(0xc982)
> -#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(p)		((p) << 8)
> +#define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(p)		(((p) - 1) << 8)
>  #define MT9M114_CAM_PORT_OUTPUT_CONTROL			CCI_REG16(0xc984)
>  #define MT9M114_CAM_PORT_PORT_SELECT_PARALLEL			(0 << 0)
>  #define MT9M114_CAM_PORT_PORT_SELECT_MIPI			(1 << 0)
> @@ -330,7 +332,7 @@
>   * minimum values that have been seen in register lists are 303 and 38, use
>   * them.
>   *
> - * Set the default to achieve 1280x960 at 30fps.
> + * Set the default to achieve 1280x960 at 30fps with a 48 MHz pixclock.
>   */
>  #define MT9M114_MIN_HBLANK				303
>  #define MT9M114_MIN_VBLANK				38
> @@ -340,6 +342,8 @@
>  #define MT9M114_DEF_FRAME_RATE				30
>  #define MT9M114_MAX_FRAME_RATE				120
>  
> +#define MT9M114_DEF_PIXCLOCK				48000000
> +
>  #define MT9M114_PIXEL_ARRAY_WIDTH			1296U
>  #define MT9M114_PIXEL_ARRAY_HEIGHT			976U
>  
> @@ -384,11 +388,7 @@ struct mt9m114 {
>  	struct v4l2_fwnode_endpoint bus_cfg;
>  	bool bypass_pll;
>  
> -	struct {
> -		unsigned int m;
> -		unsigned int n;
> -		unsigned int p;
> -	} pll;
> +	struct aptina_pll pll;
>  
>  	unsigned int pixrate;
>  	bool streaming;
> @@ -758,7 +758,7 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>  							       sensor->pll.n),
>  			  &ret);
>  		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P,
> -			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p),
> +			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p1),
>  			  &ret);
>  	}
>  
> @@ -2283,12 +2283,25 @@ static int mt9m114_verify_link_frequency(struct mt9m114 *sensor,
>  
>  static int mt9m114_clk_init(struct mt9m114 *sensor)
>  {
> +	static const struct aptina_pll_limits limits = {
> +		.ext_clock_min = 6000000,
> +		.ext_clock_max = 54000000,
> +		/* int_clock_* limits are not documented taken from mt9p031.c */
> +		.int_clock_min = 2000000,
> +		.int_clock_max = 13500000,
> +		/* out_clock_min is not documented, taken from mt9p031.c */
> +		.out_clock_min = 180000000,
> +		.out_clock_max = 384000000,
> +		.pix_clock_max = 48000000,
> +		.n_min = 1,
> +		.n_max = 64,
> +		.m_min = 16,
> +		.m_max = 192,
> +		.p1_min = 8,
> +		.p1_max = 8,
> +	};
>  	unsigned int pixrate;
> -
> -	/* Hardcode the PLL multiplier and dividers to default settings. */
> -	sensor->pll.m = 32;
> -	sensor->pll.n = 1;
> -	sensor->pll.p = 7;
> +	int ret;
>  
>  	/*
>  	 * Calculate the pixel rate and link frequency. The CSI-2 bus is clocked
> @@ -2308,8 +2321,15 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
>  	}
>  
>  	/* Check if the PLL configuration fits the configured link frequency. */
> +	sensor->pll.ext_clock = clk_get_rate(sensor->clk);
> +	sensor->pll.pix_clock = MT9M114_DEF_PIXCLOCK;
> +
> +	ret = aptina_pll_calculate(&sensor->client->dev, &limits, &sensor->pll);
> +	if (ret)
> +		return ret;
> +
>  	pixrate = clk_get_rate(sensor->clk) * sensor->pll.m

You can replace this with

	pixrate = sensor->pll.ext_clock * sensor->pll.m

to avoid the double call to clk_get_rate(). With this and the comment
from 02/15 moved to this file (probably just above the limits in this
function),

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -		/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
> +		/ (sensor->pll.n * sensor->pll.p1);
>  	if (mt9m114_verify_link_frequency(sensor, pixrate) == 0) {
>  		sensor->pixrate = pixrate;
>  		sensor->bypass_pll = false;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 13/15] media: mt9m114: Drop start-, stop-streaming sequence from initialize

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Wed, Dec 24, 2025 at 01:31:22PM +0100, Hans de Goede wrote:
> Drop the start-, stop-streaming sequence from initialize.
> 
> When streaming is started with a runtime-suspended sensor,
> mt9m114_start_streaming() will runtime-resume the sensor which calls
> mt9m114_initialize() immediately followed by calling
> mt9m114_set_state(ENTER_CONFIG_CHANGE).
> 
> This results in the following state changes in quick succession:
> 
> mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING
> mt9m114_set_state(ENTER_SUSPEND)       -> transitions to SUSPENDED
> mt9m114_set_state(ENTER_CONFIG_CHANGE) -> transitions to STREAMING
> 
> these quick state changes confuses the CSI receiver on atomisp devices
> causing streaming to not work.
> 
> Drop the state changes from mt9m114_initialize() and move
> the mt9m114_initialize() call to mt9m114_start_streaming()
> so that only a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call
> is made when streaming is started with a runtime-suspend sensor.
> 
> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> ---
> Changes in v4:
> - Move the mt9m114_initialize() call to mt9m114_start_streaming()
>   and drop the config_change_pending flag
> ---
>  drivers/media/i2c/mt9m114.c | 33 +++++++--------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 447a5eb34a6137a8e87bd119401571b5592fc77d..41e98f719a23045293ee47d8980675510a142afa 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -789,14 +789,6 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_SUSPEND);
> -	if (ret < 0)
> -		return ret;
> -
>  	return 0;
>  }
>  
> @@ -967,6 +959,10 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>  	if (ret)
>  		return ret;
>  
> +	ret = mt9m114_initialize(sensor);
> +	if (ret)
> +		goto error;
> +
>  	ret = mt9m114_configure_ifp(sensor, ifp_state);
>  	if (ret)
>  		goto error;
> @@ -2318,19 +2314,8 @@ static int __maybe_unused mt9m114_runtime_resume(struct device *dev)
>  {
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
> -	int ret;
>  
> -	ret = mt9m114_power_on(sensor);
> -	if (ret)
> -		return ret;
> -
> -	ret = mt9m114_initialize(sensor);
> -	if (ret) {
> -		mt9m114_power_off(sensor);
> -		return ret;
> -	}
> -
> -	return 0;
> +	return mt9m114_power_on(sensor);
>  }
>  
>  static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
> @@ -2562,8 +2547,8 @@ static int mt9m114_probe(struct i2c_client *client)
>  	/*
>  	 * Identify the sensor. The driver supports runtime PM, but needs to
>  	 * work when runtime PM is disabled in the kernel. To that end, power
> -	 * the sensor on manually here, and initialize it after identification
> -	 * to reach the same state as if resumed through runtime PM.
> +	 * the sensor on manually here and identify it to reach the same state

s/and identify it //

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 * as if resumed through runtime PM.
>  	 */
>  	ret = mt9m114_power_on(sensor);
>  	if (ret < 0) {
> @@ -2575,10 +2560,6 @@ static int mt9m114_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		goto error_power_off;
>  
> -	ret = mt9m114_initialize(sensor);
> -	if (ret < 0)
> -		goto error_power_off;
> -
>  	/*
>  	 * Enable runtime PM with autosuspend. As the device has been powered
>  	 * manually, mark it as active, and increase the usage count without

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 02/15] media: aptina-pll: Add comment documenting the PLL chain

[Hans de Goede]

Hi Laurent,

On 28-Dec-25 13:50, Laurent Pinchart wrote:
> On Wed, Dec 24, 2025 at 01:31:11PM +0100, Hans de Goede wrote:
>> Add a code-comment documenting the PLL chain, this is a verbatim
>> copy of Laurent's ASCII-art PLL chain from the mailinglist.
>>
>> Link: https://lore.kernel.org/linux-media/20250629204655.GA2059@pendragon.ideasonboard.com/
>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
>> ---
>> Suggested-by: should really be Co-authored-by since I just copy and
>> pasted Laurent's comment from the list, but that requires Laurent's S-o-B.
>>
>> Laurent can you give your S-o-B for adding a Co-authored-by ?
> 
> If you insist,
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you.

> but it's such a small patch that it doesn't matter much to me.

Given your request to add this to mt9m114.c, I'll just squash this into
"[PATCH v4 03/15] media: mt9m114: Use aptina-PLL helper to get PLL values"

and not add a Co-authored-by.

> 
>> ---
>> Changes in v4:
>> - New patch in v4 of this series
>> ---
>>  drivers/media/i2c/aptina-pll.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/media/i2c/aptina-pll.c b/drivers/media/i2c/aptina-pll.c
>> index cd2ed4583c97ec87e516acfd249fdccf2f9efbb8..4a519ab587ba4cfb9945a1bb05e87a3b5e6d28c9 100644
>> --- a/drivers/media/i2c/aptina-pll.c
>> +++ b/drivers/media/i2c/aptina-pll.c
>> @@ -12,6 +12,16 @@
>>  
>>  #include "aptina-pll.h"
>>  
>> +/*
>> + * Based on the docs the PLL is believed to have the following setup:
>> + *
>> + *         +-----+     +-----+     +-----+     +-----+     +-----+
>> + * Fin --> | / N | --> | x M | --> | x 2 | --> | / P | --> | / 2 | -->
>> + *         +-----+     +-----+     +-----+     +-----+     +-----+
>> + *                                         fBit       fWord       fSensor
>> + * ext_clock    int_clock   out_clock                             pix_clock
>> + */
> 
> I think this belongs to mt9m114.c. The other sensor that uses
> aptina-pll, MT9P031, does not include the x2 multiplier or /2 divider,
> and has no concept of fBit as it has a parallel output only.
> 
> Could you please also capture that the datasheet has a constraint on
> fBit, which we translate to a constraint on out_clock for the PLL
> calculator by dividing it by 2 ?

Ack will do and as mentioned above, I'll squash this into:

[PATCH v4 03/15] media: mt9m114: Use aptina-PLL helper to get PLL values

for v5 then.

Regards,

Hans




> 
>> +
>>  int aptina_pll_calculate(struct device *dev,
>>  			 const struct aptina_pll_limits *limits,
>>  			 struct aptina_pll *pll)
>