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

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

[Hans de Goede]

Hi All,

Here is v2 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 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 (12):
  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: Fix scaler bypass mode
  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    | 166 ++++++++++++++++++++++++---------
 3 files changed, 127 insertions(+), 42 deletions(-)


base-commit: 2a8e220f6a2bcd5828ab67c7cf194874590ede24
-- 
2.49.0

[PATCH v2 01/12] 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 v2 02/12] 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 v2 03/12] 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 v2:
- Add select VIDEO_APTINA_PLL to Kconfig
- Use correct aptina_pll_limits
---
 drivers/media/i2c/Kconfig   |  1 +
 drivers/media/i2c/mt9m114.c | 54 ++++++++++++++++++++++++++-----------
 2 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index dc2c429734fc..1820ec37404a 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -285,6 +285,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..e12c69dc9df0 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,29 @@ 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
+		 * different PLL settings then 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 +2304,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

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

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sat, May 31, 2025 at 06:31:38PM +0200, Hans de Goede wrote:
> Before this change the driver used hardcoded PLL m, n and p values to
> achieve a 48MHz pixclock when used with an external clock with a frequency
> of 24 MHz.
> 
> Use aptina_pll_calculate() to allow the driver to work with different
> external clock frequencies. The m, n, and p values will be unchanged
> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
> clock where m gets increased from 32 to 40.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> 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 | 54 ++++++++++++++++++++++++++-----------
>  2 files changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index dc2c429734fc..1820ec37404a 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -285,6 +285,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..e12c69dc9df0 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,29 @@ 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
> +		 * different PLL settings then used by the vendor's drivers.

s/then/than/

Is that an issue though ? Does it prevent the sensor from working ?

> +		 */
> +		.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 +2304,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;

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

On 3-Jun-25 12:57 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sat, May 31, 2025 at 06:31:38PM +0200, Hans de Goede wrote:
>> Before this change the driver used hardcoded PLL m, n and p values to
>> achieve a 48MHz pixclock when used with an external clock with a frequency
>> of 24 MHz.
>>
>> Use aptina_pll_calculate() to allow the driver to work with different
>> external clock frequencies. The m, n, and p values will be unchanged
>> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
>> clock where m gets increased from 32 to 40.
>>
>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>> 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 | 54 ++++++++++++++++++++++++++-----------
>>  2 files changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index dc2c429734fc..1820ec37404a 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -285,6 +285,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..e12c69dc9df0 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,29 @@ 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
>> +		 * different PLL settings then used by the vendor's drivers.
> 
> s/then/than/
> 
> Is that an issue though ? Does it prevent the sensor from working ?

I did not try. It seems safer to just stick with the tested / proven
values from the older register-list based drivers?

Even if it does work on my single mt9m114 sensor, that hardly
constitutes testing on a representative sample.

Regards,

Hans




>> +		 */
>> +		.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 +2304,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;
> 

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

[Laurent Pinchart]

On Tue, Jun 03, 2025 at 03:29:42PM +0200, Hans de Goede wrote:
> On 3-Jun-25 12:57 PM, Laurent Pinchart wrote:
> > On Sat, May 31, 2025 at 06:31:38PM +0200, Hans de Goede wrote:
> >> Before this change the driver used hardcoded PLL m, n and p values to
> >> achieve a 48MHz pixclock when used with an external clock with a frequency
> >> of 24 MHz.
> >>
> >> Use aptina_pll_calculate() to allow the driver to work with different
> >> external clock frequencies. The m, n, and p values will be unchanged
> >> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
> >> clock where m gets increased from 32 to 40.
> >>
> >> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Signed-off-by: Hans de Goede <hansg@kernel.org>
> >> ---
> >> 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 | 54 ++++++++++++++++++++++++++-----------
> >>  2 files changed, 40 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> >> index dc2c429734fc..1820ec37404a 100644
> >> --- a/drivers/media/i2c/Kconfig
> >> +++ b/drivers/media/i2c/Kconfig
> >> @@ -285,6 +285,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..e12c69dc9df0 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,29 @@ 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
> >> +		 * different PLL settings then used by the vendor's drivers.
> > 
> > s/then/than/
> > 
> > Is that an issue though ? Does it prevent the sensor from working ?
> 
> I did not try. It seems safer to just stick with the tested / proven
> values from the older register-list based drivers?

Sometimes there are multiple options without any practical differences.
I wouldn't necessarily assume we have to follow the hardcoded register
values. Can you share the two PLL configurations ?

> Even if it does work on my single mt9m114 sensor, that hardly
> constitutes testing on a representative sample.

This sensor is rather old and not widely used. If using the correct
limits doesn't cause issues on platforms we can test, that's good enough
for me. We can always address problems later if any arise.

> >> +		 */
> >> +		.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 +2304,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;

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

On 3-Jun-25 4:05 PM, Laurent Pinchart wrote:
> On Tue, Jun 03, 2025 at 03:29:42PM +0200, Hans de Goede wrote:
>> On 3-Jun-25 12:57 PM, Laurent Pinchart wrote:
>>> On Sat, May 31, 2025 at 06:31:38PM +0200, Hans de Goede wrote:
>>>> Before this change the driver used hardcoded PLL m, n and p values to
>>>> achieve a 48MHz pixclock when used with an external clock with a frequency
>>>> of 24 MHz.
>>>>
>>>> Use aptina_pll_calculate() to allow the driver to work with different
>>>> external clock frequencies. The m, n, and p values will be unchanged
>>>> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
>>>> clock where m gets increased from 32 to 40.
>>>>
>>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>>>> ---
>>>> 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 | 54 ++++++++++++++++++++++++++-----------
>>>>  2 files changed, 40 insertions(+), 15 deletions(-)

...

>>>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
>>>> index 5a7c45ce2169..e12c69dc9df0 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)

...

>>>> @@ -2262,12 +2262,29 @@ 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
>>>> +		 * different PLL settings then used by the vendor's drivers.
>>>
>>> s/then/than/
>>>
>>> Is that an issue though ? Does it prevent the sensor from working ?
>>
>> I did not try. It seems safer to just stick with the tested / proven
>> values from the older register-list based drivers?
> 
> Sometimes there are multiple options without any practical differences.
> I wouldn't necessarily assume we have to follow the hardcoded register
> values. Can you share the two PLL configurations ?
> 
>> Even if it does work on my single mt9m114 sensor, that hardly
>> constitutes testing on a representative sample.
> 
> This sensor is rather old and not widely used. If using the correct
> limits doesn't cause issues on platforms we can test, that's good enough
> for me. We can always address problems later if any arise.

