[PATCH v1 0/8] MT9M114 driver bugfix and improvements

[PATCH v1 0/8] MT9M114 driver bugfix and improvements

[Mathis Foerst]

Hi,

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

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

Compatibility:
- Implement the missing get_mbus_config() function to be compatible
  with the i.MX6 camera framework

New Features:
- Allow to bypass the internal PLL (configurable via DT)
- 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 (8):
  MT9M114: Add bypass-pll DT-binding
  MT9M114: Add pad-slew-rate DT-binding
  MT9M114: Add get_mbus_config
  MT9M114: Add option to bypass PLL
  MT9M114: Factor out mt9m114_configure_pa
  MT9M114: Allow set_selection while streaming
  MT9M114: Fix deadlock in get_frame_interval/set_frame_interval
  MT9M114: Set pad-slew-rate

 .../bindings/media/i2c/onnn,mt9m114.yaml      |  10 ++
 drivers/media/i2c/mt9m114.c                   | 161 +++++++++++++-----
 2 files changed, 129 insertions(+), 42 deletions(-)


base-commit: ac9c34d1e45a4c25174ced4fc0cfc33ff3ed08c7
-- 
2.34.1

[PATCH v1 1/8] MT9M114: Add bypass-pll DT-binding

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

Add the 'bypass-pll' property to the MT9M114 DT-bindings to allow selecting
the PLL bypass mode.

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

diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
index f6b87892068a..72e258d57186 100644
--- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
@@ -70,6 +70,10 @@ properties:
           - bus-type
           - link-frequencies
 
+  onnn,bypass-pll:
+    description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
+    type: boolean
+
 required:
   - compatible
   - reg
-- 
2.34.1

[PATCH v1 2/8] MT9M114: Add pad-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 'pad-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         | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
index 72e258d57186..666afe10c538 100644
--- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
@@ -74,6 +74,12 @@ properties:
     description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
     type: boolean
 
+  onnn,slew-rate:
+    $ref: /schemas/types.yaml#/definitions/uint8
+    description: Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and PIXCLK
+    minimum: 0
+    maximum: 7
+
 required:
   - compatible
   - reg
-- 
2.34.1

[PATCH v1 3/8] MT9M114: Add get_mbus_config

[Mathis Foerst]

The i.MX6 camera frameworks requires get_mbus_config to be implemented.
See [0].

[0] drivers/staging/media/imx/imx-media-csi.c - line 211..216

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

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index 5f0b0ad8f885..fa64d6d315a1 100644
--- a/drivers/media/i2c/mt9m114.c
+++ b/drivers/media/i2c/mt9m114.c
@@ -1977,6 +1977,24 @@ static int mt9m114_ifp_registered(struct v4l2_subdev *sd)
 		v4l2_device_unregister_subdev(&sensor->pa.sd);
 		return ret;
 	}
+	return 0;
+}
+
+static int mt9m114_ifp_get_mbus_config(struct v4l2_subdev *sd,
+				   unsigned int pad,
+				   struct v4l2_mbus_config *cfg)
+{
+	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
+
+	if (sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY) {
+		cfg->type = V4L2_MBUS_CSI2_DPHY;
+	} else {
+		cfg->type = V4L2_MBUS_PARALLEL;
+		cfg->bus.parallel.flags = V4L2_MBUS_MASTER |
+					  V4L2_MBUS_PCLK_SAMPLE_RISING |
+					  V4L2_MBUS_DATA_ACTIVE_HIGH;
+		cfg->bus.parallel.bus_width = 8;
+	}
 
 	return 0;
 }
@@ -1993,6 +2011,7 @@ static const struct v4l2_subdev_pad_ops mt9m114_ifp_pad_ops = {
 	.set_fmt = mt9m114_ifp_set_fmt,
 	.get_selection = mt9m114_ifp_get_selection,
 	.set_selection = mt9m114_ifp_set_selection,
+	.get_mbus_config = mt9m114_ifp_get_mbus_config,
 	.get_frame_interval = mt9m114_ifp_get_frame_interval,
 	.set_frame_interval = mt9m114_ifp_set_frame_interval,
 };
