[PATCH 0/3] media: i2c: max9286: Small new features

[PATCH 0/3] media: i2c: max9286: Small new features

[Laurent Pinchart]

Hello,

This small patch series adds two new features to the max9286 driver:
support for per-port supplies, and support for manual framesync
operation. Please see individual patches for details.

Thomas, patch 2/3 is authored by you (and modified by me, see its commit
message, feedback is welcome) but the initial version from your branch
didn't include your Signed-off-by. Could you reply to the mail with an
SoB line ?

Laurent Pinchart (2):
  dt-bindings: media: i2c: max9286: Add support for per-port supplies
  media: i2c: max9286: Support manual framesync operation

Thomas Nizan (1):
  media: i2c: max9286: Add support for port regulators

 .../bindings/media/i2c/maxim,max9286.yaml     |  25 ++-
 drivers/media/i2c/max9286.c                   | 178 ++++++++++++++++--
 2 files changed, 185 insertions(+), 18 deletions(-)


base-commit: 9b4d7b5c81a2578e080da33b5cddc3149fa611aa
-- 
Regards,

Laurent Pinchart

[PATCH 1/3] dt-bindings: media: i2c: max9286: Add support for per-port supplies

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Power supplies for the ports can be controlled per port depending on the
hardware design. Support per-port supplies in the DT bindings, mutually
exclusive with the global supply.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../bindings/media/i2c/maxim,max9286.yaml     | 25 ++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 02f656e78700..33aa307e8ee5 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -39,7 +39,7 @@ properties:
     maxItems: 1
 
   poc-supply:
-    description: Regulator providing Power over Coax to the cameras
+    description: Regulator providing Power over Coax to all the ports
 
   enable-gpios:
     description: GPIO connected to the \#PWDN pin with inverted polarity
@@ -160,6 +160,10 @@ properties:
 
             additionalProperties: false
 
+patternProperties:
+  "^port[0-3]-poc-supply$":
+    description: Regulator providing Power over Coax for a particular port
+
 required:
   - compatible
   - reg
@@ -167,6 +171,25 @@ required:
   - i2c-mux
   - gpio-controller
 
+allOf:
+  - if:
+      required:
+        - poc-supply
+    then:
+      allOf:
+        - not:
+            required:
+              - port0-poc-supply
+        - not:
+            required:
+              - port1-poc-supply
+        - not:
+            required:
+              - port2-poc-supply
+        - not:
+            required:
+              - port3-poc-supply
+
 additionalProperties: false
 
 examples:
-- 
Regards,

Laurent Pinchart

[PATCH 2/3] media: i2c: max9286: Add support for port regulators

[Laurent Pinchart]

From: Thomas Nizan <tnizan@witekio.com>

Allow users to use one PoC regulator per port, instead of a global
regulator.

The properties '^port[0-3]-poc-supply$' in the DT node are used to
indicate the regulators for individual ports.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 112 +++++++++++++++++++++++++++++++-----
 1 file changed, 98 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 7c663fd587bb..fa2f6a823fe6 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -136,8 +136,10 @@
 #define MAX9286_SRC_PAD			4
 
 struct max9286_source {
+	unsigned int index;
 	struct v4l2_subdev *sd;
 	struct fwnode_handle *fwnode;
+	struct regulator *regulator;
 };
 
 struct max9286_asd {
@@ -1072,6 +1074,49 @@ static int max9286_register_gpio(struct max9286_priv *priv)
 	return ret;
 }
 
+static int max9286_poc_power_on(struct max9286_priv *priv)
+{
+	struct max9286_source *source;
+	unsigned int enabled = 0;
+	int ret;
+
+	/* Enable the global regulator if available. */
+	if (priv->regulator)
+		return regulator_enable(priv->regulator);
+
+	/* Otherwise use the per-port regulators. */
+	for_each_source(priv, source) {
+		ret = regulator_enable(source->regulator);
+		if (ret < 0)
+			goto error;
+
+		enabled |= BIT(source->index);
+	}
+
+	return 0;
+
+error:
+	for_each_source(priv, source) {
+		if (enabled & BIT(source->index))
+			regulator_disable(source->regulator);
+	}
+
+	return ret;
+}
+
+static void max9286_poc_power_off(struct max9286_priv *priv)
+{
+	struct max9286_source *source;
+
+	if (priv->regulator) {
+		regulator_disable(priv->regulator);
+		return;
+	}
+
+	for_each_source(priv, source)
+		regulator_disable(source->regulator);
+}
+
 static int max9286_init(struct device *dev)
 {
 	struct max9286_priv *priv;
@@ -1082,9 +1127,9 @@ static int max9286_init(struct device *dev)
 	priv = i2c_get_clientdata(client);
 
 	/* Enable the bus power. */
-	ret = regulator_enable(priv->regulator);
+	ret = max9286_poc_power_on(priv);
 	if (ret < 0) {
-		dev_err(&client->dev, "Unable to turn PoC on\n");
+		dev_err(dev, "Unable to turn PoC on\n");
 		return ret;
 	}
 
@@ -1118,7 +1163,7 @@ static int max9286_init(struct device *dev)
 err_v4l2_register:
 	max9286_v4l2_unregister(priv);
 err_regulator:
-	regulator_disable(priv->regulator);
+	max9286_poc_power_off(priv);
 
 	return ret;
 }
@@ -1215,6 +1260,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 		}
 
 		source = &priv->sources[ep.port];
+		source->index = ep.port;
 		source->fwnode = fwnode_graph_get_remote_endpoint(
 						of_fwnode_handle(node));
 		if (!source->fwnode) {
@@ -1249,6 +1295,50 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	return 0;
 }
 
+static int max9286_get_poc_supplies(struct max9286_priv *priv)
+{
+	struct device *dev = &priv->client->dev;
+	struct max9286_source *source;
+
+	/*
+	 * Start by getting the global regulator. Usage of the exclusive API is
+	 * required to receive an error in case the supply isn't specified in
+	 * the device tree.
+	 */
+	priv->regulator = devm_regulator_get_exclusive(dev, "poc");
+	if (!IS_ERR(priv->regulator))
+		return 0;
+
+	if (PTR_ERR(priv->regulator) != -ENODEV) {
+		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
+			dev_err(dev, "Unable to get PoC regulator: %ld\n",
+				PTR_ERR(priv->regulator));
+		return PTR_ERR(priv->regulator);
+	}
+
+	/* If there's no global regulator, get per-port regulators. */
+	dev_dbg(dev,
+		"No global PoC regulator, looking for per-port regulators\n");
+	priv->regulator = NULL;
+
+	for_each_source(priv, source) {
+		char name[10];
+
+		snprintf(name, sizeof(name), "port%u-poc", source->index);
+		source->regulator = devm_regulator_get_exclusive(dev, name);
+		if (IS_ERR(source->regulator)) {
+			if (PTR_ERR(source->regulator) != -EPROBE_DEFER)
+				dev_err(dev,
+					"Unable to get port %u PoC regulator: %ld\n",
+					source->index,
+					PTR_ERR(source->regulator));
+			return PTR_ERR(source->regulator);
+		}
+	}
+
+	return 0;
+}
+
 static int max9286_probe(struct i2c_client *client)
 {
 	struct max9286_priv *priv;
@@ -1293,17 +1383,11 @@ static int max9286_probe(struct i2c_client *client)
 	if (ret)
 		goto err_powerdown;
 
-	priv->regulator = devm_regulator_get(&client->dev, "poc");
-	if (IS_ERR(priv->regulator)) {
-		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
-			dev_err(&client->dev,
-				"Unable to get PoC regulator (%ld)\n",
-				PTR_ERR(priv->regulator));
-		ret = PTR_ERR(priv->regulator);
-		goto err_powerdown;
-	}
-
 	ret = max9286_parse_dt(priv);
+	if (ret)
+		goto err_cleanup_dt;
+
+	ret = max9286_get_poc_supplies(priv);
 	if (ret)
 		goto err_powerdown;
 
@@ -1329,7 +1413,7 @@ static int max9286_remove(struct i2c_client *client)
 
 	max9286_v4l2_unregister(priv);
 
-	regulator_disable(priv->regulator);
+	max9286_poc_power_off(priv);
 
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
 
-- 
Regards,

Laurent Pinchart

[PATCH 3/3] media: i2c: max9286: Support manual framesync operation

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The MAX9286 can generate a framesync signal to synchronize the cameras,
using an internal timer. Support this mode of operation and configure it
through the .s_frameinterval() operation. If the frame interval is not
0, framesync is switched to manual mode with the specified interval,
otherwise automatic mode is used.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index fa2f6a823fe6..dce1146635d2 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -174,6 +174,7 @@ struct max9286_priv {
 	struct v4l2_ctrl *pixelrate;
 
 	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
+	struct v4l2_fract interval;
 
 	/* Protects controls and fmt structures */
 	struct mutex mutex;
@@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
 	return 0;
 }
 
+static void max9286_set_fsync_period(struct max9286_priv *priv)
+{
+	u32 fsync;
+
+	if (!priv->interval.numerator || !priv->interval.denominator) {
+		/*
+		 * Special case, a null interval enables automatic FRAMESYNC
+		 * mode. FRAMESYNC is taken from the slowest link.
+		 */
+		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
+			      MAX9286_FSYNCMETH_AUTO);
+		return;
+	}
+
+	/*
+	 * Manual FRAMESYNC
+	 *
+	 * The FRAMESYNC generator is configured with a period expressed as a
+	 * number of PCLK periods, which runs at 75MHz.
+	 */
+	fsync = div_u64(75000000ULL * priv->interval.numerator,
+			priv->interval.denominator);
+
+	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
+		      MAX9286_FSYNCMETH_MANUAL);
+
+	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
+	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
+	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
+}
+
 /* -----------------------------------------------------------------------------
  * V4L2 Subdev
  */