Ok, so I've tried setting out_clock_max to 768MHz, but that results
in PLL setting which cause the sensor to fail to stream.

So this should be kept at 400, I'll update the comment to reflect
that the datasheet says 768 but that that does not work.

Regards,

Hans



> 
>>>> +		 */
>>>> +		.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 +2304,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;
> 

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

[Laurent Pinchart]

Hi Hans,

On Fri, Jun 27, 2025 at 04:33:46PM +0200, Hans de Goede wrote:
> On 3-Jun-25 4:05 PM, Laurent Pinchart wrote:
> > On Tue, Jun 03, 2025 at 03:29:42PM +0200, Hans de Goede wrote:
> >> On 3-Jun-25 12:57 PM, Laurent Pinchart wrote:
> >>> On Sat, May 31, 2025 at 06:31:38PM +0200, Hans de Goede wrote:
> >>>> Before this change the driver used hardcoded PLL m, n and p values to
> >>>> achieve a 48MHz pixclock when used with an external clock with a frequency
> >>>> of 24 MHz.
> >>>>
> >>>> Use aptina_pll_calculate() to allow the driver to work with different
> >>>> external clock frequencies. The m, n, and p values will be unchanged
> >>>> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
> >>>> clock where m gets increased from 32 to 40.
> >>>>
> >>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
> >>>> ---
> >>>> 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 | 54 ++++++++++++++++++++++++++-----------
> >>>>  2 files changed, 40 insertions(+), 15 deletions(-)
> 
> ...
> 
> >>>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> >>>> index 5a7c45ce2169..e12c69dc9df0 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)
> 
> ...
> 
> >>>> @@ -2262,12 +2262,29 @@ 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
> >>>> +		 * different PLL settings then used by the vendor's drivers.
> >>>
> >>> s/then/than/
> >>>
> >>> Is that an issue though ? Does it prevent the sensor from working ?
> >>
> >> I did not try. It seems safer to just stick with the tested / proven
> >> values from the older register-list based drivers?
> > 
> > Sometimes there are multiple options without any practical differences.
> > I wouldn't necessarily assume we have to follow the hardcoded register
> > values. Can you share the two PLL configurations ?
> > 
> >> Even if it does work on my single mt9m114 sensor, that hardly
> >> constitutes testing on a representative sample.
> > 
> > This sensor is rather old and not widely used. If using the correct
> > limits doesn't cause issues on platforms we can test, that's good enough
> > for me. We can always address problems later if any arise.
> 
> Ok, so I've tried setting out_clock_max to 768MHz, but that results
> in PLL setting which cause the sensor to fail to stream.

Thank you for testing.

Could you share the PLL configuration you obtain with the 400 MHz and
768 MHz limits ?

> So this should be kept at 400, I'll update the comment to reflect
> that the datasheet says 768 but that that does not work.
> 
> >>>> +		 */
> >>>> +		.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 +2304,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;

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

On 27-Jun-25 8:06 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Fri, Jun 27, 2025 at 04:33:46PM +0200, Hans de Goede wrote:
>> On 3-Jun-25 4:05 PM, Laurent Pinchart wrote:
>>> On Tue, Jun 03, 2025 at 03:29:42PM +0200, Hans de Goede wrote:
>>>> On 3-Jun-25 12:57 PM, Laurent Pinchart wrote:
>>>>> On Sat, May 31, 2025 at 06:31:38PM +0200, Hans de Goede wrote:
>>>>>> Before this change the driver used hardcoded PLL m, n and p values to
>>>>>> achieve a 48MHz pixclock when used with an external clock with a frequency
>>>>>> of 24 MHz.
>>>>>>
>>>>>> Use aptina_pll_calculate() to allow the driver to work with different
>>>>>> external clock frequencies. The m, n, and p values will be unchanged
>>>>>> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
>>>>>> clock where m gets increased from 32 to 40.
>>>>>>
>>>>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>>>>>> ---
>>>>>> 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 | 54 ++++++++++++++++++++++++++-----------
>>>>>>  2 files changed, 40 insertions(+), 15 deletions(-)

...

>>>>>> @@ -2262,12 +2262,29 @@ 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
>>>>>> +		 * different PLL settings then used by the vendor's drivers.
>>>>>
>>>>> s/then/than/
>>>>>
>>>>> Is that an issue though ? Does it prevent the sensor from working ?
>>>>
>>>> I did not try. It seems safer to just stick with the tested / proven
>>>> values from the older register-list based drivers?
>>>
>>> Sometimes there are multiple options without any practical differences.
>>> I wouldn't necessarily assume we have to follow the hardcoded register
>>> values. Can you share the two PLL configurations ?
>>>
>>>> Even if it does work on my single mt9m114 sensor, that hardly
>>>> constitutes testing on a representative sample.
>>>
>>> This sensor is rather old and not widely used. If using the correct
>>> limits doesn't cause issues on platforms we can test, that's good enough
>>> for me. We can always address problems later if any arise.
>>
>> Ok, so I've tried setting out_clock_max to 768MHz, but that results
>> in PLL setting which cause the sensor to fail to stream.
> 
> Thank you for testing.
> 
> Could you share the PLL configuration you obtain with the 400 MHz and
> 768 MHz limits ?

400 MHz out_clock_max, working setup:

[   40.136435] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
[   40.136453] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
[   40.136463] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 8
[   40.136473] mt9m114 i2c-INT33F0:00: PLL: N 2 M 40 P1 8

768 MHz non working setup:

[   28.388940] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
[   28.388955] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
[   28.388966] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 16
[   28.388976] mt9m114 i2c-INT33F0:00: PLL: N 2 M 80 P1 16

Regards,

Hans

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

[Laurent Pinchart]

