[PATCH v3 00/12] media: i2c: max9286: Small new features

[PATCH v3 00/12] media: i2c: max9286: Small new features

[Laurent Pinchart]

Hello,

This small patch series adds a few new features to the max9286 driver:

- Support for per-port supplies (01/12 and 04/12)
- Remote I2C bus speed selection (02/12 and 09/12)
- GMSL bus width selection (03/12 and 10/12)
- Manual framesync operation (05/12)
- RAW12 support (06/12 and 07/12)

The remaining patches are small cleanups. Please see individual patches
for details.

Compared to v2, I've incorporated all review comments and rebased the
series on top of the latest media tree (with a notable conflict due to
the PoC GPIO support that has been merged in the mainline kernel). Most
of v2 has received Reviewed-by tags, only a few patches are missing
them, so I have good hopes to land this in v6.3.

Laurent Pinchart (11):
  dt-bindings: media: i2c: max9286: Add support for per-port supplies
  dt-bindings: media: i2c: max9286: Add property to select I2C speed
  dt-bindings: media: i2c: max9286: Add property to select bus width
  media: i2c: max9286: Support manual framesync operation
  media: i2c: max9286: Rename MAX9286_DATATYPE_RAW11 to RAW12
  media: i2c: max9286: Support 12-bit raw bayer formats
  media: i2c: max9286: Define macros for all bits of register 0x15
  media: i2c: max9286: Configure remote I2C speed from device tree
  media: i2c: max9286: Configure bus width from device tree
  media: i2c: max9286: Select HS as data enable signal
  media: i2c: max9286: Print power-up GMSL link configuration

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

 .../bindings/media/i2c/maxim,max9286.yaml     |  51 +-
 drivers/media/i2c/max9286.c                   | 465 +++++++++++++++---
 2 files changed, 430 insertions(+), 86 deletions(-)


base-commit: 3178804c64ef7c8c87a53cd5bba0b2942dd64fec
-- 
Regards,

Laurent Pinchart

[PATCH v3 01/12] dt-bindings: media: i2c: max9286: Add support for per-port supplies

[Laurent Pinchart]

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+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes since v2:

- Make maxim,gpio-poc and port[0-3]-poc-supply mutually exclusive

Changes since v1:

- Simplify mutual exclusion condition
---
 .../bindings/media/i2c/maxim,max9286.yaml     | 35 +++++++++++++------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 90315e217003..4f28690eabcd 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
@@ -182,21 +182,36 @@ properties:
 
             additionalProperties: false
 
+patternProperties:
+  "^port[0-3]-poc-supply$":
+    description: Regulator providing Power over Coax for a particular port
+
 required:
   - compatible
   - reg
   - ports
   - i2c-mux
 
-# If 'maxim,gpio-poc' is present, then 'poc-supply' and 'gpio-controller'
-# are not allowed.
-if:
-  required:
-    - maxim,gpio-poc
-then:
-  properties:
-    poc-supply: false
-    gpio-controller: false
+allOf:
+  # Only one way of specifying power supplies is allowed: 'maxim,gpio-poc',
+  # 'poc-supply' or per-port poc-supply. Additionally, if 'maxim,gpio-poc' is
+  # present, then 'gpio-controller' isn't allowed.
+  - if:
+      required:
+        - maxim,gpio-poc
+    then:
+      properties:
+        poc-supply: false
+        gpio-controller: false
+      patternProperties:
+        "^port[0-3]-poc-supply$": false
+
+  - if:
+      required:
+        - poc-supply
+    then:
+      patternProperties:
+        "^port[0-3]-poc-supply$": false
 
 additionalProperties: false
 
-- 
Regards,

Laurent Pinchart

[PATCH v3 02/12] dt-bindings: media: i2c: max9286: Add property to select I2C speed

[Laurent Pinchart]

The I2C speed on the remote side (the I2C master bus of the connected
serializers) is configurable, and doesn't need to match the speed of the
local bus (the slave bus of the MAX9286). All remote buses must use the
same speed, and the MAX9286 needs to be programmed accordingly. Add a
new DT property to select the speed to make it configurable.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes since v2:

- Rename property to maxim,i2c-remote-bus-hz
- Specify the property type
---
 .../devicetree/bindings/media/i2c/maxim,max9286.yaml      | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 4f28690eabcd..315a6eef6198 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -50,6 +50,14 @@ properties:
   '#gpio-cells':
     const: 2
 
+  maxim,i2c-remote-bus-hz:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 8470, 28300, 84700, 105000, 173000, 339000, 533000, 837000 ]
+    default: 105000
+    description: |
+      The I2C clock frequency for the remote I2C buses. The value must match
+      the configuration of the remote serializers.
+
   maxim,reverse-channel-microvolt:
     minimum: 30000
     maximum: 200000
-- 
Regards,

Laurent Pinchart

[PATCH v3 03/12] dt-bindings: media: i2c: max9286: Add property to select bus width

[Laurent Pinchart]

The GMSL serial data bus width is normally selected by the BWS pin, but
it can also be configured by software. Add a DT property that allows
overriding the value of the BWS-selected bus width to support systems
whose BWS pin doesn't result in the correct value.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes since v2:

- Specify the property type
---
 .../devicetree/bindings/media/i2c/maxim,max9286.yaml      | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 315a6eef6198..fbacf2a45dfc 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -50,6 +50,14 @@ properties:
   '#gpio-cells':
     const: 2
 
+  maxim,bus-width:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 24, 27, 32 ]
+    description: |
+      The GMSL serial data bus width. This setting is normally controlled by
+      the BWS pin, but may be overridden with this property. The value must
+      match the configuration of the remote serializers.
+
   maxim,i2c-remote-bus-hz:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [ 8470, 28300, 84700, 105000, 173000, 339000, 533000, 837000 ]
-- 
Regards,

Laurent Pinchart

[PATCH v3 04/12] 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: Thomas Nizan <tnizan@witekio.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Adapt to the poc-gpio support

Changes since v1:

- Use to_index()
- Use dev_err_probe()
- Fix error path in probe()
- Use devm_regulator_get_optional() instead of
  devm_regulator_get_exclusive()
---
 drivers/media/i2c/max9286.c | 138 ++++++++++++++++++++++++++++++------
 1 file changed, 115 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 9c083cf14231..d79077939847 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -139,6 +139,7 @@
 struct max9286_source {
 	struct v4l2_subdev *sd;
 	struct fwnode_handle *fwnode;
+	struct regulator *regulator;
 };
 
 struct max9286_asd {
@@ -169,6 +170,7 @@ struct max9286_priv {
 	u32 init_rev_chan_mv;
 	u32 rev_chan_mv;
 
+	bool use_gpio_poc;
 	u32 gpio_poc[2];
 
 	struct v4l2_ctrl_handler ctrls;
@@ -1088,9 +1090,6 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
 	struct device *dev = &priv->client->dev;
 	int ret;
 
-	/* GPIO values default to high */
-	priv->gpio_state = BIT(0) | BIT(1);
-
 	/*
 	 * Parse the "gpio-poc" vendor property. If the property is not
 	 * specified the camera power is controlled by a regulator.
@@ -1102,18 +1101,7 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
 		 * If gpio lines are not used for the camera power, register
 		 * a gpio controller for consumers.
 		 */
-		ret = max9286_register_gpio(priv);
-		if (ret)
-			return ret;
-
-		priv->regulator = devm_regulator_get(dev, "poc");
-		if (IS_ERR(priv->regulator)) {
-			return dev_err_probe(dev, PTR_ERR(priv->regulator),
-					     "Unable to get PoC regulator (%ld)\n",
-					     PTR_ERR(priv->regulator));
-		}
-
-		return 0;
+		return max9286_register_gpio(priv);
 	}
 
 	/* If the property is specified make sure it is well formed. */
@@ -1124,21 +1112,75 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
 		return -EINVAL;
 	}
 
+	priv->use_gpio_poc = true;
 	return 0;
 }
 