@@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (enable) {
+		max9286_set_fsync_period(priv);
+
 		/*
 		 * The frame sync between cameras is transmitted across the
 		 * reverse channel as GPIO. We must open all channels while
@@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	return 0;
 }
 
+static int max9286_g_frame_interval(struct v4l2_subdev *sd,
+				    struct v4l2_subdev_frame_interval *interval)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+
+	if (interval->pad == MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	interval->interval = priv->interval;
+
+	return 0;
+}
+
+static int max9286_s_frame_interval(struct v4l2_subdev *sd,
+				    struct v4l2_subdev_frame_interval *interval)
+{
+	struct max9286_priv *priv = sd_to_max9286(sd);
+
+	if (interval->pad == MAX9286_SRC_PAD)
+		return -EINVAL;
+
+	priv->interval = interval->interval;
+
+	return 0;
+}
+
 static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_mbus_code_enum *code)
@@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 
 static const struct v4l2_subdev_video_ops max9286_video_ops = {
 	.s_stream	= max9286_s_stream,
+	.g_frame_interval = max9286_g_frame_interval,
+	.s_frame_interval = max9286_s_frame_interval,
 };
 
 static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
@@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
 		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
 		      MAX9286_DATATYPE_YUV422_8BIT);
 
-	/* Automatic: FRAMESYNC taken from the slowest Link. */
-	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
-		      MAX9286_FSYNCMETH_AUTO);
+	max9286_set_fsync_period(priv);
 
 	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
 	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] dt-bindings: media: i2c: max9286: Add support for per-port supplies

[Jacopo Mondi]

Hi LAurent

On Fri, Dec 17, 2021 at 12:09:44AM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Power supplies for the ports can be controlled per port depending on the
> hardware design. Support per-port supplies in the DT bindings, mutually
> exclusive with the global supply.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../bindings/media/i2c/maxim,max9286.yaml     | 25 ++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index 02f656e78700..33aa307e8ee5 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -39,7 +39,7 @@ properties:
>      maxItems: 1
>
>    poc-supply:
> -    description: Regulator providing Power over Coax to the cameras
> +    description: Regulator providing Power over Coax to all the ports
>

Can anything but a camera be connected to a port ?

>    enable-gpios:
>      description: GPIO connected to the \#PWDN pin with inverted polarity
> @@ -160,6 +160,10 @@ properties:
>
>              additionalProperties: false
>
> +patternProperties:
> +  "^port[0-3]-poc-supply$":
> +    description: Regulator providing Power over Coax for a particular port
> +
>  required:
>    - compatible
>    - reg
> @@ -167,6 +171,25 @@ required:
>    - i2c-mux
>    - gpio-controller
>
> +allOf:
> +  - if:
> +      required:
> +        - poc-supply
> +    then:
> +      allOf:
> +        - not:
> +            required:
> +              - port0-poc-supply
> +        - not:
> +            required:
> +              - port1-poc-supply
> +        - not:
> +            required:
> +              - port2-poc-supply
> +        - not:
> +            required:
> +              - port3-poc-supply

Isn't this simply expressed as

if:
  required:
    - poc-supply
then:
  properties:
    port0-poc-supply: false
    port1-poc-supply: false
    port2-poc-supply: false
    port3-poc-supply: false

I tried tweaking the DTS file example with the above applied as

        poc-supply = <&camera_poc_12v>;
        port0-poc-supply = <&camera0_poc>;

And validation fails as expected
.../maxim,max9286.example.dt.yaml: gmsl-deserializer@2c: port0-poc-supply: False schema does not allow [[4294967295]]

Also, could you make sure this does not conflict with the introduction
of gpio-poc in "dt-bindings: media: max9286: Define 'maxim,gpio-poc'".

Thanks
   j


> +
>  additionalProperties: false
>
>  examples:
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 2/3] media: i2c: max9286: Add support for port regulators

[Jacopo Mondi]

Hi Laurent,

On Fri, Dec 17, 2021 at 12:09:45AM +0200, Laurent Pinchart wrote:
> From: Thomas Nizan <tnizan@witekio.com>
>
> Allow users to use one PoC regulator per port, instead of a global
> regulator.
>
> The properties '^port[0-3]-poc-supply$' in the DT node are used to
> indicate the regulators for individual ports.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

These patches will badly conflic with the gpio-poc series :(

> ---
>  drivers/media/i2c/max9286.c | 112 +++++++++++++++++++++++++++++++-----
>  1 file changed, 98 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7c663fd587bb..fa2f6a823fe6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -136,8 +136,10 @@
>  #define MAX9286_SRC_PAD			4
>
>  struct max9286_source {
> +	unsigned int index;

Can't you use the to_index() macro ?

>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> +	struct regulator *regulator;
>  };
>
>  struct max9286_asd {
> @@ -1072,6 +1074,49 @@ static int max9286_register_gpio(struct max9286_priv *priv)
>  	return ret;
>  }
>
> +static int max9286_poc_power_on(struct max9286_priv *priv)
> +{
> +	struct max9286_source *source;
> +	unsigned int enabled = 0;
> +	int ret;
> +
> +	/* Enable the global regulator if available. */
> +	if (priv->regulator)
> +		return regulator_enable(priv->regulator);
> +
> +	/* Otherwise use the per-port regulators. */
> +	for_each_source(priv, source) {
> +		ret = regulator_enable(source->regulator);
> +		if (ret < 0)
> +			goto error;
> +
> +		enabled |= BIT(source->index);
> +	}
> +
> +	return 0;
> +
> +error:
> +	for_each_source(priv, source) {
> +		if (enabled & BIT(source->index))
> +			regulator_disable(source->regulator);

I would if a regulator can be unconditionally disabled...

> +	}
> +
> +	return ret;
> +}
> +
> +static void max9286_poc_power_off(struct max9286_priv *priv)
> +{
> +	struct max9286_source *source;
> +
> +	if (priv->regulator) {
> +		regulator_disable(priv->regulator);
> +		return;
> +	}
> +
> +	for_each_source(priv, source)
> +		regulator_disable(source->regulator);
> +}
> +
>  static int max9286_init(struct device *dev)
>  {
>  	struct max9286_priv *priv;
> @@ -1082,9 +1127,9 @@ static int max9286_init(struct device *dev)
>  	priv = i2c_get_clientdata(client);
>
>  	/* Enable the bus power. */
> -	ret = regulator_enable(priv->regulator);
> +	ret = max9286_poc_power_on(priv);
>  	if (ret < 0) {
> -		dev_err(&client->dev, "Unable to turn PoC on\n");
> +		dev_err(dev, "Unable to turn PoC on\n");
>  		return ret;
>  	}
>
> @@ -1118,7 +1163,7 @@ static int max9286_init(struct device *dev)
>  err_v4l2_register:
>  	max9286_v4l2_unregister(priv);
>  err_regulator:
> -	regulator_disable(priv->regulator);
> +	max9286_poc_power_off(priv);
>
>  	return ret;
>  }
> @@ -1215,6 +1260,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  		}
>
>  		source = &priv->sources[ep.port];
> +		source->index = ep.port;
>  		source->fwnode = fwnode_graph_get_remote_endpoint(
>  						of_fwnode_handle(node));
>  		if (!source->fwnode) {
> @@ -1249,6 +1295,50 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	return 0;
>  }
>
> +static int max9286_get_poc_supplies(struct max9286_priv *priv)
> +{
> +	struct device *dev = &priv->client->dev;
> +	struct max9286_source *source;
> +
> +	/*
> +	 * Start by getting the global regulator. Usage of the exclusive API is
> +	 * required to receive an error in case the supply isn't specified in
> +	 * the device tree.
> +	 */
> +	priv->regulator = devm_regulator_get_exclusive(dev, "poc");

doesn't get_optional() return -ENODEV if there's no such regulator too
without locking the usage of the regulator ? Oh wait, maybe we want
exclusive locking ? :)

