[PATCH v4 0/6] MT9M114 driver bugfix and improvements

[PATCH v4 0/6] MT9M114 driver bugfix and improvements

[Mathis Foerst]

Hi,

this patch series contains the following bugfix and improvements
for the MT9M114 camera driver:

Changelog:

v3 -> v4:
- Rename DT binding from "onnn,slew-rate" to "slew-rate" in PATCH 1 and 6 as
  requested in the review comment.

v2 -> v3:
- Dropped PATCH 2 ("media: mt9m114: Add get_mbus_config").
  Based on the comments, this issure won't be fixed in the MT9M114
  driver but in "imx-media-csi.c" in a separate patch.
- Renumbered patches accordingly.
- Fix the incomplete renaming of the DT property from 'pad-slew-rate'
  to 'onnn,slew-rate' in PATCH 1 and 6.
- Fix checkpatch formatting suggestions in PATCH 2 and 6.

v1 -> v2:
- Fix the subjects of the patches
- Dropped PATCH 1 ("Add bypass-pll DT-binding") as it can be automatically
  detected if the PLL should be bypassed.
- Renumbered patches accordingly
- Switch to uint32, add default value and clarify documentation in PATCH 1
- Add 'Fixes' and 'Cc' tags as suggested in PATCH 6

Link to v1 discussion:
https://lore.kernel.org/linux-media/20250226153929.274562-1-mathis.foerst@mt.com/
Link to v2 discussion:
https://lore.kernel.org/linux-media/20250304103647.34235-1-mathis.foerst@mt.com/
Link to v3 discussion:
https://lore.kernel.org/linux-media/20250305101453.708270-1-mathis.foerst@mt.com/


Bugfixes:
- Fix a deadlock when using the V4L2 pad-ops get/set_frame_interval

New Features:
- Bypass the internal PLL if EXTCLK matches the configured link_frequency
- Make the slew-rate of the output pads configurable via DT
- Allow to change the cropping configuration and the horizontal/vertical
  flipping while the sensor is in streaming state

Thanks,
Mathis


Mathis Foerst (6):
  media: dt-bindings: mt9m114: Add slew-rate DT-binding
  media: mt9m114: Bypass PLL if required
  media: mt9m114: Factor out mt9m114_configure_pa
  media: mt9m114: Allow set_selection while streaming
  media: mt9m114: Fix deadlock in get_frame_interval/set_frame_interval
  media: mt9m114: Set pad-slew-rate

 .../bindings/media/i2c/onnn,mt9m114.yaml      |   9 +
 drivers/media/i2c/mt9m114.c                   | 172 ++++++++++++------
 2 files changed, 130 insertions(+), 51 deletions(-)


base-commit: ac9c34d1e45a4c25174ced4fc0cfc33ff3ed08c7
-- 
2.34.1

[PATCH v4 1/6] media: dt-bindings: mt9m114: Add slew-rate DT-binding

[Mathis Foerst]

The MT9M114 supports the different slew rates (0 to 7) on the output pads.
At the moment, this is hardcoded to 7 (the fastest rate).
The user might want to change this values due to EMC requirements.

Add the 'slew-rate' property to the MT9M114 DT-bindings for selecting
the desired slew rate.

Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
---
 .../devicetree/bindings/media/i2c/onnn,mt9m114.yaml      | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
index f6b87892068a..a89f740214f7 100644
--- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
@@ -70,6 +70,15 @@ properties:
           - bus-type
           - link-frequencies
 
+  slew-rate:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and
+      PIXCLK. Higher values imply steeper voltage-flanks on the pads.
+    minimum: 0
+    maximum: 7
+    default: 7
+
 required:
   - compatible
   - reg
-- 
2.34.1

Re: [PATCH v4 1/6] media: dt-bindings: mt9m114: Add slew-rate DT-binding

[Krzysztof Kozlowski]

On Fri, Mar 07, 2025 at 10:31:35AM +0100, Mathis Foerst wrote:
> The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> At the moment, this is hardcoded to 7 (the fastest rate).
> The user might want to change this values due to EMC requirements.
> 
> Add the 'slew-rate' property to the MT9M114 DT-bindings for selecting
> the desired slew rate.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  .../devicetree/bindings/media/i2c/onnn,mt9m114.yaml      | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 1/6] media: dt-bindings: mt9m114: Add slew-rate DT-binding

[Laurent Pinchart]

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:35AM +0100, Mathis Foerst wrote:
> The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> At the moment, this is hardcoded to 7 (the fastest rate).
> The user might want to change this values due to EMC requirements.
> 
> Add the 'slew-rate' property to the MT9M114 DT-bindings for selecting
> the desired slew rate.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  .../devicetree/bindings/media/i2c/onnn,mt9m114.yaml      | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> index f6b87892068a..a89f740214f7 100644
> --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> @@ -70,6 +70,15 @@ properties:
>            - bus-type
>            - link-frequencies
>  
> +  slew-rate:

Shouldn't this be a vendor property, with a vendor prefix (i.e.
'onnn,slew-rate') ? There's a 'slew-rate' property defined in
Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml, but that's
for pin config nodes.