+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);
+
+	if (priv->use_gpio_poc)
+		return max9286_gpio_set(priv, priv->gpio_poc[0],
+					!priv->gpio_poc[1]);
+
+	/* Otherwise use the per-port regulators. */
+	for_each_source(priv, source) {
+		ret = regulator_enable(source->regulator);
+		if (ret < 0)
+			goto error;
+
+		enabled |= BIT(to_index(priv, source));
+	}
+
+	return 0;
+
+error:
+	for_each_source(priv, source) {
+		if (enabled & BIT(to_index(priv, source)))
+			regulator_disable(source->regulator);
+	}
+
+	return ret;
+}
+
+static int max9286_poc_power_off(struct max9286_priv *priv)
+{
+	struct max9286_source *source;
+	int ret = 0;
+
+	if (priv->regulator)
+		return regulator_disable(priv->regulator);
+
+	if (priv->use_gpio_poc)
+		return max9286_gpio_set(priv, priv->gpio_poc[0],
+					priv->gpio_poc[1]);
+
+	for_each_source(priv, source) {
+		int err;
+
+		err = regulator_disable(source->regulator);
+		if (!ret)
+			ret = err;
+	}
+
+	return ret;
+}
+
 static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
 {
 	int ret;
 
-	/* If the regulator is not available, use gpio to control power. */
-	if (!priv->regulator)
-		ret = max9286_gpio_set(priv, priv->gpio_poc[0],
-				       enable ^ priv->gpio_poc[1]);
-	else if (enable)
-		ret = regulator_enable(priv->regulator);
+	if (enable)
+		ret = max9286_poc_power_on(priv);
 	else
-		ret = regulator_disable(priv->regulator);
+		ret = max9286_poc_power_off(priv);
 
 	if (ret < 0)
 		dev_err(&priv->client->dev, "Unable to turn power %s\n",
@@ -1317,6 +1359,47 @@ 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;
+	int ret;
+
+	/* Start by getting the global regulator. */
+	priv->regulator = devm_regulator_get_optional(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) {
+		unsigned int index = to_index(priv, source);
+		char name[10];
+
+		snprintf(name, sizeof(name), "port%u-poc", index);
+		source->regulator = devm_regulator_get(dev, name);
+		if (IS_ERR(source->regulator)) {
+			ret = PTR_ERR(source->regulator);
+			dev_err_probe(dev, ret,
+				      "Unable to get port %u PoC regulator\n",
+				      index);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int max9286_probe(struct i2c_client *client)
 {
 	struct max9286_priv *priv;
@@ -1330,6 +1413,9 @@ static int max9286_probe(struct i2c_client *client)
 
 	priv->client = client;
 
+	/* GPIO values default to high */
+	priv->gpio_state = BIT(0) | BIT(1);
+
 	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
 						   GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->gpiod_pwdn))
@@ -1362,7 +1448,13 @@ static int max9286_probe(struct i2c_client *client)
 
 	ret = max9286_parse_dt(priv);
 	if (ret)
-		goto err_powerdown;
+		goto err_cleanup_dt;
+
+	if (!priv->use_gpio_poc) {
+		ret = max9286_get_poc_supplies(priv);
+		if (ret)
+			goto err_cleanup_dt;
+	}
 
 	ret = max9286_init(priv);
 	if (ret < 0)
-- 
Regards,

Laurent Pinchart

[PATCH v3 05/12] media: i2c: max9286: Support manual framesync operation

[Laurent Pinchart]

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+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changes since v1:

- Use pixel rate to calculate frame sync counter
---
 drivers/media/i2c/max9286.c | 84 +++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index d79077939847..cb734a97afea 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -174,9 +174,11 @@ struct max9286_priv {
 	u32 gpio_poc[2];
 
 	struct v4l2_ctrl_handler ctrls;
-	struct v4l2_ctrl *pixelrate;
+	struct v4l2_ctrl *pixelrate_ctrl;
+	unsigned int pixelrate;
 
 	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
+	struct v4l2_fract interval;
 
 	/* Protects controls and fmt structures */
 	struct mutex mutex;
@@ -477,6 +479,40 @@ 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.
+	 */
+	fsync = div_u64((u64)priv->pixelrate * priv->interval.numerator,
+			priv->interval.denominator);
+
+	dev_dbg(&priv->client->dev, "fsync period %u (pclk %u)\n", fsync,
+		priv->pixelrate);
+
+	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
  */
@@ -515,11 +551,13 @@ static int max9286_set_pixelrate(struct max9286_priv *priv)
 		return -EINVAL;
 	}
 
+	priv->pixelrate = pixelrate;
+
 	/*
 	 * The CSI-2 transmitter pixel rate is the single source rate multiplied
 	 * by the number of available sources.
 	 */
-	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate,
+	return v4l2_ctrl_s_ctrl_int64(priv->pixelrate_ctrl,
 				      pixelrate * priv->nsources);
 }
 
@@ -659,6 +697,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
@@ -718,6 +758,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)
@@ -809,6 +875,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 = {
@@ -893,10 +961,10 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	v4l2_ctrl_handler_init(&priv->ctrls, 1);
-	priv->pixelrate = v4l2_ctrl_new_std(&priv->ctrls,
-					    &max9286_ctrl_ops,
-					    V4L2_CID_PIXEL_RATE,
-					    1, INT_MAX, 1, 50000000);
+	priv->pixelrate_ctrl = v4l2_ctrl_new_std(&priv->ctrls,
+						 &max9286_ctrl_ops,
+						 V4L2_CID_PIXEL_RATE,
+						 1, INT_MAX, 1, 50000000);
 
 	priv->sd.ctrl_handler = &priv->ctrls;
 	ret = priv->ctrls.error;
@@ -1006,9 +1074,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

[PATCH v3 06/12] media: i2c: max9286: Rename MAX9286_DATATYPE_RAW11 to RAW12

[Laurent Pinchart]

The MAX9286_DATATYPE_RAW11 value is used to configure the MAX9286 for
11-bit or 12-bit input data. While 11-bit data is supported on the GMSL
side, CSI-2 doesn't have a RAW11 format. 11-bit data is transferred over
CSI-2 as RAW12. Rename the macro accordingly to avoid confusion.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index cb734a97afea..028fa3547282 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -72,7 +72,7 @@
 #define MAX9286_DATATYPE_USER_YUV_12BIT	(10 << 0)
 #define MAX9286_DATATYPE_USER_24BIT	(9 << 0)
 #define MAX9286_DATATYPE_RAW14		(8 << 0)
-#define MAX9286_DATATYPE_RAW11		(7 << 0)
+#define MAX9286_DATATYPE_RAW12		(7 << 0)
 #define MAX9286_DATATYPE_RAW10		(6 << 0)
 #define MAX9286_DATATYPE_RAW8		(5 << 0)
 #define MAX9286_DATATYPE_YUV422_10BIT	(4 << 0)
-- 
Regards,

Laurent Pinchart

[PATCH v3 07/12] media: i2c: max9286: Support 12-bit raw bayer formats

[Laurent Pinchart]

Add support for 12-bit raw bayer formats to the driver, configuring the
GMSL format accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v1:

- Fix incorrect array index when media bus code isn't found
---
 drivers/media/i2c/max9286.c | 128 ++++++++++++++++++++++++++----------
 1 file changed, 95 insertions(+), 33 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 028fa3547282..3f1228c5053b 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -136,6 +136,11 @@
 #define MAX9286_N_PADS			5
 #define MAX9286_SRC_PAD			4
 
+struct max9286_format_info {
+	u32 code;
+	u8 datatype;
+};
+
 struct max9286_source {
 	struct v4l2_subdev *sd;
 	struct fwnode_handle *fwnode;
@@ -218,6 +223,34 @@ static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
 	return container_of(sd, struct max9286_priv, sd);
 }
 
+static const struct max9286_format_info max9286_formats[] = {
+	{
+		.code = MEDIA_BUS_FMT_UYVY8_1X16,
+		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
+	}, {
+		.code = MEDIA_BUS_FMT_VYUY8_1X16,
+		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
+	}, {
+		.code = MEDIA_BUS_FMT_YUYV8_1X16,
+		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
+	}, {
+		.code = MEDIA_BUS_FMT_YVYU8_1X16,
+		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
+	}, {
+		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
+		.datatype = MAX9286_DATATYPE_RAW12,
+	}, {
+		.code = MEDIA_BUS_FMT_SGBRG12_1X12,
+		.datatype = MAX9286_DATATYPE_RAW12,
+	}, {
+		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
+		.datatype = MAX9286_DATATYPE_RAW12,
+	}, {
+		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
+		.datatype = MAX9286_DATATYPE_RAW12,
+	},
+};
+
 /* -----------------------------------------------------------------------------
  * I2C IO
  */
@@ -479,6 +512,38 @@ static int max9286_check_config_link(struct max9286_priv *priv,
 	return 0;
 }
 
+static void max9286_set_video_format(struct max9286_priv *priv,
+				     const struct v4l2_mbus_framefmt *format)
+{
+	const struct max9286_format_info *info = NULL;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
+		if (max9286_formats[i].code == format->code) {
+			info = &max9286_formats[i];
+			break;
+		}
+	}
+
+	if (WARN_ON(!info))
+		return;
+
+	/*
+	 * Video format setup:
+	 * Disable CSI output, VC is set according to Link number.
+	 */
+	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
+
+	/* Enable CSI-2 Lane D0-D3 only, DBL mode. */
+	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
+		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
+		      info->datatype);
+
+	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
+	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
+		      MAX9286_HVSRC_D14);
+}
+
 static void max9286_set_fsync_period(struct max9286_priv *priv)
 {
 	u32 fsync;
@@ -697,6 +762,15 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 	int ret;
 
 	if (enable) {
+		const struct v4l2_mbus_framefmt *format;
+
+		/*
+		 * Get the format from the first used sink pad, as all sink
+		 * formats must be identical.
+		 */
+		format = &priv->fmt[__ffs(priv->bound_sources)];
+
+		max9286_set_video_format(priv, format);
 		max9286_set_fsync_period(priv);
 
 		/*
@@ -817,22 +891,20 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
 	struct v4l2_mbus_framefmt *cfg_fmt;
+	unsigned int i;
 
 	if (format->pad == MAX9286_SRC_PAD)
 		return -EINVAL;
 
-	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
-	switch (format->format.code) {
-	case MEDIA_BUS_FMT_UYVY8_1X16:
-	case MEDIA_BUS_FMT_VYUY8_1X16:
-	case MEDIA_BUS_FMT_YUYV8_1X16:
-	case MEDIA_BUS_FMT_YVYU8_1X16:
-		break;
-	default:
-		format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
-		break;
+	/* Validate the format. */
+	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
+		if (max9286_formats[i].code == format->format.code)
+			break;
 	}
 
+	if (i == ARRAY_SIZE(max9286_formats))
+		format->format.code = max9286_formats[0].code;
+
 	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
 					 format->which);
 	if (!cfg_fmt)
@@ -890,16 +962,20 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
 	.pad		= &max9286_pad_ops,
 };
 
+static const struct v4l2_mbus_framefmt max9286_default_format = {
+	.width		= 1280,
+	.height		= 800,
+	.code		= MEDIA_BUS_FMT_UYVY8_1X16,
+	.colorspace	= V4L2_COLORSPACE_SRGB,
+	.field		= V4L2_FIELD_NONE,
+	.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
+	.quantization	= V4L2_QUANTIZATION_DEFAULT,
+	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
+};
+
 static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
 {
-	fmt->width		= 1280;
-	fmt->height		= 800;
-	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
-	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
-	fmt->field		= V4L2_FIELD_NONE;
-	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
-	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
-	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
+	*fmt = max9286_default_format;
 }
 
 static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
@@ -1063,23 +1139,9 @@ static int max9286_setup(struct max9286_priv *priv)
 	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
 	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
 
-	/*
-	 * Video format setup:
-	 * Disable CSI output, VC is set according to Link number.
-	 */
-	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
-
-	/* Enable CSI-2 Lane D0-D3 only, DBL mode, YUV422 8-bit. */
-	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
-		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
-		      MAX9286_DATATYPE_YUV422_8BIT);
-
+	max9286_set_video_format(priv, &max9286_default_format);
 	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 |
-		      MAX9286_HVSRC_D14);
-
 	/*
 	 * The overlap window seems to provide additional validation by tracking
 	 * the delay between vsync and frame sync, generating an error if the
-- 
Regards,

Laurent Pinchart

[PATCH v3 08/12] media: i2c: max9286: Define macros for all bits of register 0x15

[Laurent Pinchart]

Macros are easier to read than numerical values.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 3f1228c5053b..e78456c8d24c 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -81,10 +81,13 @@
 #define MAX9286_DATATYPE_RGB565		(1 << 0)
 #define MAX9286_DATATYPE_RGB888		(0 << 0)
 /* Register 0x15 */