-- 
2.34.1

[PATCH v1 4/8] MT9M114: Add option to bypass PLL

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

Read the property 'bypass-pll' from the DT in mt9m114_parse_dt().
Depending on this value, write the correct PLL register values and
calculate the correct pixel clock value.

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

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index fa64d6d315a1..edbc0447141d 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);
 
@@ -2268,8 +2276,12 @@ 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
+	if (sensor->bypass_pll) {
+		sensor->pixrate = clk_get_rate(sensor->clk) / 2;
+	} else {
+		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;
@@ -2321,6 +2333,7 @@ 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;
 	int ret;
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
@@ -2349,6 +2362,8 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
 		goto error;
 	}
 
+	sensor->bypass_pll = of_property_read_bool(dev_node, "bypass-pll");
+
 	return 0;
 
 error:
-- 
2.34.1

[PATCH v1 5/8] 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 edbc0447141d..a1976da74c08 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

[PATCH v1 6/8] 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 a1976da74c08..5c0a8a940fd2 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

[PATCH v1 7/8] 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

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 5c0a8a940fd2..f96f6c010e1b 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

[PATCH v1 8/8] 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 'pad-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 | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
index f96f6c010e1b..327384e8427d 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;
+	u8 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 |
+		sensor->pad_slew_rate << 4 |
+		sensor->pad_slew_rate << 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;
@@ -2363,6 +2371,7 @@ 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;
+	u8 slew_rate;
 	int ret;
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
@@ -2393,6 +2402,11 @@ static int mt9m114_parse_dt(struct mt9m114 *sensor)
 
 	sensor->bypass_pll = of_property_read_bool(dev_node, "bypass-pll");
 
+	ret = of_property_read_u8(dev_node, "pad-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 v1 1/8] MT9M114: Add bypass-pll DT-binding

[Sakari Ailus]

Hi Mathis,

Please see which subject prefix has been used in the past for these
bindings.

On Wed, Feb 26, 2025 at 04:39:22PM +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.
> 
> Add the 'bypass-pll' property to the MT9M114 DT-bindings to allow selecting
> the PLL bypass mode.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> index f6b87892068a..72e258d57186 100644
> --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> @@ -70,6 +70,10 @@ properties:
>            - bus-type
>            - link-frequencies
>  
> +  onnn,bypass-pll:
> +    description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
> +    type: boolean
> +

But on the content itself: do you need this? Could you instead compare the
external clock frequency to the link-frequencies property value(s)?

>  required:
>    - compatible
>    - reg

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 3/8] MT9M114: Add get_mbus_config

[Dave Stevenson]

Hi Mathis

On Wed, 26 Feb 2025 at 15:45, Mathis Foerst <mathis.foerst@mt.com> wrote:
>
> The i.MX6 camera frameworks requires get_mbus_config to be implemented.
> See [0].
>
> [0] drivers/staging/media/imx/imx-media-csi.c - line 211..216

The docs for get_mbus_config say
 * @get_mbus_config: get the media bus configuration of a remote sub-device.
 *             The media bus configuration is usually retrieved from the
 *             firmware interface at sub-device probe time, immediately
 *             applied to the hardware and eventually adjusted by the
 *             driver.
https://github.com/torvalds/linux/blob/master/include/media/v4l2-subdev.h#L814

All other receiver drivers (including imx6-mipi-csi2.c) that call
get_mbus_config handle it returning -ENOIOCTLCMD by reverting to the
static configuration or the receiver node from device tree / ACPI.

I may be missing something, but as imx-media-csi.c appears to be the
exception, isn't it better to fix that up rather than having to fix up
all the sensor drivers that ever might get attached to it?

> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  drivers/media/i2c/mt9m114.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 5f0b0ad8f885..fa64d6d315a1 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1977,6 +1977,24 @@ static int mt9m114_ifp_registered(struct v4l2_subdev *sd)
>                 v4l2_device_unregister_subdev(&sensor->pa.sd);
>                 return ret;
>         }
> +       return 0;
> +}
> +
> +static int mt9m114_ifp_get_mbus_config(struct v4l2_subdev *sd,
> +                                  unsigned int pad,
> +                                  struct v4l2_mbus_config *cfg)
> +{
> +       struct mt9m114 *sensor = ifp_to_mt9m114(sd);
> +
> +       if (sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY) {
> +               cfg->type = V4L2_MBUS_CSI2_DPHY;

Not setting cfg->bus.mipi_csi2.num_data_lanes is going to cause some
confusion. What does an assumed 0 data lanes mean?

Likewise it would be sensible to set cfg->bus.mipi_csi2.flags so as to
avoid any ambiguities (did the caller memset all fields before
calling?)

  Dave

> +       } else {
> +               cfg->type = V4L2_MBUS_PARALLEL;
> +               cfg->bus.parallel.flags = V4L2_MBUS_MASTER |
> +                                         V4L2_MBUS_PCLK_SAMPLE_RISING |
> +                                         V4L2_MBUS_DATA_ACTIVE_HIGH;
> +               cfg->bus.parallel.bus_width = 8;
> +       }
>
>         return 0;
>  }
> @@ -1993,6 +2011,7 @@ static const struct v4l2_subdev_pad_ops mt9m114_ifp_pad_ops = {
>         .set_fmt = mt9m114_ifp_set_fmt,
>         .get_selection = mt9m114_ifp_get_selection,
>         .set_selection = mt9m114_ifp_set_selection,
> +       .get_mbus_config = mt9m114_ifp_get_mbus_config,
>         .get_frame_interval = mt9m114_ifp_get_frame_interval,
>         .set_frame_interval = mt9m114_ifp_set_frame_interval,
>  };
> --
> 2.34.1
>
>

Re: [PATCH v1 3/8] MT9M114: Add get_mbus_config

[Sakari Ailus]

Hi Mathis,

On Wed, Feb 26, 2025 at 04:39:24PM +0100, Mathis Foerst wrote:
> The i.MX6 camera frameworks requires get_mbus_config to be implemented.
> See [0].
> 
> [0] drivers/staging/media/imx/imx-media-csi.c - line 211..216

The imx driver should really be fixed instead of working around a staging
driver issue in sensor drivers.

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 7/8] MT9M114: Fix deadlock in get_frame_interval/set_frame_interval

[Sakari Ailus]

Hi Mathis,

On Wed, Feb 26, 2025 at 04:39:28PM +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
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>

Could you add:

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

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 2/8] MT9M114: Add pad-slew-rate DT-binding

[Sakari Ailus]

Hi Mathis,

On Wed, Feb 26, 2025 at 04:39:23PM +0100, Mathis Foerst wrote:
> The MT9M114 supports the different slew rates (0 to 7) on the output pads.

"the output pads" probably means pixel data interface (DVP or CSI-2)
signals in this cases? I suppose this is about clock modulation?
It'd be good to say that.

> 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 'pad-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         | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> index 72e258d57186..666afe10c538 100644
> --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> @@ -74,6 +74,12 @@ properties:
>      description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
>      type: boolean
>  
> +  onnn,slew-rate:
> +    $ref: /schemas/types.yaml#/definitions/uint8

No need to make this 8-bit (i.e. just use 32 bits).

> +    description: Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and PIXCLK

Please wrap at 80 characters.

Is there more information on the effect than 7 is the fastest?

> +    minimum: 0
> +    maximum: 7

Please also add a default.

> +
>  required:
>    - compatible
>    - reg

-- 
Regards,

Sakari Ailus

Re: [PATCH v1 0/8] MT9M114 driver bugfix and improvements

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

On Wed, Feb 26, 2025 at 04:39:21PM +0100, Mathis Foerst wrote:
> Hi,
> 
> this patch series contains the following bugfix and improvements
> for the MT9M114 camera driver:
> 
> Bugfixes:
> - Fix a deadlock when using the V4L2 pad-ops get/set_frame_interval
> 
> Compatibility:
> - Implement the missing get_mbus_config() function to be compatible
>   with the i.MX6 camera framework
> 
> New Features:
> - Allow to bypass the internal PLL (configurable via DT)
> - 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 (8):
>   MT9M114: Add bypass-pll DT-binding
>   MT9M114: Add pad-slew-rate DT-binding
>   MT9M114: Add get_mbus_config
>   MT9M114: Add option to bypass PLL
>   MT9M114: Factor out mt9m114_configure_pa
>   MT9M114: Allow set_selection while streaming
>   MT9M114: Fix deadlock in get_frame_interval/set_frame_interval
>   MT9M114: Set pad-slew-rate

