[PATCH 0/5] media/i2c: max96717: a few changes

[PATCH 0/5] media/i2c: max96717: a few changes

[Laurentiu Palcu]

Hi,

This series adds support for:
 * get_frame_sync(), so that we can pass the VCs and DTs from the
   incoming sensor streams over to the downstream drivers (CSI, ISI);
 * frame synchronization in multi-sensor setups. This one comes with
   some dt-bindings changes in order to allow the user to specify the
   incoming GPIO RX ID and the output pin;
 * operation mode override. This would allow toggling from the pin
   configured mode (for example: pixel mode) to the other one (tunneling
   mode). Also, vice versa is possible as well but the driver only
   supports tunneling mode currently;

On the frame synchronization bindings, I would need some advice on the
property naming. Recently, I sent a RFC adding support for MAX96724
deserializer chip, see [1], and I also added support for FSYNC for that
chip. The max96724 property is also named "maxim,fsync-config" but,
since the chip is a deserializer and it's the one actually sending the
FSYNC signal, that property has an extra item: the fsync mode.

My question is: would it be OK to have the same "maxim,fsync-config"
property name for both or should we have different names?

[1] https://patchwork.linuxtv.org/project/linux-media/list/?series=14427

Thanks,
Laurentiu

Laurentiu Palcu (5):
  media/i2c: max96717: change internal regulator voltage
  media/i2c: max96717: implement the .get_frame_desc() operation
  dt-bindings: i2c: maxim,max96717: add new properties
  media/i2c: max96717: add FSYNC support
  media/i2c: max96717: allow user to override operation mode from DT

 .../bindings/media/i2c/maxim,max96717.yaml    |  28 ++++
 drivers/media/i2c/max96717.c                  | 137 +++++++++++++++++-
 2 files changed, 157 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/5] media/i2c: max96717: change internal regulator voltage

[Laurentiu Palcu]

The Programming Notes section of the specifications states:

"""
MANDATORY REGISTER PROGRAMMING
Make the following register writes to ensure proper operation of the
MAX96717F. Without these writes, the operation of the device specified
in the data sheet cannot be guaranteed.
Set bits [6:4] = 3'b001 in register 0x302
"""

Set this register before going on with the chip initialization.

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 drivers/media/i2c/max96717.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
index 9259d58ba734e..b1116aade0687 100644
--- a/drivers/media/i2c/max96717.c
+++ b/drivers/media/i2c/max96717.c
@@ -78,6 +78,15 @@
 #define MAX96717_GPIO_TX_EN       BIT(1)
 #define MAX96717_GPIO_OUT_DIS     BIT(0)
 
+/* CMU */
+#define MAX96717_CMU_CMU2		CCI_REG8(0x0302)
+#define MAX96717_PFDDIV_RSHORT_MASK	GENMASK(6, 4)
+#define MAX96717_PFDDIV_RSHORT_SHIFT	4
+#define MAX96717_PFDDIV_VREG_1V0	0
+#define MAX96717_PFDDIV_VREG_1V1	1
+#define MAX96717_PFDDIV_VREG_0V875	2
+#define MAX96717_PFDDIV_VREG_0V94	3
+
 /* FRONTTOP */
 /* MAX96717 only have CSI port 'B' */
 #define MAX96717_FRONTOP0     CCI_REG8(0x308)
@@ -981,6 +990,14 @@ static int max96717_hw_init(struct max96717_priv *priv)
 	dev_dbg(dev, "Found %x (rev %lx)\n", (u8)dev_id,
 		(u8)val & MAX96717_DEV_REV_MASK);
 