The rest looks good, so with this change (or without it if there's a
consensus we don't need a vendor prefix),

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

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and
> +      PIXCLK. Higher values imply steeper voltage-flanks on the pads.
> +    minimum: 0
> +    maximum: 7
> +    default: 7
> +
>  required:
>    - compatible
>    - reg

-- 
Regards,

Laurent Pinchart

[PATCH v4 2/6] media: mt9m114: Bypass PLL if required

[Mathis Foerst]

The MT9M114 sensor has an internal PLL that generates the required SYSCLK
from EXTCLK. It also has the option to bypass the PLL and use EXTCLK
directly as SYSCLK.
The current driver implementation uses a hardcoded PLL configuration that
requires a specific EXTCLK frequency. Depending on the available clocks,
it can be desirable to use a different PLL configuration or to bypass it.

The link-frequency of the output bus (Parallel or MIPI-CSI) is configured
in the device tree.

Check if EXTCLK can be used as SYSCLK to achieve this link-frequency. If
yes, bypass the PLL.
Otherwise, (as before) check if EXTCLK and the default PLL configuration
provide the required SYSCLK to achieve the link-frequency. If yes, use the
PLL. If no, throw an error.

Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
---
 drivers/media/i2c/mt9m114.c | 62 ++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 5f0b0ad8f885..b06003b69f6f 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -261,6 +261,7 @@
 #define MT9M114_CAM_PGA_PGA_CONTROL			CCI_REG16(0xc95e)
 #define MT9M114_CAM_SYSCTL_PLL_ENABLE			CCI_REG8(0xc97e)
 #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_P		CCI_REG16(0xc982)
@@ -377,6 +378,7 @@ struct mt9m114 {
 	struct gpio_desc *reset;
 	struct regulator_bulk_data supplies[3];
 	struct v4l2_fwnode_endpoint bus_cfg;
+	bool bypass_pll;
 
 	struct {
 		unsigned int m;
@@ -743,14 +745,20 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
 	}
 
 	/* Configure the PLL. */
-	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
-		  MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE, &ret);
-	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N,
-		  MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(sensor->pll.m,
-						       sensor->pll.n),
-		  &ret);
-	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P,
-		  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p), &ret);
+	if (sensor->bypass_pll) {
+		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
+			  MT9M114_CAM_SYSCTL_PLL_DISABLE_VALUE, &ret);
+	} else {
+		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
+			  MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE, &ret);
+		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N,
+			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(sensor->pll.m,
+							       sensor->pll.n),
+			  &ret);
+		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P,
+			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p),
+			  &ret);
+	}
 	cci_write(sensor->regmap, MT9M114_CAM_SENSOR_CFG_PIXCLK,
 		  sensor->pixrate, &ret);
 
@@ -2235,9 +2243,19 @@ static const struct dev_pm_ops mt9m114_pm_ops = {
  * Probe & Remove
  */
 
+static int mt9m114_verify_link_frequency(struct mt9m114 *sensor)
+{
+	unsigned int link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY
+				? sensor->pixrate * 8 : sensor->pixrate * 2;
+	if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
+	    sensor->bus_cfg.link_frequencies[0] != link_freq)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int mt9m114_clk_init(struct mt9m114 *sensor)
 {
-	unsigned int link_freq;
 
 	/* Hardcode the PLL multiplier and dividers to default settings. */
 	sensor->pll.m = 32;
@@ -2249,19 +2267,27 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
 	 * for 16-bit per pixel, transmitted in DDR over a single lane. For
 	 * parallel mode, the sensor ouputs one pixel in two PIXCLK cycles.
 	 */
-	sensor->pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
-			/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
 
-	link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY
-		  ? sensor->pixrate * 8 : sensor->pixrate * 2;
+	/*
+	 * Check if EXTCLK fits the configured link frequency. Bypass the PLL
+	 * in this case.
+	 */
+	sensor->pixrate = clk_get_rate(sensor->clk) / 2;
+	if (mt9m114_verify_link_frequency(sensor) == 0) {
+		sensor->bypass_pll = true;
+		return 0;
+	}
 
-	if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
-	    sensor->bus_cfg.link_frequencies[0] != link_freq) {
-		dev_err(&sensor->client->dev, "Unsupported DT link-frequencies\n");
-		return -EINVAL;
+	/* Check if the PLL configuration fits the configured link frequency */
+	sensor->pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
+			/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
+	if (mt9m114_verify_link_frequency(sensor) == 0) {
+		sensor->bypass_pll = false;
+		return 0;
 	}
 
-	return 0;
+	dev_err(&sensor->client->dev, "Unsupported DT link-frequencies\n");
+	return -EINVAL;
 }
 
 static int mt9m114_identify(struct mt9m114 *sensor)
-- 
2.34.1

Re: [PATCH v4 2/6] media: mt9m114: Bypass PLL if required

[Laurent Pinchart]

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:36AM +0100, Mathis Foerst wrote:
> The MT9M114 sensor has an internal PLL that generates the required SYSCLK
> from EXTCLK. It also has the option to bypass the PLL and use EXTCLK
> directly as SYSCLK.
> The current driver implementation uses a hardcoded PLL configuration that
> requires a specific EXTCLK frequency. Depending on the available clocks,
> it can be desirable to use a different PLL configuration or to bypass it.
> 
> The link-frequency of the output bus (Parallel or MIPI-CSI) is configured
> in the device tree.
> 
> Check if EXTCLK can be used as SYSCLK to achieve this link-frequency. If
> yes, bypass the PLL.
> Otherwise, (as before) check if EXTCLK and the default PLL configuration
> provide the required SYSCLK to achieve the link-frequency. If yes, use the
> PLL. If no, throw an error.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  drivers/media/i2c/mt9m114.c | 62 ++++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 5f0b0ad8f885..b06003b69f6f 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -261,6 +261,7 @@
>  #define MT9M114_CAM_PGA_PGA_CONTROL			CCI_REG16(0xc95e)
>  #define MT9M114_CAM_SYSCTL_PLL_ENABLE			CCI_REG8(0xc97e)
>  #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_P		CCI_REG16(0xc982)
> @@ -377,6 +378,7 @@ struct mt9m114 {
>  	struct gpio_desc *reset;
>  	struct regulator_bulk_data supplies[3];
>  	struct v4l2_fwnode_endpoint bus_cfg;
> +	bool bypass_pll;
>  
>  	struct {
>  		unsigned int m;
> @@ -743,14 +745,20 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>  	}
>  
>  	/* Configure the PLL. */
> -	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> -		  MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE, &ret);
> -	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N,
> -		  MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(sensor->pll.m,
> -						       sensor->pll.n),
> -		  &ret);
> -	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P,
> -		  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p), &ret);
> +	if (sensor->bypass_pll) {
> +		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> +			  MT9M114_CAM_SYSCTL_PLL_DISABLE_VALUE, &ret);
> +	} else {
> +		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> +			  MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE, &ret);
> +		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N,
> +			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(sensor->pll.m,
> +							       sensor->pll.n),
> +			  &ret);
> +		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P,
> +			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p),
> +			  &ret);
> +	}

