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

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

[Hans de Goede]

Hi All,

Here is v3 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 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: mt9m114: Add support for clock-frequency property
  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 src format when src pixelfmt
    changes to/from RAW10
  media: mt9m114: Update src pad sel 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 |   2 +
 drivers/media/i2c/mt9m114.c    | 255 +++++++++++++++++++++++++--------
 3 files changed, 196 insertions(+), 62 deletions(-)

-- 
2.49.0

[PATCH v3 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 <hansg@kernel.org>
---
 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 b1f89bbf9d47..cd2ed4583c97 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.49.0

[PATCH v3 02/15] media: mt9m114: Add support for clock-frequency property

[Hans de Goede]

Add support for platforms that do not have a clock provider, but instead
specify the clock frequency by using the "clock-frequency" property.

E.g. ACPI platforms turn the clock on/off through ACPI power-resources
depending on the runtime-pm state, so there is no clock provider.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Note as discussed during review of v1, this needs to be moved over to
the solution from:

https://lore.kernel.org/r/20250321130329.342236-1-mehdi.djait@linux.intel.com

once that has landed upstream. I'll submit a follow-up patch to move to
that solution once it has landed upstream.
---
 drivers/media/i2c/mt9m114.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 3f540ca40f3c..5a7c45ce2169 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -388,6 +388,7 @@ struct mt9m114 {
 
 	unsigned int pixrate;
 	bool streaming;
+	u32 clk_freq;
 
 	/* Pixel Array */
 	struct {
@@ -2134,14 +2135,13 @@ static int mt9m114_power_on(struct mt9m114 *sensor)
 
 	/* Perform a hard reset if available, or a soft reset otherwise. */
 	if (sensor->reset) {
-		long freq = clk_get_rate(sensor->clk);
 		unsigned int duration;
 
 		/*
 		 * The minimum duration is 50 clock cycles, thus typically
 		 * around 2µs. Double it to be safe.
 		 */
-		duration = DIV_ROUND_UP(2 * 50 * 1000000, freq);
+		duration = DIV_ROUND_UP(2 * 50 * 1000000, sensor->clk_freq);
 
 		gpiod_set_value(sensor->reset, 1);
 		fsleep(duration);
@@ -2279,7 +2279,7 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
 	 * Check if EXTCLK fits the configured link frequency. Bypass the PLL
 	 * in this case.
 	 */
-	pixrate = clk_get_rate(sensor->clk) / 2;
+	pixrate = sensor->clk_freq / 2;
 	if (mt9m114_verify_link_frequency(sensor, pixrate) == 0) {
 		sensor->pixrate = pixrate;
 		sensor->bypass_pll = true;
@@ -2287,7 +2287,7 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
 	}
 
 	/* Check if the PLL configuration fits the configured link frequency. */
-	pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
+	pixrate = sensor->clk_freq * sensor->pll.m
 		/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
 	if (mt9m114_verify_link_frequency(sensor, pixrate) == 0) {
 		sensor->pixrate = pixrate;
@@ -2395,13 +2395,25 @@ static int mt9m114_probe(struct i2c_client *client)
 		return ret;
 
 	/* Acquire clocks, GPIOs and regulators. */
-	sensor->clk = devm_clk_get(dev, NULL);
+	sensor->clk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(sensor->clk)) {
 		ret = PTR_ERR(sensor->clk);
 		dev_err_probe(dev, ret, "Failed to get clock\n");
 		goto error_ep_free;
 	}
 
+	if (sensor->clk) {
+		sensor->clk_freq = clk_get_rate(sensor->clk);
+	} else {
+		ret = fwnode_property_read_u32(dev_fwnode(dev),
+					       "clock-frequency",
+					       &sensor->clk_freq);
+		if (ret) {
+			dev_err_probe(dev, ret, "Failed to read clock-freq\n");
+			goto error_ep_free;
+		}
+	}
+
 	sensor->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(sensor->reset)) {
 		ret = PTR_ERR(sensor->reset);
-- 
2.49.0

[PATCH v3 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 <hansg@kernel.org>
---
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 | 55 +++++++++++++++++++++++++++----------
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 0b58197471ae..75674bc40107 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -295,6 +295,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 5a7c45ce2169..9e703012cb0e 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -31,6 +31,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)
@@ -263,9 +265,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)
@@ -326,7 +328,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
@@ -336,6 +338,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
 
@@ -380,11 +384,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;
@@ -757,7 +757,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);
 	}
 
@@ -2262,12 +2262,30 @@ 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_max is documented as 768MHz, but this leads to
+		 * a non working setup. Use 400MHz instead which results in
+		 * the same PLL settings as used by the vendor's drivers.
+		 */
+		.out_clock_min = 180000000,
+		.out_clock_max = 400000000,
+		.pix_clock_max = 48000000,
+		.n_min = 1,
+		.n_max = 64,
+		.m_min = 16,
+		.m_max = 192,
+		.p1_min = 1,
+		.p1_max = 64,
+	};
 	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
@@ -2287,8 +2305,15 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
 	}
 
 	/* Check if the PLL configuration fits the configured link frequency. */
+	sensor->pll.ext_clock = sensor->clk_freq;
+	sensor->pll.pix_clock = MT9M114_DEF_PIXCLOCK;
+
+	ret = aptina_pll_calculate(&sensor->client->dev, &limits, &sensor->pll);
+	if (ret)
+		return ret;
+
 	pixrate = sensor->clk_freq * 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.49.0