Please take a look (via git log) what normal patch subjects look like.

Thanks,
Conor.

> 
>  .../bindings/media/i2c/onnn,mt9m114.yaml      |  10 ++
>  drivers/media/i2c/mt9m114.c                   | 161 +++++++++++++-----
>  2 files changed, 129 insertions(+), 42 deletions(-)
> 
> 
> base-commit: ac9c34d1e45a4c25174ced4fc0cfc33ff3ed08c7
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/8] MT9M114: Add pad-slew-rate DT-binding

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2009 bytes --]

On Thu, Feb 27, 2025 at 10:14:02AM +0000, Sakari Ailus wrote:
> Hi Mathis,
> 
> On Wed, Feb 26, 2025 at 04:39:23PM +0100, Mathis Foerst wrote:
> > The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> 
> "the output pads" probably means pixel data interface (DVP or CSI-2)
> signals in this cases? I suppose this is about clock modulation?
> It'd be good to say that.
> 
> > 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 'pad-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         | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > index 72e258d57186..666afe10c538 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > @@ -74,6 +74,12 @@ properties:
> >      description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
> >      type: boolean
> >  
> > +  onnn,slew-rate:
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> 
> No need to make this 8-bit (i.e. just use 32 bits).
> 
> > +    description: Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and PIXCLK
> 
> Please wrap at 80 characters.
> 
> Is there more information on the effect than 7 is the fastest?
> 
> > +    minimum: 0
> > +    maximum: 7
> 
> Please also add a default.

It'd also be great (IMO) if it were given in terms of actual units, not
nebulous values that I assume to be the register values.

> 
> > +
> >  required:
> >    - compatible
> >    - reg
> 
> -- 
> Regards,
> 
> Sakari Ailus

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/8] MT9M114: Add pad-slew-rate DT-binding

[Mathis Foerst]

Hi Conor, Hi Sakari,

On Fri, Feb 28, 2025 at 07:11:31PM +0000, Conor Dooley wrote:
> On Thu, Feb 27, 2025 at 10:14:02AM +0000, Sakari Ailus wrote:
> > Hi Mathis,
> > 
> > On Wed, Feb 26, 2025 at 04:39:23PM +0100, Mathis Foerst wrote:
> > > The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> > 
> > "the output pads" probably means pixel data interface (DVP or CSI-2)
> > signals in this cases? I suppose this is about clock modulation?
> > It'd be good to say that.

The slew rate defines the slope of the voltage flanks on the output pads (how fast
the pads go from LOW to HIGH or vice versa). I tried to clarify the description.

> > 
> > > 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 'pad-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         | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > index 72e258d57186..666afe10c538 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > @@ -74,6 +74,12 @@ properties:
> > >      description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
> > >      type: boolean
> > >  
> > > +  onnn,slew-rate:
> > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > 
> > No need to make this 8-bit (i.e. just use 32 bits).

Okay, I thought 8-bit would fit the small value range [0,7]. Changed it to 32 bits.

> > 
> > > +    description: Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and PIXCLK
> > 
> > Please wrap at 80 characters.
> > 
> > Is there more information on the effect than 7 is the fastest?

There is no more information about the exact meaning of the values.
As described above, the higher the value, the steeper the voltage flanks.

> > 
> > > +    minimum: 0
> > > +    maximum: 7
> > 
> > Please also add a default.

Sure, I added the default value 7 that matches the currently hardcoded 
value in the driver.

> 
> It'd also be great (IMO) if it were given in terms of actual units, not
> nebulous values that I assume to be the register values.

I agree, but the documentation does not provide further details about the
unit of the value. So using the register value is my best-effort approach.