+#define MAX9286_CSI_IMAGE_TYP		BIT(7)
 #define MAX9286_VC(n)			((n) << 5)
 #define MAX9286_VCTYPE			BIT(4)
 #define MAX9286_CSIOUTEN		BIT(3)
-#define MAX9286_0X15_RESV		(3 << 0)
+#define MAX9286_SWP_ENDIAN		BIT(2)
+#define MAX9286_EN_CCBSYB_CLK_STR	BIT(1)
+#define MAX9286_EN_GPI_CCBSYB		BIT(0)
 /* Register 0x1b */
 #define MAX9286_SWITCHIN(n)		(1 << ((n) + 4))
 #define MAX9286_ENEQ(n)			(1 << (n))
@@ -529,10 +532,12 @@ static void max9286_set_video_format(struct max9286_priv *priv,
 		return;
 
 	/*
-	 * Video format setup:
-	 * Disable CSI output, VC is set according to Link number.
+	 * Video format setup: disable CSI output, set VC according to Link
+	 * number, enable I2C clock stretching when CCBSY is low, enable CCBSY
+	 * in external GPI-to-GPO mode.
 	 */
-	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
+	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_EN_CCBSYB_CLK_STR |
+		      MAX9286_EN_GPI_CCBSYB);
 
 	/* Enable CSI-2 Lane D0-D3 only, DBL mode. */
 	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
@@ -814,13 +819,17 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		}
 
 		/*
-		 * Enable CSI output, VC set according to link number.
-		 * Bit 7 must be set (chip manual says it's 0 and reserved).
+		 * Configure the CSI-2 output to line interleaved mode (W x (N
+		 * x H), as opposed to the (N x W) x H mode that outputs the
+		 * images stitched side-by-side) and enable it.
 		 */
-		max9286_write(priv, 0x15, 0x80 | MAX9286_VCTYPE |
-			      MAX9286_CSIOUTEN | MAX9286_0X15_RESV);
+		max9286_write(priv, 0x15, MAX9286_CSI_IMAGE_TYP | MAX9286_VCTYPE |
+			      MAX9286_CSIOUTEN | MAX9286_EN_CCBSYB_CLK_STR |
+			      MAX9286_EN_GPI_CCBSYB);
 	} else {
-		max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
+		max9286_write(priv, 0x15, MAX9286_VCTYPE |
+			      MAX9286_EN_CCBSYB_CLK_STR |
+			      MAX9286_EN_GPI_CCBSYB);
 
 		/* Stop all cameras. */
 		for_each_source(priv, source)
-- 
Regards,

Laurent Pinchart

[PATCH v3 09/12] media: i2c: max9286: Configure remote I2C speed from device tree

[Laurent Pinchart]

Read the maxim,i2c-clock-frequency DT property that specifies the speed
of the remote I2C bus, and configure the MAX9286 accordingly. The remote
serializers must all have a matching configuration.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v2:

- Rename DT property to "maxim,i2c-remote-bus-hz"
---
 drivers/media/i2c/max9286.c | 56 +++++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index e78456c8d24c..fffb0d2da416 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -144,6 +144,11 @@ struct max9286_format_info {
 	u8 datatype;
 };
 
+struct max9286_i2c_speed {
+	u32 rate;
+	u8 mstbt;
+};
+
 struct max9286_source {
 	struct v4l2_subdev *sd;
 	struct fwnode_handle *fwnode;
@@ -177,6 +182,7 @@ struct max9286_priv {
 	/* The initial reverse control channel amplitude. */
 	u32 init_rev_chan_mv;
 	u32 rev_chan_mv;
+	u8 i2c_mstbt;
 
 	bool use_gpio_poc;
 	u32 gpio_poc[2];
@@ -254,6 +260,17 @@ static const struct max9286_format_info max9286_formats[] = {
 	},
 };
 
+static const struct max9286_i2c_speed max9286_i2c_speeds[] = {
+	{ .rate =   8470, .mstbt = MAX9286_I2CMSTBT_8KBPS },
+	{ .rate =  28300, .mstbt = MAX9286_I2CMSTBT_28KBPS },
+	{ .rate =  84700, .mstbt = MAX9286_I2CMSTBT_84KBPS },
+	{ .rate = 105000, .mstbt = MAX9286_I2CMSTBT_105KBPS },
+	{ .rate = 173000, .mstbt = MAX9286_I2CMSTBT_173KBPS },
+	{ .rate = 339000, .mstbt = MAX9286_I2CMSTBT_339KBPS },
+	{ .rate = 533000, .mstbt = MAX9286_I2CMSTBT_533KBPS },
+	{ .rate = 837000, .mstbt = MAX9286_I2CMSTBT_837KBPS },
+};
+
 /* -----------------------------------------------------------------------------
  * I2C IO
  */
@@ -374,7 +391,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv)
 static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
 {
 	u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US |
-		    MAX9286_I2CMSTBT_105KBPS;
+		    priv->i2c_mstbt;
 
 	if (localack)
 		config |= MAX9286_I2CLOCACK;
@@ -1387,6 +1404,8 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	struct device_node *node = NULL;
 	unsigned int i2c_mux_mask = 0;
 	u32 reverse_channel_microvolt;
+	u32 i2c_clk_freq = 105000;
+	unsigned int i;
 
 	/* Balance the of_node_put() performed by of_find_node_by_name(). */
 	of_node_get(dev->of_node);
@@ -1477,6 +1496,23 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	}
 	of_node_put(node);
 
+	of_property_read_u32(dev->of_node, "maxim,i2c-remote-bus-hz",
+			     &i2c_clk_freq);
+	for (i = 0; i < ARRAY_SIZE(max9286_i2c_speeds); ++i) {
+		const struct max9286_i2c_speed *speed = &max9286_i2c_speeds[i];
+
+		if (speed->rate == i2c_clk_freq) {
+			priv->i2c_mstbt = speed->mstbt;
+			break;
+		}
+	}
+
+	if (i == ARRAY_SIZE(max9286_i2c_speeds)) {
+		dev_err(dev, "Invalid %s value %u\n", "maxim,i2c-remote-bus-hz",
+			i2c_clk_freq);
+		return -EINVAL;
+	}
+
 	/*
 	 * Parse the initial value of the reverse channel amplitude from
 	 * the firmware interface and convert it to millivolts.
@@ -1553,10 +1589,16 @@ static int max9286_probe(struct i2c_client *client)
 	/* GPIO values default to high */
 	priv->gpio_state = BIT(0) | BIT(1);
 
+	ret = max9286_parse_dt(priv);
+	if (ret)
+		goto err_cleanup_dt;
+
 	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
 						   GPIOD_OUT_HIGH);
-	if (IS_ERR(priv->gpiod_pwdn))
-		return PTR_ERR(priv->gpiod_pwdn);
+	if (IS_ERR(priv->gpiod_pwdn)) {
+		ret = PTR_ERR(priv->gpiod_pwdn);
+		goto err_cleanup_dt;
+	}
 
 	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
@@ -1583,10 +1625,6 @@ static int max9286_probe(struct i2c_client *client)
 	if (ret)
 		goto err_powerdown;
 
-	ret = max9286_parse_dt(priv);
-	if (ret)
-		goto err_cleanup_dt;
-
 	if (!priv->use_gpio_poc) {
 		ret = max9286_get_poc_supplies(priv);
 		if (ret)
@@ -1599,10 +1637,10 @@ static int max9286_probe(struct i2c_client *client)
 
 	return 0;
 
-err_cleanup_dt:
-	max9286_cleanup_dt(priv);
 err_powerdown:
 	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
+err_cleanup_dt:
+	max9286_cleanup_dt(priv);
 
 	return ret;
 }
-- 
Regards,

Laurent Pinchart

[PATCH v3 10/12] media: i2c: max9286: Configure bus width from device tree

[Laurent Pinchart]

The GMSL serial data bus width is normally selected through the BWS pin.
On some systems, the pin may not be wired to the correct value. Support
overriding the bus width by software, using the value specified in the
device tree.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Changes since v1:

- Add comment about the bus_width == 0 case
---
 drivers/media/i2c/max9286.c | 40 +++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index fffb0d2da416..125b4d434f57 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -91,6 +91,11 @@
 /* Register 0x1b */
 #define MAX9286_SWITCHIN(n)		(1 << ((n) + 4))
 #define MAX9286_ENEQ(n)			(1 << (n))
+/* Register 0x1c */
+#define MAX9286_HIGHIMM(n)		BIT((n) + 4)
+#define MAX9286_I2CSEL			BIT(2)
+#define MAX9286_HIBW			BIT(1)
+#define MAX9286_BWS			BIT(0)
 /* Register 0x27 */
 #define MAX9286_LOCKED			BIT(7)
 /* Register 0x31 */