On Sat, Jun 28, 2025 at 11:27:23AM +0200, Hans de Goede wrote:
> On 27-Jun-25 8:06 PM, Laurent Pinchart wrote:
> > On Fri, Jun 27, 2025 at 04:33:46PM +0200, Hans de Goede wrote:
> >> On 3-Jun-25 4:05 PM, Laurent Pinchart wrote:
> >>> On Tue, Jun 03, 2025 at 03:29:42PM +0200, Hans de Goede wrote:
> >>>> On 3-Jun-25 12:57 PM, Laurent Pinchart wrote:
> >>>>> On Sat, May 31, 2025 at 06:31:38PM +0200, Hans de Goede wrote:
> >>>>>> Before this change the driver used hardcoded PLL m, n and p values to
> >>>>>> achieve a 48MHz pixclock when used with an external clock with a frequency
> >>>>>> of 24 MHz.
> >>>>>>
> >>>>>> Use aptina_pll_calculate() to allow the driver to work with different
> >>>>>> external clock frequencies. The m, n, and p values will be unchanged
> >>>>>> with a 24 MHz extclk and this has also been tested with a 19.2 MHz
> >>>>>> clock where m gets increased from 32 to 40.
> >>>>>>
> >>>>>> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>> Signed-off-by: Hans de Goede <hansg@kernel.org>
> >>>>>> ---
> >>>>>> 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 | 54 ++++++++++++++++++++++++++-----------
> >>>>>>  2 files changed, 40 insertions(+), 15 deletions(-)
> 
> ...
> 
> >>>>>> @@ -2262,12 +2262,29 @@ 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
> >>>>>> +		 * different PLL settings then used by the vendor's drivers.
> >>>>>
> >>>>> s/then/than/
> >>>>>
> >>>>> Is that an issue though ? Does it prevent the sensor from working ?
> >>>>
> >>>> I did not try. It seems safer to just stick with the tested / proven
> >>>> values from the older register-list based drivers?
> >>>
> >>> Sometimes there are multiple options without any practical differences.
> >>> I wouldn't necessarily assume we have to follow the hardcoded register
> >>> values. Can you share the two PLL configurations ?
> >>>
> >>>> Even if it does work on my single mt9m114 sensor, that hardly
> >>>> constitutes testing on a representative sample.
> >>>
> >>> This sensor is rather old and not widely used. If using the correct
> >>> limits doesn't cause issues on platforms we can test, that's good enough
> >>> for me. We can always address problems later if any arise.
> >>
> >> Ok, so I've tried setting out_clock_max to 768MHz, but that results
> >> in PLL setting which cause the sensor to fail to stream.
> > 
> > Thank you for testing.
> > 
> > Could you share the PLL configuration you obtain with the 400 MHz and
> > 768 MHz limits ?
> 
> 400 MHz out_clock_max, working setup:
> 
> [   40.136435] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
> [   40.136453] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
> [   40.136463] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 8
> [   40.136473] mt9m114 i2c-INT33F0:00: PLL: N 2 M 40 P1 8
> 
> 768 MHz non working setup:
> 
> [   28.388940] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
> [   28.388955] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
> [   28.388966] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 16
> [   28.388976] mt9m114 i2c-INT33F0:00: PLL: N 2 M 80 P1 16

Hmmm...

Re-reading the documentation, I wonder if I had misunderstood the PLL.

There is no single clear description of the PLL (it would be too easy),
but there's informatio scattered in multiple places. One of them states
that

Fout = (Fin*2*m)/((n+1)*(p+1)).

Note the *2 multiplier. In another location, multiple frequencies are
defined:

fSensor = (M x fin) / ((N + 1) x (P + 1)) (max 48 MHz)
fWord = fSensor x 2 (max 96 MHz)
fBit = fWord * 8 (max 763 MHz)

The '+1' for N and P are understood, they relate to the values
programmed in the registers. From the point of view of the PLL
calculator, we should reason with that offset, with

Fout = (Fin*2*m)/(n*p).

and

fSensor = (M x fin) / (N x P)

This leads me to believe that the PLL is organized as follows:

        +-----+     +-----+     +-----+     +-----+     +-----+
Fin --> | / N | --> | x M | --> | x 2 | --> | / P | --> | / 2 | -->
        +-----+     +-----+     +-----+     +-----+     +-----+
	                                fBit       fWord        fSensor
ext_clock    int_clock   out_clock                             pix_clock

Seen this way, the maximum limit for out_clock should be 384 MHz, and
the P factor should be hardcoded to 8 for CSI-2 output.

Does that make sense to you ?

-- 
Regards,

Laurent Pinchart

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

[Hans de Goede]

Hi Laurent,

First of all sorry for dropping the ball on this series.

I'm preparing a v4 of this series now.

On 29-Jun-25 22:46, Laurent Pinchart wrote:
> On Sat, Jun 28, 2025 at 11:27:23AM +0200, Hans de Goede wrote:

...

>>> Could you share the PLL configuration you obtain with the 400 MHz and
>>> 768 MHz limits ?
>>
>> 400 MHz out_clock_max, working setup:
>>
>> [   40.136435] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
>> [   40.136453] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
>> [   40.136463] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 8
>> [   40.136473] mt9m114 i2c-INT33F0:00: PLL: N 2 M 40 P1 8
>>
>> 768 MHz non working setup:
>>
>> [   28.388940] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
>> [   28.388955] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
>> [   28.388966] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 16
>> [   28.388976] mt9m114 i2c-INT33F0:00: PLL: N 2 M 80 P1 16
> 
> Hmmm...
> 
> Re-reading the documentation, I wonder if I had misunderstood the PLL.
> 
> There is no single clear description of the PLL (it would be too easy),
> but there's informatio scattered in multiple places. One of them states
> that
> 
> Fout = (Fin*2*m)/((n+1)*(p+1)).
> 
> Note the *2 multiplier. In another location, multiple frequencies are
> defined:
> 
> fSensor = (M x fin) / ((N + 1) x (P + 1)) (max 48 MHz)
> fWord = fSensor x 2 (max 96 MHz)
> fBit = fWord * 8 (max 763 MHz)
> 
> The '+1' for N and P are understood, they relate to the values
> programmed in the registers. From the point of view of the PLL
> calculator, we should reason with that offset, with
> 
> Fout = (Fin*2*m)/(n*p).
> 
> and
> 
> fSensor = (M x fin) / (N x P)
> 
> This leads me to believe that the PLL is organized as follows:
> 
>         +-----+     +-----+     +-----+     +-----+     +-----+
> Fin --> | / N | --> | x M | --> | x 2 | --> | / P | --> | / 2 | -->
>         +-----+     +-----+     +-----+     +-----+     +-----+
> 	                                fBit       fWord        fSensor
> ext_clock    int_clock   out_clock                             pix_clock
> 
> Seen this way, the maximum limit for out_clock should be 384 MHz, and
> the P factor should be hardcoded to 8 for CSI-2 output.
> 
> Does that make sense to you ?