> 
> > 
> > > +
> > >  required:
> > >    - compatible
> > >    - reg
> > 
> > -- 
> > Regards,
> > 
> > Sakari Ailus

Best regards,
Mathis Foerst

Re: [PATCH v1 1/8] MT9M114: Add bypass-pll DT-binding

[Mathis Foerst]

Hi Sakari

On Wed, Feb 26, 2025 at 04:08:39PM +0000, Sakari Ailus wrote:
> Hi Mathis,
> 
> Please see which subject prefix has been used in the past for these
> bindings.

Yes, I'm sorry for that. I adapted the subjects of the patches.

> 
> On Wed, Feb 26, 2025 at 04:39:22PM +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.
> > 
> > Add the 'bypass-pll' property to the MT9M114 DT-bindings to allow selecting
> > the PLL bypass mode.
> > 
> > Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > index f6b87892068a..72e258d57186 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > @@ -70,6 +70,10 @@ properties:
> >            - bus-type
> >            - link-frequencies
> >  
> > +  onnn,bypass-pll:
> > +    description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
> > +    type: boolean
> > +
> 
> But on the content itself: do you need this? Could you instead compare the
> external clock frequency to the link-frequencies property value(s)?

That should also work. I removed the DT binding and let the driver check if
EXTCLK matches the required SYSCLK for the given link frequency to decide if
the PLL should be bypassed.

> 
> >  required:
> >    - compatible
> >    - reg
> 
> -- 
> Regards,
> 
> Sakari Ailus

Best regards,
Mathis Foerst

Re: [PATCH v1 2/8] MT9M114: Add pad-slew-rate DT-binding

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3038 bytes --]

On Tue, Mar 04, 2025 at 12:48:58PM +0100, Mathis Foerst wrote:
> Hi Conor, Hi Sakari,
> 
> On Fri, Feb 28, 2025 at 07:11:31PM +0000, Conor Dooley wrote:
> > On Thu, Feb 27, 2025 at 10:14:02AM +0000, Sakari Ailus wrote:
> > > Hi Mathis,
> > > 
> > > On Wed, Feb 26, 2025 at 04:39:23PM +0100, Mathis Foerst wrote:
> > > > The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> > > 
> > > "the output pads" probably means pixel data interface (DVP or CSI-2)
> > > signals in this cases? I suppose this is about clock modulation?
> > > It'd be good to say that.
> 
> The slew rate defines the slope of the voltage flanks on the output pads (how fast
> the pads go from LOW to HIGH or vice versa). I tried to clarify the description.
> 
> > > 
> > > > 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 'pad-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         | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > index 72e258d57186..666afe10c538 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > @@ -74,6 +74,12 @@ properties:
> > > >      description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
> > > >      type: boolean
> > > >  
> > > > +  onnn,slew-rate:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > 
> > > No need to make this 8-bit (i.e. just use 32 bits).
> 
> Okay, I thought 8-bit would fit the small value range [0,7]. Changed it to 32 bits.
> 
> > > 
> > > > +    description: Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and PIXCLK
> > > 
> > > Please wrap at 80 characters.
> > > 
> > > Is there more information on the effect than 7 is the fastest?
> 
> There is no more information about the exact meaning of the values.
> As described above, the higher the value, the steeper the voltage flanks.
> 
> > > 
> > > > +    minimum: 0
> > > > +    maximum: 7
> > > 
> > > Please also add a default.
> 
> Sure, I added the default value 7 that matches the currently hardcoded 
> value in the driver.
> 
> > 
> > It'd also be great (IMO) if it were given in terms of actual units, not
> > nebulous values that I assume to be the register values.
> 
> I agree, but the documentation does not provide further details about the
> unit of the value. So using the register value is my best-effort approach.

If they don't provide em, how is anyone meant to calculate what's
correct? Trial and error?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/8] MT9M114: Add pad-slew-rate DT-binding

[Mathis Foerst]