You can add a blank line here.

>  	cci_write(sensor->regmap, MT9M114_CAM_SENSOR_CFG_PIXCLK,
>  		  sensor->pixrate, &ret);
>  
> @@ -2235,9 +2243,19 @@ static const struct dev_pm_ops mt9m114_pm_ops = {
>   * Probe & Remove
>   */
>  
> +static int mt9m114_verify_link_frequency(struct mt9m114 *sensor)
> +{
> +	unsigned int link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY
> +				? sensor->pixrate * 8 : sensor->pixrate * 2;

			       ? sensor->pixrate * 8 : sensor->pixrate * 2;

And missing blank line.

> +	if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> +	    sensor->bus_cfg.link_frequencies[0] != link_freq)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int mt9m114_clk_init(struct mt9m114 *sensor)
>  {
> -	unsigned int link_freq;
>  
>  	/* Hardcode the PLL multiplier and dividers to default settings. */
>  	sensor->pll.m = 32;
> @@ -2249,19 +2267,27 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
>  	 * for 16-bit per pixel, transmitted in DDR over a single lane. For
>  	 * parallel mode, the sensor ouputs one pixel in two PIXCLK cycles.
>  	 */
> -	sensor->pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
> -			/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
>  
> -	link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY
> -		  ? sensor->pixrate * 8 : sensor->pixrate * 2;
> +	/*
> +	 * Check if EXTCLK fits the configured link frequency. Bypass the PLL
> +	 * in this case.
> +	 */
> +	sensor->pixrate = clk_get_rate(sensor->clk) / 2;
> +	if (mt9m114_verify_link_frequency(sensor) == 0) {

I would be cleaner to pass the pixel rate as a parameter to the
function:

	unsigned int pixrate;

	...

	pixrate = clk_get_rate(sensor->clk) / 2;
	if (mt9m114_verify_link_frequency(sensor, pixrate) == 0) {
		sensor->pixrate = pixrate;
		sensor->bypass_pll = true;
		return 0;
	}

> +		sensor->bypass_pll = true;
> +		return 0;
> +	}
>  
> -	if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> -	    sensor->bus_cfg.link_frequencies[0] != link_freq) {
> -		dev_err(&sensor->client->dev, "Unsupported DT link-frequencies\n");
> -		return -EINVAL;
> +	/* Check if the PLL configuration fits the configured link frequency */

s/frequency/frequency./

With those small issues addressed,

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

> +	sensor->pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
> +			/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
> +	if (mt9m114_verify_link_frequency(sensor) == 0) {
> +		sensor->bypass_pll = false;
> +		return 0;
>  	}
>  
> -	return 0;
> +	dev_err(&sensor->client->dev, "Unsupported DT link-frequencies\n");
> +	return -EINVAL;
>  }
>  
>  static int mt9m114_identify(struct mt9m114 *sensor)

-- 
Regards,

Laurent Pinchart

[PATCH v4 3/6] media: mt9m114: Factor out mt9m114_configure_pa

[Mathis Foerst]

The function mt9m114_configure writes the configuration registers of both,
the pixel array (pa) and the image flow processor (ifp).
This is undesirable if only the config of the pa should be changed without
affecting the ifp.

Factor out the function mt9m114_configure_pa() that just writes the
pa-configuration.

Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
---
 drivers/media/i2c/mt9m114.c | 49 +++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index b06003b69f6f..9a49dab65180 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -789,39 +789,22 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
 	return 0;
 }
 