Yes that sounds right and setting out_clock_max to 384MHz works.

I've set out_clock_max to 384MH and limited P to min = max = 8
for the upcoming v4 of this series.

Regards,

Hans

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

[Laurent Pinchart]

Hi Hans,

On Tue, Dec 23, 2025 at 02:27:50PM +0100, Hans de Goede wrote:
> Hi Laurent,
> 
> First of all sorry for dropping the ball on this series.

I won't blame you, it happens to me way too often too.

> I'm preparing a v4 of this series now.
> 
> On 29-Jun-25 22:46, Laurent Pinchart wrote:
> > On Sat, Jun 28, 2025 at 11:27:23AM +0200, Hans de Goede wrote:
> 
> ...
> 
> >>> Could you share the PLL configuration you obtain with the 400 MHz and
> >>> 768 MHz limits ?
> >>
> >> 400 MHz out_clock_max, working setup:
> >>
> >> [   40.136435] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
> >> [   40.136453] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
> >> [   40.136463] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 8
> >> [   40.136473] mt9m114 i2c-INT33F0:00: PLL: N 2 M 40 P1 8
> >>
> >> 768 MHz non working setup:
> >>
> >> [   28.388940] mt9m114 i2c-INT33F0:00: PLL: ext clock 19200000 pix clock 48000000
> >> [   28.388955] mt9m114 i2c-INT33F0:00: pll: mf min 4 max 38
> >> [   28.388966] mt9m114 i2c-INT33F0:00: pll: p1 min 4 max 16
> >> [   28.388976] mt9m114 i2c-INT33F0:00: PLL: N 2 M 80 P1 16
> > 
> > Hmmm...
> > 
> > Re-reading the documentation, I wonder if I had misunderstood the PLL.
> > 
> > There is no single clear description of the PLL (it would be too easy),
> > but there's informatio scattered in multiple places. One of them states
> > that
> > 
> > Fout = (Fin*2*m)/((n+1)*(p+1)).
> > 
> > Note the *2 multiplier. In another location, multiple frequencies are
> > defined:
> > 
> > fSensor = (M x fin) / ((N + 1) x (P + 1)) (max 48 MHz)
> > fWord = fSensor x 2 (max 96 MHz)
> > fBit = fWord * 8 (max 763 MHz)
> > 
> > The '+1' for N and P are understood, they relate to the values
> > programmed in the registers. From the point of view of the PLL
> > calculator, we should reason with that offset, with
> > 
> > Fout = (Fin*2*m)/(n*p).
> > 
> > and
> > 
> > fSensor = (M x fin) / (N x P)
> > 
> > This leads me to believe that the PLL is organized as follows:
> > 
> >         +-----+     +-----+     +-----+     +-----+     +-----+
> > Fin --> | / N | --> | x M | --> | x 2 | --> | / P | --> | / 2 | -->
> >         +-----+     +-----+     +-----+     +-----+     +-----+
> > 	                                fBit       fWord        fSensor
> > ext_clock    int_clock   out_clock                             pix_clock

Could you capture this in a comment in v4 ?

> > Seen this way, the maximum limit for out_clock should be 384 MHz, and
> > the P factor should be hardcoded to 8 for CSI-2 output.
> > 
> > Does that make sense to you ?
> 
> Yes that sounds right and setting out_clock_max to 384MHz works.
> 
> I've set out_clock_max to 384MH and limited P to min = max = 8
> for the upcoming v4 of this series.

-- 
Regards,

Laurent Pinchart

[PATCH v2 04/12] 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 e12c69dc9df0..5f59bfd83402 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 v2 05/12] 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 5f59bfd83402..aa401b3fb4a7 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 v2 06/12] 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 aa401b3fb4a7..371e96d9e281 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 v2 07/12] 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 371e96d9e281..fe30b9ff84ad 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2439,7 +2439,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 v2 08/12] media: mt9m114: Put sensor in reset on power-down

[Hans de Goede]

Put sensor back in reset on power-down.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
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 fe30b9ff84ad..7a1451006cfe 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

Re: [PATCH v2 08/12] media: mt9m114: Put sensor in reset on power-down

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sat, May 31, 2025 at 06:31:43PM +0200, Hans de Goede wrote:
> Put sensor back in reset on power-down.

The commit message should explain why. With that,

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

> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> 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 fe30b9ff84ad..7a1451006cfe 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);
>  }

-- 
Regards,

Laurent Pinchart

[PATCH v2 09/12] media: mt9m114: Fix scaler bypass mode

[Hans de Goede]

As indicated by the comment in mt9m114_ifp_set_fmt():

	/* If the output format is RAW10, bypass the scaler. */
	if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
		...

The intend of the driver is that the scalar is bypassed when the ISP
source/output pad's pixel-format is set to MEDIA_BUS_FMT_SGRBG10_1X10.

This patch makes 2 changes which are required to get this to work properly:

1. Set the MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE bit in
   the MT9M114_CAM_OUTPUT_FORMAT register.

2. Disable cropping/composing by setting crop and compose selections on
   the ISP sink/input format to the format widthxheight @ 0x0.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
Changes in v2:
- 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
---
 drivers/media/i2c/mt9m114.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 7a1451006cfe..d954f2be8f0d 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -467,7 +467,8 @@ static const struct mt9m114_format_info mt9m114_format_infos[] = {
 		/* Keep the format compatible with the IFP sink pad last. */
 		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
 		.output_format = MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_RAWR10
-			| MT9M114_CAM_OUTPUT_FORMAT_FORMAT_BAYER,
+			| MT9M114_CAM_OUTPUT_FORMAT_FORMAT_BAYER
+			| MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE,
 		.flags = MT9M114_FMT_FLAG_PARALLEL | MT9M114_FMT_FLAG_CSI2,
 	}
 };
@@ -850,6 +851,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;
 
@@ -869,10 +871,12 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
 	 * by demosaicing that are taken into account in the crop rectangle but
 	 * not in the hardware.
 	 */
+	border = (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) ? 0 : 4;
+
 	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,
@@ -911,7 +915,8 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
 			   MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_MASK |
 			   MT9M114_CAM_OUTPUT_FORMAT_FORMAT_MASK |
 			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_BYTES |