@@ -183,6 +188,7 @@ struct max9286_priv {
 	u32 init_rev_chan_mv;
 	u32 rev_chan_mv;
 	u8 i2c_mstbt;
+	u32 bus_width;
 
 	bool use_gpio_poc;
 	u32 gpio_poc[2];
@@ -1168,6 +1174,23 @@ static int max9286_setup(struct max9286_priv *priv)
 	max9286_set_video_format(priv, &max9286_default_format);
 	max9286_set_fsync_period(priv);
 
+	if (priv->bus_width) {
+		int val;
+
+		val = max9286_read(priv, 0x1c);
+		if (val < 0)
+			return val;
+
+		val &= ~(MAX9286_HIBW | MAX9286_BWS);
+
+		if (priv->bus_width == 27)
+			val |= MAX9286_HIBW;
+		else if (priv->bus_width == 32)
+			val |= MAX9286_BWS;
+
+		max9286_write(priv, 0x1c, val);
+	}
+
 	/*
 	 * The overlap window seems to provide additional validation by tracking
 	 * the delay between vsync and frame sync, generating an error if the
@@ -1496,6 +1519,23 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 	}
 	of_node_put(node);
 
+	of_property_read_u32(dev->of_node, "maxim,bus-width", &priv->bus_width);
+	switch (priv->bus_width) {
+	case 0:
+		/*
+		 * The property isn't specified in the device tree, the driver
+		 * will keep the default value selected by the BWS pin.
+		 */
+	case 24:
+	case 27:
+	case 32:
+		break;
+	default:
+		dev_err(dev, "Invalid %s value %u\n", "maxim,bus-width",
+			priv->bus_width);
+		return -EINVAL;
+	}
+
 	of_property_read_u32(dev->of_node, "maxim,i2c-remote-bus-hz",
 			     &i2c_clk_freq);
 	for (i = 0; i < ARRAY_SIZE(max9286_i2c_speeds); ++i) {
-- 
Regards,

Laurent Pinchart

[PATCH v3 11/12] media: i2c: max9286: Select HS as data enable signal

[Laurent Pinchart]

GMSL can transport three synchronization signals: VSync, HSync and Data
Enable. The MAX9286 can select either HS or DE as a line valid signal.

Not all serializers (and transmission formats) support the DE signal.
The MAX9271, used by the RDACM20 and RDACM21 cameras, doesn't document
DE support. Nonetheless, the max9286 driver selects the DE signal as
line valid in register 0x0c (by not setting the DESEL bit). It's not
clear why this works. As HS is a more common line valid qualifier, set
the DESEL bit by default. This is needed to support the onsemi MARS
cameras.

If a camera requires usage of the DE signal in the future, this will
need to be made configurable.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo@jmondi.org> # On Eagle V3M with RDACM20
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 125b4d434f57..58cd2ff49e08 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -567,9 +567,12 @@ static void max9286_set_video_format(struct max9286_priv *priv,
 		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
 		      info->datatype);
 
-	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
-	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
-		      MAX9286_HVSRC_D14);
+	/*
+	 * Enable HS/VS encoding, use HS as line valid source, use D14/15 for
+	 * HS/VS, invert VS.
+	 */
+	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_DESEL |
+		      MAX9286_INVVS | MAX9286_HVSRC_D14);
 }
 
 static void max9286_set_fsync_period(struct max9286_priv *priv)
-- 
Regards,

Laurent Pinchart

[PATCH v3 12/12] media: i2c: max9286: Print power-up GMSL link configuration

[Laurent Pinchart]

The power-up GMSL link configuration is controlled by the HIM and BWS
pins, whose state is reflected in register 0x1c. Print the detected
power-up config in a debug message to help debugging.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/max9286.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 58cd2ff49e08..34a59a7d3ab4 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1156,6 +1156,7 @@ static int max9286_setup(struct max9286_priv *priv)
 		(2 << 6) | (1 << 4) | (0 << 2) | (3 << 0), /* 210x */
 		(3 << 6) | (2 << 4) | (1 << 2) | (0 << 0), /* 3210 */
 	};
+	int cfg;
 
 	/*
 	 * Set the I2C bus speed.
@@ -1177,21 +1178,23 @@ static int max9286_setup(struct max9286_priv *priv)
 	max9286_set_video_format(priv, &max9286_default_format);
 	max9286_set_fsync_period(priv);
 
+	cfg = max9286_read(priv, 0x1c);
+	if (cfg < 0)
+		return cfg;
+
+	dev_dbg(&priv->client->dev, "power-up config: %s immunity, %u-bit bus\n",
+		cfg & MAX9286_HIGHIMM(0) ? "high" : "legacy",
+		cfg & MAX9286_BWS ? 32 : cfg & MAX9286_HIBW ? 27 : 24);
+
 	if (priv->bus_width) {
-		int val;
-
-		val = max9286_read(priv, 0x1c);
-		if (val < 0)
-			return val;
-
-		val &= ~(MAX9286_HIBW | MAX9286_BWS);
+		cfg &= ~(MAX9286_HIBW | MAX9286_BWS);
 
 		if (priv->bus_width == 27)
-			val |= MAX9286_HIBW;
+			cfg |= MAX9286_HIBW;
 		else if (priv->bus_width == 32)
-			val |= MAX9286_BWS;
+			cfg |= MAX9286_BWS;
 
-		max9286_write(priv, 0x1c, val);
+		max9286_write(priv, 0x1c, cfg);
 	}
 
 	/*
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/12] dt-bindings: media: i2c: max9286: Add property to select I2C speed

[Rob Herring]

On Thu, 15 Dec 2022 01:38:15 +0200, Laurent Pinchart wrote:
> The I2C speed on the remote side (the I2C master bus of the connected
> serializers) is configurable, and doesn't need to match the speed of the
> local bus (the slave bus of the MAX9286). All remote buses must use the
> same speed, and the MAX9286 needs to be programmed accordingly. Add a
> new DT property to select the speed to make it configurable.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Changes since v2:
> 
> - Rename property to maxim,i2c-remote-bus-hz
> - Specify the property type
> ---
>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml      | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml: properties:maxim,i2c-remote-bus-hz: '$ref' should not be valid under {'const': '$ref'}
	hint: Standard unit suffix properties don't need a type $ref
	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221214233825.13050-3-laurent.pinchart+renesas@ideasonboard.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 02/12] dt-bindings: media: i2c: max9286: Add property to select I2C speed

[Laurent Pinchart]

Hi Rob,

On Thu, Dec 15, 2022 at 07:24:38AM -0600, Rob Herring wrote:
> On Thu, 15 Dec 2022 01:38:15 +0200, Laurent Pinchart wrote:
> > The I2C speed on the remote side (the I2C master bus of the connected
> > serializers) is configurable, and doesn't need to match the speed of the
> > local bus (the slave bus of the MAX9286). All remote buses must use the
> > same speed, and the MAX9286 needs to be programmed accordingly. Add a
> > new DT property to select the speed to make it configurable.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> > Changes since v2:
> > 
> > - Rename property to maxim,i2c-remote-bus-hz
> > - Specify the property type
> > ---
> >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml      | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml: properties:maxim,i2c-remote-bus-hz: '$ref' should not be valid under {'const': '$ref'}
> 	hint: Standard unit suffix properties don't need a type $ref
> 	from schema $id: http://devicetree.org/meta-schemas/core.yaml#

I wonder how I missed that, as I've run dt_binding_check before
submitting. I'll fix it.

I'm a bit surprised though, all unit-suffixed properties use 32-bit
integers in the DT schema, while I can imagine that some may need a
64-bit integer. What's the recommendation in that case ?

> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221214233825.13050-3-laurent.pinchart+renesas@ideasonboard.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 02/12] dt-bindings: media: i2c: max9286: Add property to select I2C speed

[Rob Herring]

On Thu, Dec 15, 2022 at 03:43:28PM +0200, Laurent Pinchart wrote:
> Hi Rob,
> 
> On Thu, Dec 15, 2022 at 07:24:38AM -0600, Rob Herring wrote:
> > On Thu, 15 Dec 2022 01:38:15 +0200, Laurent Pinchart wrote:
> > > The I2C speed on the remote side (the I2C master bus of the connected
> > > serializers) is configurable, and doesn't need to match the speed of the
> > > local bus (the slave bus of the MAX9286). All remote buses must use the
> > > same speed, and the MAX9286 needs to be programmed accordingly. Add a
> > > new DT property to select the speed to make it configurable.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > ---
> > > Changes since v2:
> > > 
> > > - Rename property to maxim,i2c-remote-bus-hz
> > > - Specify the property type
> > > ---
> > >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml      | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > 
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > 
> > yamllint warnings/errors:
> > 
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml: properties:maxim,i2c-remote-bus-hz: '$ref' should not be valid under {'const': '$ref'}
> > 	hint: Standard unit suffix properties don't need a type $ref
> > 	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> 
> I wonder how I missed that, as I've run dt_binding_check before
> submitting. I'll fix it.
> 
> I'm a bit surprised though, all unit-suffixed properties use 32-bit
> integers in the DT schema, while I can imagine that some may need a
> 64-bit integer. What's the recommendation in that case ?

Use -mhz.

Anything outside the norm we have to special case in 
property-units.yaml.

Rob

Re: [PATCH v3 02/12] dt-bindings: media: i2c: max9286: Add property to select I2C speed

[Laurent Pinchart]

Hi Rob,

On Thu, Dec 15, 2022 at 10:22:16AM -0600, Rob Herring wrote:
> On Thu, Dec 15, 2022 at 03:43:28PM +0200, Laurent Pinchart wrote:
> > On Thu, Dec 15, 2022 at 07:24:38AM -0600, Rob Herring wrote:
> > > On Thu, 15 Dec 2022 01:38:15 +0200, Laurent Pinchart wrote:
> > > > The I2C speed on the remote side (the I2C master bus of the connected
> > > > serializers) is configurable, and doesn't need to match the speed of the
> > > > local bus (the slave bus of the MAX9286). All remote buses must use the
> > > > same speed, and the MAX9286 needs to be programmed accordingly. Add a
> > > > new DT property to select the speed to make it configurable.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > > Changes since v2:
> > > > 
> > > > - Rename property to maxim,i2c-remote-bus-hz
> > > > - Specify the property type
> > > > ---
> > > >  .../devicetree/bindings/media/i2c/maxim,max9286.yaml      | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > 
> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> > > 
> > > yamllint warnings/errors:
> > > 
> > > dtschema/dtc warnings/errors:
> > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml: properties:maxim,i2c-remote-bus-hz: '$ref' should not be valid under {'const': '$ref'}
> > > 	hint: Standard unit suffix properties don't need a type $ref
> > > 	from schema $id: http://devicetree.org/meta-schemas/core.yaml#
> > 
> > I wonder how I missed that, as I've run dt_binding_check before
> > submitting. I'll fix it.
> > 
> > I'm a bit surprised though, all unit-suffixed properties use 32-bit
> > integers in the DT schema, while I can imagine that some may need a
> > 64-bit integer. What's the recommendation in that case ?
> 
> Use -mhz.

I expected that answer :-) It's not an issue with this specific
property, so I'm fine. If I ever run into the need of a 64-bit Hz value
to have both range and precision, we'll talk about it then :-) I don't
expect that to be very common.

> Anything outside the norm we have to special case in 
> property-units.yaml.

-- 
Regards,

Laurent Pinchart

[PATCH v3.1 02/12] dt-bindings: media: i2c: max9286: Add property to select I2C speed

[Laurent Pinchart]

The I2C speed on the remote side (the I2C master bus of the connected
serializers) is configurable, and doesn't need to match the speed of the
local bus (the slave bus of the MAX9286). All remote buses must use the
same speed, and the MAX9286 needs to be programmed accordingly. Add a
new DT property to select the speed to make it configurable.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes since v3:

- Drop the property type
- Add maxim,i2c-remote-bus-hz property to example

Changes since v2:

- Rename property to maxim,i2c-remote-bus-hz
- Specify the property type
---
 .../devicetree/bindings/media/i2c/maxim,max9286.yaml      | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 4f28690eabcd..75c2d8b8c809 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -50,6 +50,13 @@ properties:
   '#gpio-cells':
     const: 2
 
+  maxim,i2c-remote-bus-hz:
+    enum: [ 8470, 28300, 84700, 105000, 173000, 339000, 533000, 837000 ]
+    default: 105000
+    description: |
+      The I2C clock frequency for the remote I2C buses. The value must match
+      the configuration of the remote serializers.
+
   maxim,reverse-channel-microvolt:
     minimum: 30000
     maximum: 200000
@@ -234,6 +241,7 @@ examples:
             gpio-controller;
             #gpio-cells = <2>;
 
+            maxim,i2c-remote-bus-hz = <339000>;
             maxim,reverse-channel-microvolt = <170000>;
 
             ports {
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 04/12] media: i2c: max9286: Add support for port regulators

[Jacopo Mondi]

Hi Laurent

On Thu, Dec 15, 2022 at 01:38:17AM +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: Thomas Nizan <tnizan@witekio.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
>
> - Adapt to the poc-gpio support
>
> Changes since v1:
>
> - Use to_index()
> - Use dev_err_probe()
> - Fix error path in probe()
> - Use devm_regulator_get_optional() instead of
>   devm_regulator_get_exclusive()
> ---
>  drivers/media/i2c/max9286.c | 138 ++++++++++++++++++++++++++++++------
>  1 file changed, 115 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 9c083cf14231..d79077939847 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -139,6 +139,7 @@
>  struct max9286_source {
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> +	struct regulator *regulator;
>  };
>
>  struct max9286_asd {
> @@ -169,6 +170,7 @@ struct max9286_priv {
>  	u32 init_rev_chan_mv;
>  	u32 rev_chan_mv;
>
> +	bool use_gpio_poc;
>  	u32 gpio_poc[2];
>
>  	struct v4l2_ctrl_handler ctrls;
> @@ -1088,9 +1090,6 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
>  	struct device *dev = &priv->client->dev;
>  	int ret;
>
> -	/* GPIO values default to high */
> -	priv->gpio_state = BIT(0) | BIT(1);
> -
>  	/*
>  	 * Parse the "gpio-poc" vendor property. If the property is not
>  	 * specified the camera power is controlled by a regulator.
> @@ -1102,18 +1101,7 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
>  		 * If gpio lines are not used for the camera power, register
>  		 * a gpio controller for consumers.
>  		 */
> -		ret = max9286_register_gpio(priv);
> -		if (ret)
> -			return ret;
> -
> -		priv->regulator = devm_regulator_get(dev, "poc");
> -		if (IS_ERR(priv->regulator)) {
> -			return dev_err_probe(dev, PTR_ERR(priv->regulator),
> -					     "Unable to get PoC regulator (%ld)\n",
> -					     PTR_ERR(priv->regulator));
> -		}
> -
> -		return 0;
> +		return max9286_register_gpio(priv);
>  	}
>
>  	/* If the property is specified make sure it is well formed. */
> @@ -1124,21 +1112,75 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
>  		return -EINVAL;
>  	}
>
> +	priv->use_gpio_poc = true;
>  	return 0;
>  }
>
> +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);
> +
> +	if (priv->use_gpio_poc)
> +		return max9286_gpio_set(priv, priv->gpio_poc[0],
> +					!priv->gpio_poc[1]);