> +	if (!IS_ERR(priv->regulator))
> +		return 0;
> +
> +	if (PTR_ERR(priv->regulator) != -ENODEV) {
> +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> +			dev_err(dev, "Unable to get PoC regulator: %ld\n",
> +				PTR_ERR(priv->regulator));
> +		return PTR_ERR(priv->regulator);
> +	}
> +
> +	/* If there's no global regulator, get per-port regulators. */
> +	dev_dbg(dev,
> +		"No global PoC regulator, looking for per-port regulators\n");
> +	priv->regulator = NULL;
> +
> +	for_each_source(priv, source) {
> +		char name[10];
> +
> +		snprintf(name, sizeof(name), "port%u-poc", source->index);
> +		source->regulator = devm_regulator_get_exclusive(dev, name);

I guess the fact we don't have a fixed number of sources makes it even
harder to use the _bulk version of the regulators API...

> +		if (IS_ERR(source->regulator)) {
> +			if (PTR_ERR(source->regulator) != -EPROBE_DEFER)
> +				dev_err(dev,
> +					"Unable to get port %u PoC regulator: %ld\n",
> +					source->index,
> +					PTR_ERR(source->regulator));
> +			return PTR_ERR(source->regulator);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int max9286_probe(struct i2c_client *client)
>  {
>  	struct max9286_priv *priv;
> @@ -1293,17 +1383,11 @@ static int max9286_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_powerdown;
>
> -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> -	if (IS_ERR(priv->regulator)) {
> -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> -			dev_err(&client->dev,
> -				"Unable to get PoC regulator (%ld)\n",
> -				PTR_ERR(priv->regulator));
> -		ret = PTR_ERR(priv->regulator);
> -		goto err_powerdown;
> -	}
> -
>  	ret = max9286_parse_dt(priv);
> +	if (ret)
> +		goto err_cleanup_dt;
> +
> +	ret = max9286_get_poc_supplies(priv);
>  	if (ret)
>  		goto err_powerdown;

I think the error path is wrong, you should cleanup_dt here.

Thanks
   j

>
> @@ -1329,7 +1413,7 @@ static int max9286_remove(struct i2c_client *client)
>
>  	max9286_v4l2_unregister(priv);
>
> -	regulator_disable(priv->regulator);
> +	max9286_poc_power_off(priv);
>
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 3/3] media: i2c: max9286: Support manual framesync operation

[Jacopo Mondi]

Hi Laurent,

On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The MAX9286 can generate a framesync signal to synchronize the cameras,
> using an internal timer. Support this mode of operation and configure it
> through the .s_frameinterval() operation. If the frame interval is not
> 0, framesync is switched to manual mode with the specified interval,
> otherwise automatic mode is used.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index fa2f6a823fe6..dce1146635d2 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -174,6 +174,7 @@ struct max9286_priv {
>  	struct v4l2_ctrl *pixelrate;
>
>  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> +	struct v4l2_fract interval;
>
>  	/* Protects controls and fmt structures */
>  	struct mutex mutex;
> @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  	return 0;
>  }
>
> +static void max9286_set_fsync_period(struct max9286_priv *priv)
> +{
> +	u32 fsync;
> +
> +	if (!priv->interval.numerator || !priv->interval.denominator) {
> +		/*
> +		 * Special case, a null interval enables automatic FRAMESYNC
> +		 * mode. FRAMESYNC is taken from the slowest link.
> +		 */
> +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> +			      MAX9286_FSYNCMETH_AUTO);
> +		return;
> +	}
> +
> +	/*
> +	 * Manual FRAMESYNC
> +	 *
> +	 * The FRAMESYNC generator is configured with a period expressed as a
> +	 * number of PCLK periods, which runs at 75MHz.
> +	 */
> +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> +			priv->interval.denominator);

Don't we calculate the pixel rate dynamically based on the number of
enabled sources ?

> +
> +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> +		      MAX9286_FSYNCMETH_MANUAL);
> +
> +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * V4L2 Subdev
>   */
> @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>
>  	if (enable) {
> +		max9286_set_fsync_period(priv);
> +
>  		/*
>  		 * The frame sync between cameras is transmitted across the
>  		 * reverse channel as GPIO. We must open all channels while
> @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	return 0;
>  }
>
> +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> +				    struct v4l2_subdev_frame_interval *interval)
> +{
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +
> +	if (interval->pad == MAX9286_SRC_PAD)

Ah!

I would have expected, as this setting applies to all cameras, and
consequentially to the output image stream frame rate, that the
operation should have been done on the single source pad... Why is
this not the case ?

Thanks
  j


> +		return -EINVAL;
> +
> +	interval->interval = priv->interval;
> +
> +	return 0;
> +}
> +
> +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> +				    struct v4l2_subdev_frame_interval *interval)
> +{
> +	struct max9286_priv *priv = sd_to_max9286(sd);
> +
> +	if (interval->pad == MAX9286_SRC_PAD)
> +		return -EINVAL;
> +
> +	priv->interval = interval->interval;
> +
> +	return 0;
> +}
> +
>  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_mbus_code_enum *code)
> @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>
>  static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  	.s_stream	= max9286_s_stream,
> +	.g_frame_interval = max9286_g_frame_interval,
> +	.s_frame_interval = max9286_s_frame_interval,
>  };
>
>  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
>  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
>  		      MAX9286_DATATYPE_YUV422_8BIT);
>
> -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> -		      MAX9286_FSYNCMETH_AUTO);
> +	max9286_set_fsync_period(priv);
>
>  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
>  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 2/3] media: i2c: max9286: Add support for port regulators

[Jacopo Mondi]

Hello again

On Fri, Dec 17, 2021 at 12:09:45AM +0200, Laurent Pinchart wrote:
> From: Thomas Nizan <tnizan@witekio.com>
>
> Allow users to use one PoC regulator per port, instead of a global
> regulator.
>
> The properties '^port[0-3]-poc-supply$' in the DT node are used to
> indicate the regulators for individual ports.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 112 +++++++++++++++++++++++++++++++-----
>  1 file changed, 98 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7c663fd587bb..fa2f6a823fe6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -136,8 +136,10 @@
>  #define MAX9286_SRC_PAD			4
>
>  struct max9286_source {
> +	unsigned int index;
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> +	struct regulator *regulator;
>  };
>
>  struct max9286_asd {
> @@ -1072,6 +1074,49 @@ static int max9286_register_gpio(struct max9286_priv *priv)
>  	return ret;
>  }
>
> +static int max9286_poc_power_on(struct max9286_priv *priv)
> +{
> +	struct max9286_source *source;
> +	unsigned int enabled = 0;
> +	int ret;
> +
> +	/* Enable the global regulator if available. */
> +	if (priv->regulator)
> +		return regulator_enable(priv->regulator);
> +
> +	/* Otherwise use the per-port regulators. */
> +	for_each_source(priv, source) {
> +		ret = regulator_enable(source->regulator);
> +		if (ret < 0)
> +			goto error;
> +
> +		enabled |= BIT(source->index);
> +	}
> +
> +	return 0;
> +
> +error:
> +	for_each_source(priv, source) {
> +		if (enabled & BIT(source->index))
> +			regulator_disable(source->regulator);
> +	}
> +
> +	return ret;
> +}
> +
> +static void max9286_poc_power_off(struct max9286_priv *priv)
> +{
> +	struct max9286_source *source;
> +
> +	if (priv->regulator) {
> +		regulator_disable(priv->regulator);
> +		return;
> +	}
> +
> +	for_each_source(priv, source)
> +		regulator_disable(source->regulator);
> +}
> +
>  static int max9286_init(struct device *dev)
>  {
>  	struct max9286_priv *priv;
> @@ -1082,9 +1127,9 @@ static int max9286_init(struct device *dev)
>  	priv = i2c_get_clientdata(client);
>
>  	/* Enable the bus power. */
> -	ret = regulator_enable(priv->regulator);
> +	ret = max9286_poc_power_on(priv);
>  	if (ret < 0) {
> -		dev_err(&client->dev, "Unable to turn PoC on\n");
> +		dev_err(dev, "Unable to turn PoC on\n");
>  		return ret;
>  	}
>
> @@ -1118,7 +1163,7 @@ static int max9286_init(struct device *dev)
>  err_v4l2_register:
>  	max9286_v4l2_unregister(priv);
>  err_regulator:
> -	regulator_disable(priv->regulator);
> +	max9286_poc_power_off(priv);
>
>  	return ret;
>  }
> @@ -1215,6 +1260,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  		}
>
>  		source = &priv->sources[ep.port];
> +		source->index = ep.port;
>  		source->fwnode = fwnode_graph_get_remote_endpoint(
>  						of_fwnode_handle(node));
>  		if (!source->fwnode) {
> @@ -1249,6 +1295,50 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	return 0;
>  }
>
> +static int max9286_get_poc_supplies(struct max9286_priv *priv)
> +{
> +	struct device *dev = &priv->client->dev;
> +	struct max9286_source *source;
> +
> +	/*
> +	 * Start by getting the global regulator. Usage of the exclusive API is
> +	 * required to receive an error in case the supply isn't specified in
> +	 * the device tree.
> +	 */
> +	priv->regulator = devm_regulator_get_exclusive(dev, "poc");
> +	if (!IS_ERR(priv->regulator))
> +		return 0;
> +
> +	if (PTR_ERR(priv->regulator) != -ENODEV) {
> +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)

Use of the dev_err_probe() intoduced by a787e5400a1ceeb0ef92d71ec43aeb35b1fa1334
would save you from open-coding this

Thanks
  j

> +			dev_err(dev, "Unable to get PoC regulator: %ld\n",
> +				PTR_ERR(priv->regulator));
> +		return PTR_ERR(priv->regulator);
> +	}
> +
> +	/* If there's no global regulator, get per-port regulators. */
> +	dev_dbg(dev,
> +		"No global PoC regulator, looking for per-port regulators\n");
> +	priv->regulator = NULL;
> +
> +	for_each_source(priv, source) {
> +		char name[10];
> +
> +		snprintf(name, sizeof(name), "port%u-poc", source->index);
> +		source->regulator = devm_regulator_get_exclusive(dev, name);
> +		if (IS_ERR(source->regulator)) {
> +			if (PTR_ERR(source->regulator) != -EPROBE_DEFER)
> +				dev_err(dev,
> +					"Unable to get port %u PoC regulator: %ld\n",
> +					source->index,
> +					PTR_ERR(source->regulator));
> +			return PTR_ERR(source->regulator);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int max9286_probe(struct i2c_client *client)
>  {
>  	struct max9286_priv *priv;
> @@ -1293,17 +1383,11 @@ static int max9286_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_powerdown;
>
> -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> -	if (IS_ERR(priv->regulator)) {
> -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> -			dev_err(&client->dev,
> -				"Unable to get PoC regulator (%ld)\n",
> -				PTR_ERR(priv->regulator));
> -		ret = PTR_ERR(priv->regulator);
> -		goto err_powerdown;
> -	}
> -
>  	ret = max9286_parse_dt(priv);
> +	if (ret)
> +		goto err_cleanup_dt;
> +
> +	ret = max9286_get_poc_supplies(priv);
>  	if (ret)
>  		goto err_powerdown;
>
> @@ -1329,7 +1413,7 @@ static int max9286_remove(struct i2c_client *client)
>
>  	max9286_v4l2_unregister(priv);
>
> -	regulator_disable(priv->regulator);
> +	max9286_poc_power_off(priv);
>
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH 1/3] dt-bindings: media: i2c: max9286: Add support for per-port supplies

[Rob Herring]

On Fri, Dec 17, 2021 at 12:09:44AM +0200, Laurent Pinchart wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Power supplies for the ports can be controlled per port depending on the
> hardware design. Support per-port supplies in the DT bindings, mutually
> exclusive with the global supply.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../bindings/media/i2c/maxim,max9286.yaml     | 25 ++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index 02f656e78700..33aa307e8ee5 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -39,7 +39,7 @@ properties:
>      maxItems: 1
>  
>    poc-supply:
> -    description: Regulator providing Power over Coax to the cameras
> +    description: Regulator providing Power over Coax to all the ports
>  
>    enable-gpios:
>      description: GPIO connected to the \#PWDN pin with inverted polarity
> @@ -160,6 +160,10 @@ properties:
>  
>              additionalProperties: false
>  
> +patternProperties:
> +  "^port[0-3]-poc-supply$":
> +    description: Regulator providing Power over Coax for a particular port
> +
>  required:
>    - compatible
>    - reg
> @@ -167,6 +171,25 @@ required:
>    - i2c-mux
>    - gpio-controller
>  
> +allOf:
> +  - if:
> +      required:
> +        - poc-supply
> +    then:
> +      allOf:
> +        - not:
> +            required:
> +              - port0-poc-supply
> +        - not:
> +            required:
> +              - port1-poc-supply
> +        - not:
> +            required:
> +              - port2-poc-supply
> +        - not:
> +            required:
> +              - port3-poc-supply

I think you can invert the if and move patternProperties to the 'then' 
and...

> +
>  additionalProperties: false

then use unevaluatedProperties here.

Rob

Re: [PATCH 1/3] dt-bindings: media: i2c: max9286: Add support for per-port supplies

[Laurent Pinchart]

Hi Jacopo,

On Fri, Dec 17, 2021 at 11:47:10AM +0100, Jacopo Mondi wrote:
> On Fri, Dec 17, 2021 at 12:09:44AM +0200, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Power supplies for the ports can be controlled per port depending on the
> > hardware design. Support per-port supplies in the DT bindings, mutually
> > exclusive with the global supply.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../bindings/media/i2c/maxim,max9286.yaml     | 25 ++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index 02f656e78700..33aa307e8ee5 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -39,7 +39,7 @@ properties:
> >      maxItems: 1
> >
> >    poc-supply:
> > -    description: Regulator providing Power over Coax to the cameras
> > +    description: Regulator providing Power over Coax to all the ports
> >
> 
> Can anything but a camera be connected to a port ?
> 
> >    enable-gpios:
> >      description: GPIO connected to the \#PWDN pin with inverted polarity
> > @@ -160,6 +160,10 @@ properties:
> >
> >              additionalProperties: false
> >
> > +patternProperties:
> > +  "^port[0-3]-poc-supply$":
> > +    description: Regulator providing Power over Coax for a particular port
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -167,6 +171,25 @@ required:
> >    - i2c-mux
> >    - gpio-controller
> >
> > +allOf:
> > +  - if:
> > +      required:
> > +        - poc-supply
> > +    then:
> > +      allOf:
> > +        - not:
> > +            required:
> > +              - port0-poc-supply
> > +        - not:
> > +            required:
> > +              - port1-poc-supply
> > +        - not:
> > +            required:
> > +              - port2-poc-supply
> > +        - not:
> > +            required:
> > +              - port3-poc-supply
> 
> Isn't this simply expressed as
> 
> if:
>   required:
>     - poc-supply
> then:
>   properties:
>     port0-poc-supply: false
>     port1-poc-supply: false
>     port2-poc-supply: false
>     port3-poc-supply: false

I would have sworn I had tried that and that it didn't work... I now
have

allOf:
  - if:
      required:
        - poc-supply
    then:
      patternProperties:
        '^port[0-3]-poc-supply$': false

additionalProperties: false

and it seems to do the job. I'll use that in a v2.

> I tried tweaking the DTS file example with the above applied as
> 
>         poc-supply = <&camera_poc_12v>;
>         port0-poc-supply = <&camera0_poc>;
> 
> And validation fails as expected
> .../maxim,max9286.example.dt.yaml: gmsl-deserializer@2c: port0-poc-supply: False schema does not allow [[4294967295]]
> 
> Also, could you make sure this does not conflict with the introduction
> of gpio-poc in "dt-bindings: media: max9286: Define 'maxim,gpio-poc'".
> 
> > +
> >  additionalProperties: false
> >
> >  examples:

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] dt-bindings: media: i2c: max9286: Add support for per-port supplies

[Laurent Pinchart]

Hi Rob,

On Fri, Dec 17, 2021 at 08:18:06AM -0600, Rob Herring wrote:
> On Fri, Dec 17, 2021 at 12:09:44AM +0200, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Power supplies for the ports can be controlled per port depending on the
> > hardware design. Support per-port supplies in the DT bindings, mutually
> > exclusive with the global supply.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../bindings/media/i2c/maxim,max9286.yaml     | 25 ++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index 02f656e78700..33aa307e8ee5 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -39,7 +39,7 @@ properties:
> >      maxItems: 1
> >  
> >    poc-supply:
> > -    description: Regulator providing Power over Coax to the cameras
> > +    description: Regulator providing Power over Coax to all the ports
> >  
> >    enable-gpios:
> >      description: GPIO connected to the \#PWDN pin with inverted polarity
> > @@ -160,6 +160,10 @@ properties:
> >  
> >              additionalProperties: false
> >  
> > +patternProperties:
> > +  "^port[0-3]-poc-supply$":
> > +    description: Regulator providing Power over Coax for a particular port
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -167,6 +171,25 @@ required:
> >    - i2c-mux
> >    - gpio-controller
> >  
> > +allOf:
> > +  - if:
> > +      required:
> > +        - poc-supply
> > +    then:
> > +      allOf:
> > +        - not:
> > +            required:
> > +              - port0-poc-supply
> > +        - not:
> > +            required:
> > +              - port1-poc-supply
> > +        - not:
> > +            required:
> > +              - port2-poc-supply
> > +        - not:
> > +            required:
> > +              - port3-poc-supply
> 
> I think you can invert the if and move patternProperties to the 'then' 
> and...
> 
> > +
> >  additionalProperties: false
> 
> then use unevaluatedProperties here.

I ended up doing the following, which I would have sworn I had tried
before:

allOf:
  - if:
      required:
        - poc-supply
    then:
      patternProperties:
        '^port[0-3]-poc-supply$': false

additionalProperties: false

It seems to work, so I'll use it in v2.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] media: i2c: max9286: Support manual framesync operation

[Laurent Pinchart]

Hi Jacopo,

On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote:
> On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > The MAX9286 can generate a framesync signal to synchronize the cameras,
> > using an internal timer. Support this mode of operation and configure it
> > through the .s_frameinterval() operation. If the frame interval is not
> > 0, framesync is switched to manual mode with the specified interval,
> > otherwise automatic mode is used.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 63 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index fa2f6a823fe6..dce1146635d2 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -174,6 +174,7 @@ struct max9286_priv {
> >  	struct v4l2_ctrl *pixelrate;
> >
> >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > +	struct v4l2_fract interval;
> >
> >  	/* Protects controls and fmt structures */
> >  	struct mutex mutex;
> > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> >  	return 0;
> >  }
> >
> > +static void max9286_set_fsync_period(struct max9286_priv *priv)
> > +{
> > +	u32 fsync;
> > +
> > +	if (!priv->interval.numerator || !priv->interval.denominator) {
> > +		/*
> > +		 * Special case, a null interval enables automatic FRAMESYNC
> > +		 * mode. FRAMESYNC is taken from the slowest link.
> > +		 */
> > +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > +			      MAX9286_FSYNCMETH_AUTO);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Manual FRAMESYNC
> > +	 *
> > +	 * The FRAMESYNC generator is configured with a period expressed as a
> > +	 * number of PCLK periods, which runs at 75MHz.
> > +	 */
> > +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> > +			priv->interval.denominator);
> 
> Don't we calculate the pixel rate dynamically based on the number of
> enabled sources ?

That's a good point, this hardcodes operation for 4 cameras. I'll see
how I can fix that.

> > +
> > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> > +		      MAX9286_FSYNCMETH_MANUAL);
> > +
> > +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> > +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> > +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2 Subdev
> >   */
> > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  	int ret;
> >
> >  	if (enable) {
> > +		max9286_set_fsync_period(priv);
> > +
> >  		/*
> >  		 * The frame sync between cameras is transmitted across the
> >  		 * reverse channel as GPIO. We must open all channels while
> > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  	return 0;
> >  }
> >
> > +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> > +				    struct v4l2_subdev_frame_interval *interval)
> > +{
> > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > +
> > +	if (interval->pad == MAX9286_SRC_PAD)
> 
> Ah!
> 
> I would have expected, as this setting applies to all cameras, and
> consequentially to the output image stream frame rate, that the
> operation should have been done on the single source pad... Why is
> this not the case ?

I went back and forth between the two, I think it would work fine on the
source pad as well. I'll give it a try.

> > +		return -EINVAL;
> > +
> > +	interval->interval = priv->interval;
> > +
> > +	return 0;
> > +}
> > +
> > +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> > +				    struct v4l2_subdev_frame_interval *interval)
> > +{
> > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > +
> > +	if (interval->pad == MAX9286_SRC_PAD)
> > +		return -EINVAL;
> > +
> > +	priv->interval = interval->interval;
> > +
> > +	return 0;
> > +}
> > +
> >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >  				  struct v4l2_subdev_state *sd_state,
> >  				  struct v4l2_subdev_mbus_code_enum *code)
> > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> >
> >  static const struct v4l2_subdev_video_ops max9286_video_ops = {
> >  	.s_stream	= max9286_s_stream,
> > +	.g_frame_interval = max9286_g_frame_interval,
> > +	.s_frame_interval = max9286_s_frame_interval,
> >  };
> >
> >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
> >  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> >  		      MAX9286_DATATYPE_YUV422_8BIT);
> >
> > -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > -		      MAX9286_FSYNCMETH_AUTO);
> > +	max9286_set_fsync_period(priv);
> >
> >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: i2c: max9286: Add support for port regulators

[Laurent Pinchart]

Hi Jacopo,

On Fri, Dec 17, 2021 at 12:08:08PM +0100, Jacopo Mondi wrote:
> On Fri, Dec 17, 2021 at 12:09:45AM +0200, Laurent Pinchart wrote:
> > From: Thomas Nizan <tnizan@witekio.com>
> >
> > Allow users to use one PoC regulator per port, instead of a global
> > regulator.
> >
> > The properties '^port[0-3]-poc-supply$' in the DT node are used to
> > indicate the regulators for individual ports.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> These patches will badly conflic with the gpio-poc series :(

I don't mind rebasing.

> > ---
> >  drivers/media/i2c/max9286.c | 112 +++++++++++++++++++++++++++++++-----
> >  1 file changed, 98 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 7c663fd587bb..fa2f6a823fe6 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -136,8 +136,10 @@
> >  #define MAX9286_SRC_PAD			4
> >
> >  struct max9286_source {
> > +	unsigned int index;
> 
> Can't you use the to_index() macro ?

I think I can, indeed.

> >  	struct v4l2_subdev *sd;
> >  	struct fwnode_handle *fwnode;
> > +	struct regulator *regulator;
> >  };
> >
> >  struct max9286_asd {
> > @@ -1072,6 +1074,49 @@ static int max9286_register_gpio(struct max9286_priv *priv)
> >  	return ret;
> >  }
> >
> > +static int max9286_poc_power_on(struct max9286_priv *priv)
> > +{
> > +	struct max9286_source *source;
> > +	unsigned int enabled = 0;
> > +	int ret;
> > +
> > +	/* Enable the global regulator if available. */
> > +	if (priv->regulator)
> > +		return regulator_enable(priv->regulator);
> > +
> > +	/* Otherwise use the per-port regulators. */
> > +	for_each_source(priv, source) {
> > +		ret = regulator_enable(source->regulator);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		enabled |= BIT(source->index);
> > +	}
> > +
> > +	return 0;
> > +
> > +error:
> > +	for_each_source(priv, source) {
> > +		if (enabled & BIT(source->index))
> > +			regulator_disable(source->regulator);
> 
> I would if a regulator can be unconditionally disabled...

They're reference-counted, so they should only be disabled here if they
have been enabled above.

> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void max9286_poc_power_off(struct max9286_priv *priv)
> > +{
> > +	struct max9286_source *source;
> > +
> > +	if (priv->regulator) {
> > +		regulator_disable(priv->regulator);
> > +		return;
> > +	}
> > +
> > +	for_each_source(priv, source)
> > +		regulator_disable(source->regulator);
> > +}
> > +
> >  static int max9286_init(struct device *dev)
> >  {
> >  	struct max9286_priv *priv;
> > @@ -1082,9 +1127,9 @@ static int max9286_init(struct device *dev)
> >  	priv = i2c_get_clientdata(client);
> >
> >  	/* Enable the bus power. */
> > -	ret = regulator_enable(priv->regulator);
> > +	ret = max9286_poc_power_on(priv);
> >  	if (ret < 0) {
> > -		dev_err(&client->dev, "Unable to turn PoC on\n");
> > +		dev_err(dev, "Unable to turn PoC on\n");
> >  		return ret;
> >  	}
> >
> > @@ -1118,7 +1163,7 @@ static int max9286_init(struct device *dev)
> >  err_v4l2_register:
> >  	max9286_v4l2_unregister(priv);
> >  err_regulator:
> > -	regulator_disable(priv->regulator);
> > +	max9286_poc_power_off(priv);
> >
> >  	return ret;
> >  }
> > @@ -1215,6 +1260,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  		}
> >
> >  		source = &priv->sources[ep.port];
> > +		source->index = ep.port;
> >  		source->fwnode = fwnode_graph_get_remote_endpoint(
> >  						of_fwnode_handle(node));
> >  		if (!source->fwnode) {
> > @@ -1249,6 +1295,50 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  	return 0;
> >  }
> >
> > +static int max9286_get_poc_supplies(struct max9286_priv *priv)
> > +{
> > +	struct device *dev = &priv->client->dev;
> > +	struct max9286_source *source;
> > +
> > +	/*
> > +	 * Start by getting the global regulator. Usage of the exclusive API is
> > +	 * required to receive an error in case the supply isn't specified in
> > +	 * the device tree.
> > +	 */
> > +	priv->regulator = devm_regulator_get_exclusive(dev, "poc");
> 
> doesn't get_optional() return -ENODEV if there's no such regulator too
> without locking the usage of the regulator ? Oh wait, maybe we want
> exclusive locking ? :)

No, you're right, I'll use an optional get.

> > +	if (!IS_ERR(priv->regulator))
> > +		return 0;
> > +
> > +	if (PTR_ERR(priv->regulator) != -ENODEV) {
> > +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > +			dev_err(dev, "Unable to get PoC regulator: %ld\n",
> > +				PTR_ERR(priv->regulator));
> > +		return PTR_ERR(priv->regulator);
> > +	}
> > +
> > +	/* If there's no global regulator, get per-port regulators. */
> > +	dev_dbg(dev,
> > +		"No global PoC regulator, looking for per-port regulators\n");
> > +	priv->regulator = NULL;
> > +
> > +	for_each_source(priv, source) {
> > +		char name[10];
> > +
> > +		snprintf(name, sizeof(name), "port%u-poc", source->index);
> > +		source->regulator = devm_regulator_get_exclusive(dev, name);
> 
> I guess the fact we don't have a fixed number of sources makes it even
> harder to use the _bulk version of the regulators API...

Yes, and that fact that we may at some point want to power cameras
up/down individually.

> > +		if (IS_ERR(source->regulator)) {
> > +			if (PTR_ERR(source->regulator) != -EPROBE_DEFER)
> > +				dev_err(dev,
> > +					"Unable to get port %u PoC regulator: %ld\n",
> > +					source->index,
> > +					PTR_ERR(source->regulator));
> > +			return PTR_ERR(source->regulator);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int max9286_probe(struct i2c_client *client)
> >  {
> >  	struct max9286_priv *priv;
> > @@ -1293,17 +1383,11 @@ static int max9286_probe(struct i2c_client *client)
> >  	if (ret)
> >  		goto err_powerdown;
> >
> > -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> > -	if (IS_ERR(priv->regulator)) {
> > -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > -			dev_err(&client->dev,
> > -				"Unable to get PoC regulator (%ld)\n",
> > -				PTR_ERR(priv->regulator));
> > -		ret = PTR_ERR(priv->regulator);
> > -		goto err_powerdown;
> > -	}
> > -
> >  	ret = max9286_parse_dt(priv);
> > +	if (ret)
> > +		goto err_cleanup_dt;
> > +
> > +	ret = max9286_get_poc_supplies(priv);
> >  	if (ret)
> >  		goto err_powerdown;
> 
> I think the error path is wrong, you should cleanup_dt here.

Oops.

> > @@ -1329,7 +1413,7 @@ static int max9286_remove(struct i2c_client *client)
> >
> >  	max9286_v4l2_unregister(priv);
> >
> > -	regulator_disable(priv->regulator);
> > +	max9286_poc_power_off(priv);
> >
> >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> >

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] media: i2c: max9286: Support manual framesync operation

[Laurent Pinchart]

Hi Jacopo,

On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote:
> On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote:
> > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > The MAX9286 can generate a framesync signal to synchronize the cameras,
> > > using an internal timer. Support this mode of operation and configure it
> > > through the .s_frameinterval() operation. If the frame interval is not
> > > 0, framesync is switched to manual mode with the specified interval,
> > > otherwise automatic mode is used.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 63 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index fa2f6a823fe6..dce1146635d2 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -174,6 +174,7 @@ struct max9286_priv {
> > >  	struct v4l2_ctrl *pixelrate;
> > >
> > >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > +	struct v4l2_fract interval;
> > >
> > >  	/* Protects controls and fmt structures */
> > >  	struct mutex mutex;
> > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> > >  	return 0;
> > >  }
> > >
> > > +static void max9286_set_fsync_period(struct max9286_priv *priv)
> > > +{
> > > +	u32 fsync;
> > > +
> > > +	if (!priv->interval.numerator || !priv->interval.denominator) {
> > > +		/*
> > > +		 * Special case, a null interval enables automatic FRAMESYNC
> > > +		 * mode. FRAMESYNC is taken from the slowest link.
> > > +		 */
> > > +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > +			      MAX9286_FSYNCMETH_AUTO);
> > > +		return;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Manual FRAMESYNC
> > > +	 *
> > > +	 * The FRAMESYNC generator is configured with a period expressed as a
> > > +	 * number of PCLK periods, which runs at 75MHz.
> > > +	 */
> > > +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> > > +			priv->interval.denominator);
> > 
> > Don't we calculate the pixel rate dynamically based on the number of
> > enabled sources ?
> 
> That's a good point, this hardcodes operation for 4 cameras. I'll see
> how I can fix that.
> 
> > > +
> > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> > > +		      MAX9286_FSYNCMETH_MANUAL);
> > > +
> > > +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> > > +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> > > +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * V4L2 Subdev
> > >   */
> > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	int ret;
> > >
> > >  	if (enable) {
> > > +		max9286_set_fsync_period(priv);
> > > +
> > >  		/*
> > >  		 * The frame sync between cameras is transmitted across the
> > >  		 * reverse channel as GPIO. We must open all channels while
> > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > >  	return 0;
> > >  }
> > >
> > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> > > +				    struct v4l2_subdev_frame_interval *interval)
> > > +{
> > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > +
> > > +	if (interval->pad == MAX9286_SRC_PAD)
> > 
> > Ah!
> > 
> > I would have expected, as this setting applies to all cameras, and
> > consequentially to the output image stream frame rate, that the
> > operation should have been done on the single source pad... Why is
> > this not the case ?
> 
> I went back and forth between the two, I think it would work fine on the
> source pad as well. I'll give it a try.

Note that once we'll move to the muxed streams API, the frame interval
will be per stream, so setting it on the source pad won't make much of a
difference. What do you think ?

> > > +		return -EINVAL;
> > > +
> > > +	interval->interval = priv->interval;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> > > +				    struct v4l2_subdev_frame_interval *interval)
> > > +{
> > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > +
> > > +	if (interval->pad == MAX9286_SRC_PAD)
> > > +		return -EINVAL;
> > > +
> > > +	priv->interval = interval->interval;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > >  				  struct v4l2_subdev_state *sd_state,
> > >  				  struct v4l2_subdev_mbus_code_enum *code)
> > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > >
> > >  static const struct v4l2_subdev_video_ops max9286_video_ops = {
> > >  	.s_stream	= max9286_s_stream,
> > > +	.g_frame_interval = max9286_g_frame_interval,
> > > +	.s_frame_interval = max9286_s_frame_interval,
> > >  };
> > >
> > >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > >  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> > >  		      MAX9286_DATATYPE_YUV422_8BIT);
> > >
> > > -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > > -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > -		      MAX9286_FSYNCMETH_AUTO);
> > > +	max9286_set_fsync_period(priv);
> > >
> > >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> > >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] media: i2c: max9286: Support manual framesync operation

[Jacopo Mondi]

Hi Laurent,

On Fri, Dec 17, 2021 at 07:49:07PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote:
> > > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >
> > > > The MAX9286 can generate a framesync signal to synchronize the cameras,
> > > > using an internal timer. Support this mode of operation and configure it
> > > > through the .s_frameinterval() operation. If the frame interval is not
> > > > 0, framesync is switched to manual mode with the specified interval,
> > > > otherwise automatic mode is used.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 63 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > index fa2f6a823fe6..dce1146635d2 100644
> > > > --- a/drivers/media/i2c/max9286.c
> > > > +++ b/drivers/media/i2c/max9286.c
> > > > @@ -174,6 +174,7 @@ struct max9286_priv {
> > > >  	struct v4l2_ctrl *pixelrate;
> > > >
> > > >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > > +	struct v4l2_fract interval;
> > > >
> > > >  	/* Protects controls and fmt structures */
> > > >  	struct mutex mutex;
> > > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static void max9286_set_fsync_period(struct max9286_priv *priv)
> > > > +{
> > > > +	u32 fsync;
> > > > +
> > > > +	if (!priv->interval.numerator || !priv->interval.denominator) {
> > > > +		/*
> > > > +		 * Special case, a null interval enables automatic FRAMESYNC
> > > > +		 * mode. FRAMESYNC is taken from the slowest link.
> > > > +		 */
> > > > +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > +			      MAX9286_FSYNCMETH_AUTO);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Manual FRAMESYNC
> > > > +	 *
> > > > +	 * The FRAMESYNC generator is configured with a period expressed as a
> > > > +	 * number of PCLK periods, which runs at 75MHz.
> > > > +	 */
> > > > +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> > > > +			priv->interval.denominator);
> > >
> > > Don't we calculate the pixel rate dynamically based on the number of
> > > enabled sources ?
> >
> > That's a good point, this hardcodes operation for 4 cameras. I'll see
> > how I can fix that.
> >
> > > > +
> > > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> > > > +		      MAX9286_FSYNCMETH_MANUAL);
> > > > +
> > > > +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> > > > +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> > > > +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> > > > +}
> > > > +
> > > >  /* -----------------------------------------------------------------------------
> > > >   * V4L2 Subdev
> > > >   */
> > > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > > >  	int ret;
> > > >
> > > >  	if (enable) {
> > > > +		max9286_set_fsync_period(priv);
> > > > +
> > > >  		/*
> > > >  		 * The frame sync between cameras is transmitted across the
> > > >  		 * reverse channel as GPIO. We must open all channels while
> > > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> > > > +				    struct v4l2_subdev_frame_interval *interval)
> > > > +{
> > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > +
> > > > +	if (interval->pad == MAX9286_SRC_PAD)
> > >
> > > Ah!
> > >
> > > I would have expected, as this setting applies to all cameras, and
> > > consequentially to the output image stream frame rate, that the
> > > operation should have been done on the single source pad... Why is
> > > this not the case ?
> >
> > I went back and forth between the two, I think it would work fine on the
> > source pad as well. I'll give it a try.
>
> Note that once we'll move to the muxed streams API, the frame interval
> will be per stream, so setting it on the source pad won't make much of a
> difference. What do you think ?
>

Per-stream means you have it apply on all the enabled source pads, and
potentially configure cameras differently, which is not supported
because frame sync is a configuration parameter which applies to all
sources.

If I were you I would have used the single source pad, as from the
user perspective what is relevant is the max9286 produced frame rate,
but I understand the counter-argument, so up to you :)

Thanks
   j

> > > > +		return -EINVAL;
> > > > +
> > > > +	interval->interval = priv->interval;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> > > > +				    struct v4l2_subdev_frame_interval *interval)
> > > > +{
> > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > +
> > > > +	if (interval->pad == MAX9286_SRC_PAD)
> > > > +		return -EINVAL;
> > > > +
> > > > +	priv->interval = interval->interval;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > > >  				  struct v4l2_subdev_state *sd_state,
> > > >  				  struct v4l2_subdev_mbus_code_enum *code)
> > > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > > >
> > > >  static const struct v4l2_subdev_video_ops max9286_video_ops = {
> > > >  	.s_stream	= max9286_s_stream,
> > > > +	.g_frame_interval = max9286_g_frame_interval,
> > > > +	.s_frame_interval = max9286_s_frame_interval,
> > > >  };
> > > >
> > > >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > > >  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> > > >  		      MAX9286_DATATYPE_YUV422_8BIT);
> > > >
> > > > -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > > > -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > -		      MAX9286_FSYNCMETH_AUTO);
> > > > +	max9286_set_fsync_period(priv);
> > > >
> > > >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> > > >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 3/3] media: i2c: max9286: Support manual framesync operation

[Laurent Pinchart]

Hi Jacopo,

On Mon, Dec 20, 2021 at 09:43:57AM +0100, Jacopo Mondi wrote:
> On Fri, Dec 17, 2021 at 07:49:07PM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 17, 2021 at 06:05:02PM +0200, Laurent Pinchart wrote:
> > > On Fri, Dec 17, 2021 at 12:14:03PM +0100, Jacopo Mondi wrote:
> > > > On Fri, Dec 17, 2021 at 12:09:46AM +0200, Laurent Pinchart wrote:
> > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >
> > > > > The MAX9286 can generate a framesync signal to synchronize the cameras,
> > > > > using an internal timer. Support this mode of operation and configure it
> > > > > through the .s_frameinterval() operation. If the frame interval is not
> > > > > 0, framesync is switched to manual mode with the specified interval,
> > > > > otherwise automatic mode is used.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > >  drivers/media/i2c/max9286.c | 66 +++++++++++++++++++++++++++++++++++--
> > > > >  1 file changed, 63 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > > > index fa2f6a823fe6..dce1146635d2 100644
> > > > > --- a/drivers/media/i2c/max9286.c
> > > > > +++ b/drivers/media/i2c/max9286.c
> > > > > @@ -174,6 +174,7 @@ struct max9286_priv {
> > > > >  	struct v4l2_ctrl *pixelrate;
> > > > >
> > > > >  	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> > > > > +	struct v4l2_fract interval;
> > > > >
> > > > >  	/* Protects controls and fmt structures */
> > > > >  	struct mutex mutex;
> > > > > @@ -474,6 +475,37 @@ static int max9286_check_config_link(struct max9286_priv *priv,
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +static void max9286_set_fsync_period(struct max9286_priv *priv)
> > > > > +{
> > > > > +	u32 fsync;
> > > > > +
> > > > > +	if (!priv->interval.numerator || !priv->interval.denominator) {
> > > > > +		/*
> > > > > +		 * Special case, a null interval enables automatic FRAMESYNC
> > > > > +		 * mode. FRAMESYNC is taken from the slowest link.
> > > > > +		 */
> > > > > +		max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > > +			      MAX9286_FSYNCMETH_AUTO);
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * Manual FRAMESYNC
> > > > > +	 *
> > > > > +	 * The FRAMESYNC generator is configured with a period expressed as a
> > > > > +	 * number of PCLK periods, which runs at 75MHz.
> > > > > +	 */
> > > > > +	fsync = div_u64(75000000ULL * priv->interval.numerator,
> > > > > +			priv->interval.denominator);
> > > >
> > > > Don't we calculate the pixel rate dynamically based on the number of
> > > > enabled sources ?
> > >
> > > That's a good point, this hardcodes operation for 4 cameras. I'll see
> > > how I can fix that.
> > >
> > > > > +
> > > > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_OUT |
> > > > > +		      MAX9286_FSYNCMETH_MANUAL);
> > > > > +
> > > > > +	max9286_write(priv, 0x06, (fsync >> 0) & 0xff);
> > > > > +	max9286_write(priv, 0x07, (fsync >> 8) & 0xff);
> > > > > +	max9286_write(priv, 0x08, (fsync >> 16) & 0xff);
> > > > > +}
> > > > > +
> > > > >  /* -----------------------------------------------------------------------------
> > > > >   * V4L2 Subdev
> > > > >   */
> > > > > @@ -656,6 +688,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > > > >  	int ret;
> > > > >
> > > > >  	if (enable) {
> > > > > +		max9286_set_fsync_period(priv);
> > > > > +
> > > > >  		/*
> > > > >  		 * The frame sync between cameras is transmitted across the
> > > > >  		 * reverse channel as GPIO. We must open all channels while
> > > > > @@ -715,6 +749,32 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > +static int max9286_g_frame_interval(struct v4l2_subdev *sd,
> > > > > +				    struct v4l2_subdev_frame_interval *interval)
> > > > > +{
> > > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > > +
> > > > > +	if (interval->pad == MAX9286_SRC_PAD)
> > > >
> > > > Ah!
> > > >
> > > > I would have expected, as this setting applies to all cameras, and
> > > > consequentially to the output image stream frame rate, that the
> > > > operation should have been done on the single source pad... Why is
> > > > this not the case ?
> > >
> > > I went back and forth between the two, I think it would work fine on the
> > > source pad as well. I'll give it a try.
> >
> > Note that once we'll move to the muxed streams API, the frame interval
> > will be per stream, so setting it on the source pad won't make much of a
> > difference. What do you think ?
> >
> 
> Per-stream means you have it apply on all the enabled source pads, and
> potentially configure cameras differently, which is not supported
> because frame sync is a configuration parameter which applies to all
> sources.
> 
> If I were you I would have used the single source pad, as from the
> user perspective what is relevant is the max9286 produced frame rate,
> but I understand the counter-argument, so up to you :)

The single source pad will have four streams, so it will still be
per-stream, even on a single pad.

> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	interval->interval = priv->interval;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int max9286_s_frame_interval(struct v4l2_subdev *sd,
> > > > > +				    struct v4l2_subdev_frame_interval *interval)
> > > > > +{
> > > > > +	struct max9286_priv *priv = sd_to_max9286(sd);
> > > > > +
> > > > > +	if (interval->pad == MAX9286_SRC_PAD)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	priv->interval = interval->interval;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> > > > >  				  struct v4l2_subdev_state *sd_state,
> > > > >  				  struct v4l2_subdev_mbus_code_enum *code)
> > > > > @@ -806,6 +866,8 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
> > > > >
> > > > >  static const struct v4l2_subdev_video_ops max9286_video_ops = {
> > > > >  	.s_stream	= max9286_s_stream,
> > > > > +	.g_frame_interval = max9286_g_frame_interval,
> > > > > +	.s_frame_interval = max9286_s_frame_interval,
> > > > >  };
> > > > >
> > > > >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> > > > > @@ -998,9 +1060,7 @@ static int max9286_setup(struct max9286_priv *priv)
> > > > >  		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> > > > >  		      MAX9286_DATATYPE_YUV422_8BIT);
> > > > >
> > > > > -	/* Automatic: FRAMESYNC taken from the slowest Link. */
> > > > > -	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > > > -		      MAX9286_FSYNCMETH_AUTO);
> > > > > +	max9286_set_fsync_period(priv);
> > > > >
> > > > >  	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> > > > >  	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: i2c: max9286: Add support for port regulators

[Thomas Nizan]

Hi Laurent,
On 2021-12-16 11:09 p.m., Laurent Pinchart wrote:
> From: Thomas Nizan <tnizan@witekio.com>
> 
> Allow users to use one PoC regulator per port, instead of a global
> regulator.
> 
> The properties '^port[0-3]-poc-supply$' in the DT node are used to
> indicate the regulators for individual ports.
> 
Signed-off-by: Thomas Nizan <tnizan@witekio.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 112 +++++++++++++++++++++++++++++++-----
>  1 file changed, 98 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7c663fd587bb..fa2f6a823fe6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -136,8 +136,10 @@
>  #define MAX9286_SRC_PAD			4
>  
>  struct max9286_source {
> +	unsigned int index;
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> +	struct regulator *regulator;
>  };
>  
>  struct max9286_asd {
> @@ -1072,6 +1074,49 @@ static int max9286_register_gpio(struct max9286_priv *priv)
>  	return ret;
>  }
>  
> +static int max9286_poc_power_on(struct max9286_priv *priv)
> +{
> +	struct max9286_source *source;
> +	unsigned int enabled = 0;
> +	int ret;
> +
> +	/* Enable the global regulator if available. */
> +	if (priv->regulator)
> +		return regulator_enable(priv->regulator);
> +
> +	/* Otherwise use the per-port regulators. */
> +	for_each_source(priv, source) {
> +		ret = regulator_enable(source->regulator);
> +		if (ret < 0)
> +			goto error;
> +
> +		enabled |= BIT(source->index);
> +	}
> +
> +	return 0;
> +
> +error:
> +	for_each_source(priv, source) {
> +		if (enabled & BIT(source->index))
> +			regulator_disable(source->regulator);
> +	}
> +
> +	return ret;
> +}
> +
> +static void max9286_poc_power_off(struct max9286_priv *priv)
> +{
> +	struct max9286_source *source;
> +
> +	if (priv->regulator) {
> +		regulator_disable(priv->regulator);
> +		return;
> +	}
> +
> +	for_each_source(priv, source)
> +		regulator_disable(source->regulator);
> +}
> +
>  static int max9286_init(struct device *dev)
>  {
>  	struct max9286_priv *priv;
> @@ -1082,9 +1127,9 @@ static int max9286_init(struct device *dev)
>  	priv = i2c_get_clientdata(client);
>  
>  	/* Enable the bus power. */
> -	ret = regulator_enable(priv->regulator);
> +	ret = max9286_poc_power_on(priv);
>  	if (ret < 0) {
> -		dev_err(&client->dev, "Unable to turn PoC on\n");
> +		dev_err(dev, "Unable to turn PoC on\n");
>  		return ret;
>  	}
>  
> @@ -1118,7 +1163,7 @@ static int max9286_init(struct device *dev)
>  err_v4l2_register:
>  	max9286_v4l2_unregister(priv);
>  err_regulator:
> -	regulator_disable(priv->regulator);
> +	max9286_poc_power_off(priv);
>  
>  	return ret;
>  }
> @@ -1215,6 +1260,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  		}
>  
>  		source = &priv->sources[ep.port];
> +		source->index = ep.port;
>  		source->fwnode = fwnode_graph_get_remote_endpoint(
>  						of_fwnode_handle(node));
>  		if (!source->fwnode) {
> @@ -1249,6 +1295,50 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	return 0;
>  }
>  
> +static int max9286_get_poc_supplies(struct max9286_priv *priv)
> +{
> +	struct device *dev = &priv->client->dev;
> +	struct max9286_source *source;
> +
> +	/*
> +	 * Start by getting the global regulator. Usage of the exclusive API is
> +	 * required to receive an error in case the supply isn't specified in
> +	 * the device tree.
> +	 */
> +	priv->regulator = devm_regulator_get_exclusive(dev, "poc");
> +	if (!IS_ERR(priv->regulator))
> +		return 0;
> +
> +	if (PTR_ERR(priv->regulator) != -ENODEV) {
> +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> +			dev_err(dev, "Unable to get PoC regulator: %ld\n",
> +				PTR_ERR(priv->regulator));
> +		return PTR_ERR(priv->regulator);
> +	}
> +
> +	/* If there's no global regulator, get per-port regulators. */
> +	dev_dbg(dev,
> +		"No global PoC regulator, looking for per-port regulators\n");
> +	priv->regulator = NULL;
> +
> +	for_each_source(priv, source) {
> +		char name[10];
> +
> +		snprintf(name, sizeof(name), "port%u-poc", source->index);
> +		source->regulator = devm_regulator_get_exclusive(dev, name);
> +		if (IS_ERR(source->regulator)) {
> +			if (PTR_ERR(source->regulator) != -EPROBE_DEFER)
> +				dev_err(dev,
> +					"Unable to get port %u PoC regulator: %ld\n",
> +					source->index,
> +					PTR_ERR(source->regulator));
> +			return PTR_ERR(source->regulator);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int max9286_probe(struct i2c_client *client)
>  {
>  	struct max9286_priv *priv;
> @@ -1293,17 +1383,11 @@ static int max9286_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_powerdown;
>  
> -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> -	if (IS_ERR(priv->regulator)) {
> -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> -			dev_err(&client->dev,
> -				"Unable to get PoC regulator (%ld)\n",
> -				PTR_ERR(priv->regulator));
> -		ret = PTR_ERR(priv->regulator);
> -		goto err_powerdown;
> -	}
> -
>  	ret = max9286_parse_dt(priv);
> +	if (ret)
> +		goto err_cleanup_dt;
> +
> +	ret = max9286_get_poc_supplies(priv);
>  	if (ret)
>  		goto err_powerdown;
>  
> @@ -1329,7 +1413,7 @@ static int max9286_remove(struct i2c_client *client)
>  
>  	max9286_v4l2_unregister(priv);
>  
> -	regulator_disable(priv->regulator);
> +	max9286_poc_power_off(priv);
>  
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
>  
> 
Regards,
Thomas

Re: [PATCH 2/3] media: i2c: max9286: Add support for port regulators

[Laurent Pinchart]

Hi Thomas,

On Mon, Dec 20, 2021 at 04:06:26PM +0100, Thomas Nizan wrote:
> On 2021-12-16 11:09 p.m., Laurent Pinchart wrote:
> > From: Thomas Nizan <tnizan@witekio.com>
> > 
> > Allow users to use one PoC regulator per port, instead of a global
> > regulator.
> > 
> > The properties '^port[0-3]-poc-supply$' in the DT node are used to
> > indicate the regulators for individual ports.
>
> Signed-off-by: Thomas Nizan <tnizan@witekio.com>

Thank you.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/i2c/max9286.c | 112 +++++++++++++++++++++++++++++++-----
> >  1 file changed, 98 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 7c663fd587bb..fa2f6a823fe6 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -136,8 +136,10 @@
> >  #define MAX9286_SRC_PAD			4
> >  
> >  struct max9286_source {
> > +	unsigned int index;
> >  	struct v4l2_subdev *sd;
> >  	struct fwnode_handle *fwnode;
> > +	struct regulator *regulator;
> >  };
> >  
> >  struct max9286_asd {
> > @@ -1072,6 +1074,49 @@ static int max9286_register_gpio(struct max9286_priv *priv)
> >  	return ret;
> >  }
> >  
> > +static int max9286_poc_power_on(struct max9286_priv *priv)
> > +{
> > +	struct max9286_source *source;
> > +	unsigned int enabled = 0;
> > +	int ret;
> > +
> > +	/* Enable the global regulator if available. */
> > +	if (priv->regulator)
> > +		return regulator_enable(priv->regulator);
> > +
> > +	/* Otherwise use the per-port regulators. */
> > +	for_each_source(priv, source) {
> > +		ret = regulator_enable(source->regulator);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		enabled |= BIT(source->index);
> > +	}
> > +
> > +	return 0;
> > +
> > +error:
> > +	for_each_source(priv, source) {
> > +		if (enabled & BIT(source->index))
> > +			regulator_disable(source->regulator);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void max9286_poc_power_off(struct max9286_priv *priv)
> > +{
> > +	struct max9286_source *source;
> > +
> > +	if (priv->regulator) {
> > +		regulator_disable(priv->regulator);
> > +		return;
> > +	}
> > +
> > +	for_each_source(priv, source)
> > +		regulator_disable(source->regulator);
> > +}
> > +
> >  static int max9286_init(struct device *dev)
> >  {
> >  	struct max9286_priv *priv;
> > @@ -1082,9 +1127,9 @@ static int max9286_init(struct device *dev)
> >  	priv = i2c_get_clientdata(client);
> >  
> >  	/* Enable the bus power. */
> > -	ret = regulator_enable(priv->regulator);
> > +	ret = max9286_poc_power_on(priv);
> >  	if (ret < 0) {
> > -		dev_err(&client->dev, "Unable to turn PoC on\n");
> > +		dev_err(dev, "Unable to turn PoC on\n");
> >  		return ret;
> >  	}
> >  
> > @@ -1118,7 +1163,7 @@ static int max9286_init(struct device *dev)
> >  err_v4l2_register:
> >  	max9286_v4l2_unregister(priv);
> >  err_regulator:
> > -	regulator_disable(priv->regulator);
> > +	max9286_poc_power_off(priv);
> >  
> >  	return ret;
> >  }
> > @@ -1215,6 +1260,7 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  		}
> >  
> >  		source = &priv->sources[ep.port];
> > +		source->index = ep.port;
> >  		source->fwnode = fwnode_graph_get_remote_endpoint(
> >  						of_fwnode_handle(node));
> >  		if (!source->fwnode) {
> > @@ -1249,6 +1295,50 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  	return 0;
> >  }
> >  
> > +static int max9286_get_poc_supplies(struct max9286_priv *priv)
> > +{
> > +	struct device *dev = &priv->client->dev;
> > +	struct max9286_source *source;
> > +
> > +	/*
> > +	 * Start by getting the global regulator. Usage of the exclusive API is
> > +	 * required to receive an error in case the supply isn't specified in
> > +	 * the device tree.
> > +	 */
> > +	priv->regulator = devm_regulator_get_exclusive(dev, "poc");
> > +	if (!IS_ERR(priv->regulator))
> > +		return 0;
> > +
> > +	if (PTR_ERR(priv->regulator) != -ENODEV) {
> > +		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > +			dev_err(dev, "Unable to get PoC regulator: %ld\n",
> > +				PTR_ERR(priv->regulator));
> > +		return PTR_ERR(priv->regulator);
> > +	}
> > +
> > +	/* If there's no global regulator, get per-port regulators. */
> > +	dev_dbg(dev,
> > +		"No global PoC regulator, looking for per-port regulators\n");
> > +	priv->regulator = NULL;
> > +
> > +	for_each_source(priv, source) {
> > +		char name[10];
> > +
> > +		snprintf(name, sizeof(name), "port%u-poc", source->index);
> > +		source->regulator = devm_regulator_get_exclusive(dev, name);
> > +		if (IS_ERR(source->regulator)) {
> > +			if (PTR_ERR(source->regulator) != -EPROBE_DEFER)
> > +				dev_err(dev,
> > +					"Unable to get port %u PoC regulator: %ld\n",
> > +					source->index,
> > +					PTR_ERR(source->regulator));
> > +			return PTR_ERR(source->regulator);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int max9286_probe(struct i2c_client *client)
> >  {
> >  	struct max9286_priv *priv;
> > @@ -1293,17 +1383,11 @@ static int max9286_probe(struct i2c_client *client)
> >  	if (ret)
> >  		goto err_powerdown;
> >  
> > -	priv->regulator = devm_regulator_get(&client->dev, "poc");
> > -	if (IS_ERR(priv->regulator)) {
> > -		if (PTR_ERR(priv->regulator) != -EPROBE_DEFER)
> > -			dev_err(&client->dev,
> > -				"Unable to get PoC regulator (%ld)\n",
> > -				PTR_ERR(priv->regulator));
> > -		ret = PTR_ERR(priv->regulator);
> > -		goto err_powerdown;
> > -	}
> > -
> >  	ret = max9286_parse_dt(priv);
> > +	if (ret)
> > +		goto err_cleanup_dt;
> > +
> > +	ret = max9286_get_poc_supplies(priv);
> >  	if (ret)
> >  		goto err_powerdown;
> >  
> > @@ -1329,7 +1413,7 @@ static int max9286_remove(struct i2c_client *client)
> >  
> >  	max9286_v4l2_unregister(priv);
> >  
> > -	regulator_disable(priv->regulator);
> > +	max9286_poc_power_off(priv);
> >  
> >  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> >  

-- 
Regards,

Laurent Pinchart