[PATCH v3 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 <hansg@kernel.org>
---
 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 9e703012cb0e..25d2b3fd4293 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -325,13 +325,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.49.0

[PATCH v3 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 <hansg@kernel.org>
---
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 25d2b3fd4293..10ddcc102b8a 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -328,12 +328,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.49.0

[PATCH v3 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 <hansg@kernel.org>
---
 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 10ddcc102b8a..06f835b08f8e 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -333,8 +333,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.49.0

[PATCH v3 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 <hansg@kernel.org>
---
 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 06f835b08f8e..c10100d8fd4a 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2440,7 +2440,7 @@ static int mt9m114_probe(struct i2c_client *client)
 		}
 	}
 
-	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.49.0

[PATCH v3 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 <hansg@kernel.org>
---
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 c10100d8fd4a..d4aad77b095b 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2207,6 +2207,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, sensor->clk_freq);
+	fsleep(duration);
+
 	clk_disable_unprepare(sensor->clk);
 	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
 }
-- 
2.49.0

[PATCH v3 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.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v3:
- New patch in v3 of this patch-set
---
 drivers/media/i2c/mt9m114.c | 54 ++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index d4aad77b095b..020caae95a3d 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -843,6 +843,18 @@ static int mt9m114_configure_pa(struct mt9m114 *sensor,
 	return ret;
 }
 
+/*
+ * For src pad fmts 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)
 {
@@ -850,6 +862,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;
 
@@ -864,15 +877,18 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
 		return ret;
 
 	/*
+	 * For src pad fmts other then RAW10 adjust cropping coordinates for
 	 * 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.
 	 */
+	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,
@@ -1845,6 +1861,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. */
@@ -1859,15 +1876,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 src pad fmts 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:
@@ -1902,6 +1921,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)
@@ -1917,21 +1937,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 src pad fmts 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.49.0

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

[Laurent Pinchart]

Hi Hans,

On Sun, Jun 29, 2025 at 10:56:19PM +0200, Hans de Goede wrote:
> 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.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> Changes in v3:
> - New patch in v3 of this patch-set
> ---
>  drivers/media/i2c/mt9m114.c | 54 ++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index d4aad77b095b..020caae95a3d 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -843,6 +843,18 @@ static int mt9m114_configure_pa(struct mt9m114 *sensor,
>  	return ret;
>  }
>  
> +/*
> + * For src pad fmts other then RAW10 the IFP removes a 4 pixel border from its

"source pad formats"

> + * 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)
>  {
> @@ -850,6 +862,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;
>  
> @@ -864,15 +877,18 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
>  		return ret;
>  
>  	/*
> +	 * For src pad fmts other then RAW10 adjust cropping coordinates for

Ditto.

>  	 * 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.

But while at it this can be improved, as it took me a moment to remember
what was going on.

	/*
	 * 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,
> @@ -1845,6 +1861,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. */
> @@ -1859,15 +1876,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 src pad fmts other then RAW10 this gets reduced by 4

"For source pad formats".

> +		 * 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:
> @@ -1902,6 +1921,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)
> @@ -1917,21 +1937,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 src pad fmts other then RAW10

Same here.

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

> +		 * 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;
>  

-- 
Regards,

Laurent Pinchart

[PATCH v3 10/15] media: mt9m114: Adjust IFP selections and src format when src pixelfmt changes to/from RAW10

[Hans de Goede]

Changing the IFP src pad pixelfmt 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 src pad pixelfmt back from RAW10 to another pixelfmt
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 src pad pixelfmt back from RAW10 to another pixelfmt
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 src pixelfmt into account and
call this helper whenever the src pad pixelfmt changes to/from RAW10.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
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 020caae95a3d..ef27780384f2 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1820,6 +1820,40 @@ static int mt9m114_ifp_enum_frameintervals(struct v4l2_subdev *sd,
 	return 0;
 }
 