On Tue, Mar 04, 2025 at 04:39:34PM +0000, Conor Dooley wrote:
> On Tue, Mar 04, 2025 at 12:48:58PM +0100, Mathis Foerst wrote:
> > Hi Conor, Hi Sakari,
> > 
> > On Fri, Feb 28, 2025 at 07:11:31PM +0000, Conor Dooley wrote:
> > > On Thu, Feb 27, 2025 at 10:14:02AM +0000, Sakari Ailus wrote:
> > > > Hi Mathis,
> > > > 
> > > > On Wed, Feb 26, 2025 at 04:39:23PM +0100, Mathis Foerst wrote:
> > > > > The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> > > > 
> > > > "the output pads" probably means pixel data interface (DVP or CSI-2)
> > > > signals in this cases? I suppose this is about clock modulation?
> > > > It'd be good to say that.
> > 
> > The slew rate defines the slope of the voltage flanks on the output pads (how fast
> > the pads go from LOW to HIGH or vice versa). I tried to clarify the description.
> > 
> > > > 
> > > > > 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 'pad-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         | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > index 72e258d57186..666afe10c538 100644
> > > > > --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > @@ -74,6 +74,12 @@ properties:
> > > > >      description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
> > > > >      type: boolean
> > > > >  
> > > > > +  onnn,slew-rate:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > > 
> > > > No need to make this 8-bit (i.e. just use 32 bits).
> > 
> > Okay, I thought 8-bit would fit the small value range [0,7]. Changed it to 32 bits.
> > 
> > > > 
> > > > > +    description: Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and PIXCLK
> > > > 
> > > > Please wrap at 80 characters.
> > > > 
> > > > Is there more information on the effect than 7 is the fastest?
> > 
> > There is no more information about the exact meaning of the values.
> > As described above, the higher the value, the steeper the voltage flanks.
> > 
> > > > 
> > > > > +    minimum: 0
> > > > > +    maximum: 7
> > > > 
> > > > Please also add a default.
> > 
> > Sure, I added the default value 7 that matches the currently hardcoded 
> > value in the driver.
> > 
> > > 
> > > It'd also be great (IMO) if it were given in terms of actual units, not
> > > nebulous values that I assume to be the register values.
> > 
> > I agree, but the documentation does not provide further details about the
> > unit of the value. So using the register value is my best-effort approach.
> 
> If they don't provide em, how is anyone meant to calculate what's
> correct? Trial and error?

The correct slew-rate is a trade-off:

You would usually start with the fastest slew-rate as it leads to an
output signal that's as close as possible to a perfect square-wave.
On higher link frequencies a too slow slew rate can cause the signal to
not reach the HIGH voltage level before going to LOW again s.t. the
reveiver cannot interpret the digital signal correctly.

But steeper voltage flanks lead to higher electromagnetic emissions s.t.
a device might not pass the electromagnetic compatibility (EMC)
certification with the high slew rate.
In this case you would lower the slew rate until your emissions are
within the allowed range.

The actual emissions depend on many factors like the PCB layout, the
length and shielding of cables etc. This makes it hard to fully simulate
them.
So even if would know the exact unit of the configured slew rate of the
camera sensor, it would not fully allow us to calculate the correct value
for it.

Re: [PATCH v1 3/8] MT9M114: Add get_mbus_config

[Mathis Foerst]

Hi Sakari,

thank you for your input.

On Thu, Feb 27, 2025 at 08:53:23AM +0000, Sakari Ailus wrote:
> Hi Mathis,
> 
> On Wed, Feb 26, 2025 at 04:39:24PM +0100, Mathis Foerst wrote:
> > The i.MX6 camera frameworks requires get_mbus_config to be implemented.
> > See [0].
> > 
> > [0] drivers/staging/media/imx/imx-media-csi.c - line 211..216
> 
> The imx driver should really be fixed instead of working around a staging
> driver issue in sensor drivers.

Makes sense for me, I implemented a fix in imx-media-csi.c in and moved it
to a new patch series as it's no longer related to the MT9M114 driver:
https://lore.kernel.org/linux-media/20250305113802.897087-1-mathis.foerst@mt.com/

> 
> -- 
> Regards,
> 
> Sakari Ailus

Best regards,
Mathis Foerst