-			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE);
+			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE |
+			   MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE);
 	output_format |= info->output_format;
 
 	cci_write(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
@@ -1810,6 +1815,7 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
 {
 	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
 	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
 
 	format = v4l2_subdev_state_get_format(state, fmt->pad);
 
@@ -1830,8 +1836,15 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
 		format->code = info->code;
 
 		/* If the output format is RAW10, bypass the scaler. */
-		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
+		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
 			*format = *v4l2_subdev_state_get_format(state, 0);
+			crop = v4l2_subdev_state_get_crop(state, 0);
+			crop->left = 0;
+			crop->top = 0;
+			crop->width = format->width;
+			crop->height = format->height;
+			*v4l2_subdev_state_get_compose(state, 0) = *crop;
+		}
 	}
 
 	fmt->format = *format;
@@ -1851,6 +1864,12 @@ static int mt9m114_ifp_get_selection(struct v4l2_subdev *sd,
 	if (sel->pad != 0)
 		return -EINVAL;
 
+	/* Crop and compose cannot be changed when bypassing the scaler */
+	if (v4l2_subdev_state_get_format(state, 1)->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
+		sel->r = *v4l2_subdev_state_get_crop(state, 0);
+		return 0;
+	}
+
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP:
 		sel->r = *v4l2_subdev_state_get_crop(state, 0);
@@ -1911,6 +1930,12 @@ 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 */
+	if (v4l2_subdev_state_get_format(state, 1)->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);
-- 
2.49.0

Re: [PATCH v2 09/12] media: mt9m114: Fix scaler bypass mode

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sat, May 31, 2025 at 06:31:44PM +0200, Hans de Goede wrote:
> As indicated by the comment in mt9m114_ifp_set_fmt():
> 
> 	/* If the output format is RAW10, bypass the scaler. */
> 	if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
> 		...
> 
> The intend of the driver is that the scalar is bypassed when the ISP
> source/output pad's pixel-format is set to MEDIA_BUS_FMT_SGRBG10_1X10.
> 
> This patch makes 2 changes which are required to get this to work properly:
> 
> 1. Set the MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE bit in
>    the MT9M114_CAM_OUTPUT_FORMAT register.

Does that bit actually make a difference ? It's documented as only being
effective when MT9M114_CAM_OUTPUT_FORMAT_BT656_ENABLE is also set, and
the driver never sets that.

> 
> 2. Disable cropping/composing by setting crop and compose selections on
>    the ISP sink/input format to the format widthxheight @ 0x0.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> Changes in v2:
> - 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
> ---
>  drivers/media/i2c/mt9m114.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 7a1451006cfe..d954f2be8f0d 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -467,7 +467,8 @@ static const struct mt9m114_format_info mt9m114_format_infos[] = {
>  		/* Keep the format compatible with the IFP sink pad last. */
>  		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
>  		.output_format = MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_RAWR10
> -			| MT9M114_CAM_OUTPUT_FORMAT_FORMAT_BAYER,
> +			| MT9M114_CAM_OUTPUT_FORMAT_FORMAT_BAYER
> +			| MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE,
>  		.flags = MT9M114_FMT_FLAG_PARALLEL | MT9M114_FMT_FLAG_CSI2,
>  	}
>  };
> @@ -850,6 +851,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;
>  
> @@ -869,10 +871,12 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
>  	 * by demosaicing that are taken into account in the crop rectangle but
>  	 * not in the hardware.
>  	 */

The comment should be updated.

> +	border = (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) ? 0 : 4;

No need for parentheses.

> +
>  	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,
> @@ -911,7 +915,8 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
>  			   MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_MASK |
>  			   MT9M114_CAM_OUTPUT_FORMAT_FORMAT_MASK |
>  			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_BYTES |
> -			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE);
> +			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE |
> +			   MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE);
>  	output_format |= info->output_format;
>  
>  	cci_write(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
> @@ -1810,6 +1815,7 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>  	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_rect *crop;
>  
>  	format = v4l2_subdev_state_get_format(state, fmt->pad);
>  
> @@ -1830,8 +1836,15 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
>  		format->code = info->code;
>  
>  		/* If the output format is RAW10, bypass the scaler. */
> -		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
> +		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
>  			*format = *v4l2_subdev_state_get_format(state, 0);
> +			crop = v4l2_subdev_state_get_crop(state, 0);
> +			crop->left = 0;
> +			crop->top = 0;
> +			crop->width = format->width;
> +			crop->height = format->height;
> +			*v4l2_subdev_state_get_compose(state, 0) = *crop;
> +		}

This one is annoying. Normally changing a format or selection rectangle
in a subdev should only propagate the configuration downstream. Here
you're modiying selection rectangles upstream of the source pad. I think
we'll need to live with it, I don't really see a way around that. It's a
non-standard behaviour though, and would require sensor-specific
userspace to handle this right.

There's however one issue. When switching back from a raw output to a
processed output, the crop and compose rectangles will remain the same,
and will have values that are not valid for processed output as they
won't have the 4 pixels border subtracted. I think you'll need to update
them, but only when the source format actually changes. Setting the
source format without modifying the media bus code should not result in
the selection rectangles being reset.

All this needs to be explained in a comment in the code.

>  	}
>  
>  	fmt->format = *format;
> @@ -1851,6 +1864,12 @@ static int mt9m114_ifp_get_selection(struct v4l2_subdev *sd,
>  	if (sel->pad != 0)
>  		return -EINVAL;
>  
> +	/* Crop and compose cannot be changed when bypassing the scaler */

s/scaler/scaler./

Same below.

> +	if (v4l2_subdev_state_get_format(state, 1)->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
> +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> +		return 0;
> +	}

Instead of special-casing this, can you use a similar approach as in
mt9m114_configure_ifp() and replace the 4 and 8 below with border and
boarder * 2 (with an update to the comment too) ?

> +
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP:
>  		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> @@ -1911,6 +1930,12 @@ 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 */
> +	if (v4l2_subdev_state_get_format(state, 1)->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
> +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> +		return 0;
> +	}

Let's add a source_format variable to avoid duplicating the
v4l2_subdev_state_get_format(), call, as we use the source format at the
end of the function.

> +
>  	format = v4l2_subdev_state_get_format(state, 0);
>  	crop = v4l2_subdev_state_get_crop(state, 0);
>  	compose = v4l2_subdev_state_get_compose(state, 0);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 09/12] media: mt9m114: Fix scaler bypass mode

[Hans de Goede]