-static int mt9m114_configure(struct mt9m114 *sensor,
-			     struct v4l2_subdev_state *pa_state,
-			     struct v4l2_subdev_state *ifp_state)
+static int mt9m114_configure_pa(struct mt9m114 *sensor, struct v4l2_subdev_state *pa_state)
 {
 	const struct v4l2_mbus_framefmt *pa_format;
 	const struct v4l2_rect *pa_crop;
-	const struct mt9m114_format_info *ifp_info;
-	const struct v4l2_mbus_framefmt *ifp_format;
-	const struct v4l2_rect *ifp_crop;
-	const struct v4l2_rect *ifp_compose;
-	unsigned int hratio, vratio;
-	u64 output_format;
 	u64 read_mode;
+	unsigned int hratio, vratio;
 	int ret = 0;
 
 	pa_format = v4l2_subdev_state_get_format(pa_state, 0);
 	pa_crop = v4l2_subdev_state_get_crop(pa_state, 0);
 
-	ifp_format = v4l2_subdev_state_get_format(ifp_state, 1);
-	ifp_info = mt9m114_format_info(sensor, 1, ifp_format->code);
-	ifp_crop = v4l2_subdev_state_get_crop(ifp_state, 0);
-	ifp_compose = v4l2_subdev_state_get_compose(ifp_state, 0);
-
 	ret = cci_read(sensor->regmap, MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
 		       &read_mode, NULL);
 	if (ret < 0)
 		return ret;
 
-	ret = cci_read(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
-		       &output_format, NULL);
-	if (ret < 0)
-		return ret;
-
 	hratio = pa_crop->width / pa_format->width;
 	vratio = pa_crop->height / pa_format->height;
 
@@ -853,6 +836,34 @@ static int mt9m114_configure(struct mt9m114 *sensor,
 	cci_write(sensor->regmap, MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
 		  read_mode, &ret);
 
+	return ret;
+}
+
+static int mt9m114_configure(struct mt9m114 *sensor,
+			     struct v4l2_subdev_state *pa_state,
+			     struct v4l2_subdev_state *ifp_state)
+{
+	const struct mt9m114_format_info *ifp_info;
+	const struct v4l2_mbus_framefmt *ifp_format;
+	const struct v4l2_rect *ifp_crop;
+	const struct v4l2_rect *ifp_compose;
+	u64 output_format;
+	int ret = 0;
+
+	ifp_format = v4l2_subdev_state_get_format(ifp_state, 1);
+	ifp_info = mt9m114_format_info(sensor, 1, ifp_format->code);
+	ifp_crop = v4l2_subdev_state_get_crop(ifp_state, 0);
+	ifp_compose = v4l2_subdev_state_get_compose(ifp_state, 0);
+
+	ret = cci_read(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
+		       &output_format, NULL);
+	if (ret < 0)
+		return ret;
+
+	ret = mt9m114_configure_pa(sensor, pa_state);
+	if (ret < 0)
+		return ret;
+
 	/*
 	 * Color pipeline (IFP) cropping and scaling. Subtract 4 from the left
 	 * and top coordinates to compensate for the lines and columns removed
-- 
2.34.1

Re: [PATCH v4 3/6] media: mt9m114: Factor out mt9m114_configure_pa

[Laurent Pinchart]

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:37AM +0100, Mathis Foerst wrote:
> The function mt9m114_configure writes the configuration registers of both,
> the pixel array (pa) and the image flow processor (ifp).
> This is undesirable if only the config of the pa should be changed without
> affecting the ifp.

I assume I'll see in a later patch why this is needed.

> Factor out the function mt9m114_configure_pa() that just writes the
> pa-configuration.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  drivers/media/i2c/mt9m114.c | 49 +++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index b06003b69f6f..9a49dab65180 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -789,39 +789,22 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>  	return 0;
>  }
>  
> -static int mt9m114_configure(struct mt9m114 *sensor,
> -			     struct v4l2_subdev_state *pa_state,
> -			     struct v4l2_subdev_state *ifp_state)
> +static int mt9m114_configure_pa(struct mt9m114 *sensor, struct v4l2_subdev_state *pa_state)

You can name the variable 'state' now that there's a single one.

static int mt9m114_configure_pa(struct mt9m114 *sensor,
				struct v4l2_subdev_state *state)

>  {
>  	const struct v4l2_mbus_framefmt *pa_format;
>  	const struct v4l2_rect *pa_crop;

Similarly, these can be renamed to format and crop.

> -	const struct mt9m114_format_info *ifp_info;
> -	const struct v4l2_mbus_framefmt *ifp_format;
> -	const struct v4l2_rect *ifp_crop;
> -	const struct v4l2_rect *ifp_compose;
> -	unsigned int hratio, vratio;
> -	u64 output_format;
>  	u64 read_mode;
> +	unsigned int hratio, vratio;

You can keep the order of the variables unchanged.

>  	int ret = 0;
>  
>  	pa_format = v4l2_subdev_state_get_format(pa_state, 0);
>  	pa_crop = v4l2_subdev_state_get_crop(pa_state, 0);
>  
> -	ifp_format = v4l2_subdev_state_get_format(ifp_state, 1);
> -	ifp_info = mt9m114_format_info(sensor, 1, ifp_format->code);
> -	ifp_crop = v4l2_subdev_state_get_crop(ifp_state, 0);
> -	ifp_compose = v4l2_subdev_state_get_compose(ifp_state, 0);
> -
>  	ret = cci_read(sensor->regmap, MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
>  		       &read_mode, NULL);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = cci_read(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
> -		       &output_format, NULL);
> -	if (ret < 0)
> -		return ret;
> -
>  	hratio = pa_crop->width / pa_format->width;
>  	vratio = pa_crop->height / pa_format->height;
>  
> @@ -853,6 +836,34 @@ static int mt9m114_configure(struct mt9m114 *sensor,
>  	cci_write(sensor->regmap, MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
>  		  read_mode, &ret);
>  
> +	return ret;
> +}
> +
> +static int mt9m114_configure(struct mt9m114 *sensor,
> +			     struct v4l2_subdev_state *pa_state,
> +			     struct v4l2_subdev_state *ifp_state)
> +{
> +	const struct mt9m114_format_info *ifp_info;
> +	const struct v4l2_mbus_framefmt *ifp_format;
> +	const struct v4l2_rect *ifp_crop;
> +	const struct v4l2_rect *ifp_compose;

And here you can also drop the ifp_ prefix.

> +	u64 output_format;
> +	int ret = 0;

No need to initialize ret to 0.

> +
> +	ifp_format = v4l2_subdev_state_get_format(ifp_state, 1);
> +	ifp_info = mt9m114_format_info(sensor, 1, ifp_format->code);
> +	ifp_crop = v4l2_subdev_state_get_crop(ifp_state, 0);
> +	ifp_compose = v4l2_subdev_state_get_compose(ifp_state, 0);
> +
> +	ret = cci_read(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
> +		       &output_format, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = mt9m114_configure_pa(sensor, pa_state);
> +	if (ret < 0)
> +		return ret;
> +
>  	/*
>  	 * Color pipeline (IFP) cropping and scaling. Subtract 4 from the left
>  	 * and top coordinates to compensate for the lines and columns removed

For symmetry, could you call this mt9m114_configure_ifp() and move the
call to mt9m114_configure_pa() to mt9m114_start_streaming() ?

With those issues addressed, and assuming that the rationale for this
patch is good,

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

-- 
Regards,

Laurent Pinchart

[PATCH v4 4/6] media: mt9m114: Allow set_selection while streaming

[Mathis Foerst]

The current implementation does not apply changes to the crop-
configuration of the sensor immediately if the sensor is in
streaming state. The user has to stop and restart the stream for
the changes to be applied.
The same holds for changes to the V4L2 controls HFLIP & VFLIP.
This can be undesirable e.g. in a calibration usecase where the user
wants to see the impact of his changes in a live video stream.

Call mt9m114_configure_pa() in mt9m114_pa_set_selection() if the sensor is
in streaming state and issue a CONFIG_CHANGE to apply the changes
immediately.
Issue a CONFIG_CHANGE when the V4L2 controls HFLIP or VFLIP are set if the
sensor is in streaming state to apply the change immediately.

Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
---
 drivers/media/i2c/mt9m114.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 9a49dab65180..65b9124e464f 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1098,6 +1098,13 @@ static int mt9m114_pa_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = cci_update_bits(sensor->regmap,
 				      MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
 				      mask, ctrl->val ? mask : 0, NULL);
+		if (ret)
+			return ret;
+		if (sensor->streaming) {
+			// Changing the flip config while streaming requires a CONFIG_CHANGE
+			ret = mt9m114_set_state(sensor,
+						MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
+		}
 		break;
 
 	case V4L2_CID_VFLIP:
@@ -1105,6 +1112,13 @@ static int mt9m114_pa_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = cci_update_bits(sensor->regmap,
 				      MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
 				      mask, ctrl->val ? mask : 0, NULL);
+		if (ret)
+			return ret;
+		if (sensor->streaming) {
+			// Changing the flip config while streaming requires a CONFIG_CHANGE
+			ret = mt9m114_set_state(sensor,
+						MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
+		}
 		break;
 
 	default:
@@ -1286,6 +1300,7 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
 	struct mt9m114 *sensor = pa_to_mt9m114(sd);
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
+	int ret = 0;
 
 	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
@@ -1316,10 +1331,21 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
 	format->width = crop->width;
 	format->height = crop->height;
 
-	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		mt9m114_pa_ctrl_update_blanking(sensor, format);
+	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+		return ret;
 
-	return 0;
+	mt9m114_pa_ctrl_update_blanking(sensor, format);
+
+	/* Apply values immediately if streaming */
+	if (sensor->streaming) {
+		ret = mt9m114_configure_pa(sensor, state);
+		if (ret)
+			return ret;
+		// Changing the cropping config requires a CONFIG_CHANGE
+		ret = mt9m114_set_state(sensor,	MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
+	}
+
+	return ret;
 }
 
 static const struct v4l2_subdev_pad_ops mt9m114_pa_pad_ops = {
-- 
2.34.1

Re: [PATCH v4 4/6] media: mt9m114: Allow set_selection while streaming

[Laurent Pinchart]

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:38AM +0100, Mathis Foerst wrote:
> The current implementation does not apply changes to the crop-
> configuration of the sensor immediately if the sensor is in
> streaming state. The user has to stop and restart the stream for
> the changes to be applied.
> The same holds for changes to the V4L2 controls HFLIP & VFLIP.

Could you please split this in two patches, one for flip, abd one for
crop ? From flipping I think it's just a driver bug that I forgot to
issue a CONFIG_CHANGE, while for cropping it was by design. The commit
message for the crop patch can explain why this has to change, while the
commit message for the flip patch can just explains it's fixing a bug.
This will also reflect all the changes in the commit subjects, while
here the subject only mentions set_selection and hides the flip change.

> This can be undesirable e.g. in a calibration usecase where the user
> wants to see the impact of his changes in a live video stream.
> 
> Call mt9m114_configure_pa() in mt9m114_pa_set_selection() if the sensor is
> in streaming state and issue a CONFIG_CHANGE to apply the changes
> immediately.
> Issue a CONFIG_CHANGE when the V4L2 controls HFLIP or VFLIP are set if the
> sensor is in streaming state to apply the change immediately.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  drivers/media/i2c/mt9m114.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 9a49dab65180..65b9124e464f 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1098,6 +1098,13 @@ static int mt9m114_pa_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = cci_update_bits(sensor->regmap,
>  				      MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
>  				      mask, ctrl->val ? mask : 0, NULL);
> +		if (ret)
> +			return ret;

Add a blank line.

But it's a bug, you'll leak a PM reference count. You need to break
instead.

> +		if (sensor->streaming) {
> +			// Changing the flip config while streaming requires a CONFIG_CHANGE

C-style comments please, and reflow at 80 columns. Or possibly better,
you could copy the code from mt9m114_ifp_s_ctrl() for consistency:


		/*
		 * A Config-Change needs to be issued for the change to take
		 * effect. If we're not streaming ignore this, the change will
		 * be applied when the stream is started.
		 */
		if (ret || !sensor->streaming)
			break;

		ret = mt9m114_set_state(sensor,
					MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);

Same comments for V4L2_CID_VFLIP.

> +			ret = mt9m114_set_state(sensor,
> +						MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);

As this can be an expensive operation, I think the hflip and vflip
controls should be put in a cluster, to be able to change them both in
one go.

> +		}
>  		break;
>  
>  	case V4L2_CID_VFLIP:
> @@ -1105,6 +1112,13 @@ static int mt9m114_pa_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = cci_update_bits(sensor->regmap,
>  				      MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
>  				      mask, ctrl->val ? mask : 0, NULL);
> +		if (ret)
> +			return ret;
> +		if (sensor->streaming) {
> +			// Changing the flip config while streaming requires a CONFIG_CHANGE
> +			ret = mt9m114_set_state(sensor,
> +						MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> +		}
>  		break;
>  
>  	default:
> @@ -1286,6 +1300,7 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
>  	struct mt9m114 *sensor = pa_to_mt9m114(sd);
>  	struct v4l2_mbus_framefmt *format;
>  	struct v4l2_rect *crop;
> +	int ret = 0;
>  
>  	if (sel->target != V4L2_SEL_TGT_CROP)
>  		return -EINVAL;
> @@ -1316,10 +1331,21 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
>  	format->width = crop->width;
>  	format->height = crop->height;
>  
> -	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		mt9m114_pa_ctrl_update_blanking(sensor, format);
> +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> +		return ret;
>  
> -	return 0;
> +	mt9m114_pa_ctrl_update_blanking(sensor, format);
> +
> +	/* Apply values immediately if streaming */

Changing the crop rectangle modifies the output size of the PA. It will
not match the size the IFP expects at its input anymore. Could you
please explain how this will work and why it won't cause issues ?

> +	if (sensor->streaming) {
> +		ret = mt9m114_configure_pa(sensor, state);
> +		if (ret)
> +			return ret;
> +		// Changing the cropping config requires a CONFIG_CHANGE
> +		ret = mt9m114_set_state(sensor,	MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> +	}
> +
> +	return ret;
>  }
>  
>  static const struct v4l2_subdev_pad_ops mt9m114_pa_pad_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/6] media: mt9m114: Allow set_selection while streaming

[Mathis Foerst]

Hi Laurent,

thanks a lot for your review.

On Wed, Apr 23, 2025 at 09:39:00PM +0300, Laurent Pinchart wrote:
> Hi Mathis,
> 
> Thank you for the patch.
> 
> On Fri, Mar 07, 2025 at 10:31:38AM +0100, Mathis Foerst wrote:
> > The current implementation does not apply changes to the crop-
> > configuration of the sensor immediately if the sensor is in
> > streaming state. The user has to stop and restart the stream for
> > the changes to be applied.
> > The same holds for changes to the V4L2 controls HFLIP & VFLIP.
> 
> Could you please split this in two patches, one for flip, abd one for
> crop ? From flipping I think it's just a driver bug that I forgot to
> issue a CONFIG_CHANGE, while for cropping it was by design. The commit
> message for the crop patch can explain why this has to change, while the
> commit message for the flip patch can just explains it's fixing a bug.
> This will also reflect all the changes in the commit subjects, while
> here the subject only mentions set_selection and hides the flip change.

As suggested, I split the changes into two patches.

> 
> > This can be undesirable e.g. in a calibration usecase where the user
> > wants to see the impact of his changes in a live video stream.
> > 
> > Call mt9m114_configure_pa() in mt9m114_pa_set_selection() if the sensor is
> > in streaming state and issue a CONFIG_CHANGE to apply the changes
> > immediately.
> > Issue a CONFIG_CHANGE when the V4L2 controls HFLIP or VFLIP are set if the
> > sensor is in streaming state to apply the change immediately.
> > 
> > Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> > ---
> >  drivers/media/i2c/mt9m114.c | 32 +++++++++++++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> > index 9a49dab65180..65b9124e464f 100644
> > --- a/drivers/media/i2c/mt9m114.c
> > +++ b/drivers/media/i2c/mt9m114.c
> > @@ -1098,6 +1098,13 @@ static int mt9m114_pa_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		ret = cci_update_bits(sensor->regmap,
> >  				      MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
> >  				      mask, ctrl->val ? mask : 0, NULL);
> > +		if (ret)
> > +			return ret;
> 
> Add a blank line.
> 
> But it's a bug, you'll leak a PM reference count. You need to break
> instead.
> 
> > +		if (sensor->streaming) {
> > +			// Changing the flip config while streaming requires a CONFIG_CHANGE
> 
> C-style comments please, and reflow at 80 columns. Or possibly better,
> you could copy the code from mt9m114_ifp_s_ctrl() for consistency:
> 
> 
> 		/*
> 		 * A Config-Change needs to be issued for the change to take
> 		 * effect. If we're not streaming ignore this, the change will
> 		 * be applied when the stream is started.
> 		 */
> 		if (ret || !sensor->streaming)
> 			break;
> 
> 		ret = mt9m114_set_state(sensor,
> 					MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> 
> Same comments for V4L2_CID_VFLIP.
> 
> > +			ret = mt9m114_set_state(sensor,
> > +						MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> 
> As this can be an expensive operation, I think the hflip and vflip
> controls should be put in a cluster, to be able to change them both in
> one go.
> 
> > +		}
> >  		break;
> >  
> >  	case V4L2_CID_VFLIP:
> > @@ -1105,6 +1112,13 @@ static int mt9m114_pa_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		ret = cci_update_bits(sensor->regmap,
> >  				      MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
> >  				      mask, ctrl->val ? mask : 0, NULL);
> > +		if (ret)
> > +			return ret;
> > +		if (sensor->streaming) {
> > +			// Changing the flip config while streaming requires a CONFIG_CHANGE
> > +			ret = mt9m114_set_state(sensor,
> > +						MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> > +		}
> >  		break;
> >  
> >  	default:
> > @@ -1286,6 +1300,7 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
> >  	struct mt9m114 *sensor = pa_to_mt9m114(sd);
> >  	struct v4l2_mbus_framefmt *format;
> >  	struct v4l2_rect *crop;
> > +	int ret = 0;
> >  
> >  	if (sel->target != V4L2_SEL_TGT_CROP)
> >  		return -EINVAL;
> > @@ -1316,10 +1331,21 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
> >  	format->width = crop->width;
> >  	format->height = crop->height;
> >  
> > -	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		mt9m114_pa_ctrl_update_blanking(sensor, format);
> > +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > +		return ret;
> >  
> > -	return 0;
> > +	mt9m114_pa_ctrl_update_blanking(sensor, format);
> > +
> > +	/* Apply values immediately if streaming */
> 
> Changing the crop rectangle modifies the output size of the PA. It will
> not match the size the IFP expects at its input anymore. Could you
> please explain how this will work and why it won't cause issues ?

That's a good point. I implicitly assumed that the user would only change the location,
but not the size of the cropping rectangle.
Of course, the IFP is not happy, if the image size changes on the fly, but moving the
cropping rectangle around without changing its size does not affect the IFP.
I adjusted the patch to only call mt9m114_configure_pa() if the siez of the cropping
rectangle didn't change.

> 
> > +	if (sensor->streaming) {
> > +		ret = mt9m114_configure_pa(sensor, state);
> > +		if (ret)
> > +			return ret;
> > +		// Changing the cropping config requires a CONFIG_CHANGE
> > +		ret = mt9m114_set_state(sensor,	MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> > +	}
> > +
> > +	return ret;
> >  }
> >  
> >  static const struct v4l2_subdev_pad_ops mt9m114_pa_pad_ops = {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Best regards,
Mathis Foerst

[PATCH v4 5/6] media: mt9m114: Fix deadlock in get_frame_interval/set_frame_interval

[Mathis Foerst]

Getting / Setting the frame interval using the V4L2 subdev pad ops
get_frame_interval/set_frame_interval causes a deadlock, as the
subdev state is locked in the [1] but also in the driver itself.

In [2] it's described that the caller is responsible to acquire and
release the lock in this case. Therefore, acquiring the lock in the
driver is wrong.

Remove the lock acquisitions/releases from mt9m114_ifp_get_frame_interval()
and mt9m114_ifp_set_frame_interval().

[1] drivers/media/v4l2-core/v4l2-subdev.c - line 1129
[2] Documentation/driver-api/media/v4l2-subdev.rst

Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor")
Cc: stable@vger.kernel.org

Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
---
 drivers/media/i2c/mt9m114.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 65b9124e464f..79c97ab19be9 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1644,13 +1644,9 @@ static int mt9m114_ifp_get_frame_interval(struct v4l2_subdev *sd,
 	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
 		return -EINVAL;
 
-	mutex_lock(sensor->ifp.hdl.lock);
-
 	ival->numerator = 1;
 	ival->denominator = sensor->ifp.frame_rate;
 
-	mutex_unlock(sensor->ifp.hdl.lock);
-
 	return 0;
 }
 
@@ -1669,8 +1665,6 @@ static int mt9m114_ifp_set_frame_interval(struct v4l2_subdev *sd,
 	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
 		return -EINVAL;
 
-	mutex_lock(sensor->ifp.hdl.lock);
-
 	if (ival->numerator != 0 && ival->denominator != 0)
 		sensor->ifp.frame_rate = min_t(unsigned int,
 					       ival->denominator / ival->numerator,
@@ -1684,8 +1678,6 @@ static int mt9m114_ifp_set_frame_interval(struct v4l2_subdev *sd,
 	if (sensor->streaming)
 		ret = mt9m114_set_frame_rate(sensor);
 
-	mutex_unlock(sensor->ifp.hdl.lock);
-
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH v4 5/6] media: mt9m114: Fix deadlock in get_frame_interval/set_frame_interval

[Laurent Pinchart]

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:39AM +0100, Mathis Foerst wrote:
> Getting / Setting the frame interval using the V4L2 subdev pad ops
> get_frame_interval/set_frame_interval causes a deadlock, as the
> subdev state is locked in the [1] but also in the driver itself.
> 
> In [2] it's described that the caller is responsible to acquire and
> release the lock in this case. Therefore, acquiring the lock in the
> driver is wrong.
> 
> Remove the lock acquisitions/releases from mt9m114_ifp_get_frame_interval()
> and mt9m114_ifp_set_frame_interval().
> 
> [1] drivers/media/v4l2-core/v4l2-subdev.c - line 1129
> [2] Documentation/driver-api/media/v4l2-subdev.rst
> 
> Fixes: 24d756e914fc ("media: i2c: Add driver for onsemi MT9M114 camera sensor")
> Cc: stable@vger.kernel.org
> 

You can delete this blank line.

> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>

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

> ---
>  drivers/media/i2c/mt9m114.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 65b9124e464f..79c97ab19be9 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1644,13 +1644,9 @@ static int mt9m114_ifp_get_frame_interval(struct v4l2_subdev *sd,
>  	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return -EINVAL;
>  
> -	mutex_lock(sensor->ifp.hdl.lock);
> -
>  	ival->numerator = 1;
>  	ival->denominator = sensor->ifp.frame_rate;
>  
> -	mutex_unlock(sensor->ifp.hdl.lock);
> -
>  	return 0;
>  }
>  
> @@ -1669,8 +1665,6 @@ static int mt9m114_ifp_set_frame_interval(struct v4l2_subdev *sd,
>  	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return -EINVAL;
>  
> -	mutex_lock(sensor->ifp.hdl.lock);
> -
>  	if (ival->numerator != 0 && ival->denominator != 0)
>  		sensor->ifp.frame_rate = min_t(unsigned int,
>  					       ival->denominator / ival->numerator,
> @@ -1684,8 +1678,6 @@ static int mt9m114_ifp_set_frame_interval(struct v4l2_subdev *sd,
>  	if (sensor->streaming)
>  		ret = mt9m114_set_frame_rate(sensor);
>  
> -	mutex_unlock(sensor->ifp.hdl.lock);
> -
>  	return ret;
>  }
>  

-- 
Regards,

Laurent Pinchart

[PATCH v4 6/6] media: mt9m114: Set pad-slew-rate

[Mathis Foerst]

The MT9M114 supports the different slew rates (0 to 7) on the output pads.
At the moment, this is hardcoded to 7 (the fastest rate).
The user might want to change this values due to EMC requirements.

Read the 'slew-rate' from the DT and configure the pad slew rates of
the output pads accordingly in mt9m114_initialize().
Remove the hardcoded slew rate setting from the mt9m114_init table.

Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
---
 drivers/media/i2c/mt9m114.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 79c97ab19be9..fce24c587782 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -42,6 +42,9 @@
 #define MT9M114_RESET_AND_MISC_CONTROL			CCI_REG16(0x001a)
 #define MT9M114_RESET_SOC					BIT(0)
 #define MT9M114_PAD_SLEW				CCI_REG16(0x001e)
+#define MT9M114_PAD_SLEW_MIN					0x00
+#define MT9M114_PAD_SLEW_MAX					0x07
+#define MT9M114_PAD_SLEW_DEFAULT				0x07
 #define MT9M114_PAD_CONTROL				CCI_REG16(0x0032)
 
 /* XDMA registers */
@@ -388,6 +391,7 @@ struct mt9m114 {
 
 	unsigned int pixrate;
 	bool streaming;
+	u32 pad_slew_rate;
 
 	/* Pixel Array */
 	struct {
@@ -645,9 +649,6 @@ static const struct cci_reg_sequence mt9m114_init[] = {
 	{ MT9M114_CAM_SENSOR_CFG_FINE_INTEG_TIME_MAX,	1459 },
 	{ MT9M114_CAM_SENSOR_CFG_FINE_CORRECTION,	96 },
 	{ MT9M114_CAM_SENSOR_CFG_REG_0_DATA,		32 },
-
-	/* Miscellaneous settings */
-	{ MT9M114_PAD_SLEW,				0x0777 },
 };
 
 /* -----------------------------------------------------------------------------
@@ -778,6 +779,13 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
 	if (ret < 0)
 		return ret;
 
+	value = (sensor->pad_slew_rate & 0xF)
+	      | (sensor->pad_slew_rate & 0xF) << 4
+	      |	(sensor->pad_slew_rate & 0xF) << 8;
+	cci_write(sensor->regmap, MT9M114_PAD_SLEW, value, &ret);
+	if (ret < 0)
+		return ret;
+
 	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
 	if (ret < 0)
 		return ret;
@@ -2357,6 +2365,8 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
 {
 	struct fwnode_handle *fwnode = dev_fwnode(&sensor->client->dev);
 	struct fwnode_handle *ep;
+	struct device_node *dev_node = sensor->client->dev.of_node;
+	u32 slew_rate;
 	int ret;
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
@@ -2385,6 +2395,11 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
 		goto error;
 	}
 
+	ret = of_property_read_u32(dev_node, "slew-rate", &slew_rate);
+	if (ret || slew_rate < MT9M114_PAD_SLEW_MIN || slew_rate > MT9M114_PAD_SLEW_MAX)
+		slew_rate = MT9M114_PAD_SLEW_DEFAULT;
+	sensor->pad_slew_rate = slew_rate;
+
 	return 0;
 
 error:
-- 
2.34.1

Re: [PATCH v4 6/6] media: mt9m114: Set pad-slew-rate

[Laurent Pinchart]

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:40AM +0100, Mathis Foerst wrote:
> The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> At the moment, this is hardcoded to 7 (the fastest rate).
> The user might want to change this values due to EMC requirements.
> 
> Read the 'slew-rate' from the DT and configure the pad slew rates of
> the output pads accordingly in mt9m114_initialize().
> Remove the hardcoded slew rate setting from the mt9m114_init table.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  drivers/media/i2c/mt9m114.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 79c97ab19be9..fce24c587782 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -42,6 +42,9 @@
>  #define MT9M114_RESET_AND_MISC_CONTROL			CCI_REG16(0x001a)
>  #define MT9M114_RESET_SOC					BIT(0)
>  #define MT9M114_PAD_SLEW				CCI_REG16(0x001e)
> +#define MT9M114_PAD_SLEW_MIN					0x00
> +#define MT9M114_PAD_SLEW_MAX					0x07
> +#define MT9M114_PAD_SLEW_DEFAULT				0x07

You can use decimal values here.

>  #define MT9M114_PAD_CONTROL				CCI_REG16(0x0032)
>  
>  /* XDMA registers */
> @@ -388,6 +391,7 @@ struct mt9m114 {
>  
>  	unsigned int pixrate;
>  	bool streaming;
> +	u32 pad_slew_rate;
>  
>  	/* Pixel Array */
>  	struct {
> @@ -645,9 +649,6 @@ static const struct cci_reg_sequence mt9m114_init[] = {
>  	{ MT9M114_CAM_SENSOR_CFG_FINE_INTEG_TIME_MAX,	1459 },
>  	{ MT9M114_CAM_SENSOR_CFG_FINE_CORRECTION,	96 },
>  	{ MT9M114_CAM_SENSOR_CFG_REG_0_DATA,		32 },
> -
> -	/* Miscellaneous settings */
> -	{ MT9M114_PAD_SLEW,				0x0777 },
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -778,6 +779,13 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>  	if (ret < 0)
>  		return ret;
>  
> +	value = (sensor->pad_slew_rate & 0xF)
> +	      | (sensor->pad_slew_rate & 0xF) << 4
> +	      |	(sensor->pad_slew_rate & 0xF) << 8;

No need for ' & 0xF' as you've ensured the slew rate value is in the
valid [0, 7] range.

> +	cci_write(sensor->regmap, MT9M114_PAD_SLEW, value, &ret);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = mt9m114_set_state(sensor, MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
>  	if (ret < 0)
>  		return ret;
> @@ -2357,6 +2365,8 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
>  {
>  	struct fwnode_handle *fwnode = dev_fwnode(&sensor->client->dev);
>  	struct fwnode_handle *ep;
> +	struct device_node *dev_node = sensor->client->dev.of_node;
> +	u32 slew_rate;
>  	int ret;
>  
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> @@ -2385,6 +2395,11 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
>  		goto error;
>  	}
>  
> +	ret = of_property_read_u32(dev_node, "slew-rate", &slew_rate);

Direct usage of OF functions is discouraged. Use
device_property_read_u32() instead, which abstracts the firmware backend
(OF, ACPI, ...). Don't forget to include linux/property.h.

> +	if (ret || slew_rate < MT9M114_PAD_SLEW_MIN || slew_rate > MT9M114_PAD_SLEW_MAX)
> +		slew_rate = MT9M114_PAD_SLEW_DEFAULT;

If the value is erroneous, it indicates the DT is incorrect. I'd log a
message and return an error. As the DT property is optional, you can do
something like

	sensor->slew_rate = MT9M114_PAD_SLEW_DEFAULT;
	device_property_read_u32(&sensor->client.dev, "slew-rate",
				 &sensor->slew_rate);

	if (sensor->slew_rate < MT9M114_PAD_SLEW_MIN ||
	    sensor->slew_rate > MT9M114_PAD_SLEW_MAX) {
	    	dev_err(&sensor->client.dev, "Invalid slew-rate %u\n",
			sensor->slew_rate);
		return -EINVAL;
	}

With this,

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

> +	sensor->pad_slew_rate = slew_rate;
> +
>  	return 0;
>  
>  error:

-- 
Regards,

Laurent Pinchart