Re: [PATCH v1 2/8] MT9M114: Add pad-slew-rate DT-binding

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4507 bytes --]

On Wed, Mar 05, 2025 at 10:59:27AM +0100, Mathis Foerst wrote:
> On Tue, Mar 04, 2025 at 04:39:34PM +0000, Conor Dooley wrote:
> > On Tue, Mar 04, 2025 at 12:48:58PM +0100, Mathis Foerst wrote:
> > > Hi Conor, Hi Sakari,
> > > 
> > > On Fri, Feb 28, 2025 at 07:11:31PM +0000, Conor Dooley wrote:
> > > > On Thu, Feb 27, 2025 at 10:14:02AM +0000, Sakari Ailus wrote:
> > > > > Hi Mathis,
> > > > > 
> > > > > On Wed, Feb 26, 2025 at 04:39:23PM +0100, Mathis Foerst wrote:
> > > > > > The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> > > > > 
> > > > > "the output pads" probably means pixel data interface (DVP or CSI-2)
> > > > > signals in this cases? I suppose this is about clock modulation?
> > > > > It'd be good to say that.
> > > 
> > > The slew rate defines the slope of the voltage flanks on the output pads (how fast
> > > the pads go from LOW to HIGH or vice versa). I tried to clarify the description.
> > > 
> > > > > 
> > > > > > 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 'pad-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         | 6 ++++++
> > > > > >  1 file changed, 6 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > > index 72e258d57186..666afe10c538 100644
> > > > > > --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > > @@ -74,6 +74,12 @@ properties:
> > > > > >      description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
> > > > > >      type: boolean
> > > > > >  
> > > > > > +  onnn,slew-rate:
> > > > > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > > > 
> > > > > No need to make this 8-bit (i.e. just use 32 bits).
> > > 
> > > Okay, I thought 8-bit would fit the small value range [0,7]. Changed it to 32 bits.
> > > 
> > > > > 
> > > > > > +    description: Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and PIXCLK
> > > > > 
> > > > > Please wrap at 80 characters.
> > > > > 
> > > > > Is there more information on the effect than 7 is the fastest?
> > > 
> > > There is no more information about the exact meaning of the values.
> > > As described above, the higher the value, the steeper the voltage flanks.
> > > 
> > > > > 
> > > > > > +    minimum: 0
> > > > > > +    maximum: 7
> > > > > 
> > > > > Please also add a default.
> > > 
> > > Sure, I added the default value 7 that matches the currently hardcoded 
> > > value in the driver.
> > > 
> > > > 
> > > > It'd also be great (IMO) if it were given in terms of actual units, not
> > > > nebulous values that I assume to be the register values.
> > > 
> > > I agree, but the documentation does not provide further details about the
> > > unit of the value. So using the register value is my best-effort approach.
> > 
> > If they don't provide em, how is anyone meant to calculate what's
> > correct? Trial and error?
> 
> The correct slew-rate is a trade-off:
> 
> You would usually start with the fastest slew-rate as it leads to an
> output signal that's as close as possible to a perfect square-wave.
> On higher link frequencies a too slow slew rate can cause the signal to
> not reach the HIGH voltage level before going to LOW again s.t. the
> reveiver cannot interpret the digital signal correctly.
> 
> But steeper voltage flanks lead to higher electromagnetic emissions s.t.
> a device might not pass the electromagnetic compatibility (EMC)
> certification with the high slew rate.
> In this case you would lower the slew rate until your emissions are
> within the allowed range.
> 
> The actual emissions depend on many factors like the PCB layout, the
> length and shielding of cables etc. This makes it hard to fully simulate
> them.
> So even if would know the exact unit of the configured slew rate of the
> camera sensor, it would not fully allow us to calculate the correct value
> for it.

So the answer is trial and error then?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/8] MT9M114: Add pad-slew-rate DT-binding

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4863 bytes --]