On 3-Jun-25 2:48 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sat, May 31, 2025 at 06:31:44PM +0200, Hans de Goede wrote:
>> As indicated by the comment in mt9m114_ifp_set_fmt():
>>
>> 	/* If the output format is RAW10, bypass the scaler. */
>> 	if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
>> 		...
>>
>> The intend of the driver is that the scalar is bypassed when the ISP
>> source/output pad's pixel-format is set to MEDIA_BUS_FMT_SGRBG10_1X10.
>>
>> This patch makes 2 changes which are required to get this to work properly:
>>
>> 1. Set the MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE bit in
>>    the MT9M114_CAM_OUTPUT_FORMAT register.
> 
> Does that bit actually make a difference ? It's documented as only being
> effective when MT9M114_CAM_OUTPUT_FORMAT_BT656_ENABLE is also set, and
> the driver never sets that.

I've just given not setting the bit a try and indeed setting the bit
is not necessary. I'll drop this for the next version.

>> 2. Disable cropping/composing by setting crop and compose selections on
>>    the ISP sink/input format to the format widthxheight @ 0x0.
>>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> ---
>> Changes in v2:
>> - 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
>> ---
>>  drivers/media/i2c/mt9m114.c | 35 ++++++++++++++++++++++++++++++-----
>>  1 file changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
>> index 7a1451006cfe..d954f2be8f0d 100644
>> --- a/drivers/media/i2c/mt9m114.c
>> +++ b/drivers/media/i2c/mt9m114.c
>> @@ -467,7 +467,8 @@ static const struct mt9m114_format_info mt9m114_format_infos[] = {
>>  		/* Keep the format compatible with the IFP sink pad last. */
>>  		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
>>  		.output_format = MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_RAWR10
>> -			| MT9M114_CAM_OUTPUT_FORMAT_FORMAT_BAYER,
>> +			| MT9M114_CAM_OUTPUT_FORMAT_FORMAT_BAYER
>> +			| MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE,
>>  		.flags = MT9M114_FMT_FLAG_PARALLEL | MT9M114_FMT_FLAG_CSI2,
>>  	}
>>  };
>> @@ -850,6 +851,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;
>>  
>> @@ -869,10 +871,12 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
>>  	 * by demosaicing that are taken into account in the crop rectangle but
>>  	 * not in the hardware.
>>  	 */
> 
> The comment should be updated.

Ack.

>> +	border = (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) ? 0 : 4;
> 
> No need for parentheses.

Ack.

>> +
>>  	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,
>> @@ -911,7 +915,8 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
>>  			   MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_MASK |
>>  			   MT9M114_CAM_OUTPUT_FORMAT_FORMAT_MASK |
>>  			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_BYTES |
>> -			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE);
>> +			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE |
>> +			   MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE);
>>  	output_format |= info->output_format;
>>  
>>  	cci_write(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
>> @@ -1810,6 +1815,7 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
>>  {
>>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>>  	struct v4l2_mbus_framefmt *format;
>> +	struct v4l2_rect *crop;
>>  
>>  	format = v4l2_subdev_state_get_format(state, fmt->pad);
>>  
>> @@ -1830,8 +1836,15 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
>>  		format->code = info->code;
>>  
>>  		/* If the output format is RAW10, bypass the scaler. */
>> -		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
>> +		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
>>  			*format = *v4l2_subdev_state_get_format(state, 0);
>> +			crop = v4l2_subdev_state_get_crop(state, 0);
>> +			crop->left = 0;
>> +			crop->top = 0;
>> +			crop->width = format->width;
>> +			crop->height = format->height;
>> +			*v4l2_subdev_state_get_compose(state, 0) = *crop;
>> +		}
> 
> This one is annoying. Normally changing a format or selection rectangle
> in a subdev should only propagate the configuration downstream. Here
> you're modiying selection rectangles upstream of the source pad. I think
> we'll need to live with it, I don't really see a way around that. It's a
> non-standard behaviour though, and would require sensor-specific
> userspace to handle this right.
> 
> There's however one issue. When switching back from a raw output to a
> processed output, the crop and compose rectangles will remain the same,
> and will have values that are not valid for processed output as they
> won't have the 4 pixels border subtracted. I think you'll need to update
> them, but only when the source format actually changes. Setting the
> source format without modifying the media bus code should not result in
> the selection rectangles being reset.
> 
> All this needs to be explained in a comment in the code.

Ok, I'll modify the code to fixup the rectangles when the output-format
changes from MEDIA_BUS_FMT_SGRBG10_1X10 -> not MEDIA_BUS_FMT_SGRBG10_1X10.

And I'll add a comment explaining this.

Actually given the above comment it is best to split this change
into multiple patches, so for v3 I'm going to do as you request
but it will be split out a bit more.

>>  	}
>>  
>>  	fmt->format = *format;
>> @@ -1851,6 +1864,12 @@ static int mt9m114_ifp_get_selection(struct v4l2_subdev *sd,
>>  	if (sel->pad != 0)
>>  		return -EINVAL;
>>  
>> +	/* Crop and compose cannot be changed when bypassing the scaler */
> 
> s/scaler/scaler./
> 
> Same below.

Ack.

>> +	if (v4l2_subdev_state_get_format(state, 1)->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
>> +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
>> +		return 0;
>> +	}
> 
> Instead of special-casing this, can you use a similar approach as in
> mt9m114_configure_ifp() and replace the 4 and 8 below with border and
> boarder * 2 (with an update to the comment too) ?

Ack, this is part of the splitting of this patch mentioned above.

> 
>> +
>>  	switch (sel->target) {
>>  	case V4L2_SEL_TGT_CROP:
>>  		sel->r = *v4l2_subdev_state_get_crop(state, 0);
>> @@ -1911,6 +1930,12 @@ 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 */
>> +	if (v4l2_subdev_state_get_format(state, 1)->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
>> +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
>> +		return 0;
>> +	}
> 
> Let's add a source_format variable to avoid duplicating the
> v4l2_subdev_state_get_format(), call, as we use the source format at the
> end of the function.

Ack.

Regards,

Hans


> 
>> +
>>  	format = v4l2_subdev_state_get_format(state, 0);
>>  	crop = v4l2_subdev_state_get_crop(state, 0);
>>  	compose = v4l2_subdev_state_get_compose(state, 0);
> 

[PATCH v2 10/12] 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 d954f2be8f0d..c4d3122d698e 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 */
@@ -782,14 +783,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;
 }
 
@@ -976,6 +970,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
 	if (ret)
 		goto error;
 
+	sensor->config_change_pending = false;
 	sensor->streaming = true;
 
 	return 0;
@@ -2267,6 +2262,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 v2 10/12] media: mt9m114: Drop start-, stop-streaming sequence from initialize

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sat, May 31, 2025 at 06:31:45PM +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.
> 
> 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 d954f2be8f0d..c4d3122d698e 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 */
> @@ -782,14 +783,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;