#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

So negating it is correct

> +
> +	/* Otherwise use the per-port regulators. */
> +	for_each_source(priv, source) {
> +		ret = regulator_enable(source->regulator);
> +		if (ret < 0)
> +			goto error;
> +
> +		enabled |= BIT(to_index(priv, source));
> +	}
> +
> +	return 0;
> +
> +error:
> +	for_each_source(priv, source) {
> +		if (enabled & BIT(to_index(priv, source)))
> +			regulator_disable(source->regulator);
> +	}
> +
> +	return ret;
> +}
> +
> +static int max9286_poc_power_off(struct max9286_priv *priv)
> +{
> +	struct max9286_source *source;
> +	int ret = 0;
> +
> +	if (priv->regulator)
> +		return regulator_disable(priv->regulator);
> +
> +	if (priv->use_gpio_poc)
> +		return max9286_gpio_set(priv, priv->gpio_poc[0],
> +					priv->gpio_poc[1]);
> +
> +	for_each_source(priv, source) {
> +		int err;
> +
> +		err = regulator_disable(source->regulator);
> +		if (!ret)

Should this be err ?

And I wonder if
                ret |= regulator_disable(..)

isn't enough

> +			ret = err;
> +	}
> +
> +	return ret;
> +}
> +
>  static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
>  {
>  	int ret;
>
> -	/* If the regulator is not available, use gpio to control power. */
> -	if (!priv->regulator)
> -		ret = max9286_gpio_set(priv, priv->gpio_poc[0],
> -				       enable ^ priv->gpio_poc[1]);
> -	else if (enable)
> -		ret = regulator_enable(priv->regulator);
> +	if (enable)
> +		ret = max9286_poc_power_on(priv);
>  	else
> -		ret = regulator_disable(priv->regulator);
> +		ret = max9286_poc_power_off(priv);
>
>  	if (ret < 0)
>  		dev_err(&priv->client->dev, "Unable to turn power %s\n",
> @@ -1317,6 +1359,47 @@ 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;
> +	int ret;
> +
> +	/* Start by getting the global regulator. */
> +	priv->regulator = devm_regulator_get_optional(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);

Could this be dev_err_probe ? Or you want to avoid the dev_dbg message ?

> +	}
> +
> +	/* 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) {
> +		unsigned int index = to_index(priv, source);
> +		char name[10];

Will overflow with index > 9 ? But I don't think we can have more than
9 ports ?

> +
> +		snprintf(name, sizeof(name), "port%u-poc", index);
> +		source->regulator = devm_regulator_get(dev, name);
> +		if (IS_ERR(source->regulator)) {
> +			ret = PTR_ERR(source->regulator);
> +			dev_err_probe(dev, ret,
> +				      "Unable to get port %u PoC regulator\n",
> +				      index);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int max9286_probe(struct i2c_client *client)
>  {
>  	struct max9286_priv *priv;
> @@ -1330,6 +1413,9 @@ static int max9286_probe(struct i2c_client *client)
>
>  	priv->client = client;
>
> +	/* GPIO values default to high */
> +	priv->gpio_state = BIT(0) | BIT(1);
> +
>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>  						   GPIOD_OUT_HIGH);
>  	if (IS_ERR(priv->gpiod_pwdn))
> @@ -1362,7 +1448,13 @@ static int max9286_probe(struct i2c_client *client)
>
>  	ret = max9286_parse_dt(priv);
>  	if (ret)
> -		goto err_powerdown;
> +		goto err_cleanup_dt;
> +
> +	if (!priv->use_gpio_poc) {
> +		ret = max9286_get_poc_supplies(priv);
> +		if (ret)
> +			goto err_cleanup_dt;
> +	}
>
>  	ret = max9286_init(priv);
>  	if (ret < 0)

Do you need to powerdown here ?

> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v3 07/12] media: i2c: max9286: Support 12-bit raw bayer formats

[Jacopo Mondi]

Hi Laurent