+	/*
+	 * According to specs, in the Programming Notes section, there's a mandatory register
+	 * programming notice that advises to enable the 1.1V internal regulator to guarantee proper
+	 * device operation. Let's do this before any other operations.
+	 */
+	cci_write(priv->regmap, MAX96717_CMU_CMU2,
+		  MAX96717_PFDDIV_VREG_1V1 << MAX96717_PFDDIV_RSHORT_SHIFT, NULL);
+
 	ret = cci_read(priv->regmap, MAX96717_MIPI_RX_EXT11, &val, NULL);
 	if (ret)
 		return dev_err_probe(dev, ret,
-- 
2.34.1

[PATCH 2/5] media/i2c: max96717: implement the .get_frame_desc() operation

[Laurentiu Palcu]

Since the max96717 serializer can work with various sensors, we need to
implement the .get_frame_desc() callback to get the VCs and DTs for the
incoming stream(s).

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 drivers/media/i2c/max96717.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
index b1116aade0687..6a668a004c717 100644
--- a/drivers/media/i2c/max96717.c
+++ b/drivers/media/i2c/max96717.c
@@ -575,12 +575,33 @@ static int max96717_disable_streams(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int max96717_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				   struct v4l2_mbus_frame_desc *fd)
+{
+	struct max96717_priv *priv = sd_to_max96717(sd);
+	int ret;
+	struct v4l2_mbus_frame_desc source_fd;
+
+	if (pad != MAX96717_PAD_SOURCE)
+		return -EINVAL;
+
+	ret = v4l2_subdev_call(priv->source_sd, pad, get_frame_desc,
+			       priv->source_sd_pad, &source_fd);
+	if (ret)
+		return ret;
+
+	*fd = source_fd;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops max96717_pad_ops = {
 	.enable_streams = max96717_enable_streams,
 	.disable_streams = max96717_disable_streams,
 	.set_routing = max96717_set_routing,
 	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = max96717_set_fmt,
+	.get_frame_desc = max96717_get_frame_desc,
 };
 
 static const struct v4l2_subdev_core_ops max96717_subdev_core_ops = {
-- 
2.34.1

[PATCH 3/5] dt-bindings: i2c: maxim,max96717: add new properties

[Laurentiu Palcu]

Add 'maxim,override-mode' property to allow the user to toggle the pin
configured chip operation mode and 'maxim,fsync-config' to configure the
chip for relaying a frame synchronization signal, received from
deserializer, to the attached sensor. The latter is needed for
synchronizing the images in multi-sensor setups.

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 .../bindings/media/i2c/maxim,max96717.yaml    | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
index d1e8ba6e368ec..fae578d55fd4d 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
@@ -42,10 +42,35 @@ properties:
       number must be in range of [0, 10].
 
   gpio-controller: true
+  gpio-reserved-ranges: true
 
   '#clock-cells':
     const: 0
 
+  maxim,override-mode:
+    description: Toggle the operation mode from the pin configured one.
+    type: boolean
+
+  maxim,fsync-config:
+    description:
+      Frame synchronization (FSYNC) is used to align images sent from multiple
+      sources in surround-view applications and is required for concatenation.
+      In FSYNC mode, the deserializer sends a sync signal to each serializer;
+      the serializers then send the signal to the connected sensor.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    items:
+      - description: FSYNC RX ID, needs to match the TX ID configured in the deserializer.
+        minimum: 0
+        maximum: 31
+        default: 0
+      - description:
+          Output GPIO pin used for sending the FSYNC to the sensor. The pin, however, needs
+          to be excluded from the gpiochip using the gpio-reserved-ranges property since
+          it will be used exclusively for FSYNC generation.
+        minimum: 0
+        maximum: 10
+        default: 0
+
   reg:
     maxItems: 1
 
@@ -113,6 +138,9 @@ examples:
             #gpio-cells = <2>;
             #clock-cells = <0>;
 
+            gpio-reserved-ranges = <0 1>;
+            maxim,fsync-config = <0 0>;
+
             ports {
                 #address-cells = <1>;
                 #size-cells = <0>;
-- 
2.34.1

[PATCH 4/5] media/i2c: max96717: add FSYNC support

[Laurentiu Palcu]

According to specs:

"""
Frame synchronization (FSYNC) is used to align images sent from multiple
sources in surround-view applications and is required for concatenation.
In FSYNC mode, the GMSL2 CSI-2 quad deserializer sends a sync signal to
each serializer; the serializers then send the signal to the connected
sensor.
"""

Since max96717 can be used in multi-sensor setups, we need FSYNC
support. For that, I added a DT property("maxim,fsync-config") that will
be used to configure the frame sync output pin and the RX ID of the
GPIO as sent by the deserializer chip.

Also, add the .request() callback for the gpiochip so that we can use
'gpio-reserved-ranges' in DT to exclude the pins that are used for
FSYNC from being used as GPIOs.

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 drivers/media/i2c/max96717.c | 87 ++++++++++++++++++++++++++++++++----
 1 file changed, 79 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
index 6a668a004c717..47a3be195a971 100644
--- a/drivers/media/i2c/max96717.c
+++ b/drivers/media/i2c/max96717.c
@@ -70,13 +70,28 @@
 #define MAX96717_VTX_CHKB_ALT          CCI_REG8(0x275)
 
 /* GPIO */
-#define MAX96717_NUM_GPIO         11
-#define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
-#define MAX96717_GPIO_OUT         BIT(4)
-#define MAX96717_GPIO_IN          BIT(3)
-#define MAX96717_GPIO_RX_EN       BIT(2)
-#define MAX96717_GPIO_TX_EN       BIT(1)
-#define MAX96717_GPIO_OUT_DIS     BIT(0)
+#define MAX96717_NUM_GPIO		11
+#define MAX96717_GPIO_REG_A(gpio)	CCI_REG8(0x2be + (gpio) * 3)
+#define MAX96717_GPIO_OUT		BIT(4)
+#define MAX96717_GPIO_IN		BIT(3)
+#define MAX96717_GPIO_RX_EN		BIT(2)
+#define MAX96717_GPIO_TX_EN		BIT(1)
+#define MAX96717_GPIO_OUT_DIS		BIT(0)
+#define MAX96717_GPIO_REG_B(gpio)	CCI_REG8(0x2bf + (gpio) * 3)
+#define MAX96717_GPIO_TX_ID_MASK	GENMASK(4, 0)
+#define MAX96717_GPIO_TX_ID_SHIFT	0
+#define MAX96717_OUT_TYPE		BIT(5)
+#define MAX96717_OUT_TYPE_PUSH_PULL	BIT(5)
+#define MAX96717_OUT_TYPE_OPEN_DRAIN	0
+#define MAX96717_PULL_UPDN_SEL_MASK	GENMASK(7, 6)
+#define MAX96717_PULL_UPDN_SEL_SHIFT	6
+#define MAX96717_GPIO_NO_PULL		0
+#define MAX96717_GPIO_PULL_UP		1
+#define MAX96717_GPIO_PULL_DOWN		2
+#define MAX96717_GPIO_REG_C(gpio)	CCI_REG8(0x2c0 + (gpio) * 3)
+#define MAX96717_GPIO_RX_ID_MASK	GENMASK(4, 0)
+#define MAX96717_GPIO_RX_ID_SHIFT	0
+#define MAX96717_OVR_RES_CFG		BIT(7)
 
 /* CMU */
 #define MAX96717_CMU_CMU2		CCI_REG8(0x0302)
@@ -125,6 +140,11 @@ enum max96717_vpg_mode {
 	MAX96717_VPG_GRADIENT = 2,
 };
 
+struct max96717_fsync_desc {
+	int pin;
+	int rx_id;
+};
+
 struct max96717_priv {
 	struct i2c_client		  *client;
 	struct regmap			  *regmap;
@@ -141,6 +161,7 @@ struct max96717_priv {
 	struct clk_hw                     clk_hw;
 	struct gpio_chip                  gpio_chip;
 	enum max96717_vpg_mode            pattern;
+	struct max96717_fsync_desc	  fsync;
 };
 
 static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
@@ -364,6 +385,7 @@ static int max96717_gpiochip_probe(struct max96717_priv *priv)
 	gc->get_direction = max96717_gpio_get_direction;
 	gc->direction_input = max96717_gpio_direction_in;
 	gc->direction_output = max96717_gpio_direction_out;
+	gc->request = gpiochip_generic_request;
 	gc->set = max96717_gpiochip_set;
 	gc->get = max96717_gpiochip_get;
 	gc->of_gpio_n_cells = 2;
@@ -386,6 +408,26 @@ static int max96717_gpiochip_probe(struct max96717_priv *priv)
 	return 0;
 }
 
+static int max96717_fsync_setup(struct max96717_priv *priv)
+{
+	int ret = 0;
+
+	if (priv->fsync.pin == -1)
+		return 0;
+
+	cci_update_bits(priv->regmap, MAX96717_GPIO_REG_C(priv->fsync.pin),
+			MAX96717_GPIO_RX_ID_MASK,
+			priv->fsync.rx_id << MAX96717_GPIO_RX_ID_SHIFT, &ret);
+
+	cci_update_bits(priv->regmap, MAX96717_GPIO_REG_B(priv->fsync.pin),
+			MAX96717_PULL_UPDN_SEL_MASK,
+			1 << MAX96717_PULL_UPDN_SEL_SHIFT, &ret);
+
+	return cci_update_bits(priv->regmap, MAX96717_GPIO_REG_A(priv->fsync.pin),
+			       MAX96717_GPIO_RX_EN | MAX96717_GPIO_OUT_DIS,
+			       MAX96717_GPIO_RX_EN, &ret);
+}
+
 static int _max96717_set_routing(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *state,
 				 struct v4l2_subdev_krouting *routing)
@@ -1037,7 +1079,8 @@ static int max96717_parse_dt(struct max96717_priv *priv)
 	struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
 	struct fwnode_handle *ep_fwnode;
 	unsigned char num_data_lanes;
-	int ret;
+	int ret, count;
+	u32 dt_val[2];
 
 	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
 						    MAX96717_PAD_SINK, 0, 0);
@@ -1058,6 +1101,30 @@ static int max96717_parse_dt(struct max96717_priv *priv)
 
 	priv->mipi_csi2 = vep.bus.mipi_csi2;
 
+	priv->fsync.pin = -1;
+	count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
+	if (count > 0) {
+		ret = fwnode_property_read_u32_array(dev_fwnode(dev), "maxim,fsync-config",
+						     dt_val, count);
+		if (ret) {
+			dev_err(dev, "Unable to read FSYNC config from DT.\n");
+			return ret;
+		}
+
+		priv->fsync.rx_id = dt_val[0];
+		if (priv->fsync.rx_id > 31) {
+			dev_err(dev, "Wrong GPIO RX ID. Allowed: 0 -> 31\n");
+			return -EINVAL;
+		}
+
+		priv->fsync.pin = dt_val[1];
+		if (priv->fsync.pin >= MAX96717_NUM_GPIO) {
+			dev_err(dev, "Wrong GPIO pin used for FSYNC. Allowed: 0 -> %d\n",
+				MAX96717_NUM_GPIO - 1);
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
@@ -1092,6 +1159,10 @@ static int max96717_probe(struct i2c_client *client)
 		return dev_err_probe(&client->dev, ret,
 				     "Failed to init gpiochip\n");
 
+	ret = max96717_fsync_setup(priv);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Failed to setup FSYNC\n");
+
 	ret = max96717_register_clkout(priv);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to register clkout\n");
-- 
2.34.1

[PATCH 5/5] media/i2c: max96717: allow user to override operation mode from DT

[Laurentiu Palcu]

There are situations when the CFG pins set the chip up for a certain
mode of operation (ie: pixel mode or tunneling mode), because the HW
designers decided this way, and we, the users, want to change that. For
that, add an optional DT property that would allow toggling the
operation mode from the configured one to the other one.

The driver still only supports tunneling mode, that didn't change.

Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
 drivers/media/i2c/max96717.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
index 47a3be195a971..a591ca5d5f44f 100644
--- a/drivers/media/i2c/max96717.c
+++ b/drivers/media/i2c/max96717.c
@@ -161,6 +161,7 @@ struct max96717_priv {
 	struct clk_hw                     clk_hw;
 	struct gpio_chip                  gpio_chip;
 	enum max96717_vpg_mode            pattern;
+	bool				  mode_override;
 	struct max96717_fsync_desc	  fsync;
 };
 
@@ -1066,6 +1067,14 @@ static int max96717_hw_init(struct max96717_priv *priv)
 		return dev_err_probe(dev, ret,
 				     "Fail to read mipi rx extension");
 
+	if (priv->mode_override) {
+		val ^= MAX96717_TUN_MODE;
+
+		ret = cci_write(priv->regmap, MAX96717_MIPI_RX_EXT11, val, NULL);
+		if (ret)
+			return dev_err_probe(dev, ret, "Unable to update operation mode\n");
+	}
+
 	if (!(val & MAX96717_TUN_MODE))
 		return dev_err_probe(dev, -EOPNOTSUPP,
 				     "Only supporting tunnel mode");
@@ -1101,6 +1110,9 @@ static int max96717_parse_dt(struct max96717_priv *priv)
 
 	priv->mipi_csi2 = vep.bus.mipi_csi2;
 
+	if (fwnode_property_present(dev_fwnode(dev), "maxim,cfg-mode-override"))
+		priv->mode_override = true;
+
 	priv->fsync.pin = -1;
 	count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
 	if (count > 0) {
-- 
2.34.1

Re: [PATCH 3/5] dt-bindings: i2c: maxim,max96717: add new properties

[Conor Dooley]

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

On Fri, Feb 07, 2025 at 01:29:55PM +0200, Laurentiu Palcu wrote:
> Add 'maxim,override-mode' property to allow the user to toggle the pin
> configured chip operation mode and 'maxim,fsync-config' to configure the
> chip for relaying a frame synchronization signal, received from
> deserializer, to the attached sensor. The latter is needed for
> synchronizing the images in multi-sensor setups.
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  .../bindings/media/i2c/maxim,max96717.yaml    | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> index d1e8ba6e368ec..fae578d55fd4d 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> @@ -42,10 +42,35 @@ properties:
>        number must be in range of [0, 10].
>  
>    gpio-controller: true
> +  gpio-reserved-ranges: true
>  
>    '#clock-cells':
>      const: 0
>  
> +  maxim,override-mode:
> +    description: Toggle the operation mode from the pin configured one.
> +    type: boolean

type: flag


> +  maxim,fsync-config:
> +    description:
> +      Frame synchronization (FSYNC) is used to align images sent from multiple
> +      sources in surround-view applications and is required for concatenation.
> +      In FSYNC mode, the deserializer sends a sync signal to each serializer;
> +      the serializers then send the signal to the connected sensor.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    items:
> +      - description: FSYNC RX ID, needs to match the TX ID configured in the deserializer.
> +        minimum: 0
> +        maximum: 31
> +        default: 0
> +      - description:
> +          Output GPIO pin used for sending the FSYNC to the sensor. The pin, however, needs
> +          to be excluded from the gpiochip using the gpio-reserved-ranges property since
> +          it will be used exclusively for FSYNC generation.
> +        minimum: 0
> +        maximum: 10
> +        default: 0
> +
>    reg:
>      maxItems: 1
>  
> @@ -113,6 +138,9 @@ examples:
>              #gpio-cells = <2>;
>              #clock-cells = <0>;
>  
> +            gpio-reserved-ranges = <0 1>;
> +            maxim,fsync-config = <0 0>;
> +
>              ports {
>                  #address-cells = <1>;
>                  #size-cells = <0>;
> -- 
> 2.34.1
> 

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

Re: [PATCH 3/5] dt-bindings: i2c: maxim,max96717: add new properties

[Rob Herring]

On Tue, Feb 11, 2025 at 06:46:10PM +0000, Conor Dooley wrote:
> On Fri, Feb 07, 2025 at 01:29:55PM +0200, Laurentiu Palcu wrote:
> > Add 'maxim,override-mode' property to allow the user to toggle the pin
> > configured chip operation mode and 'maxim,fsync-config' to configure the
> > chip for relaying a frame synchronization signal, received from
> > deserializer, to the attached sensor. The latter is needed for
> > synchronizing the images in multi-sensor setups.
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> >  .../bindings/media/i2c/maxim,max96717.yaml    | 28 +++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > index d1e8ba6e368ec..fae578d55fd4d 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > @@ -42,10 +42,35 @@ properties:
> >        number must be in range of [0, 10].
> >  
> >    gpio-controller: true
> > +  gpio-reserved-ranges: true
> >  
> >    '#clock-cells':
> >      const: 0
> >  
> > +  maxim,override-mode:
> > +    description: Toggle the operation mode from the pin configured one.
> > +    type: boolean
> 
> type: flag

Err, no.

You can do as-is or:

$ref: /schemas/types.yaml#/definitions/flag

I am neutral as to which way. If I wasn't we'd make the meta-schema 
enforce one way or the other.

Rob

Re: [PATCH 3/5] dt-bindings: i2c: maxim,max96717: add new properties

[Rob Herring (Arm)]

On Fri, 07 Feb 2025 13:29:55 +0200, Laurentiu Palcu wrote:
> Add 'maxim,override-mode' property to allow the user to toggle the pin
> configured chip operation mode and 'maxim,fsync-config' to configure the
> chip for relaying a frame synchronization signal, received from
> deserializer, to the attached sensor. The latter is needed for
> synchronizing the images in multi-sensor setups.
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  .../bindings/media/i2c/maxim,max96717.yaml    | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 3/5] dt-bindings: i2c: maxim,max96717: add new properties

[Conor Dooley]

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

On Wed, Feb 12, 2025 at 11:42:09AM -0600, Rob Herring wrote:
> On Tue, Feb 11, 2025 at 06:46:10PM +0000, Conor Dooley wrote:
> > On Fri, Feb 07, 2025 at 01:29:55PM +0200, Laurentiu Palcu wrote:
> > > Add 'maxim,override-mode' property to allow the user to toggle the pin
> > > configured chip operation mode and 'maxim,fsync-config' to configure the
> > > chip for relaying a frame synchronization signal, received from
> > > deserializer, to the attached sensor. The latter is needed for
> > > synchronizing the images in multi-sensor setups.
> > > 
> > > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > > ---
> > >  .../bindings/media/i2c/maxim,max96717.yaml    | 28 +++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > > index d1e8ba6e368ec..fae578d55fd4d 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > > @@ -42,10 +42,35 @@ properties:
> > >        number must be in range of [0, 10].
> > >  
> > >    gpio-controller: true
> > > +  gpio-reserved-ranges: true
> > >  
> > >    '#clock-cells':
> > >      const: 0
> > >  
> > > +  maxim,override-mode:
> > > +    description: Toggle the operation mode from the pin configured one.
> > > +    type: boolean
> > 
> > type: flag
> 
> Err, no.
> 
> You can do as-is or:
> 
> $ref: /schemas/types.yaml#/definitions/flag

Eh, that's sloppy. I must have been rushing or distracted. Sorry.

> I am neutral as to which way. If I wasn't we'd make the meta-schema 
> enforce one way or the other.

I'm biased towards flag, since I've seen confusion about setting the
boolean ones to false to disable them a bunch.

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

Re: [PATCH 1/5] media/i2c: max96717: change internal regulator voltage

[Julien Massot]

Hi Laurentiu,

Thanks for your patch,
On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> The Programming Notes section of the specifications states:
> 
> """
> MANDATORY REGISTER PROGRAMMING
> Make the following register writes to ensure proper operation of the
> MAX96717F. Without these writes, the operation of the device specified
> in the data sheet cannot be guaranteed.
> Set bits [6:4] = 3'b001 in register 0x302
> """
> 
> Set this register before going on with the chip initialization.
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  drivers/media/i2c/max96717.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> index 9259d58ba734e..b1116aade0687 100644
> --- a/drivers/media/i2c/max96717.c
> +++ b/drivers/media/i2c/max96717.c
> @@ -78,6 +78,15 @@
>  #define MAX96717_GPIO_TX_EN       BIT(1)
>  #define MAX96717_GPIO_OUT_DIS     BIT(0)
>  
> +/* CMU */
> +#define MAX96717_CMU_CMU2		CCI_REG8(0x0302)
> +#define MAX96717_PFDDIV_RSHORT_MASK	GENMASK(6, 4)
> +#define MAX96717_PFDDIV_RSHORT_SHIFT	4
> +#define MAX96717_PFDDIV_VREG_1V0	0
> +#define MAX96717_PFDDIV_VREG_1V1	1
> +#define MAX96717_PFDDIV_VREG_0V875	2
> +#define MAX96717_PFDDIV_VREG_0V94	3
> +
>  /* FRONTTOP */
>  /* MAX96717 only have CSI port 'B' */
>  #define MAX96717_FRONTOP0     CCI_REG8(0x308)
> @@ -981,6 +990,14 @@ static int max96717_hw_init(struct max96717_priv *priv)
>  	dev_dbg(dev, "Found %x (rev %lx)\n", (u8)dev_id,
>  		(u8)val & MAX96717_DEV_REV_MASK);
>  
> +	/*
> +	 * According to specs, in the Programming Notes section, there's a mandatory register
> +	 * programming notice that advises to enable the 1.1V internal regulator to guarantee
> proper
> +	 * device operation. Let's do this before any other operations.
> +	 */
Latest MAX96717{,K,F} revision 6 seems not affected by this issue. Can you please make this write
conditional to rev < 6 ? Register 0xe.

> +	cci_write(priv->regmap, MAX96717_CMU_CMU2,
> +		  MAX96717_PFDDIV_VREG_1V1 << MAX96717_PFDDIV_RSHORT_SHIFT, NULL);
> +
>  	ret = cci_read(priv->regmap, MAX96717_MIPI_RX_EXT11, &val, NULL);
>  	if (ret)
>  		return dev_err_probe(dev, ret,
Regards,
-- 
Julien

Re: [PATCH 2/5] media/i2c: max96717: implement the .get_frame_desc() operation

[Julien Massot]

Hi,

On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> Since the max96717 serializer can work with various sensors, we need to
> implement the .get_frame_desc() callback to get the VCs and DTs for the
> incoming stream(s).
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  drivers/media/i2c/max96717.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> index b1116aade0687..6a668a004c717 100644
> --- a/drivers/media/i2c/max96717.c
> +++ b/drivers/media/i2c/max96717.c
> @@ -575,12 +575,33 @@ static int max96717_disable_streams(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int max96717_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				   struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct max96717_priv *priv = sd_to_max96717(sd);
> +	int ret;
> +	struct v4l2_mbus_frame_desc source_fd;
> +
> +	if (pad != MAX96717_PAD_SOURCE)
> +		return -EINVAL;
> +
Please check priv->source_sd first, we support the case where we only have a test pattern from
the serializer. Then we can simply return the result of v4l2_subdev_call.

        return v4l2_subdev_call(priv->source_sd, pad, get_frame_desc,
			       priv->source_sd_pad, fd);

> +	ret = v4l2_subdev_call(priv->source_sd, pad, get_frame_desc,
> +			       priv->source_sd_pad, &source_fd);
> +	if (ret)
> +		return ret;
> +
> +	*fd = source_fd;
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_subdev_pad_ops max96717_pad_ops = {
>  	.enable_streams = max96717_enable_streams,
>  	.disable_streams = max96717_disable_streams,
>  	.set_routing = max96717_set_routing,
>  	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = max96717_set_fmt,
> +	.get_frame_desc = max96717_get_frame_desc,
>  };
>  
>  static const struct v4l2_subdev_core_ops max96717_subdev_core_ops = {

Regards,
-- 
Julien

Re: [PATCH 3/5] dt-bindings: i2c: maxim,max96717: add new properties

[Julien Massot]

Hi Laurentiu,

Thanks for your patch

On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> Add 'maxim,override-mode' property to allow the user to toggle the pin
> configured chip operation mode and 'maxim,fsync-config' to configure the
> chip for relaying a frame synchronization signal, received from
> deserializer, to the attached sensor. The latter is needed for
> synchronizing the images in multi-sensor setups.
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  .../bindings/media/i2c/maxim,max96717.yaml    | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> index d1e8ba6e368ec..fae578d55fd4d 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> @@ -42,10 +42,35 @@ properties:
>        number must be in range of [0, 10].
>  
>    gpio-controller: true
> +  gpio-reserved-ranges: true
>  
>    '#clock-cells':
>      const: 0
>  
> +  maxim,override-mode:
> +    description: Toggle the operation mode from the pin configured one.
> +    type: boolean
I understand that this property is intended to flip the GMSL link mode between
pixel and tunnel mode.
What about adding a property 'maxim,tunnel-mode' to the GMSL 'port@1'.
Here the MAX96717 only have one GMSL port but other devices, such as MAX96724 can
have 2 GMSL link and may have each link in different mode.

> 
> +
> +  maxim,fsync-config:
> +    description:
> +      Frame synchronization (FSYNC) is used to align images sent from multiple
> +      sources in surround-view applications and is required for concatenation.
> +      In FSYNC mode, the deserializer sends a sync signal to each serializer;
> +      the serializers then send the signal to the connected sensor.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    items:
> +      - description: FSYNC RX ID, needs to match the TX ID configured in the deserializer.
> +        minimum: 0
> +        maximum: 31
> +        default: 0
> +      - description:
> +          Output GPIO pin used for sending the FSYNC to the sensor. The pin, however, needs
> +          to be excluded from the gpiochip using the gpio-reserved-ranges property since
> +          it will be used exclusively for FSYNC generation.
> +        minimum: 0
> +        maximum: 10
> +        default: 0
> +

MAX96717 do not have any knowledge of the frame synchronisation, but this device can forward some
GPIO to/from the deserializer.

GPIO forwarding need some information 
- The local GPIO number
- The forwarding direction Rx, Tx, Bi-directionnal
- The GPIO ID on the GMSL link (RX_ID/TX_ID)

Can we add a maxim,forward-gpio property reflecting that instead ?

>    reg:
>      maxItems: 1
>  
> @@ -113,6 +138,9 @@ examples:
>              #gpio-cells = <2>;
>              #clock-cells = <0>;
>  
> +            gpio-reserved-ranges = <0 1>;
> +            maxim,fsync-config = <0 0>;
> +
>              ports {
>                  #address-cells = <1>;
>                  #size-cells = <0>;

Regards,
-- 
Julien

Re: [PATCH 4/5] media/i2c: max96717: add FSYNC support

[Julien Massot]

Hi Laurentiu,

Thanks for your patch
On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> According to specs:
> 
> """
> Frame synchronization (FSYNC) is used to align images sent from multiple
> sources in surround-view applications and is required for concatenation.
> In FSYNC mode, the GMSL2 CSI-2 quad deserializer sends a sync signal to
> each serializer; the serializers then send the signal to the connected
> sensor.
> """
> 
> Since max96717 can be used in multi-sensor setups, we need FSYNC
> support. For that, I added a DT property("maxim,fsync-config") that will
> be used to configure the frame sync output pin and the RX ID of the
> GPIO as sent by the deserializer chip.
> 
> Also, add the .request() callback for the gpiochip so that we can use
> 'gpio-reserved-ranges' in DT to exclude the pins that are used for
> FSYNC from being used as GPIOs.
> 
> 
> I'm seeing different features in this patch:
- Adding the request callback 
- Adding GPIO forwarding 
- Adding support to some pinctrl features such as pullup/pulldown



> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  drivers/media/i2c/max96717.c | 87 ++++++++++++++++++++++++++++++++----
>  1 file changed, 79 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> index 6a668a004c717..47a3be195a971 100644
> --- a/drivers/media/i2c/max96717.c
> +++ b/drivers/media/i2c/max96717.c
> @@ -70,13 +70,28 @@
>  #define MAX96717_VTX_CHKB_ALT          CCI_REG8(0x275)
>  
>  /* GPIO */
> -#define MAX96717_NUM_GPIO         11
> -#define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
> -#define MAX96717_GPIO_OUT         BIT(4)
> -#define MAX96717_GPIO_IN          BIT(3)
> -#define MAX96717_GPIO_RX_EN       BIT(2)
> -#define MAX96717_GPIO_TX_EN       BIT(1)
> -#define MAX96717_GPIO_OUT_DIS     BIT(0)
> +#define MAX96717_NUM_GPIO		11
> +#define MAX96717_GPIO_REG_A(gpio)	CCI_REG8(0x2be + (gpio) * 3)
> +#define MAX96717_GPIO_OUT		BIT(4)
> +#define MAX96717_GPIO_IN		BIT(3)
> +#define MAX96717_GPIO_RX_EN		BIT(2)
> +#define MAX96717_GPIO_TX_EN		BIT(1)
> +#define MAX96717_GPIO_OUT_DIS		BIT(0)
> +#define MAX96717_GPIO_REG_B(gpio)	CCI_REG8(0x2bf + (gpio) * 3)
> +#define MAX96717_GPIO_TX_ID_MASK	GENMASK(4, 0)
> +#define MAX96717_GPIO_TX_ID_SHIFT	0
> +#define MAX96717_OUT_TYPE		BIT(5)
> +#define MAX96717_OUT_TYPE_PUSH_PULL	BIT(5)
> +#define MAX96717_OUT_TYPE_OPEN_DRAIN	0
> +#define MAX96717_PULL_UPDN_SEL_MASK	GENMASK(7, 6)
> +#define MAX96717_PULL_UPDN_SEL_SHIFT	6
> +#define MAX96717_GPIO_NO_PULL		0
> +#define MAX96717_GPIO_PULL_UP		1
> +#define MAX96717_GPIO_PULL_DOWN		2
> +#define MAX96717_GPIO_REG_C(gpio)	CCI_REG8(0x2c0 + (gpio) * 3)
> +#define MAX96717_GPIO_RX_ID_MASK	GENMASK(4, 0)
> +#define MAX96717_GPIO_RX_ID_SHIFT	0
> +#define MAX96717_OVR_RES_CFG		BIT(7)
>  
>  /* CMU */
>  #define MAX96717_CMU_CMU2		CCI_REG8(0x0302)
> @@ -125,6 +140,11 @@ enum max96717_vpg_mode {
>  	MAX96717_VPG_GRADIENT = 2,
>  };
>  
> +struct max96717_fsync_desc {
> +	int pin;
> +	int rx_id;
> +};
> +
>  struct max96717_priv {
>  	struct i2c_client		  *client;
>  	struct regmap			  *regmap;
> @@ -141,6 +161,7 @@ struct max96717_priv {
>  	struct clk_hw                     clk_hw;
>  	struct gpio_chip                  gpio_chip;
>  	enum max96717_vpg_mode            pattern;
> +	struct max96717_fsync_desc	  fsync;
Here we can have multiple GPIOs forwarded.

>  };
>  
>  static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
> @@ -364,6 +385,7 @@ static int max96717_gpiochip_probe(struct max96717_priv *priv)
>  	gc->get_direction = max96717_gpio_get_direction;
>  	gc->direction_input = max96717_gpio_direction_in;
>  	gc->direction_output = max96717_gpio_direction_out;
> +	gc->request = gpiochip_generic_request;
>  	gc->set = max96717_gpiochip_set;
>  	gc->get = max96717_gpiochip_get;
>  	gc->of_gpio_n_cells = 2;
> @@ -386,6 +408,26 @@ static int max96717_gpiochip_probe(struct max96717_priv *priv)
>  	return 0;
>  }
>  
> +static int max96717_fsync_setup(struct max96717_priv *priv)
> +{
> +	int ret = 0;
> +
> +	if (priv->fsync.pin == -1)
> +		return 0;
> +
> +	cci_update_bits(priv->regmap, MAX96717_GPIO_REG_C(priv->fsync.pin),
> +			MAX96717_GPIO_RX_ID_MASK,
> +			priv->fsync.rx_id << MAX96717_GPIO_RX_ID_SHIFT, &ret);
                        FIELD_PREP(MAX96717_GPIO_RX_ID_MASK, priv->fsync.rx_id), &ret);
And you can get rid of the _SHIFT define.

> 
> +
> +	cci_update_bits(priv->regmap, MAX96717_GPIO_REG_B(priv->fsync.pin),
> +			MAX96717_PULL_UPDN_SEL_MASK,
> +			1 << MAX96717_PULL_UPDN_SEL_SHIFT, &ret);

The serializer can't guess what kind of pin configuration is required for the design.
This change deserves his own patch most likely implementing pinconf support.

> +
> +	return cci_update_bits(priv->regmap, MAX96717_GPIO_REG_A(priv->fsync.pin),
> +			       MAX96717_GPIO_RX_EN | MAX96717_GPIO_OUT_DIS,
> +			       MAX96717_GPIO_RX_EN, &ret);
> +}


> +
>  static int _max96717_set_routing(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *state,
>  				 struct v4l2_subdev_krouting *routing)
> @@ -1037,7 +1079,8 @@ static int max96717_parse_dt(struct max96717_priv *priv)
>  	struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
>  	struct fwnode_handle *ep_fwnode;
>  	unsigned char num_data_lanes;
> -	int ret;
> +	int ret, count;
> +	u32 dt_val[2];
>  
>  	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
>  						    MAX96717_PAD_SINK, 0, 0);
> @@ -1058,6 +1101,30 @@ static int max96717_parse_dt(struct max96717_priv *priv)
>  
>  	priv->mipi_csi2 = vep.bus.mipi_csi2;
>  
> +	priv->fsync.pin = -1;
> +	count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
> +	if (count > 0) {
> +		ret = fwnode_property_read_u32_array(dev_fwnode(dev), "maxim,fsync-config",
> +						     dt_val, count);
> +		if (ret) {
> +			dev_err(dev, "Unable to read FSYNC config from DT.\n");
> +			return ret;
> +		}
> +
> +		priv->fsync.rx_id = dt_val[0];
> +		if (priv->fsync.rx_id > 31) {
> +			dev_err(dev, "Wrong GPIO RX ID. Allowed: 0 -> 31\n");
> +			return -EINVAL;
> +		}
> +
> +		priv->fsync.pin = dt_val[1];
> +		if (priv->fsync.pin >= MAX96717_NUM_GPIO) {
> +			dev_err(dev, "Wrong GPIO pin used for FSYNC. Allowed: 0 -> %d\n",
> +				MAX96717_NUM_GPIO - 1);
> +			return -EINVAL;
> +		}
> +	}
> 
> +
>  	return 0;
>  }
>  
> @@ -1092,6 +1159,10 @@ static int max96717_probe(struct i2c_client *client)
>  		return dev_err_probe(&client->dev, ret,
>  				     "Failed to init gpiochip\n");
>  
> +	ret = max96717_fsync_setup(priv);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to setup FSYNC\n");
> +
Configuring GPIO forwarding can be done when calling GPIO chip probe.

>  	ret = max96717_register_clkout(priv);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to register clkout\n");

Best regards,
-- 
Julien

Re: [PATCH 5/5] media/i2c: max96717: allow user to override operation mode from DT

[Julien Massot]

Hi Laurentiu,

On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> There are situations when the CFG pins set the chip up for a certain
> mode of operation (ie: pixel mode or tunneling mode), because the HW
> designers decided this way, and we, the users, want to change that. For
> that, add an optional DT property that would allow toggling the
> operation mode from the configured one to the other one.
> 
> The driver still only supports tunneling mode, that didn't change.
> 
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
>  drivers/media/i2c/max96717.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> index 47a3be195a971..a591ca5d5f44f 100644
> --- a/drivers/media/i2c/max96717.c
> +++ b/drivers/media/i2c/max96717.c


enum gmsl2_mode {
  GMSL2_PIXEL_MODE,
  GMSL2_MODE_TUNNEL,
};

> @@ -161,6 +161,7 @@ struct max96717_priv {
>  	struct clk_hw                     clk_hw;
>  	struct gpio_chip                  gpio_chip;
>  	enum max96717_vpg_mode            pattern;
> +	bool				  mode_override;
	enum gmsl2_mode                   mode;
I would prefer to set the mode in an explicit way instead of toggling
the bit in the register.

>  	struct max96717_fsync_desc	  fsync;
>  };
>  
> @@ -1066,6 +1067,14 @@ static int max96717_hw_init(struct max96717_priv *priv)
>  		return dev_err_probe(dev, ret,
>  				     "Fail to read mipi rx extension");
>  
> +	if (priv->mode_override) {
        if (priv->mode_override && priv->mode == GMSL2_MODE_TUNNEL) {
> +		
> +
> +		ret = cci_write(priv->regmap, MAX96717_MIPI_RX_EXT11, val, NULL);
		ret = cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, MAX96717_TUN_MODE,
					MAX96717_TUN_MODE, NULL);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Unable to update operation mode\n");
> +	}
> +
In case we are overwriting the mode to tunnel mode then no need to read the EXT11 register.

>  	if (!(val & MAX96717_TUN_MODE))
>  		return dev_err_probe(dev, -EOPNOTSUPP,
>  				     "Only supporting tunnel mode");

In fact the driver can works in pixel mode, but since we don't set the "stream id" the
deserializer have to configured with the right one :)

> @@ -1101,6 +1110,9 @@ static int max96717_parse_dt(struct max96717_priv *priv)
>  
>  	priv->mipi_csi2 = vep.bus.mipi_csi2;
>  
> +	if (fwnode_property_present(dev_fwnode(dev), "maxim,cfg-mode-override"))
> +		priv->mode_override = true;
> +
	source_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
							MAX96717_PAD_SOURCE, 0, 0);
	if (fwnode_property_present(source_fwnode, "maxim,tunnel-mode")) {
		priv->mode_override = true;
		priv->mode = GMSL2_MODE_TUNNEL;
	}
So we can parse the tunnel property from the GMSL port.

>  	priv->fsync.pin = -1;
>  	count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
>  	if (count > 0) {

Best Regards,

-- 
Julien

Re: [PATCH 1/5] media/i2c: max96717: change internal regulator voltage

[Laurentiu Palcu]

Hi Julien,

Thank you for taking some time to review this series and sorry for the
late reply... :/ I have been involved in other more pressing stuff
lately...

On Tue, Feb 18, 2025 at 02:14:57PM +0100, Julien Massot wrote:
> Hi Laurentiu,
> 
> Thanks for your patch,
> On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> > The Programming Notes section of the specifications states:
> > 
> > """
> > MANDATORY REGISTER PROGRAMMING
> > Make the following register writes to ensure proper operation of the
> > MAX96717F. Without these writes, the operation of the device specified
> > in the data sheet cannot be guaranteed.
> > Set bits [6:4] = 3'b001 in register 0x302
> > """
> > 
> > Set this register before going on with the chip initialization.
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> >  drivers/media/i2c/max96717.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> > index 9259d58ba734e..b1116aade0687 100644
> > --- a/drivers/media/i2c/max96717.c
> > +++ b/drivers/media/i2c/max96717.c
> > @@ -78,6 +78,15 @@
> >  #define MAX96717_GPIO_TX_EN       BIT(1)
> >  #define MAX96717_GPIO_OUT_DIS     BIT(0)
> >  
> > +/* CMU */
> > +#define MAX96717_CMU_CMU2		CCI_REG8(0x0302)
> > +#define MAX96717_PFDDIV_RSHORT_MASK	GENMASK(6, 4)
> > +#define MAX96717_PFDDIV_RSHORT_SHIFT	4
> > +#define MAX96717_PFDDIV_VREG_1V0	0
> > +#define MAX96717_PFDDIV_VREG_1V1	1
> > +#define MAX96717_PFDDIV_VREG_0V875	2
> > +#define MAX96717_PFDDIV_VREG_0V94	3
> > +
> >  /* FRONTTOP */
> >  /* MAX96717 only have CSI port 'B' */
> >  #define MAX96717_FRONTOP0     CCI_REG8(0x308)
> > @@ -981,6 +990,14 @@ static int max96717_hw_init(struct max96717_priv *priv)
> >  	dev_dbg(dev, "Found %x (rev %lx)\n", (u8)dev_id,
> >  		(u8)val & MAX96717_DEV_REV_MASK);
> >  
> > +	/*
> > +	 * According to specs, in the Programming Notes section, there's a mandatory register
> > +	 * programming notice that advises to enable the 1.1V internal regulator to guarantee
> > proper
> > +	 * device operation. Let's do this before any other operations.
> > +	 */
> Latest MAX96717{,K,F} revision 6 seems not affected by this issue. Can you please make this write
> conditional to rev < 6 ? Register 0xe.

Ok, makes sense.

Thanks,
Laurentiu
> 
> > +	cci_write(priv->regmap, MAX96717_CMU_CMU2,
> > +		  MAX96717_PFDDIV_VREG_1V1 << MAX96717_PFDDIV_RSHORT_SHIFT, NULL);
> > +
> >  	ret = cci_read(priv->regmap, MAX96717_MIPI_RX_EXT11, &val, NULL);
> >  	if (ret)
> >  		return dev_err_probe(dev, ret,
> Regards,
> -- 
> Julien

Re: [PATCH 2/5] media/i2c: max96717: implement the .get_frame_desc() operation

[Laurentiu Palcu]

Hi,

On Tue, Feb 18, 2025 at 02:29:30PM +0100, Julien Massot wrote:
> Hi,
> 
> On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> > Since the max96717 serializer can work with various sensors, we need to
> > implement the .get_frame_desc() callback to get the VCs and DTs for the
> > incoming stream(s).
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> >  drivers/media/i2c/max96717.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> > index b1116aade0687..6a668a004c717 100644
> > --- a/drivers/media/i2c/max96717.c
> > +++ b/drivers/media/i2c/max96717.c
> > @@ -575,12 +575,33 @@ static int max96717_disable_streams(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > +static int max96717_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > +				   struct v4l2_mbus_frame_desc *fd)
> > +{
> > +	struct max96717_priv *priv = sd_to_max96717(sd);
> > +	int ret;
> > +	struct v4l2_mbus_frame_desc source_fd;
> > +
> > +	if (pad != MAX96717_PAD_SOURCE)
> > +		return -EINVAL;
> > +
> Please check priv->source_sd first, we support the case where we only have a test pattern from
> the serializer. Then we can simply return the result of v4l2_subdev_call.
> 
>         return v4l2_subdev_call(priv->source_sd, pad, get_frame_desc,
> 			       priv->source_sd_pad, fd);

Ok, I guess I missed the test pattern functionality completely.

Thanks,
Laurentiu

> 
> > +	ret = v4l2_subdev_call(priv->source_sd, pad, get_frame_desc,
> > +			       priv->source_sd_pad, &source_fd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*fd = source_fd;
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct v4l2_subdev_pad_ops max96717_pad_ops = {
> >  	.enable_streams = max96717_enable_streams,
> >  	.disable_streams = max96717_disable_streams,
> >  	.set_routing = max96717_set_routing,
> >  	.get_fmt = v4l2_subdev_get_fmt,
> >  	.set_fmt = max96717_set_fmt,
> > +	.get_frame_desc = max96717_get_frame_desc,
> >  };
> >  
> >  static const struct v4l2_subdev_core_ops max96717_subdev_core_ops = {
> 
> Regards,
> -- 
> Julien

Re: [PATCH 3/5] dt-bindings: i2c: maxim,max96717: add new properties

[Laurentiu Palcu]

Hi,

On Tue, Feb 18, 2025 at 02:54:07PM +0100, Julien Massot wrote:
> Hi Laurentiu,
> 
> Thanks for your patch
> 
> On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> > Add 'maxim,override-mode' property to allow the user to toggle the pin
> > configured chip operation mode and 'maxim,fsync-config' to configure the
> > chip for relaying a frame synchronization signal, received from
> > deserializer, to the attached sensor. The latter is needed for
> > synchronizing the images in multi-sensor setups.
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> >  .../bindings/media/i2c/maxim,max96717.yaml    | 28 +++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > index d1e8ba6e368ec..fae578d55fd4d 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
> > @@ -42,10 +42,35 @@ properties:
> >        number must be in range of [0, 10].
> >  
> >    gpio-controller: true
> > +  gpio-reserved-ranges: true
> >  
> >    '#clock-cells':
> >      const: 0
> >  
> > +  maxim,override-mode:
> > +    description: Toggle the operation mode from the pin configured one.
> > +    type: boolean
> I understand that this property is intended to flip the GMSL link mode between
> pixel and tunnel mode.
> What about adding a property 'maxim,tunnel-mode' to the GMSL 'port@1'.
> Here the MAX96717 only have one GMSL port but other devices, such as MAX96724 can
> have 2 GMSL link and may have each link in different mode.

I'm OK with moving the property inside "port@1". But I have some
concerns about the logic. So. 'maxim,tunnel-mode' presence would
indicate that we want to force the functioning mode to "tunnel". But
what if it's absent? Do we use the pin configuration? What if the pin
configuration is "tunnel" and the user wants to override the mode to
"pixel"? In this case 'maxim,tunnel-mode' doesn't really work...
Am I missing something here?

> 
> > 
> > +
> > +  maxim,fsync-config:
> > +    description:
> > +      Frame synchronization (FSYNC) is used to align images sent from multiple
> > +      sources in surround-view applications and is required for concatenation.
> > +      In FSYNC mode, the deserializer sends a sync signal to each serializer;
> > +      the serializers then send the signal to the connected sensor.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    items:
> > +      - description: FSYNC RX ID, needs to match the TX ID configured in the deserializer.
> > +        minimum: 0
> > +        maximum: 31
> > +        default: 0
> > +      - description:
> > +          Output GPIO pin used for sending the FSYNC to the sensor. The pin, however, needs
> > +          to be excluded from the gpiochip using the gpio-reserved-ranges property since
> > +          it will be used exclusively for FSYNC generation.
> > +        minimum: 0
> > +        maximum: 10
> > +        default: 0
> > +
> 
> MAX96717 do not have any knowledge of the frame synchronisation, but this device can forward some
> GPIO to/from the deserializer.

Good point.

> 
> GPIO forwarding need some information 
> - The local GPIO number
> - The forwarding direction Rx, Tx, Bi-directionnal
> - The GPIO ID on the GMSL link (RX_ID/TX_ID)
> 
> Can we add a maxim,forward-gpio property reflecting that instead ?

Agreed.

Thanks,
Laurentiu

> 
> >    reg:
> >      maxItems: 1
> >  
> > @@ -113,6 +138,9 @@ examples:
> >              #gpio-cells = <2>;
> >              #clock-cells = <0>;
> >  
> > +            gpio-reserved-ranges = <0 1>;
> > +            maxim,fsync-config = <0 0>;
> > +
> >              ports {
> >                  #address-cells = <1>;
> >                  #size-cells = <0>;
> 
> Regards,
> -- 
> Julien

Re: [PATCH 4/5] media/i2c: max96717: add FSYNC support

[Laurentiu Palcu]

Hi Julien,

On Tue, Feb 18, 2025 at 03:46:02PM +0100, Julien Massot wrote:
> Hi Laurentiu,
> 
> Thanks for your patch
> On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> > According to specs:
> > 
> > """
> > Frame synchronization (FSYNC) is used to align images sent from multiple
> > sources in surround-view applications and is required for concatenation.
> > In FSYNC mode, the GMSL2 CSI-2 quad deserializer sends a sync signal to
> > each serializer; the serializers then send the signal to the connected
> > sensor.
> > """
> > 
> > Since max96717 can be used in multi-sensor setups, we need FSYNC
> > support. For that, I added a DT property("maxim,fsync-config") that will
> > be used to configure the frame sync output pin and the RX ID of the
> > GPIO as sent by the deserializer chip.
> > 
> > Also, add the .request() callback for the gpiochip so that we can use
> > 'gpio-reserved-ranges' in DT to exclude the pins that are used for
> > FSYNC from being used as GPIOs.
> > 
> > 
> > I'm seeing different features in this patch:
> - Adding the request callback 
> - Adding GPIO forwarding 
> - Adding support to some pinctrl features such as pullup/pulldown

Ok, I'll add support for pinctrl in a different patch. Though, I don't
believe a separate patch is needed for adding the gpiochip request
callback.

> 
> 
> 
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> >  drivers/media/i2c/max96717.c | 87 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 79 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> > index 6a668a004c717..47a3be195a971 100644
> > --- a/drivers/media/i2c/max96717.c
> > +++ b/drivers/media/i2c/max96717.c
> > @@ -70,13 +70,28 @@
> >  #define MAX96717_VTX_CHKB_ALT          CCI_REG8(0x275)
> >  
> >  /* GPIO */
> > -#define MAX96717_NUM_GPIO         11
> > -#define MAX96717_GPIO_REG_A(gpio) CCI_REG8(0x2be + (gpio) * 3)
> > -#define MAX96717_GPIO_OUT         BIT(4)
> > -#define MAX96717_GPIO_IN          BIT(3)
> > -#define MAX96717_GPIO_RX_EN       BIT(2)
> > -#define MAX96717_GPIO_TX_EN       BIT(1)
> > -#define MAX96717_GPIO_OUT_DIS     BIT(0)
> > +#define MAX96717_NUM_GPIO		11
> > +#define MAX96717_GPIO_REG_A(gpio)	CCI_REG8(0x2be + (gpio) * 3)
> > +#define MAX96717_GPIO_OUT		BIT(4)
> > +#define MAX96717_GPIO_IN		BIT(3)
> > +#define MAX96717_GPIO_RX_EN		BIT(2)
> > +#define MAX96717_GPIO_TX_EN		BIT(1)
> > +#define MAX96717_GPIO_OUT_DIS		BIT(0)
> > +#define MAX96717_GPIO_REG_B(gpio)	CCI_REG8(0x2bf + (gpio) * 3)
> > +#define MAX96717_GPIO_TX_ID_MASK	GENMASK(4, 0)
> > +#define MAX96717_GPIO_TX_ID_SHIFT	0
> > +#define MAX96717_OUT_TYPE		BIT(5)
> > +#define MAX96717_OUT_TYPE_PUSH_PULL	BIT(5)
> > +#define MAX96717_OUT_TYPE_OPEN_DRAIN	0
> > +#define MAX96717_PULL_UPDN_SEL_MASK	GENMASK(7, 6)
> > +#define MAX96717_PULL_UPDN_SEL_SHIFT	6
> > +#define MAX96717_GPIO_NO_PULL		0
> > +#define MAX96717_GPIO_PULL_UP		1
> > +#define MAX96717_GPIO_PULL_DOWN		2
> > +#define MAX96717_GPIO_REG_C(gpio)	CCI_REG8(0x2c0 + (gpio) * 3)
> > +#define MAX96717_GPIO_RX_ID_MASK	GENMASK(4, 0)
> > +#define MAX96717_GPIO_RX_ID_SHIFT	0
> > +#define MAX96717_OVR_RES_CFG		BIT(7)
> >  
> >  /* CMU */
> >  #define MAX96717_CMU_CMU2		CCI_REG8(0x0302)
> > @@ -125,6 +140,11 @@ enum max96717_vpg_mode {
> >  	MAX96717_VPG_GRADIENT = 2,
> >  };
> >  
> > +struct max96717_fsync_desc {
> > +	int pin;
> > +	int rx_id;
> > +};
> > +
> >  struct max96717_priv {
> >  	struct i2c_client		  *client;
> >  	struct regmap			  *regmap;
> > @@ -141,6 +161,7 @@ struct max96717_priv {
> >  	struct clk_hw                     clk_hw;
> >  	struct gpio_chip                  gpio_chip;
> >  	enum max96717_vpg_mode            pattern;
> > +	struct max96717_fsync_desc	  fsync;
> Here we can have multiple GPIOs forwarded.

Now that you made the point of the serializer not being aware of FSYNC,
it makes sense to change this in order to allow more forwarded pins.

> 
> >  };
> >  
> >  static inline struct max96717_priv *sd_to_max96717(struct v4l2_subdev *sd)
> > @@ -364,6 +385,7 @@ static int max96717_gpiochip_probe(struct max96717_priv *priv)
> >  	gc->get_direction = max96717_gpio_get_direction;
> >  	gc->direction_input = max96717_gpio_direction_in;
> >  	gc->direction_output = max96717_gpio_direction_out;
> > +	gc->request = gpiochip_generic_request;
> >  	gc->set = max96717_gpiochip_set;
> >  	gc->get = max96717_gpiochip_get;
> >  	gc->of_gpio_n_cells = 2;
> > @@ -386,6 +408,26 @@ static int max96717_gpiochip_probe(struct max96717_priv *priv)
> >  	return 0;
> >  }
> >  
> > +static int max96717_fsync_setup(struct max96717_priv *priv)
> > +{
> > +	int ret = 0;
> > +
> > +	if (priv->fsync.pin == -1)
> > +		return 0;
> > +
> > +	cci_update_bits(priv->regmap, MAX96717_GPIO_REG_C(priv->fsync.pin),
> > +			MAX96717_GPIO_RX_ID_MASK,
> > +			priv->fsync.rx_id << MAX96717_GPIO_RX_ID_SHIFT, &ret);
>                         FIELD_PREP(MAX96717_GPIO_RX_ID_MASK, priv->fsync.rx_id), &ret);
> And you can get rid of the _SHIFT define.

OK

> 
> > 
> > +
> > +	cci_update_bits(priv->regmap, MAX96717_GPIO_REG_B(priv->fsync.pin),
> > +			MAX96717_PULL_UPDN_SEL_MASK,
> > +			1 << MAX96717_PULL_UPDN_SEL_SHIFT, &ret);
> 
> The serializer can't guess what kind of pin configuration is required for the design.
> This change deserves his own patch most likely implementing pinconf support.

I will have a look at this and, hopefully, come up with a separate patch
adding support for pinctrl.

> 
> > +
> > +	return cci_update_bits(priv->regmap, MAX96717_GPIO_REG_A(priv->fsync.pin),
> > +			       MAX96717_GPIO_RX_EN | MAX96717_GPIO_OUT_DIS,
> > +			       MAX96717_GPIO_RX_EN, &ret);
> > +}
> 
> 
> > +
> >  static int _max96717_set_routing(struct v4l2_subdev *sd,
> >  				 struct v4l2_subdev_state *state,
> >  				 struct v4l2_subdev_krouting *routing)
> > @@ -1037,7 +1079,8 @@ static int max96717_parse_dt(struct max96717_priv *priv)
> >  	struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
> >  	struct fwnode_handle *ep_fwnode;
> >  	unsigned char num_data_lanes;
> > -	int ret;
> > +	int ret, count;
> > +	u32 dt_val[2];
> >  
> >  	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
> >  						    MAX96717_PAD_SINK, 0, 0);
> > @@ -1058,6 +1101,30 @@ static int max96717_parse_dt(struct max96717_priv *priv)
> >  
> >  	priv->mipi_csi2 = vep.bus.mipi_csi2;
> >  
> > +	priv->fsync.pin = -1;
> > +	count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
> > +	if (count > 0) {
> > +		ret = fwnode_property_read_u32_array(dev_fwnode(dev), "maxim,fsync-config",
> > +						     dt_val, count);
> > +		if (ret) {
> > +			dev_err(dev, "Unable to read FSYNC config from DT.\n");
> > +			return ret;
> > +		}
> > +
> > +		priv->fsync.rx_id = dt_val[0];
> > +		if (priv->fsync.rx_id > 31) {
> > +			dev_err(dev, "Wrong GPIO RX ID. Allowed: 0 -> 31\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		priv->fsync.pin = dt_val[1];
> > +		if (priv->fsync.pin >= MAX96717_NUM_GPIO) {
> > +			dev_err(dev, "Wrong GPIO pin used for FSYNC. Allowed: 0 -> %d\n",
> > +				MAX96717_NUM_GPIO - 1);
> > +			return -EINVAL;
> > +		}
> > +	}
> > 
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1092,6 +1159,10 @@ static int max96717_probe(struct i2c_client *client)
> >  		return dev_err_probe(&client->dev, ret,
> >  				     "Failed to init gpiochip\n");
> >  
> > +	ret = max96717_fsync_setup(priv);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "Failed to setup FSYNC\n");
> > +
> Configuring GPIO forwarding can be done when calling GPIO chip probe.

OK.

Thanks,
Laurentiu

> 
> >  	ret = max96717_register_clkout(priv);
> >  	if (ret)
> >  		return dev_err_probe(dev, ret, "Failed to register clkout\n");
> 
> Best regards,
> -- 
> Julien

Re: [PATCH 5/5] media/i2c: max96717: allow user to override operation mode from DT

[Laurentiu Palcu]

Hi Julien,

On Tue, Feb 18, 2025 at 04:21:36PM +0100, Julien Massot wrote:
> Hi Laurentiu,
> 
> On Fri, 2025-02-07 at 13:29 +0200, Laurentiu Palcu wrote:
> > There are situations when the CFG pins set the chip up for a certain
> > mode of operation (ie: pixel mode or tunneling mode), because the HW
> > designers decided this way, and we, the users, want to change that. For
> > that, add an optional DT property that would allow toggling the
> > operation mode from the configured one to the other one.
> > 
> > The driver still only supports tunneling mode, that didn't change.
> > 
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> >  drivers/media/i2c/max96717.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> > index 47a3be195a971..a591ca5d5f44f 100644
> > --- a/drivers/media/i2c/max96717.c
> > +++ b/drivers/media/i2c/max96717.c
> 
> 
> enum gmsl2_mode {
>   GMSL2_PIXEL_MODE,
>   GMSL2_MODE_TUNNEL,
> };
> 
> > @@ -161,6 +161,7 @@ struct max96717_priv {
> >  	struct clk_hw                     clk_hw;
> >  	struct gpio_chip                  gpio_chip;
> >  	enum max96717_vpg_mode            pattern;
> > +	bool				  mode_override;
> 	enum gmsl2_mode                   mode;
> I would prefer to set the mode in an explicit way instead of toggling
> the bit in the register.
> 
> >  	struct max96717_fsync_desc	  fsync;
> >  };
> >  
> > @@ -1066,6 +1067,14 @@ static int max96717_hw_init(struct max96717_priv *priv)
> >  		return dev_err_probe(dev, ret,
> >  				     "Fail to read mipi rx extension");
> >  
> > +	if (priv->mode_override) {
>         if (priv->mode_override && priv->mode == GMSL2_MODE_TUNNEL) {
> > +		
> > +
> > +		ret = cci_write(priv->regmap, MAX96717_MIPI_RX_EXT11, val, NULL);
> 		ret = cci_update_bits(priv->regmap, MAX96717_MIPI_RX_EXT11, MAX96717_TUN_MODE,
> 					MAX96717_TUN_MODE, NULL);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret, "Unable to update operation mode\n");
> > +	}
> > +
> In case we are overwriting the mode to tunnel mode then no need to read the EXT11 register.
> 
> >  	if (!(val & MAX96717_TUN_MODE))
> >  		return dev_err_probe(dev, -EOPNOTSUPP,
> >  				     "Only supporting tunnel mode");
> 
> In fact the driver can works in pixel mode, but since we don't set the "stream id" the
> deserializer have to configured with the right one :)
> 
> > @@ -1101,6 +1110,9 @@ static int max96717_parse_dt(struct max96717_priv *priv)
> >  
> >  	priv->mipi_csi2 = vep.bus.mipi_csi2;
> >  
> > +	if (fwnode_property_present(dev_fwnode(dev), "maxim,cfg-mode-override"))
> > +		priv->mode_override = true;
> > +
> 	source_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
> 							MAX96717_PAD_SOURCE, 0, 0);
> 	if (fwnode_property_present(source_fwnode, "maxim,tunnel-mode")) {
> 		priv->mode_override = true;
> 		priv->mode = GMSL2_MODE_TUNNEL;
> 	}

So, I don't think the boolean 'maxim,tunnel-mode' would work well when
the pin configuration is 'tunnel' and the user wants to switch to
'pixel'. Maybe, replace the boolean 'maxim,cfg-mode-override' property
with an optional enum property 'maxim,cfg-mode'? Does that sound better?

Thanks,
Laurentiu

> So we can parse the tunnel property from the GMSL port.
> 
> >  	priv->fsync.pin = -1;
> >  	count = fwnode_property_present(dev_fwnode(dev), "maxim,fsync-config");
> >  	if (count > 0) {
> 
> Best Regards,
> 
> -- 
> Julien

Re: [PATCH 3/5] dt-bindings: i2c: maxim,max96717: add new properties

[Julien Massot]

Hi Laurentiu,

> > >  
> > > +  maxim,override-mode:
> > > +    description: Toggle the operation mode from the pin configured one.
> > > +    type: boolean
> > I understand that this property is intended to flip the GMSL link mode between
> > pixel and tunnel mode.
> > What about adding a property 'maxim,tunnel-mode' to the GMSL 'port@1'.
> > Here the MAX96717 only have one GMSL port but other devices, such as MAX96724 can
> > have 2 GMSL link and may have each link in different mode.
> 
> I'm OK with moving the property inside "port@1". But I have some
> concerns about the logic. So. 'maxim,tunnel-mode' presence would
> indicate that we want to force the functioning mode to "tunnel". But
> what if it's absent? Do we use the pin configuration? What if the pin
> configuration is "tunnel" and the user wants to override the mode to
> "pixel"? In this case 'maxim,tunnel-mode' doesn't really work...
> Am I missing something here?
> 
> 
> What about maxim,gmsl2-mode that could be either TUNNEL or PIXEL, if the
property is missing then just run with the pin configured mode.

Regards,
-- 
Julien

Re: [PATCH 5/5] media/i2c: max96717: allow user to override operation mode from DT

[Julien Massot]

Hi Laurentiu,

> > > @@ -1101,6 +1110,9 @@ static int max96717_parse_dt(struct max96717_priv *priv)
> > >  
> > >  	priv->mipi_csi2 = vep.bus.mipi_csi2;
> > >  
> > > +	if (fwnode_property_present(dev_fwnode(dev), "maxim,cfg-mode-override"))
> > > +		priv->mode_override = true;
> > > +
> > 	source_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev),
> > 							MAX96717_PAD_SOURCE, 0, 0);
> > 	if (fwnode_property_present(source_fwnode, "maxim,tunnel-mode")) {
> > 		priv->mode_override = true;
> > 		priv->mode = GMSL2_MODE_TUNNEL;
> > 	}
> 
> So, I don't think the boolean 'maxim,tunnel-mode' would work well when
> the pin configuration is 'tunnel' and the user wants to switch to
> 'pixel'. Maybe, replace the boolean 'maxim,cfg-mode-override' property
> with an optional enum property 'maxim,cfg-mode'? Does that sound better?

Yes, please see my comment on patch 3/5

Regards,
--
Julien