Entering Config-Change here was meant to ensure the PLL and output mode
settings get applied. The PLL settings can probably wait until we start
streaming. For the output mode, I'm slightly concerned that incorrect
settings could lead to hardware issues, as the sensor starts in parallel
output mode. I suppose it shouldn't cause any hardware damage, so I
think we could try to just delay Config-Change until we start streaming.

> -
> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_SUSPEND);
> -	if (ret < 0)
> -		return ret;

This bothers me a bit more. As far as I understand, the sensor starts
streaming right after power on reset, so I'd like to disable streaming
as quickly as possible.

> -
> +	sensor->config_change_pending = true;
>  	return 0;
>  }
>  
> @@ -976,6 +970,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>  	if (ret)
>  		goto error;
>  
> +	sensor->config_change_pending = false;
>  	sensor->streaming = true;
>  
>  	return 0;
> @@ -2267,6 +2262,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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 10/12] media: mt9m114: Drop start-, stop-streaming sequence from initialize

[Hans de Goede]

Hi Laurent,

On 3-Jun-25 1:33 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sat, May 31, 2025 at 06:31:45PM +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.
>>
>> 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 d954f2be8f0d..c4d3122d698e 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 */
>> @@ -782,14 +783,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;
> 
> Entering Config-Change here was meant to ensure the PLL and output mode
> settings get applied. The PLL settings can probably wait until we start
> streaming. For the output mode, I'm slightly concerned that incorrect
> settings could lead to hardware issues, as the sensor starts in parallel
> output mode. I suppose it shouldn't cause any hardware damage, so I
> think we could try to just delay Config-Change until we start streaming.
> 
>> -
>> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_SUSPEND);
>> -	if (ret < 0)
>> -		return ret;
> 
> This bothers me a bit more. As far as I understand, the sensor starts
> streaming right after power on reset, so I'd like to disable streaming
> as quickly as possible.

I tried doing the MT9M114_SYS_STATE_ENTER_SUSPEND without a prior
MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE but that does not work, then
mt9m114_poll_command() times out.

Also I believe that the start/stop/start streaming vs just a
single start streaming is actually causing the issues on atomisp
devices.

Normally we only come out of runtime-suspend from
mt9m114_start_streaming() so in this case there is no issue
with not disabling streaming from mt9m114_initialize().

The only case where the sensor would be streaming when
we do not want it would be after the power-up +
mt9m114_initialize() from probe().

And we can get the desired "disable streaming
as quickly as possible." by forcing a runtime-suspend
from probe() directly after mt9m114_initialize() instead
of waiting for the auto runtimesuspend.

I'll make probe() immediately runtime-suspend the sensor
after mt9m114_initialize() in the next version.

Regards,

Hans



> 
>> -
>> +	sensor->config_change_pending = true;
>>  	return 0;
>>  }
>>  
>> @@ -976,6 +970,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>>  	if (ret)
>>  		goto error;
>>  
>> +	sensor->config_change_pending = false;
>>  	sensor->streaming = true;
>>  
>>  	return 0;
>> @@ -2267,6 +2262,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;
> 

Re: [PATCH v2 10/12] media: mt9m114: Drop start-, stop-streaming sequence from initialize

[Hans de Goede]

Hi Laurent,

On 3-Jun-25 1:33 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sat, May 31, 2025 at 06:31:45PM +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.
>>
>> 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 d954f2be8f0d..c4d3122d698e 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 */
>> @@ -782,14 +783,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;
> 
> Entering Config-Change here was meant to ensure the PLL and output mode
> settings get applied. The PLL settings can probably wait until we start
> streaming. For the output mode, I'm slightly concerned that incorrect
> settings could lead to hardware issues, as the sensor starts in parallel
> output mode. I suppose it shouldn't cause any hardware damage, so I
> think we could try to just delay Config-Change until we start streaming.
> 
>> -
>> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_SUSPEND);
>> -	if (ret < 0)
>> -		return ret;
> 
> This bothers me a bit more. As far as I understand, the sensor starts
> streaming right after power on reset, so I'd like to disable streaming
> as quickly as possible.

Actually while working on immediately putting the sensor back in
suspend from probe() I noticed that mt9m114_power_on() puts the
sensor in standby at the end even checks for this:

        /*
         * Before issuing any Set-State command, we must ensure that the sensor
         * reaches the standby mode (either initiated manually above in
         * parallel mode, or automatically after reset in MIPI mode).
         */
        ret = mt9m114_poll_state(sensor, MT9M114_SYS_STATE_STANDBY);
        if (ret < 0)
                goto error_clock;

        return 0;

Which means should not be streaming after power-on. So I believe that
this patch is fine as is and I'm going to keep this version  for v3
of the patch-set.

Regards,

Hans




> 
>> -
>> +	sensor->config_change_pending = true;
>>  	return 0;
>>  }
>>  
>> @@ -976,6 +970,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
>>  	if (ret)
>>  		goto error;
>>  
>> +	sensor->config_change_pending = false;
>>  	sensor->streaming = true;
>>  
>>  	return 0;
>> @@ -2267,6 +2262,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;
> 

Re: [PATCH v2 10/12] media: mt9m114: Drop start-, stop-streaming sequence from initialize

[Laurent Pinchart]