+/*
+ * Helper function to update IFP crop, compose rects and src 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)
@@ -1843,11 +1877,20 @@ 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 src format size.
+		 */
+		if (format->code != info->code &&
+		    (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.49.0

Re: [PATCH v3 10/15] media: mt9m114: Adjust IFP selections and src format when src pixelfmt changes to/from RAW10

[Laurent Pinchart]

On Sun, Jun 29, 2025 at 10:56:20PM +0200, Hans de Goede wrote:
> Changing the IFP src pad pixelfmt to RAW10 means disabling the scaler,

s/src/source/
s/pixelfmt/format/ or "media bus code", as this isn't a pixel format

Same below.

> which means that the crop and compose rectangles must be reset to
> match the sink format size with no border.
> 
> And when changing the src pad pixelfmt back from RAW10 to another pixelfmt
> 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 src pad pixelfmt back from RAW10 to another pixelfmt
> 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 src pixelfmt into account and
> call this helper whenever the src pad pixelfmt changes to/from RAW10.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> 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 020caae95a3d..ef27780384f2 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1820,6 +1820,40 @@ static int mt9m114_ifp_enum_frameintervals(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +/*
> + * Helper function to update IFP crop, compose rects and src format when

s/rects/rectangles/

> + * 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;

Please add a blank line here.

> +	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)
> @@ -1843,11 +1877,20 @@ 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 src format size.
> +		 */
> +		if (format->code != info->code &&
> +		    (format->code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> +		     info->code == MEDIA_BUS_FMT_SGRBG10_1X10)) {

This would be shorter:

		if ((format->code == MEDIA_BUS_FMT_SGRBG10_1X10) !=
		    (info->code == MEDIA_BUS_FMT_SGRBG10_1X10)) {

Whether or not it's more readable may be a matter of personal preference
though.

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

> +			format->code = info->code;
> +			mt9m114_ifp_update_sel_and_src_fmt(state);
> +		} else {
> +			format->code = info->code;
> +		}
>  	}
>  
>  	fmt->format = *format;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 10/15] media: mt9m114: Adjust IFP selections and src format when src pixelfmt changes to/from RAW10

[Hans de Goede]

Hi Laurent,

Thank you for all the reviews.

On 2-Jul-25 02:32, Laurent Pinchart wrote:
> On Sun, Jun 29, 2025 at 10:56:20PM +0200, Hans de Goede wrote:
>> Changing the IFP src pad pixelfmt to RAW10 means disabling the scaler,
> 
> s/src/source/
> s/pixelfmt/format/ or "media bus code", as this isn't a pixel format
> 
> Same below.
> 
>> which means that the crop and compose rectangles must be reset to
>> match the sink format size with no border.
>>
>> And when changing the src pad pixelfmt back from RAW10 to another pixelfmt
>> 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 src pad pixelfmt back from RAW10 to another pixelfmt
>> 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 src pixelfmt into account and
>> call this helper whenever the src pad pixelfmt changes to/from RAW10.
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>> 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 020caae95a3d..ef27780384f2 100644
>> --- a/drivers/media/i2c/mt9m114.c
>> +++ b/drivers/media/i2c/mt9m114.c
>> @@ -1820,6 +1820,40 @@ static int mt9m114_ifp_enum_frameintervals(struct v4l2_subdev *sd,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Helper function to update IFP crop, compose rects and src format when
> 
> s/rects/rectangles/
> 
>> + * 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;
> 
> Please add a blank line here.
> 
>> +	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)
>> @@ -1843,11 +1877,20 @@ 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 src format size.
>> +		 */
>> +		if (format->code != info->code &&
>> +		    (format->code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
>> +		     info->code == MEDIA_BUS_FMT_SGRBG10_1X10)) {
> 
> This would be shorter:
> 
> 		if ((format->code == MEDIA_BUS_FMT_SGRBG10_1X10) !=
> 		    (info->code == MEDIA_BUS_FMT_SGRBG10_1X10)) {
> 
> Whether or not it's more readable may be a matter of personal preference
> though.

I like your suggestion, so I'm going with this for v4.

I'll also address all your comments about commit msg wording,
comment wording and adding a blank line here and there both
in this patch as well as in all the other patches in the series
with similar comments.

Regards,

Hans

[PATCH v3 11/15] media: mt9m114: Update src pad sel 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 src
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.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/mt9m114.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index ef27780384f2..1f305bba180e 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1871,6 +1871,8 @@ 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.49.0

Re: [PATCH v3 11/15] media: mt9m114: Update src pad sel and format when sink pad format changes

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sun, Jun 29, 2025 at 10:56:21PM +0200, Hans de Goede wrote:
> 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 src

s/src/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.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/i2c/mt9m114.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index ef27780384f2..1f305bba180e 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1871,6 +1871,8 @@ 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);

Blank line here.

> +		/* Propagate changes downstream */

s/downstream/downstream./

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

> +		mt9m114_ifp_update_sel_and_src_fmt(state);
>  	} else {
>  		const struct mt9m114_format_info *info;
>  

-- 
Regards,

Laurent Pinchart

[PATCH v3 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.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
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 | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 1f305bba180e..1280d90cd411 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1963,7 +1963,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;
@@ -1976,6 +1976,13 @@ static int mt9m114_ifp_set_selection(struct v4l2_subdev *sd,
 	if (sel->pad != 0)
 		return -EINVAL;
 
+	/* 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 = *v4l2_subdev_state_get_crop(state, 0);
+		return 0;
+	}
+
 	format = v4l2_subdev_state_get_format(state, 0);
 	crop = v4l2_subdev_state_get_crop(state, 0);
 	compose = v4l2_subdev_state_get_compose(state, 0);
@@ -2022,9 +2029,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.49.0

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

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sun, Jun 29, 2025 at 10:56:22PM +0200, Hans de Goede wrote:
> 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.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> 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 | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 1f305bba180e..1280d90cd411 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1963,7 +1963,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;
> @@ -1976,6 +1976,13 @@ static int mt9m114_ifp_set_selection(struct v4l2_subdev *sd,
>  	if (sel->pad != 0)
>  		return -EINVAL;
>  
> +	/* 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 = *v4l2_subdev_state_get_crop(state, 0);

You can move the 

 	crop = v4l2_subdev_state_get_crop(state, 0);

line from below above the if statement, and use

		sel->r = *crop;

here.

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

> +		return 0;
> +	}
> +
>  	format = v4l2_subdev_state_get_format(state, 0);
>  	crop = v4l2_subdev_state_get_crop(state, 0);
>  	compose = v4l2_subdev_state_get_compose(state, 0);
> @@ -2022,9 +2029,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;
>  }

-- 
Regards,

Laurent Pinchart

[PATCH v3 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() so that only
a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call is made
when streaming is started with a runtime-suspend sensor.

This means that the sensor may have config changes pending when
mt9m114_runtime_suspend() gets called the first time after mt9m114_probe(),
when streaming was not started within the 1 second runtime-pm timeout.
Keep track of this and do the ENTER_CONFIG_CHANGE + ENTER suspend from
mt9m114_runtime_suspend() if necessary.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/mt9m114.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 1280d90cd411..ec5e9ce24d1c 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -389,6 +389,7 @@ struct mt9m114 {
 
 	unsigned int pixrate;
 	bool streaming;
+	bool config_change_pending;
 	u32 clk_freq;
 
 	/* Pixel Array */
@@ -781,14 +782,7 @@ 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;
-
+	sensor->config_change_pending = true;
 	return 0;
 }
 
@@ -987,6 +981,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
 	if (ret)
 		goto error;
 
+	sensor->config_change_pending = false;
 	sensor->streaming = true;
 
 	return 0;
@@ -2315,6 +2310,14 @@ static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
 
+	if (sensor->config_change_pending) {
+		/* mt9m114_set_state() prints errors itself, no need to check */
+		mt9m114_set_state(sensor,
+				  MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
+		mt9m114_set_state(sensor,
+				  MT9M114_SYS_STATE_ENTER_SUSPEND);
+	}
+
 	mt9m114_power_off(sensor);
 
 	return 0;
-- 
2.49.0

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

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sun, Jun 29, 2025 at 10:56:23PM +0200, 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() so that only
> a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call is made
> when streaming is started with a runtime-suspend sensor.
> 
> This means that the sensor may have config changes pending when
> mt9m114_runtime_suspend() gets called the first time after mt9m114_probe(),
> when streaming was not started within the 1 second runtime-pm timeout.
> Keep track of this and do the ENTER_CONFIG_CHANGE + ENTER suspend from
> mt9m114_runtime_suspend() if necessary.

Could you please update the commit message with the result of the
discussion on v2 ? I'd like to record that mt9m114_power_on() leaves the
sensor in the STANDBY state, either because it is already in standby for
CSI-2 mode, or with an explicit state change in parallel mode (with the
mode configured through the CONFIG pin). That's in my opinion the reason
we can drop the ENTER_CONFIG_CHANGE and ENTER_SUSPEND in
mt9m114_initialize().

> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/i2c/mt9m114.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 1280d90cd411..ec5e9ce24d1c 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -389,6 +389,7 @@ struct mt9m114 {
>  
>  	unsigned int pixrate;
>  	bool streaming;
> +	bool config_change_pending;
>  	u32 clk_freq;
>  
>  	/* Pixel Array */
> @@ -781,14 +782,7 @@ 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;
> -
> +	sensor->config_change_pending = true;
>  	return 0;
>  }
>  
> @@ -987,6 +981,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>  	if (ret)
>  		goto error;
>  
> +	sensor->config_change_pending = false;
>  	sensor->streaming = true;
>  
>  	return 0;
> @@ -2315,6 +2310,14 @@ static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>  
> +	if (sensor->config_change_pending) {
> +		/* mt9m114_set_state() prints errors itself, no need to check */
> +		mt9m114_set_state(sensor,
> +				  MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> +		mt9m114_set_state(sensor,
> +				  MT9M114_SYS_STATE_ENTER_SUSPEND);
> +	}

What's the point of this if we're powering the sensor right after ?

> +
>  	mt9m114_power_off(sensor);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

On 2-Jul-25 03:08, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sun, Jun 29, 2025 at 10:56:23PM +0200, 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() so that only
>> a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call is made
>> when streaming is started with a runtime-suspend sensor.
>>
>> This means that the sensor may have config changes pending when
>> mt9m114_runtime_suspend() gets called the first time after mt9m114_probe(),
>> when streaming was not started within the 1 second runtime-pm timeout.
>> Keep track of this and do the ENTER_CONFIG_CHANGE + ENTER suspend from
>> mt9m114_runtime_suspend() if necessary.
> 
> Could you please update the commit message with the result of the
> discussion on v2 ? I'd like to record that mt9m114_power_on() leaves the
> sensor in the STANDBY state, either because it is already in standby for
> CSI-2 mode, or with an explicit state change in parallel mode (with the
> mode configured through the CONFIG pin). That's in my opinion the reason
> we can drop the ENTER_CONFIG_CHANGE and ENTER_SUSPEND in
> mt9m114_initialize().

Ack, I'll add a comment to the code about this and update the commit
message as requested.


> 
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>>  drivers/media/i2c/mt9m114.c | 19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
>> index 1280d90cd411..ec5e9ce24d1c 100644
>> --- a/drivers/media/i2c/mt9m114.c
>> +++ b/drivers/media/i2c/mt9m114.c
>> @@ -389,6 +389,7 @@ struct mt9m114 {
>>  
>>  	unsigned int pixrate;
>>  	bool streaming;
>> +	bool config_change_pending;
>>  	u32 clk_freq;
>>  
>>  	/* Pixel Array */
>> @@ -781,14 +782,7 @@ 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;
>> -
>> +	sensor->config_change_pending = true;
>>  	return 0;
>>  }
>>  
>> @@ -987,6 +981,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>>  	if (ret)
>>  		goto error;
>>  
>> +	sensor->config_change_pending = false;
>>  	sensor->streaming = true;
>>  
>>  	return 0;
>> @@ -2315,6 +2310,14 @@ static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
>>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>>  
>> +	if (sensor->config_change_pending) {
>> +		/* mt9m114_set_state() prints errors itself, no need to check */
>> +		mt9m114_set_state(sensor,
>> +				  MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
>> +		mt9m114_set_state(sensor,
>> +				  MT9M114_SYS_STATE_ENTER_SUSPEND);
>> +	}
> 
> What's the point of this if we're powering the sensor right after ?

The config-change state command is necessary to allow
the suspend command to succeed.

Before this initialize() would do the suspend for
the probe() path and mt9m114_stop_streaming()
does this after a stop-stream.

The mean reason is to preserve the old behavior of
always putting the sensor in suspend, which could
be useful if the regulators are shared (or just always
on) and the reset pin is not connected.

Regards,

Hans

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

[Laurent Pinchart]

On Tue, Dec 23, 2025 at 02:37:39PM +0100, Hans de Goede wrote:
> On 2-Jul-25 03:08, Laurent Pinchart wrote:
> > On Sun, Jun 29, 2025 at 10:56:23PM +0200, 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() so that only
> >> a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call is made
> >> when streaming is started with a runtime-suspend sensor.
> >>
> >> This means that the sensor may have config changes pending when
> >> mt9m114_runtime_suspend() gets called the first time after mt9m114_probe(),
> >> when streaming was not started within the 1 second runtime-pm timeout.
> >> Keep track of this and do the ENTER_CONFIG_CHANGE + ENTER suspend from
> >> mt9m114_runtime_suspend() if necessary.
> > 
> > Could you please update the commit message with the result of the
> > discussion on v2 ? I'd like to record that mt9m114_power_on() leaves the
> > sensor in the STANDBY state, either because it is already in standby for
> > CSI-2 mode, or with an explicit state change in parallel mode (with the
> > mode configured through the CONFIG pin). That's in my opinion the reason
> > we can drop the ENTER_CONFIG_CHANGE and ENTER_SUSPEND in
> > mt9m114_initialize().
> 
> Ack, I'll add a comment to the code about this and update the commit
> message as requested.
> 
> >> Signed-off-by: Hans de Goede <hansg@kernel.org>
> >> ---
> >>  drivers/media/i2c/mt9m114.c | 19 +++++++++++--------
> >>  1 file changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> >> index 1280d90cd411..ec5e9ce24d1c 100644
> >> --- a/drivers/media/i2c/mt9m114.c
> >> +++ b/drivers/media/i2c/mt9m114.c
> >> @@ -389,6 +389,7 @@ struct mt9m114 {
> >>  
> >>  	unsigned int pixrate;
> >>  	bool streaming;
> >> +	bool config_change_pending;
> >>  	u32 clk_freq;
> >>  
> >>  	/* Pixel Array */
> >> @@ -781,14 +782,7 @@ 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;
> >> -
> >> +	sensor->config_change_pending = true;
> >>  	return 0;
> >>  }
> >>  
> >> @@ -987,6 +981,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
> >>  	if (ret)
> >>  		goto error;
> >>  
> >> +	sensor->config_change_pending = false;
> >>  	sensor->streaming = true;
> >>  
> >>  	return 0;
> >> @@ -2315,6 +2310,14 @@ static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
> >>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
> >>  
> >> +	if (sensor->config_change_pending) {
> >> +		/* mt9m114_set_state() prints errors itself, no need to check */
> >> +		mt9m114_set_state(sensor,
> >> +				  MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> >> +		mt9m114_set_state(sensor,
> >> +				  MT9M114_SYS_STATE_ENTER_SUSPEND);
> >> +	}
> > 
> > What's the point of this if we're powering the sensor right after ?

(I meant "powering the sensor down")

> The config-change state command is necessary to allow
> the suspend command to succeed.
> 
> Before this initialize() would do the suspend for
> the probe() path and mt9m114_stop_streaming()
> does this after a stop-stream.
> 
> The mean reason is to preserve the old behavior of
> always putting the sensor in suspend, which could
> be useful if the regulators are shared (or just always
> on) and the reset pin is not connected.

True, but that's done in mt9m114_stop_streaming(), isn't it ?

There's still something that bothers me about this patch. The
config_change_pending flag makes the logic hard to follow, it should be
simplified somehow.

Re-reading the code, could we

- drop the mt9m114_initialize() from mt9m114_probe()
- move the mt9m114_initialize() from mt9m114_runtime_resume() to
  mt9m114_start_streaming()
- drop the ENTER_CONFIG_CHANGE and ENTER_SUSPEND from
  mt9m114_initialize()

Am I missing something ?

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi,

On 23-Dec-25 5:50 PM, Laurent Pinchart wrote:
> On Tue, Dec 23, 2025 at 02:37:39PM +0100, Hans de Goede wrote:
>> On 2-Jul-25 03:08, Laurent Pinchart wrote:
>>> On Sun, Jun 29, 2025 at 10:56:23PM +0200, 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() so that only
>>>> a single mt9m114_set_state(ENTER_CONFIG_CHANGE) call is made
>>>> when streaming is started with a runtime-suspend sensor.
>>>>
>>>> This means that the sensor may have config changes pending when
>>>> mt9m114_runtime_suspend() gets called the first time after mt9m114_probe(),
>>>> when streaming was not started within the 1 second runtime-pm timeout.
>>>> Keep track of this and do the ENTER_CONFIG_CHANGE + ENTER suspend from
>>>> mt9m114_runtime_suspend() if necessary.
>>>
>>> Could you please update the commit message with the result of the
>>> discussion on v2 ? I'd like to record that mt9m114_power_on() leaves the
>>> sensor in the STANDBY state, either because it is already in standby for
>>> CSI-2 mode, or with an explicit state change in parallel mode (with the
>>> mode configured through the CONFIG pin). That's in my opinion the reason
>>> we can drop the ENTER_CONFIG_CHANGE and ENTER_SUSPEND in
>>> mt9m114_initialize().
>>
>> Ack, I'll add a comment to the code about this and update the commit
>> message as requested.
>>
>>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>>>> ---
>>>>  drivers/media/i2c/mt9m114.c | 19 +++++++++++--------
>>>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
>>>> index 1280d90cd411..ec5e9ce24d1c 100644
>>>> --- a/drivers/media/i2c/mt9m114.c
>>>> +++ b/drivers/media/i2c/mt9m114.c
>>>> @@ -389,6 +389,7 @@ struct mt9m114 {
>>>>  
>>>>  	unsigned int pixrate;
>>>>  	bool streaming;
>>>> +	bool config_change_pending;
>>>>  	u32 clk_freq;
>>>>  
>>>>  	/* Pixel Array */
>>>> @@ -781,14 +782,7 @@ 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;
>>>> -
>>>> +	sensor->config_change_pending = true;
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -987,6 +981,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>>>>  	if (ret)
>>>>  		goto error;
>>>>  
>>>> +	sensor->config_change_pending = false;
>>>>  	sensor->streaming = true;
>>>>  
>>>>  	return 0;
>>>> @@ -2315,6 +2310,14 @@ static int __maybe_unused mt9m114_runtime_suspend(struct device *dev)
>>>>  	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>>>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>>>>  
>>>> +	if (sensor->config_change_pending) {
>>>> +		/* mt9m114_set_state() prints errors itself, no need to check */
>>>> +		mt9m114_set_state(sensor,
>>>> +				  MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
>>>> +		mt9m114_set_state(sensor,
>>>> +				  MT9M114_SYS_STATE_ENTER_SUSPEND);
>>>> +	}
>>>
>>> What's the point of this if we're powering the sensor right after ?
> 
> (I meant "powering the sensor down")
> 
>> The config-change state command is necessary to allow
>> the suspend command to succeed.
>>
>> Before this initialize() would do the suspend for
>> the probe() path and mt9m114_stop_streaming()
>> does this after a stop-stream.
>>
>> The mean reason is to preserve the old behavior of
>> always putting the sensor in suspend, which could
>> be useful if the regulators are shared (or just always
>> on) and the reset pin is not connected.
> 
> True, but that's done in mt9m114_stop_streaming(), isn't it ?
> 
> There's still something that bothers me about this patch. The
> config_change_pending flag makes the logic hard to follow, it should be
> simplified somehow.
> 
> Re-reading the code, could we
> 
> - drop the mt9m114_initialize() from mt9m114_probe()
> - move the mt9m114_initialize() from mt9m114_runtime_resume() to
>   mt9m114_start_streaming()
> - drop the ENTER_CONFIG_CHANGE and ENTER_SUSPEND from
>   mt9m114_initialize()
> 
> Am I missing something ?

Nope that should work, but at the cost of re-doing
the mt9m114_initialize() when doing stop-stream,
change some things, start-stream. Not that I expect
that to happen often with this sensor, but this is why
I kept the initialize in runtime-resume.

Regards,

Hans



> 

[PATCH v3 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().

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/i2c/mt9m114.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index ec5e9ce24d1c..5e759a23e6cc 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2448,11 +2448,14 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
 	struct fwnode_handle *ep;
 	int ret;
 
+	/*
+	 * Sometimes the fwnode graph is initialized by the bridge driver,
+	 * wait for this.
+	 */
 	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.49.0

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

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sun, Jun 29, 2025 at 10:56:24PM +0200, Hans de Goede wrote:
> With IPU# bridges, endpoints may only be created when the IPU bridge is
> initialized. This may happen after the sensor driver's first probe().
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/i2c/mt9m114.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index ec5e9ce24d1c..5e759a23e6cc 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -2448,11 +2448,14 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
>  	struct fwnode_handle *ep;
>  	int ret;
>  
> +	/*
> +	 * Sometimes the fwnode graph is initialized by the bridge driver,
> +	 * wait for this.
> +	 */

I'm still not thrilled by this, but there's no real alternative for the
time being. Still, as Sakari mentioned, the IPU bridge code should at
some point be moved to the ACPI framework, so let's record here that the
sensor driver should then be updated:

	/*
	 * 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.
	 */

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

I wouldn't like to see this being replicated in lots of sensor drivers
though.

>  	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);

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

On 2-Jul-25 02:53, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sun, Jun 29, 2025 at 10:56:24PM +0200, Hans de Goede wrote:
>> With IPU# bridges, endpoints may only be created when the IPU bridge is
>> initialized. This may happen after the sensor driver's first probe().
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>>  drivers/media/i2c/mt9m114.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
>> index ec5e9ce24d1c..5e759a23e6cc 100644
>> --- a/drivers/media/i2c/mt9m114.c
>> +++ b/drivers/media/i2c/mt9m114.c
>> @@ -2448,11 +2448,14 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
>>  	struct fwnode_handle *ep;
>>  	int ret;
>>  
>> +	/*
>> +	 * Sometimes the fwnode graph is initialized by the bridge driver,
>> +	 * wait for this.
>> +	 */
> 
> I'm still not thrilled by this, but there's no real alternative for the
> time being. Still, as Sakari mentioned, the IPU bridge code should at
> some point be moved to the ACPI framework, so let's record here that the
> sensor driver should then be updated:
> 
> 	/*
> 	 * 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.
> 	 */
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> I wouldn't like to see this being replicated in lots of sensor drivers
> though.

I understand. With the new __devm_v4l2_sensor_clk_get() helper we could
do something like this:

From f7b13937af5f706bce3eed87c67a8f484f049c6a Mon Sep 17 00:00:00 2001
From: Hans de Goede <johannes.goede@oss.qualcomm.com>
Date: Wed, 24 Dec 2025 13:04:09 +0100
Subject: [PATCH] media: v4l2-common: sensor_clk_get(): Wait for endpoint
 fwnode to show up

On ACPI systems the fwnode graph can be initialized by a bridge driver,
which may not have probed yet.

Currently all sensor drivers which are used on ACPI platforms need to wait
for this themselves.

Add a check for the endpoint fwnode being present to
the devm_v4l2_sensor_clk_get() helper, this allows sensor drivers to drop
the check for this themselves as long as they call the helper before doing
any endpoint fwnode parsing.

Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
---
 drivers/media/v4l2-core/v4l2-common.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 554c591e1113..b68b5567a508 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -743,11 +743,27 @@ struct clk *__devm_v4l2_sensor_clk_get(struct device *dev, const char *id,
 {
 	bool of_node = is_of_node(dev_fwnode(dev));
 	const char *clk_id __free(kfree) = NULL;
+	struct fwnode_handle *ep;
 	struct clk_hw *clk_hw;
 	struct clk *clk;
 	u32 rate = clk_rate;
 	int ret = 0;
 
+	/*
+	 * On ACPI systems the fwnode graph can be initialized by a bridge
+	 * driver, which may not have probed yet. The bridge driver also sets
+	 * the clock-frequency property which is used below. 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)
+		return dev_err_probe(dev, -EPROBE_DEFER,
+				     "waiting for fwnode graph endpoint\n");
+
+	fwnode_handle_put(ep);
+
 	clk = devm_clk_get_optional(dev, id);
 	if (IS_ERR(clk))
 		return clk;
-- 
2.52.0

and then in e.g. the mt9m114.c code move the devm_v4l2_sensor_clk_get()
call to above mt9m114_parse_dt() and then the handling for this can be
dropped from the mt9m114.c code (and the same for other sensor drivers
with a similar check).

The downside of this is that it makes having an endpoint fwnode available
mandatory for all sensor drivers which use the devm_v4l2_sensor_clk_get()
helper, but AFAICT having an endpoint fwnode is mandatory for all sensor
drivers anyways, so this should not be an issue ?

Regards,

Hans

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

[Laurent Pinchart]

On Wed, Dec 24, 2025 at 01:12:28PM +0100, Hans de Goede wrote:
> On 2-Jul-25 02:53, Laurent Pinchart wrote:
> > On Sun, Jun 29, 2025 at 10:56:24PM +0200, Hans de Goede wrote:
> >> With IPU# bridges, endpoints may only be created when the IPU bridge is
> >> initialized. This may happen after the sensor driver's first probe().
> >>
> >> Signed-off-by: Hans de Goede <hansg@kernel.org>
> >> ---
> >>  drivers/media/i2c/mt9m114.c | 11 +++++++----
> >>  1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> >> index ec5e9ce24d1c..5e759a23e6cc 100644
> >> --- a/drivers/media/i2c/mt9m114.c
> >> +++ b/drivers/media/i2c/mt9m114.c
> >> @@ -2448,11 +2448,14 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
> >>  	struct fwnode_handle *ep;
> >>  	int ret;
> >>  
> >> +	/*
> >> +	 * Sometimes the fwnode graph is initialized by the bridge driver,
> >> +	 * wait for this.
> >> +	 */
> > 
> > I'm still not thrilled by this, but there's no real alternative for the
> > time being. Still, as Sakari mentioned, the IPU bridge code should at
> > some point be moved to the ACPI framework, so let's record here that the
> > sensor driver should then be updated:
> > 
> > 	/*
> > 	 * 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.
> > 	 */
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > I wouldn't like to see this being replicated in lots of sensor drivers
> > though.
> 
> I understand. With the new __devm_v4l2_sensor_clk_get() helper we could
> do something like this:
> 
> From f7b13937af5f706bce3eed87c67a8f484f049c6a Mon Sep 17 00:00:00 2001
> From: Hans de Goede <johannes.goede@oss.qualcomm.com>
> Date: Wed, 24 Dec 2025 13:04:09 +0100
> Subject: [PATCH] media: v4l2-common: sensor_clk_get(): Wait for endpoint
>  fwnode to show up
> 
> On ACPI systems the fwnode graph can be initialized by a bridge driver,
> which may not have probed yet.
> 
> Currently all sensor drivers which are used on ACPI platforms need to wait
> for this themselves.
> 
> Add a check for the endpoint fwnode being present to
> the devm_v4l2_sensor_clk_get() helper, this allows sensor drivers to drop
> the check for this themselves as long as they call the helper before doing
> any endpoint fwnode parsing.
> 
> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 554c591e1113..b68b5567a508 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -743,11 +743,27 @@ struct clk *__devm_v4l2_sensor_clk_get(struct device *dev, const char *id,
>  {
>  	bool of_node = is_of_node(dev_fwnode(dev));
>  	const char *clk_id __free(kfree) = NULL;
> +	struct fwnode_handle *ep;
>  	struct clk_hw *clk_hw;
>  	struct clk *clk;
>  	u32 rate = clk_rate;
>  	int ret = 0;
>  
> +	/*
> +	 * On ACPI systems the fwnode graph can be initialized by a bridge
> +	 * driver, which may not have probed yet. The bridge driver also sets
> +	 * the clock-frequency property which is used below. 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)
> +		return dev_err_probe(dev, -EPROBE_DEFER,
> +				     "waiting for fwnode graph endpoint\n");
> +
> +	fwnode_handle_put(ep);
> +
>  	clk = devm_clk_get_optional(dev, id);
>  	if (IS_ERR(clk))
>  		return clk;
> and then in e.g. the mt9m114.c code move the devm_v4l2_sensor_clk_get()
> call to above mt9m114_parse_dt() and then the handling for this can be
> dropped from the mt9m114.c code (and the same for other sensor drivers
> with a similar check).
> 
> The downside of this is that it makes having an endpoint fwnode available
> mandatory for all sensor drivers which use the devm_v4l2_sensor_clk_get()
> helper, but AFAICT having an endpoint fwnode is mandatory for all sensor
> drivers anyways, so this should not be an issue ?

Yes I think that would work, and would avoid copying the hack in
multiple drivers. Sakari, is this OK with you ?

-- 
Regards,

Laurent Pinchart

[PATCH v3 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 <hansg@kernel.org>
---
 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 5e759a23e6cc..710c9a870458 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2643,11 +2643,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.49.0

Re: [v3,00/15] media: mt9m114: Changes to make it work with atomisp devices

[Hans de Goede]

Hi All,
On 29-Jun-25 11:28 PM, Patchwork Integration wrote:
> Dear Hans de Goede:
> 
> Thanks for your patches! Unfortunately the Media CI robot has not been
> able to test them.
> 
> Make sure that the whole series 20250629205626.68341-1-hansg@kernel.org is
> available at lore. And that it can be cherry-picked on top the "next"
> branch of "https://gitlab.freedesktop.org/linux-media/media-committers.git".
> 
> You can try something like this:
>     git fetch https://gitlab.freedesktop.org/linux-media/media-committers.git next
>     git checkout FETCH_HEAD
>     b4 shazam 20250629205626.68341-1-hansg@kernel.org

My bad, this is based on top of:

https://git.linuxtv.org/sailus/media_tree.git/log/?h=fixes

since that has some pending mt9m114 patches. I should have
added a base-commit to the cover-letter.

Regards,

Hans















> Error message:
> Trying branch next c0b1da281d84d33281fc49289f0c7f8aada450ff...
> Running in OFFLINE mode
> Analyzing 16 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
>   [PATCH v3 1/15] media: aptina-pll: Debug log p1 min and max values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-2-hansg@kernel.org
>   [PATCH v3 2/15] media: mt9m114: Add support for clock-frequency property
>     + Link: https://lore.kernel.org/r/20250629205626.68341-3-hansg@kernel.org
>   [PATCH v3 3/15] media: mt9m114: Use aptina-PLL helper to get PLL values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-4-hansg@kernel.org
>   [PATCH v3 4/15] media: mt9m114: Lower minimum vblank value
>     + Link: https://lore.kernel.org/r/20250629205626.68341-5-hansg@kernel.org
>   [PATCH v3 5/15] media: mt9m114: Fix default hblank and vblank values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-6-hansg@kernel.org
>   [PATCH v3 6/15] media: mt9m114: Tweak default hblank and vblank for more accurate fps
>     + Link: https://lore.kernel.org/r/20250629205626.68341-7-hansg@kernel.org
>   [PATCH v3 7/15] media: mt9m114: Avoid a reset low spike during probe()
>     + Link: https://lore.kernel.org/r/20250629205626.68341-8-hansg@kernel.org
>   [PATCH v3 8/15] media: mt9m114: Put sensor in reset on power down
>     + Link: https://lore.kernel.org/r/20250629205626.68341-9-hansg@kernel.org
>   [PATCH v3 9/15] media: mt9m114: Add and use mt9m114_ifp_get_border() helper function
>     + Link: https://lore.kernel.org/r/20250629205626.68341-10-hansg@kernel.org
>   [PATCH v3 10/15] media: mt9m114: Adjust IFP selections and src format when src pixelfmt changes to/from RAW10
>     + Link: https://lore.kernel.org/r/20250629205626.68341-11-hansg@kernel.org
>   [PATCH v3 11/15] media: mt9m114: Update src pad sel and format when sink pad format changes
>     + Link: https://lore.kernel.org/r/20250629205626.68341-12-hansg@kernel.org
>   [PATCH v3 12/15] media: mt9m114: Don't allow changing the IFP crop/compose selections when bypassing the scaler
>     + Link: https://lore.kernel.org/r/20250629205626.68341-13-hansg@kernel.org
>   [PATCH v3 13/15] media: mt9m114: Drop start-, stop-streaming sequence from initialize
>     + Link: https://lore.kernel.org/r/20250629205626.68341-14-hansg@kernel.org
>   [PATCH v3 14/15] media: mt9m114: Return -EPROBE_DEFER if no endpoint is found
>     + Link: https://lore.kernel.org/r/20250629205626.68341-15-hansg@kernel.org
>   [PATCH v3 15/15] media: mt9m114: Add ACPI enumeration support
>     + Link: https://lore.kernel.org/r/20250629205626.68341-16-hansg@kernel.org
> ---
> Total patches: 15
> ---
> Applying: media: aptina-pll: Debug log p1 min and max values
> Applying: media: mt9m114: Add support for clock-frequency property
> Patch failed at 0002 media: mt9m114: Add support for clock-frequency property
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> error: patch failed: drivers/media/i2c/mt9m114.c:2279
> error: drivers/media/i2c/mt9m114.c: patch does not apply
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> 
> Trying branch fixes 19272b37aa4f83ca52bdf9c16d5d81bdd1354494...
> Running in OFFLINE mode
> Analyzing 16 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
>   [PATCH v3 1/15] media: aptina-pll: Debug log p1 min and max values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-2-hansg@kernel.org
>   [PATCH v3 2/15] media: mt9m114: Add support for clock-frequency property
>     + Link: https://lore.kernel.org/r/20250629205626.68341-3-hansg@kernel.org
>   [PATCH v3 3/15] media: mt9m114: Use aptina-PLL helper to get PLL values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-4-hansg@kernel.org
>   [PATCH v3 4/15] media: mt9m114: Lower minimum vblank value
>     + Link: https://lore.kernel.org/r/20250629205626.68341-5-hansg@kernel.org
>   [PATCH v3 5/15] media: mt9m114: Fix default hblank and vblank values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-6-hansg@kernel.org
>   [PATCH v3 6/15] media: mt9m114: Tweak default hblank and vblank for more accurate fps
>     + Link: https://lore.kernel.org/r/20250629205626.68341-7-hansg@kernel.org
>   [PATCH v3 7/15] media: mt9m114: Avoid a reset low spike during probe()
>     + Link: https://lore.kernel.org/r/20250629205626.68341-8-hansg@kernel.org
>   [PATCH v3 8/15] media: mt9m114: Put sensor in reset on power down
>     + Link: https://lore.kernel.org/r/20250629205626.68341-9-hansg@kernel.org
>   [PATCH v3 9/15] media: mt9m114: Add and use mt9m114_ifp_get_border() helper function
>     + Link: https://lore.kernel.org/r/20250629205626.68341-10-hansg@kernel.org
>   [PATCH v3 10/15] media: mt9m114: Adjust IFP selections and src format when src pixelfmt changes to/from RAW10
>     + Link: https://lore.kernel.org/r/20250629205626.68341-11-hansg@kernel.org
>   [PATCH v3 11/15] media: mt9m114: Update src pad sel and format when sink pad format changes
>     + Link: https://lore.kernel.org/r/20250629205626.68341-12-hansg@kernel.org
>   [PATCH v3 12/15] media: mt9m114: Don't allow changing the IFP crop/compose selections when bypassing the scaler
>     + Link: https://lore.kernel.org/r/20250629205626.68341-13-hansg@kernel.org
>   [PATCH v3 13/15] media: mt9m114: Drop start-, stop-streaming sequence from initialize
>     + Link: https://lore.kernel.org/r/20250629205626.68341-14-hansg@kernel.org
>   [PATCH v3 14/15] media: mt9m114: Return -EPROBE_DEFER if no endpoint is found
>     + Link: https://lore.kernel.org/r/20250629205626.68341-15-hansg@kernel.org
>   [PATCH v3 15/15] media: mt9m114: Add ACPI enumeration support
>     + Link: https://lore.kernel.org/r/20250629205626.68341-16-hansg@kernel.org
> ---
> Total patches: 15
> ---
> Applying: media: aptina-pll: Debug log p1 min and max values
> Applying: media: mt9m114: Add support for clock-frequency property
> Patch failed at 0002 media: mt9m114: Add support for clock-frequency property
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> error: patch failed: drivers/media/i2c/mt9m114.c:2279
> error: drivers/media/i2c/mt9m114.c: patch does not apply
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> 
> Trying branch None 27b0a9c2a67d483a9d4a941882b779a199ff281e...
> Running in OFFLINE mode
> Analyzing 16 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
>   [PATCH v3 1/15] media: aptina-pll: Debug log p1 min and max values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-2-hansg@kernel.org
>   [PATCH v3 2/15] media: mt9m114: Add support for clock-frequency property
>     + Link: https://lore.kernel.org/r/20250629205626.68341-3-hansg@kernel.org
>   [PATCH v3 3/15] media: mt9m114: Use aptina-PLL helper to get PLL values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-4-hansg@kernel.org
>   [PATCH v3 4/15] media: mt9m114: Lower minimum vblank value
>     + Link: https://lore.kernel.org/r/20250629205626.68341-5-hansg@kernel.org
>   [PATCH v3 5/15] media: mt9m114: Fix default hblank and vblank values
>     + Link: https://lore.kernel.org/r/20250629205626.68341-6-hansg@kernel.org
>   [PATCH v3 6/15] media: mt9m114: Tweak default hblank and vblank for more accurate fps
>     + Link: https://lore.kernel.org/r/20250629205626.68341-7-hansg@kernel.org
>   [PATCH v3 7/15] media: mt9m114: Avoid a reset low spike during probe()
>     + Link: https://lore.kernel.org/r/20250629205626.68341-8-hansg@kernel.org
>   [PATCH v3 8/15] media: mt9m114: Put sensor in reset on power down
>     + Link: https://lore.kernel.org/r/20250629205626.68341-9-hansg@kernel.org
>   [PATCH v3 9/15] media: mt9m114: Add and use mt9m114_ifp_get_border() helper function
>     + Link: https://lore.kernel.org/r/20250629205626.68341-10-hansg@kernel.org
>   [PATCH v3 10/15] media: mt9m114: Adjust IFP selections and src format when src pixelfmt changes to/from RAW10
>     + Link: https://lore.kernel.org/r/20250629205626.68341-11-hansg@kernel.org
>   [PATCH v3 11/15] media: mt9m114: Update src pad sel and format when sink pad format changes
>     + Link: https://lore.kernel.org/r/20250629205626.68341-12-hansg@kernel.org
>   [PATCH v3 12/15] media: mt9m114: Don't allow changing the IFP crop/compose selections when bypassing the scaler
>     + Link: https://lore.kernel.org/r/20250629205626.68341-13-hansg@kernel.org
>   [PATCH v3 13/15] media: mt9m114: Drop start-, stop-streaming sequence from initialize
>     + Link: https://lore.kernel.org/r/20250629205626.68341-14-hansg@kernel.org
>   [PATCH v3 14/15] media: mt9m114: Return -EPROBE_DEFER if no endpoint is found
>     + Link: https://lore.kernel.org/r/20250629205626.68341-15-hansg@kernel.org
>   [PATCH v3 15/15] media: mt9m114: Add ACPI enumeration support
>     + Link: https://lore.kernel.org/r/20250629205626.68341-16-hansg@kernel.org
> ---
> Total patches: 15
> ---
> Applying: media: aptina-pll: Debug log p1 min and max values
> Applying: media: mt9m114: Add support for clock-frequency property
> Patch failed at 0002 media: mt9m114: Add support for clock-frequency property
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> error: drivers/media/i2c/mt9m114.c: does not exist in index
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> 
> 
> 
> Best regards, and Happy Hacking!
> Media CI robot on behalf of the linux-media community.
> 
> ---
> Check the latest rules for contributing your patches at:
> https://docs.kernel.org/driver-api/media/maintainer-entry-profile.html
> 
> If you believe that the CI is wrong, kindly open an issue at
> https://gitlab.freedesktop.org/linux-media/media-ci/-/issues or reply-all
> to this message.
> 

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

[Laurent Pinchart]

Hi Hans,

On Sun, Jun 29, 2025 at 10:56:10PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is v3 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 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

It appears that some of my review comments on v2 crossed your v3. That's
the case for 03/12 and 10/12. For 03/12 in particular, I think the
comments will result in changes for v4. How would you like to proceed,
should I review the rest first ?

> Hans de Goede (15):
>   media: aptina-pll: Debug log p1 min and max values
>   media: mt9m114: Add support for clock-frequency property
>   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 src format when src pixelfmt
>     changes to/from RAW10
>   media: mt9m114: Update src pad sel 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 |   2 +
>  drivers/media/i2c/mt9m114.c    | 255 +++++++++++++++++++++++++--------
>  3 files changed, 196 insertions(+), 62 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

On 1-Jul-25 12:28 AM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Sun, Jun 29, 2025 at 10:56:10PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Here is v3 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 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
> 
> It appears that some of my review comments on v2 crossed your v3. That's
> the case for 03/12 and 10/12. For 03/12 in particular, I think the
> comments will result in changes for v4.

Yes I realized that myself for 03/12 too. I think that for
10/12 we agreed to keep that as is, but I may have misunderstood ?

> How would you like to proceed,
> should I review the rest first ?

With the exception of 03/12 yes please then I can add
any necessary changes to v4.

For 03/12 I'll reply to your last answer in the v2 thread
when I can make some time for this.

Regards,

Hans



> 
>> Hans de Goede (15):
>>   media: aptina-pll: Debug log p1 min and max values
>>   media: mt9m114: Add support for clock-frequency property
>>   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 src format when src pixelfmt
>>     changes to/from RAW10
>>   media: mt9m114: Update src pad sel 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 |   2 +
>>  drivers/media/i2c/mt9m114.c    | 255 +++++++++++++++++++++++++--------
>>  3 files changed, 196 insertions(+), 62 deletions(-)
>