On Thu, Dec 15, 2022 at 01:38:20AM +0200, Laurent Pinchart wrote:
> Add support for 12-bit raw bayer formats to the driver, configuring the
> GMSL format accordingly.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
> Changes since v1:
>
> - Fix incorrect array index when media bus code isn't found
> ---
>  drivers/media/i2c/max9286.c | 128 ++++++++++++++++++++++++++----------
>  1 file changed, 95 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 028fa3547282..3f1228c5053b 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -136,6 +136,11 @@
>  #define MAX9286_N_PADS			5
>  #define MAX9286_SRC_PAD			4
>
> +struct max9286_format_info {
> +	u32 code;
> +	u8 datatype;
> +};
> +
>  struct max9286_source {
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> @@ -218,6 +223,34 @@ static inline struct max9286_priv *sd_to_max9286(struct v4l2_subdev *sd)
>  	return container_of(sd, struct max9286_priv, sd);
>  }
>
> +static const struct max9286_format_info max9286_formats[] = {
> +	{
> +		.code = MEDIA_BUS_FMT_UYVY8_1X16,
> +		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +	}, {
> +		.code = MEDIA_BUS_FMT_VYUY8_1X16,
> +		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +	}, {
> +		.code = MEDIA_BUS_FMT_YUYV8_1X16,
> +		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +	}, {
> +		.code = MEDIA_BUS_FMT_YVYU8_1X16,
> +		.datatype = MAX9286_DATATYPE_YUV422_8BIT,
> +	}, {
> +		.code = MEDIA_BUS_FMT_SBGGR12_1X12,
> +		.datatype = MAX9286_DATATYPE_RAW12,
> +	}, {
> +		.code = MEDIA_BUS_FMT_SGBRG12_1X12,
> +		.datatype = MAX9286_DATATYPE_RAW12,
> +	}, {
> +		.code = MEDIA_BUS_FMT_SGRBG12_1X12,
> +		.datatype = MAX9286_DATATYPE_RAW12,
> +	}, {
> +		.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> +		.datatype = MAX9286_DATATYPE_RAW12,
> +	},
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * I2C IO
>   */
> @@ -479,6 +512,38 @@ static int max9286_check_config_link(struct max9286_priv *priv,
>  	return 0;
>  }
>
> +static void max9286_set_video_format(struct max9286_priv *priv,
> +				     const struct v4l2_mbus_framefmt *format)
> +{
> +	const struct max9286_format_info *info = NULL;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> +		if (max9286_formats[i].code == format->code) {
> +			info = &max9286_formats[i];
> +			break;
> +		}
> +	}
> +
> +	if (WARN_ON(!info))
> +		return;
> +
> +	/*
> +	 * Video format setup:
> +	 * Disable CSI output, VC is set according to Link number.
> +	 */
> +	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
> +
> +	/* Enable CSI-2 Lane D0-D3 only, DBL mode. */
> +	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
> +		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> +		      info->datatype);
> +
> +	/* Enable HS/VS encoding, use D14/15 for HS/VS, invert VS. */
> +	max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS |
> +		      MAX9286_HVSRC_D14);
> +}
> +
>  static void max9286_set_fsync_period(struct max9286_priv *priv)
>  {
>  	u32 fsync;
> @@ -697,6 +762,15 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  	int ret;
>
>  	if (enable) {
> +		const struct v4l2_mbus_framefmt *format;
> +
> +		/*
> +		 * Get the format from the first used sink pad, as all sink
> +		 * formats must be identical.
> +		 */
> +		format = &priv->fmt[__ffs(priv->bound_sources)];
> +
> +		max9286_set_video_format(priv, format);
>  		max9286_set_fsync_period(priv);
>
>  		/*
> @@ -817,22 +891,20 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
>  	struct v4l2_mbus_framefmt *cfg_fmt;
> +	unsigned int i;
>
>  	if (format->pad == MAX9286_SRC_PAD)
>  		return -EINVAL;
>
> -	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
> -	switch (format->format.code) {
> -	case MEDIA_BUS_FMT_UYVY8_1X16:
> -	case MEDIA_BUS_FMT_VYUY8_1X16:
> -	case MEDIA_BUS_FMT_YUYV8_1X16:
> -	case MEDIA_BUS_FMT_YVYU8_1X16:
> -		break;
> -	default:
> -		format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
> -		break;
> +	/* Validate the format. */
> +	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> +		if (max9286_formats[i].code == format->format.code)
> +			break;
>  	}
>
> +	if (i == ARRAY_SIZE(max9286_formats))
> +		format->format.code = max9286_formats[0].code;
> +
>  	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
>  					 format->which);
>  	if (!cfg_fmt)
> @@ -890,16 +962,20 @@ static const struct v4l2_subdev_ops max9286_subdev_ops = {
>  	.pad		= &max9286_pad_ops,
>  };
>
> +static const struct v4l2_mbus_framefmt max9286_default_format = {
> +	.width		= 1280,
> +	.height		= 800,
> +	.code		= MEDIA_BUS_FMT_UYVY8_1X16,
> +	.colorspace	= V4L2_COLORSPACE_SRGB,
> +	.field		= V4L2_FIELD_NONE,
> +	.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> +	.quantization	= V4L2_QUANTIZATION_DEFAULT,
> +	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> +};
> +
>  static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
>  {
> -	fmt->width		= 1280;
> -	fmt->height		= 800;
> -	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
> -	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
> -	fmt->field		= V4L2_FIELD_NONE;
> -	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
> -	fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
> -	fmt->xfer_func		= V4L2_XFER_FUNC_DEFAULT;
> +	*fmt = max9286_default_format;
>  }
>
>  static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> @@ -1063,23 +1139,9 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_write(priv, 0x0b, link_order[priv->route_mask]);
>  	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
>
> -	/*
> -	 * Video format setup:
> -	 * Disable CSI output, VC is set according to Link number.
> -	 */
> -	max9286_write(priv, 0x15, MAX9286_VCTYPE | MAX9286_0X15_RESV);
> -
> -	/* Enable CSI-2 Lane D0-D3 only, DBL mode, YUV422 8-bit. */
> -	max9286_write(priv, 0x12, MAX9286_CSIDBL | MAX9286_DBL |
> -		      MAX9286_CSILANECNT(priv->csi2_data_lanes) |
> -		      MAX9286_DATATYPE_YUV422_8BIT);
> -
> +	max9286_set_video_format(priv, &max9286_default_format);
>  	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 |
> -		      MAX9286_HVSRC_D14);
> -
>  	/*
>  	 * The overlap window seems to provide additional validation by tracking
>  	 * the delay between vsync and frame sync, generating an error if the
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v3 09/12] media: i2c: max9286: Configure remote I2C speed from device tree

[Jacopo Mondi]

Hi Laurent

On Thu, Dec 15, 2022 at 01:38:22AM +0200, Laurent Pinchart wrote:
> Read the maxim,i2c-clock-frequency DT property that specifies the speed
> of the remote I2C bus, and configure the MAX9286 accordingly. The remote
> serializers must all have a matching configuration.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> Changes since v2:
>
> - Rename DT property to "maxim,i2c-remote-bus-hz"
> ---
>  drivers/media/i2c/max9286.c | 56 +++++++++++++++++++++++++++++++------
>  1 file changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index e78456c8d24c..fffb0d2da416 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -144,6 +144,11 @@ struct max9286_format_info {
>  	u8 datatype;
>  };
>
> +struct max9286_i2c_speed {
> +	u32 rate;
> +	u8 mstbt;
> +};
> +
>  struct max9286_source {
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> @@ -177,6 +182,7 @@ struct max9286_priv {
>  	/* The initial reverse control channel amplitude. */
>  	u32 init_rev_chan_mv;
>  	u32 rev_chan_mv;
> +	u8 i2c_mstbt;
>
>  	bool use_gpio_poc;
>  	u32 gpio_poc[2];
> @@ -254,6 +260,17 @@ static const struct max9286_format_info max9286_formats[] = {
>  	},
>  };
>
> +static const struct max9286_i2c_speed max9286_i2c_speeds[] = {
> +	{ .rate =   8470, .mstbt = MAX9286_I2CMSTBT_8KBPS },
> +	{ .rate =  28300, .mstbt = MAX9286_I2CMSTBT_28KBPS },
> +	{ .rate =  84700, .mstbt = MAX9286_I2CMSTBT_84KBPS },
> +	{ .rate = 105000, .mstbt = MAX9286_I2CMSTBT_105KBPS },
> +	{ .rate = 173000, .mstbt = MAX9286_I2CMSTBT_173KBPS },
> +	{ .rate = 339000, .mstbt = MAX9286_I2CMSTBT_339KBPS },
> +	{ .rate = 533000, .mstbt = MAX9286_I2CMSTBT_533KBPS },
> +	{ .rate = 837000, .mstbt = MAX9286_I2CMSTBT_837KBPS },
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * I2C IO
>   */
> @@ -374,7 +391,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv)
>  static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
>  {
>  	u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US |
> -		    MAX9286_I2CMSTBT_105KBPS;
> +		    priv->i2c_mstbt;
>
>  	if (localack)
>  		config |= MAX9286_I2CLOCACK;
> @@ -1387,6 +1404,8 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	struct device_node *node = NULL;
>  	unsigned int i2c_mux_mask = 0;
>  	u32 reverse_channel_microvolt;
> +	u32 i2c_clk_freq = 105000;
> +	unsigned int i;
>
>  	/* Balance the of_node_put() performed by of_find_node_by_name(). */
>  	of_node_get(dev->of_node);
> @@ -1477,6 +1496,23 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	}
>  	of_node_put(node);
>
> +	of_property_read_u32(dev->of_node, "maxim,i2c-remote-bus-hz",
> +			     &i2c_clk_freq);

i2c_clk_freq is only overwritten if the property is present, so I
guess this is right.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> +	for (i = 0; i < ARRAY_SIZE(max9286_i2c_speeds); ++i) {
> +		const struct max9286_i2c_speed *speed = &max9286_i2c_speeds[i];
> +
> +		if (speed->rate == i2c_clk_freq) {
> +			priv->i2c_mstbt = speed->mstbt;
> +			break;
> +		}
> +	}
> +
> +	if (i == ARRAY_SIZE(max9286_i2c_speeds)) {
> +		dev_err(dev, "Invalid %s value %u\n", "maxim,i2c-remote-bus-hz",
> +			i2c_clk_freq);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * Parse the initial value of the reverse channel amplitude from
>  	 * the firmware interface and convert it to millivolts.
> @@ -1553,10 +1589,16 @@ static int max9286_probe(struct i2c_client *client)
>  	/* GPIO values default to high */
>  	priv->gpio_state = BIT(0) | BIT(1);
>
> +	ret = max9286_parse_dt(priv);
> +	if (ret)
> +		goto err_cleanup_dt;
> +
>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>  						   GPIOD_OUT_HIGH);
> -	if (IS_ERR(priv->gpiod_pwdn))
> -		return PTR_ERR(priv->gpiod_pwdn);
> +	if (IS_ERR(priv->gpiod_pwdn)) {
> +		ret = PTR_ERR(priv->gpiod_pwdn);
> +		goto err_cleanup_dt;
> +	}
>
>  	gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn");
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 1);
> @@ -1583,10 +1625,6 @@ static int max9286_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_powerdown;
>
> -	ret = max9286_parse_dt(priv);
> -	if (ret)
> -		goto err_cleanup_dt;
> -
>  	if (!priv->use_gpio_poc) {
>  		ret = max9286_get_poc_supplies(priv);
>  		if (ret)
> @@ -1599,10 +1637,10 @@ static int max9286_probe(struct i2c_client *client)
>
>  	return 0;
>
> -err_cleanup_dt:
> -	max9286_cleanup_dt(priv);
>  err_powerdown:
>  	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);
> +err_cleanup_dt:
> +	max9286_cleanup_dt(priv);
>
>  	return ret;
>  }
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v3.1 02/12] dt-bindings: media: i2c: max9286: Add property to select I2C speed

[Jacopo Mondi]

Hi Laurent

On Fri, Dec 16, 2022 at 02:45:00AM +0200, Laurent Pinchart wrote:
> The I2C speed on the remote side (the I2C master bus of the connected
> serializers) is configurable, and doesn't need to match the speed of the
> local bus (the slave bus of the MAX9286). All remote buses must use the
> same speed, and the MAX9286 needs to be programmed accordingly. Add a
> new DT property to select the speed to make it configurable.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
> Changes since v3:
>
> - Drop the property type
> - Add maxim,i2c-remote-bus-hz property to example
>
> Changes since v2:
>
> - Rename property to maxim,i2c-remote-bus-hz
> - Specify the property type
> ---
>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml      | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index 4f28690eabcd..75c2d8b8c809 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -50,6 +50,13 @@ properties:
>    '#gpio-cells':
>      const: 2
>
> +  maxim,i2c-remote-bus-hz:
> +    enum: [ 8470, 28300, 84700, 105000, 173000, 339000, 533000, 837000 ]
> +    default: 105000
> +    description: |
> +      The I2C clock frequency for the remote I2C buses. The value must match
> +      the configuration of the remote serializers.
> +
>    maxim,reverse-channel-microvolt:
>      minimum: 30000
>      maximum: 200000
> @@ -234,6 +241,7 @@ examples:
>              gpio-controller;
>              #gpio-cells = <2>;
>
> +            maxim,i2c-remote-bus-hz = <339000>;
>              maxim,reverse-channel-microvolt = <170000>;
>
>              ports {
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v3 04/12] media: i2c: max9286: Add support for port regulators