On Wed, Mar 05, 2025 at 04:29:24PM +0000, Conor Dooley wrote:
> On Wed, Mar 05, 2025 at 10:59:27AM +0100, Mathis Foerst wrote:
> > On Tue, Mar 04, 2025 at 04:39:34PM +0000, Conor Dooley wrote:
> > > On Tue, Mar 04, 2025 at 12:48:58PM +0100, Mathis Foerst wrote:
> > > > Hi Conor, Hi Sakari,
> > > > 
> > > > On Fri, Feb 28, 2025 at 07:11:31PM +0000, Conor Dooley wrote:
> > > > > On Thu, Feb 27, 2025 at 10:14:02AM +0000, Sakari Ailus wrote:
> > > > > > Hi Mathis,
> > > > > > 
> > > > > > On Wed, Feb 26, 2025 at 04:39:23PM +0100, Mathis Foerst wrote:
> > > > > > > The MT9M114 supports the different slew rates (0 to 7) on the output pads.
> > > > > > 
> > > > > > "the output pads" probably means pixel data interface (DVP or CSI-2)
> > > > > > signals in this cases? I suppose this is about clock modulation?
> > > > > > It'd be good to say that.
> > > > 
> > > > The slew rate defines the slope of the voltage flanks on the output pads (how fast
> > > > the pads go from LOW to HIGH or vice versa). I tried to clarify the description.
> > > > 
> > > > > > 
> > > > > > > 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 'pad-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         | 6 ++++++
> > > > > > >  1 file changed, 6 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > > > index 72e258d57186..666afe10c538 100644
> > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/onnn,mt9m114.yaml
> > > > > > > @@ -74,6 +74,12 @@ properties:
> > > > > > >      description: Bypass the internal PLL of the sensor to use EXTCLK directly as SYSCLK.
> > > > > > >      type: boolean
> > > > > > >  
> > > > > > > +  onnn,slew-rate:
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > > > > 
> > > > > > No need to make this 8-bit (i.e. just use 32 bits).
> > > > 
> > > > Okay, I thought 8-bit would fit the small value range [0,7]. Changed it to 32 bits.
> > > > 
> > > > > > 
> > > > > > > +    description: Slew rate ot the output pads DOUT[7:0], LINE_VALID, FRAME_VALID and PIXCLK
> > > > > > 
> > > > > > Please wrap at 80 characters.
> > > > > > 
> > > > > > Is there more information on the effect than 7 is the fastest?
> > > > 
> > > > There is no more information about the exact meaning of the values.
> > > > As described above, the higher the value, the steeper the voltage flanks.
> > > > 
> > > > > > 
> > > > > > > +    minimum: 0
> > > > > > > +    maximum: 7
> > > > > > 
> > > > > > Please also add a default.
> > > > 
> > > > Sure, I added the default value 7 that matches the currently hardcoded 
> > > > value in the driver.
> > > > 
> > > > > 
> > > > > It'd also be great (IMO) if it were given in terms of actual units, not
> > > > > nebulous values that I assume to be the register values.
> > > > 
> > > > I agree, but the documentation does not provide further details about the
> > > > unit of the value. So using the register value is my best-effort approach.
> > > 
> > > If they don't provide em, how is anyone meant to calculate what's
> > > correct? Trial and error?
> > 
> > The correct slew-rate is a trade-off:
> > 
> > You would usually start with the fastest slew-rate as it leads to an
> > output signal that's as close as possible to a perfect square-wave.
> > On higher link frequencies a too slow slew rate can cause the signal to
> > not reach the HIGH voltage level before going to LOW again s.t. the
> > reveiver cannot interpret the digital signal correctly.
> > 
> > But steeper voltage flanks lead to higher electromagnetic emissions s.t.
> > a device might not pass the electromagnetic compatibility (EMC)
> > certification with the high slew rate.
> > In this case you would lower the slew rate until your emissions are
> > within the allowed range.
> > 
> > The actual emissions depend on many factors like the PCB layout, the
> > length and shielding of cables etc. This makes it hard to fully simulate
> > them.
> > So even if would know the exact unit of the configured slew rate of the
> > camera sensor, it would not fully allow us to calculate the correct value
> > for it.
> 
> So the answer is trial and error then?

And I guess, keep the register values if there's no actual unit
corresponding to these values.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]