On Sun, Jun 29, 2025 at 07:11:54PM +0200, Hans de Goede wrote:
> On 3-Jun-25 1:33 PM, Laurent Pinchart wrote:
> > On Sat, May 31, 2025 at 06:31:45PM +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.
> >>
> >> 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 d954f2be8f0d..c4d3122d698e 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 */
> >> @@ -782,14 +783,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;
> > 
> > Entering Config-Change here was meant to ensure the PLL and output mode
> > settings get applied. The PLL settings can probably wait until we start
> > streaming. For the output mode, I'm slightly concerned that incorrect
> > settings could lead to hardware issues, as the sensor starts in parallel
> > output mode. I suppose it shouldn't cause any hardware damage, so I
> > think we could try to just delay Config-Change until we start streaming.
> > 
> >> -
> >> -	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_SUSPEND);
> >> -	if (ret < 0)
> >> -		return ret;
> > 
> > This bothers me a bit more. As far as I understand, the sensor starts
> > streaming right after power on reset, so I'd like to disable streaming
> > as quickly as possible.
> 
> Actually while working on immediately putting the sensor back in
> suspend from probe() I noticed that mt9m114_power_on() puts the
> sensor in standby at the end even checks for this:
> 
>         /*
>          * Before issuing any Set-State command, we must ensure that the sensor
>          * reaches the standby mode (either initiated manually above in
>          * parallel mode, or automatically after reset in MIPI mode).
>          */
>         ret = mt9m114_poll_state(sensor, MT9M114_SYS_STATE_STANDBY);
>         if (ret < 0)
>                 goto error_clock;
> 
>         return 0;
> 
> Which means should not be streaming after power-on. So I believe that
> this patch is fine as is and I'm going to keep this version  for v3
> of the patch-set.

I just realized that this is governed by the external CONFIG pin. When
the pin if pulled up (or left floating, as there's an internal pull-up
resistor), the PLL is automatically configured and the sensor starts
streaming at power up time. When the pin is pulled low, the sensor will
go to the standby state.

I think we can assume that CONFIG will be pulled low when the sensor is
used with the CSI-2 output. For the parallel output, either
configuration is possible. mt9m114_power_on() issues a
MT9M114_SYS_STATE_ENTER_STANDBY in that case.

> >> -
> >> +	sensor->config_change_pending = true;
> >>  	return 0;
> >>  }
> >>  
> >> @@ -976,6 +970,7 @@ static int mt9m114_start_streaming(struct mt9m114 *sensor,
> >>  	if (ret)
> >>  		goto error;
> >>  
> >> +	sensor->config_change_pending = false;
> >>  	sensor->streaming = true;
> >>  
> >>  	return 0;
> >> @@ -2267,6 +2262,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;

-- 
Regards,

Laurent Pinchart

[PATCH v2 11/12] 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 c4d3122d698e..72914c47ec9a 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2399,11 +2399,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 v2 11/12] media: mt9m114: Return -EPROBE_DEFER if no endpoint is found

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Sat, May 31, 2025 at 06:31:46PM +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 c4d3122d698e..72914c47ec9a 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -2399,11 +2399,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");

That's a bit annoying, as in non-ACPI systems we'll then get probe
deferral, making the issue more difficult to debug.

Is there a way, on IPU-based systems, to delay probing the sensor until
the bridge has been initialized ? I suppose this is a question for
Sakari too.

>  
>  	sensor->bus_cfg.bus_type = V4L2_MBUS_UNKNOWN;
>  	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &sensor->bus_cfg);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/12] media: mt9m114: Return -EPROBE_DEFER if no endpoint is found

[Hans de Goede]

Hi Laurent,

Thank you for your reviews. I agree with most of your
comments and I'll address them when I can make some time.

On 3-Jun-25 1:03 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Sat, May 31, 2025 at 06:31:46PM +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 c4d3122d698e..72914c47ec9a 100644
>> --- a/drivers/media/i2c/mt9m114.c
>> +++ b/drivers/media/i2c/mt9m114.c
>> @@ -2399,11 +2399,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");
> 
> That's a bit annoying, as in non-ACPI systems we'll then get probe
> deferral, making the issue more difficult to debug.

With "then" I assume you mean when the fwnode graph endpoint is missing
on DT systems ?

> Is there a way, on IPU-based systems, to delay probing the sensor until
> the bridge has been initialized ?

Waiting for the fwnode graph endpoint to show up is the way to wait for
bridge init we (Sakari and me) have agreed upon, this is done on all
drivers used on x86/ACPI platforms.

Note that by using dev_err_probe() here and not just
"return -EPROBE_DEFER" the kernel will log a message
during boot some time (1 minute?) after the last probe
has succeeded about devices with a deferred probe pending
logging the reason passed to dev_err_probe(), so debugging
this is should not be that much of an issue. There will still
be a log message, it is just that it will be logged a bit later
rather then directly at probe time.

Regards,

Hans







 I suppose this is a question for
> Sakari too.
> 
>>  
>>  	sensor->bus_cfg.bus_type = V4L2_MBUS_UNKNOWN;
>>  	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &sensor->bus_cfg);
> 

Re: [PATCH v2 11/12] media: mt9m114: Return -EPROBE_DEFER if no endpoint is found

[Sakari Ailus]

Hi Hans, Laurent,

On Tue, Jun 03, 2025 at 03:27:16PM +0200, Hans de Goede wrote:
> Hi Laurent,
> 
> Thank you for your reviews. I agree with most of your
> comments and I'll address them when I can make some time.
> 
> On 3-Jun-25 1:03 PM, Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > Thank you for the patch.
> > 
> > On Sat, May 31, 2025 at 06:31:46PM +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 c4d3122d698e..72914c47ec9a 100644
> >> --- a/drivers/media/i2c/mt9m114.c
> >> +++ b/drivers/media/i2c/mt9m114.c
> >> @@ -2399,11 +2399,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");
> > 
> > That's a bit annoying, as in non-ACPI systems we'll then get probe
> > deferral, making the issue more difficult to debug.
> 
> With "then" I assume you mean when the fwnode graph endpoint is missing
> on DT systems ?
> 
> > Is there a way, on IPU-based systems, to delay probing the sensor until
> > the bridge has been initialized ?
> 
> Waiting for the fwnode graph endpoint to show up is the way to wait for
> bridge init we (Sakari and me) have agreed upon, this is done on all
> drivers used on x86/ACPI platforms.

In the long run it'd be nice to move the code from the IPU bridge to the
ACPI framework. That way we could remove this check as we could guarantee
the endpoints would be where they're needed in time.

But I don't expect this to happen any time soon: reworking the fwnode
infrastructure to accommodate more flexible node/property insertion should
probably be done first. Still, IPU bridge doesn't conflict with DisCo for
Imaging doing largely similar things under the hood as the two are
effectively mutually exclusive (and should remain so).

-- 
Kind regards,

Sakari Ailus

[PATCH v2 12/12] 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 72914c47ec9a..ec56768983e4 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -2594,11 +2594,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