[Laurent Pinchart]

Hi Jacopo,

On Fri, Dec 16, 2022 at 10:52:51AM +0100, Jacopo Mondi wrote:
> On Thu, Dec 15, 2022 at 01:38:17AM +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: Thomas Nizan <tnizan@witekio.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Adapt to the poc-gpio support
> >
> > Changes since v1:
> >
> > - Use to_index()
> > - Use dev_err_probe()
> > - Fix error path in probe()
> > - Use devm_regulator_get_optional() instead of
> >   devm_regulator_get_exclusive()
> > ---
> >  drivers/media/i2c/max9286.c | 138 ++++++++++++++++++++++++++++++------
> >  1 file changed, 115 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 9c083cf14231..d79077939847 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -139,6 +139,7 @@
> >  struct max9286_source {
> >  	struct v4l2_subdev *sd;
> >  	struct fwnode_handle *fwnode;
> > +	struct regulator *regulator;
> >  };
> >
> >  struct max9286_asd {
> > @@ -169,6 +170,7 @@ struct max9286_priv {
> >  	u32 init_rev_chan_mv;
> >  	u32 rev_chan_mv;
> >
> > +	bool use_gpio_poc;
> >  	u32 gpio_poc[2];
> >
> >  	struct v4l2_ctrl_handler ctrls;
> > @@ -1088,9 +1090,6 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
> >  	struct device *dev = &priv->client->dev;
> >  	int ret;
> >
> > -	/* GPIO values default to high */
> > -	priv->gpio_state = BIT(0) | BIT(1);
> > -
> >  	/*
> >  	 * Parse the "gpio-poc" vendor property. If the property is not
> >  	 * specified the camera power is controlled by a regulator.
> > @@ -1102,18 +1101,7 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
> >  		 * If gpio lines are not used for the camera power, register
> >  		 * a gpio controller for consumers.
> >  		 */
> > -		ret = max9286_register_gpio(priv);
> > -		if (ret)
> > -			return ret;
> > -
> > -		priv->regulator = devm_regulator_get(dev, "poc");
> > -		if (IS_ERR(priv->regulator)) {
> > -			return dev_err_probe(dev, PTR_ERR(priv->regulator),
> > -					     "Unable to get PoC regulator (%ld)\n",
> > -					     PTR_ERR(priv->regulator));
> > -		}
> > -
> > -		return 0;
> > +		return max9286_register_gpio(priv);
> >  	}
> >
> >  	/* If the property is specified make sure it is well formed. */
> > @@ -1124,21 +1112,75 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
> >  		return -EINVAL;
> >  	}
> >
> > +	priv->use_gpio_poc = true;
> >  	return 0;
> >  }
> >
> > +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);
> > +
> > +	if (priv->use_gpio_poc)
> > +		return max9286_gpio_set(priv, priv->gpio_poc[0],
> > +					!priv->gpio_poc[1]);
> 
> #define GPIO_ACTIVE_HIGH 0
> #define GPIO_ACTIVE_LOW 1
> 
> So negating it is correct
> 
> > +
> > +	/* Otherwise use the per-port regulators. */
> > +	for_each_source(priv, source) {
> > +		ret = regulator_enable(source->regulator);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		enabled |= BIT(to_index(priv, source));
> > +	}
> > +
> > +	return 0;
> > +
> > +error:
> > +	for_each_source(priv, source) {
> > +		if (enabled & BIT(to_index(priv, source)))
> > +			regulator_disable(source->regulator);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int max9286_poc_power_off(struct max9286_priv *priv)
> > +{
> > +	struct max9286_source *source;
> > +	int ret = 0;
> > +
> > +	if (priv->regulator)
> > +		return regulator_disable(priv->regulator);
> > +
> > +	if (priv->use_gpio_poc)
> > +		return max9286_gpio_set(priv, priv->gpio_poc[0],
> > +					priv->gpio_poc[1]);
> > +
> > +	for_each_source(priv, source) {
> > +		int err;
> > +
> > +		err = regulator_disable(source->regulator);
> > +		if (!ret)
> 
> Should this be err ?

No, the intent is to capture the first error. It could be written

		if (err && !ret)

but if the !ret condition is true and err == 0, executing ret = err will
be a no-op, so just testing !ret is fine.

> And I wonder if
>                 ret |= regulator_disable(..)
> 
> isn't enough

If multiple regulator_disable() calls fail with different errors, ret
will be garbage.

> > +			ret = err;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
> >  {
> >  	int ret;
> >
> > -	/* If the regulator is not available, use gpio to control power. */
> > -	if (!priv->regulator)
> > -		ret = max9286_gpio_set(priv, priv->gpio_poc[0],
> > -				       enable ^ priv->gpio_poc[1]);
> > -	else if (enable)
> > -		ret = regulator_enable(priv->regulator);
> > +	if (enable)
> > +		ret = max9286_poc_power_on(priv);
> >  	else
> > -		ret = regulator_disable(priv->regulator);
> > +		ret = max9286_poc_power_off(priv);
> >
> >  	if (ret < 0)
> >  		dev_err(&priv->client->dev, "Unable to turn power %s\n",
> > @@ -1317,6 +1359,47 @@ 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;
> > +	int ret;
> > +
> > +	/* Start by getting the global regulator. */
> > +	priv->regulator = devm_regulator_get_optional(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);
> 
> Could this be dev_err_probe ? Or you want to avoid the dev_dbg message ?

Good point, I'll fix that.

> > +	}
> > +
> > +	/* 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) {
> > +		unsigned int index = to_index(priv, source);
> > +		char name[10];
> 
> Will overflow with index > 9 ? But I don't think we can have more than
> 9 ports ?

We're limited to 4 sources (MAX9286_NUM_GMSL is 4), so it should be
fine.

> > +
> > +		snprintf(name, sizeof(name), "port%u-poc", index);
> > +		source->regulator = devm_regulator_get(dev, name);
> > +		if (IS_ERR(source->regulator)) {
> > +			ret = PTR_ERR(source->regulator);
> > +			dev_err_probe(dev, ret,
> > +				      "Unable to get port %u PoC regulator\n",
> > +				      index);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int max9286_probe(struct i2c_client *client)
> >  {
> >  	struct max9286_priv *priv;
> > @@ -1330,6 +1413,9 @@ static int max9286_probe(struct i2c_client *client)
> >
> >  	priv->client = client;
> >
> > +	/* GPIO values default to high */
> > +	priv->gpio_state = BIT(0) | BIT(1);
> > +
> >  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
> >  						   GPIOD_OUT_HIGH);
> >  	if (IS_ERR(priv->gpiod_pwdn))
> > @@ -1362,7 +1448,13 @@ static int max9286_probe(struct i2c_client *client)
> >
> >  	ret = max9286_parse_dt(priv);
> >  	if (ret)
> > -		goto err_powerdown;
> > +		goto err_cleanup_dt;
> > +
> > +	if (!priv->use_gpio_poc) {
> > +		ret = max9286_get_poc_supplies(priv);
> > +		if (ret)
> > +			goto err_cleanup_dt;
> > +	}
> >
> >  	ret = max9286_init(priv);
> >  	if (ret < 0)
> 
> Do you need to powerdown here ?

I'm not sure to see what's wrong. The error path has

err_cleanup_dt:
	max9286_cleanup_dt(priv);
err_powerdown:
	gpiod_set_value_cansleep(priv->gpiod_pwdn, 0);

	return ret;

-- 
Regards,

Laurent Pinchart

[PATCH v3.1 04/12] 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: Thomas Nizan <tnizan@witekio.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v3:

- Use dev_err_probe() everywhere

Changes since v2:

- Adapt to the poc-gpio support

Changes since v1:

- Use to_index()
- Use dev_err_probe()
- Fix error path in probe()
- Use devm_regulator_get_optional() instead of
  devm_regulator_get_exclusive()
---
 drivers/media/i2c/max9286.c | 135 ++++++++++++++++++++++++++++++------
 1 file changed, 112 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 9c083cf14231..a65457b1e7e0 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -139,6 +139,7 @@
 struct max9286_source {
 	struct v4l2_subdev *sd;
 	struct fwnode_handle *fwnode;
+	struct regulator *regulator;
 };
 
 struct max9286_asd {
@@ -169,6 +170,7 @@ struct max9286_priv {
 	u32 init_rev_chan_mv;
 	u32 rev_chan_mv;
 
+	bool use_gpio_poc;
 	u32 gpio_poc[2];
 
 	struct v4l2_ctrl_handler ctrls;
@@ -1088,9 +1090,6 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
 	struct device *dev = &priv->client->dev;
 	int ret;
 
-	/* GPIO values default to high */
-	priv->gpio_state = BIT(0) | BIT(1);
-
 	/*
 	 * Parse the "gpio-poc" vendor property. If the property is not
 	 * specified the camera power is controlled by a regulator.
@@ -1102,18 +1101,7 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
 		 * If gpio lines are not used for the camera power, register
 		 * a gpio controller for consumers.
 		 */
-		ret = max9286_register_gpio(priv);
-		if (ret)
-			return ret;
-
-		priv->regulator = devm_regulator_get(dev, "poc");
-		if (IS_ERR(priv->regulator)) {
-			return dev_err_probe(dev, PTR_ERR(priv->regulator),
-					     "Unable to get PoC regulator (%ld)\n",
-					     PTR_ERR(priv->regulator));
-		}
-
-		return 0;
+		return max9286_register_gpio(priv);
 	}
 
 	/* If the property is specified make sure it is well formed. */
@@ -1124,21 +1112,75 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
 		return -EINVAL;
 	}
 
+	priv->use_gpio_poc = true;
 	return 0;
 }
 
+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);
+
+	if (priv->use_gpio_poc)
+		return max9286_gpio_set(priv, priv->gpio_poc[0],
+					!priv->gpio_poc[1]);
+
+	/* Otherwise use the per-port regulators. */
+	for_each_source(priv, source) {
+		ret = regulator_enable(source->regulator);
+		if (ret < 0)
+			goto error;
+
+		enabled |= BIT(to_index(priv, source));
+	}
+
+	return 0;
+
+error:
+	for_each_source(priv, source) {
+		if (enabled & BIT(to_index(priv, source)))
+			regulator_disable(source->regulator);
+	}
+
+	return ret;
+}
+
+static int max9286_poc_power_off(struct max9286_priv *priv)
+{
+	struct max9286_source *source;
+	int ret = 0;
+
+	if (priv->regulator)
+		return regulator_disable(priv->regulator);
+
+	if (priv->use_gpio_poc)
+		return max9286_gpio_set(priv, priv->gpio_poc[0],
+					priv->gpio_poc[1]);
+
+	for_each_source(priv, source) {
+		int err;
+
+		err = regulator_disable(source->regulator);
+		if (!ret)
+			ret = err;
+	}
+
+	return ret;
+}
+
 static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
 {
 	int ret;
 
-	/* If the regulator is not available, use gpio to control power. */
-	if (!priv->regulator)
-		ret = max9286_gpio_set(priv, priv->gpio_poc[0],
-				       enable ^ priv->gpio_poc[1]);
-	else if (enable)
-		ret = regulator_enable(priv->regulator);
+	if (enable)
+		ret = max9286_poc_power_on(priv);
 	else
-		ret = regulator_disable(priv->regulator);
+		ret = max9286_poc_power_off(priv);
 
 	if (ret < 0)
 		dev_err(&priv->client->dev, "Unable to turn power %s\n",
@@ -1317,6 +1359,44 @@ 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;
+	int ret;
+
+	/* Start by getting the global regulator. */
+	priv->regulator = devm_regulator_get_optional(dev, "poc");
+	if (!IS_ERR(priv->regulator))
+		return 0;
+
+	if (PTR_ERR(priv->regulator) != -ENODEV)
+		return dev_err_probe(dev, PTR_ERR(priv->regulator),
+				     "Unable to get PoC regulator\n");
+
+	/* 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) {
+		unsigned int index = to_index(priv, source);
+		char name[10];
+
+		snprintf(name, sizeof(name), "port%u-poc", index);
+		source->regulator = devm_regulator_get(dev, name);
+		if (IS_ERR(source->regulator)) {
+			ret = PTR_ERR(source->regulator);
+			dev_err_probe(dev, ret,
+				      "Unable to get port %u PoC regulator\n",
+				      index);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int max9286_probe(struct i2c_client *client)
 {
 	struct max9286_priv *priv;
@@ -1330,6 +1410,9 @@ static int max9286_probe(struct i2c_client *client)
 
 	priv->client = client;
 
+	/* GPIO values default to high */
+	priv->gpio_state = BIT(0) | BIT(1);
+
 	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
 						   GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->gpiod_pwdn))
@@ -1362,7 +1445,13 @@ static int max9286_probe(struct i2c_client *client)
 
 	ret = max9286_parse_dt(priv);
 	if (ret)
-		goto err_powerdown;
+		goto err_cleanup_dt;
+
+	if (!priv->use_gpio_poc) {
+		ret = max9286_get_poc_supplies(priv);
+		if (ret)
+			goto err_cleanup_dt;
+	}
 
 	ret = max9286_init(priv);
 	if (ret < 0)
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3.1 04/12] media: i2c: max9286: Add support for port regulators

[Jacopo Mondi]

Hi Laurent

On Mon, Dec 19, 2022 at 04:18:20AM +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: Thomas Nizan <tnizan@witekio.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
> Changes since v3:
>
> - Use dev_err_probe() everywhere
>
> Changes since v2:
>
> - Adapt to the poc-gpio support
>
> Changes since v1:
>
> - Use to_index()
> - Use dev_err_probe()
> - Fix error path in probe()
> - Use devm_regulator_get_optional() instead of
>   devm_regulator_get_exclusive()
> ---
>  drivers/media/i2c/max9286.c | 135 ++++++++++++++++++++++++++++++------
>  1 file changed, 112 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 9c083cf14231..a65457b1e7e0 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -139,6 +139,7 @@
>  struct max9286_source {
>  	struct v4l2_subdev *sd;
>  	struct fwnode_handle *fwnode;
> +	struct regulator *regulator;
>  };
>
>  struct max9286_asd {
> @@ -169,6 +170,7 @@ struct max9286_priv {
>  	u32 init_rev_chan_mv;
>  	u32 rev_chan_mv;
>
> +	bool use_gpio_poc;
>  	u32 gpio_poc[2];
>
>  	struct v4l2_ctrl_handler ctrls;
> @@ -1088,9 +1090,6 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
>  	struct device *dev = &priv->client->dev;
>  	int ret;
>
> -	/* GPIO values default to high */
> -	priv->gpio_state = BIT(0) | BIT(1);
> -
>  	/*
>  	 * Parse the "gpio-poc" vendor property. If the property is not
>  	 * specified the camera power is controlled by a regulator.
> @@ -1102,18 +1101,7 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
>  		 * If gpio lines are not used for the camera power, register
>  		 * a gpio controller for consumers.
>  		 */
> -		ret = max9286_register_gpio(priv);
> -		if (ret)
> -			return ret;
> -
> -		priv->regulator = devm_regulator_get(dev, "poc");
> -		if (IS_ERR(priv->regulator)) {
> -			return dev_err_probe(dev, PTR_ERR(priv->regulator),
> -					     "Unable to get PoC regulator (%ld)\n",
> -					     PTR_ERR(priv->regulator));
> -		}
> -
> -		return 0;
> +		return max9286_register_gpio(priv);
>  	}
>
>  	/* If the property is specified make sure it is well formed. */
> @@ -1124,21 +1112,75 @@ static int max9286_parse_gpios(struct max9286_priv *priv)
>  		return -EINVAL;
>  	}
>
> +	priv->use_gpio_poc = true;
>  	return 0;
>  }
>
> +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);
> +
> +	if (priv->use_gpio_poc)
> +		return max9286_gpio_set(priv, priv->gpio_poc[0],
> +					!priv->gpio_poc[1]);
> +
> +	/* Otherwise use the per-port regulators. */
> +	for_each_source(priv, source) {
> +		ret = regulator_enable(source->regulator);
> +		if (ret < 0)
> +			goto error;
> +
> +		enabled |= BIT(to_index(priv, source));
> +	}
> +
> +	return 0;
> +
> +error:
> +	for_each_source(priv, source) {
> +		if (enabled & BIT(to_index(priv, source)))
> +			regulator_disable(source->regulator);
> +	}
> +
> +	return ret;
> +}
> +
> +static int max9286_poc_power_off(struct max9286_priv *priv)
> +{
> +	struct max9286_source *source;
> +	int ret = 0;
> +
> +	if (priv->regulator)
> +		return regulator_disable(priv->regulator);
> +
> +	if (priv->use_gpio_poc)
> +		return max9286_gpio_set(priv, priv->gpio_poc[0],
> +					priv->gpio_poc[1]);
> +
> +	for_each_source(priv, source) {
> +		int err;
> +
> +		err = regulator_disable(source->regulator);
> +		if (!ret)
> +			ret = err;
> +	}
> +
> +	return ret;
> +}
> +
>  static int max9286_poc_enable(struct max9286_priv *priv, bool enable)
>  {
>  	int ret;
>
> -	/* If the regulator is not available, use gpio to control power. */
> -	if (!priv->regulator)
> -		ret = max9286_gpio_set(priv, priv->gpio_poc[0],
> -				       enable ^ priv->gpio_poc[1]);
> -	else if (enable)
> -		ret = regulator_enable(priv->regulator);
> +	if (enable)
> +		ret = max9286_poc_power_on(priv);
>  	else
> -		ret = regulator_disable(priv->regulator);
> +		ret = max9286_poc_power_off(priv);
>
>  	if (ret < 0)
>  		dev_err(&priv->client->dev, "Unable to turn power %s\n",
> @@ -1317,6 +1359,44 @@ 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;
> +	int ret;
> +
> +	/* Start by getting the global regulator. */
> +	priv->regulator = devm_regulator_get_optional(dev, "poc");
> +	if (!IS_ERR(priv->regulator))
> +		return 0;
> +
> +	if (PTR_ERR(priv->regulator) != -ENODEV)
> +		return dev_err_probe(dev, PTR_ERR(priv->regulator),
> +				     "Unable to get PoC regulator\n");
> +
> +	/* 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) {
> +		unsigned int index = to_index(priv, source);
> +		char name[10];
> +
> +		snprintf(name, sizeof(name), "port%u-poc", index);
> +		source->regulator = devm_regulator_get(dev, name);
> +		if (IS_ERR(source->regulator)) {
> +			ret = PTR_ERR(source->regulator);
> +			dev_err_probe(dev, ret,
> +				      "Unable to get port %u PoC regulator\n",
> +				      index);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int max9286_probe(struct i2c_client *client)
>  {
>  	struct max9286_priv *priv;
> @@ -1330,6 +1410,9 @@ static int max9286_probe(struct i2c_client *client)
>
>  	priv->client = client;
>
> +	/* GPIO values default to high */
> +	priv->gpio_state = BIT(0) | BIT(1);
> +
>  	priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable",
>  						   GPIOD_OUT_HIGH);
>  	if (IS_ERR(priv->gpiod_pwdn))
> @@ -1362,7 +1445,13 @@ static int max9286_probe(struct i2c_client *client)
>
>  	ret = max9286_parse_dt(priv);
>  	if (ret)
> -		goto err_powerdown;
> +		goto err_cleanup_dt;
> +
> +	if (!priv->use_gpio_poc) {
> +		ret = max9286_get_poc_supplies(priv);
> +		if (ret)
> +			goto err_cleanup_dt;
> +	}
>
>  	ret = max9286_init(priv);
>  	if (ret < 0)
> --
> Regards,
>
> Laurent Pinchart
>