[PATCH v9.2 0/9] GMSL fixups ready for v10.

[PATCH v9.2 0/9] GMSL fixups ready for v10.

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Unfortunately we missed the 5.8 window with reviews that came in really
late in the merge window, but hey  ... more time to do more fixups to
GMSL....

Jacopo has done various updates to fix the DT Yaml validation, though
there is probably some scope there that means we might need a
meta-schema for I2C-Muxes ... but that gets more complicated and
probably a bit out of scope for now.

I've taken more review comments into consideration and handled more
fixes for the drivers, which we expect to get to a point that these can
now be merged for the next release.

Sakari has at least provisionalyl given us his blessing - so lets hope
v10 is the last - and we can finally see max9286/rdacm20 get upstream.

--
Kieran


Jacopo Mondi (1):
  fixes! [max9286-dt]: Fix dt-validation

Kieran Bingham (8):
  fixes! [max9286]: Fix whitespace indent
  fixes! [max9286]: Validate link formats
  fixes! [max9286]: Use single sample per pixel
  fixes! [max9286]: Remove redundant DPHY check
  fixes! [max9286]: Remove redundant call
  fixes! [max9286-dt]: Add GPIO controller support
  fixes! [max9286-dt]: Correctly match the hex camera node reg
  fixes! [rdacm20]: Use usleep_range over mdelay(10)

 .../bindings/media/i2c/maxim,max9286.yaml     | 91 +++++++++++++++++--
 drivers/media/i2c/max9286.c                   | 45 ++++-----
 drivers/media/i2c/rdacm20.c                   |  4 +-
 3 files changed, 107 insertions(+), 33 deletions(-)

-- 
2.25.1

[PATCH v9.2 1/9] fixes! [max9286]: Fix whitespace indent

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Fix a minor 8space->tab conversion

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 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 590f384161a5..ef824d2b26b8 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -813,7 +813,7 @@ static int max9286_s_ctrl(struct v4l2_ctrl *ctrl)
 }
 
 static const struct v4l2_ctrl_ops max9286_ctrl_ops = {
-	.s_ctrl	= max9286_s_ctrl,
+	.s_ctrl = max9286_s_ctrl,
 };
 
 static int max9286_v4l2_register(struct max9286_priv *priv)
-- 
2.25.1

[PATCH v9.2 2/9] fixes! [max9286]: Validate link formats

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Disallow setting a format on the source link, but enable link validation
by returning the format of the first bound source when getting the
format of the source pad.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index ef824d2b26b8..7e59391a5736 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -711,7 +711,11 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 	struct max9286_priv *priv = sd_to_max9286(sd);
 	struct v4l2_mbus_framefmt *cfg_fmt;
 
-	if (format->pad >= MAX9286_SRC_PAD)
+	/* TODO:
+	 * Multiplexed Stream Support: Prevent setting the format on the shared
+	 * multiplexed bus.
+	 */
+	if (format->pad == MAX9286_SRC_PAD)
 		return -EINVAL;
 
 	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
@@ -743,11 +747,18 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
 	struct v4l2_mbus_framefmt *cfg_fmt;
+	unsigned int pad = format->pad;
 
-	if (format->pad >= MAX9286_SRC_PAD)
-		return -EINVAL;
+	/* TODO:
+	 * Multiplexed Stream Support: Support link validation by returning the
+	 * format of the first bound link. All links must have the same format,
+	 * as we do not support mixing, and matching of cameras connected to
+	 * the max9286.
+	 */
+	if (pad == MAX9286_SRC_PAD)
+		pad = __ffs(priv->bound_sources);
 
-	cfg_fmt = max9286_get_pad_format(priv, cfg, format->pad, format->which);
+	cfg_fmt = max9286_get_pad_format(priv, cfg, pad, format->which);
 	if (!cfg_fmt)
 		return -EINVAL;
 
-- 
2.25.1

[PATCH v9.2 3/9] fixes! [max9286]: Use single sample per pixel

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

MBUS formats for a serial bus should be specified as a single sample
point.

This change requires updating user-space test scripts to configure as
1x16 accordingly:

-    media-ctl -d $mdev -V "'$CSI':$IDX [fmt:UYVY8_2X8/1280x800 field:none]"
+    media-ctl -d $mdev -V "'$CSI':$IDX [fmt:UYVY8_1X16/1280x800 field:none]"

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 7e59391a5736..807024a9a149 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -684,7 +684,7 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->pad || code->index > 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	code->code = MEDIA_BUS_FMT_UYVY8_1X16;
 
 	return 0;
 }
@@ -720,13 +720,13 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 
 	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
 	switch (format->format.code) {
-	case MEDIA_BUS_FMT_UYVY8_2X8:
-	case MEDIA_BUS_FMT_VYUY8_2X8:
-	case MEDIA_BUS_FMT_YUYV8_2X8:
-	case MEDIA_BUS_FMT_YVYU8_2X8:
+	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_2X8;
+		format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
 		break;
 	}
 
@@ -788,7 +788,7 @@ static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
 {
 	fmt->width		= 1280;
 	fmt->height		= 800;
-	fmt->code		= MEDIA_BUS_FMT_UYVY8_2X8;
+	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
 	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
 	fmt->field		= V4L2_FIELD_NONE;
 	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
-- 
2.25.1

[PATCH v9.2 4/9] fixes! [max9286]: Remove redundant DPHY check

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The vep.bus_type is set as a parameter to v4l2_fwnode_endpoint_parse.
It will not be changed by the framework, so checking it is redundant.

Remove the extra check.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 807024a9a149..e170540a5d72 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1174,15 +1174,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 				return ret;
 			}
 
-			if (vep.bus_type != V4L2_MBUS_CSI2_DPHY) {
-				dev_err(dev,
-					"Media bus %u type not supported\n",
-					vep.bus_type);
-				v4l2_fwnode_endpoint_free(&vep);
-				of_node_put(node);
-				return -EINVAL;
-			}
-
 			priv->csi2_data_lanes =
 				vep.bus.mipi_csi2.num_data_lanes;
 			v4l2_fwnode_endpoint_free(&vep);
-- 
2.25.1

[PATCH v9.2 5/9] fixes! [max9286]: Remove redundant call

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

v4l2_fwnode_endpoint_free() was being called, however we did not call
v4l2_fwnode_endpoint_alloc_parse() prior which would have performed an
allocation of vep->link_frequencies anyway, so the call to kfree()
becomes a no-op.

Remove the extraneous and redundant call.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index e170540a5d72..03514bd27f5c 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -1176,7 +1176,6 @@ static int max9286_parse_dt(struct max9286_priv *priv)
 
 			priv->csi2_data_lanes =
 				vep.bus.mipi_csi2.num_data_lanes;
-			v4l2_fwnode_endpoint_free(&vep);
 
 			continue;
 		}
-- 
2.25.1

[PATCH v9.2 6/9] fixes! [max9286-dt]: Add GPIO controller support

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The MAX9286 exposes a GPIO controller to control the two GPIO out pins
of the chip.

These can in some configurations be used to control the power of the
cameras, and in the case of the V3M, it is used to enable power up
of the GMSL PoC regulator.

The regulator can not (currently) be moddelled as a regulator due to
probe time issues, and instead are controlled through the use of a
gpio-hog.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 .../devicetree/bindings/media/i2c/maxim,max9286.yaml         | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index f9d3e5712c59..7d75c3b63c0b 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -46,6 +46,11 @@ properties:
     description: GPIO connected to the \#PWDN pin with inverted polarity
     maxItems: 1
 
+  gpio-controller: true
+
+  '#gpio-cells':
+      const: 2
+
   ports:
     type: object
     description: |
-- 
2.25.1

[PATCH v9.2 7/9] fixes! [max9286-dt]: Fix dt-validation

[Kieran Bingham]

From: Jacopo Mondi <jacopo+renesas@jmondi.org>

Temporary fixup to ease review. To be squashed into v10 if accepted.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/media/i2c/maxim,max9286.yaml     | 86 +++++++++++++++++--
 1 file changed, 77 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 7d75c3b63c0b..4202c1ccc684 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -135,9 +135,7 @@ properties:
     description: |
       Each GMSL link is modelled as a child bus of an i2c bus
       multiplexer/switch, in accordance with bindings described in
-      Documentation/devicetree/bindings/i2c/i2c-mux.txt. The serializer
-      device on the remote end of the GMSL link shall be modelled as a child
-      node of the corresponding I2C bus.
+      Documentation/devicetree/bindings/i2c/i2c-mux.txt.
 
     properties:
       '#address-cells':
@@ -146,7 +144,74 @@ properties:
       '#size-cells':
         const: 0
 
-  additionalProperties: false
+    patternProperties:
+      "^i2c@[0-3]$":
+        type: object
+        description: |
+          Child node of the i2c bus multiplexer which represents a GMSL link.
+          Each serializer device on the GMSL link remote end is represented with
+          an i2c-mux child node. The MAX9286 chip supports up to 4 GMSL
+          channels.
+
+        properties:
+          '#address-cells':
+            const: 1
+
+          '#size-cells':
+            const: 0
+
+          reg:
+            description: The index of the GMSL channel.
+            maxItems: 1
+
+        patternProperties:
+          "^camera@[0-9]+":
+            type: object
+            description: |
+              The remote camera device, composed by a GMSL serializer and a
+              connected video source.
+
+            properties:
+              compatible:
+                description: The remote device compatible string.
+
+              reg:
+                description: |
+                  The I2C addresses to be assigned to the remote devices through
+                  address reprogramming. The number of entries depends on the
+                  requirements of the currently connected remote device.
+
+              port:
+                type: object
+
+                properties:
+                  endpoint:
+                    type: object
+
+                    properties:
+                      remote-endpoint:
+                        description: phandle to the MAX9286 sink endpoint.
+
+                    required:
+                      - remote-endpoint
+
+                    additionalProperties: false
+
+                required:
+                  - endpoint
+
+                additionalProperties: false
+
+            required:
+              - compatible
+              - reg
+              - port
+
+            additionalProperties: false
+
+        additionalProperties: false
+
+    additionalProperties: false
 
 required:
   - compatible
@@ -225,11 +290,11 @@ examples:
           i2c@0 {
             #address-cells = <1>;
             #size-cells = <0>;
-
             reg = <0>;
 
             camera@51 {
-              reg = <0x51>;
+              compatible = "imi,rdacm20";
+              reg = <0x51 0x61>;
 
               port {
                 rdacm20_out0: endpoint {
@@ -246,7 +311,8 @@ examples:
             reg = <1>;
 
             camera@52 {
-              reg = <0x52>;
+              compatible = "imi,rdacm20";
+              reg = <0x52 0x62>;
 
               port {
                 rdacm20_out1: endpoint {
@@ -262,7 +328,8 @@ examples:
             reg = <2>;
 
             camera@53 {
-              reg = <0x53>;
+              compatible = "imi,rdacm20";
+              reg = <0x53 0x63>;
 
               port {
                 rdacm20_out2: endpoint {
@@ -278,7 +345,8 @@ examples:
             reg = <3>;
 
             camera@54 {
-              reg = <0x54>;
+              compatible = "imi,rdacm20";
+              reg = <0x54 0x64>;
 
               port {
                 rdacm20_out3: endpoint {
-- 
2.25.1

[PATCH v9.2 8/9] fixes! [max9286-dt]: Correctly match the hex camera node reg

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The camera node reg is an i2c hex address. Update the regex.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 4202c1ccc684..34e0431d0bc1 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -165,7 +165,7 @@ properties:
             maxItems: 1
 
         patternProperties:
-          "^camera@[0-9]+":
+          "^camera@[a-f0-9]+$":
             type: object
             description: |
               The remote camera device, composed by a GMSL serializer and a
-- 
2.25.1

[PATCH v9.2 9/9] fixes! [rdacm20]: Use usleep_range over mdelay(10)

[Kieran Bingham]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Prefer usleep_range rather than the busy looping mdelay for 10ms waits.

msleep( n < 20 ) may sleep up to 20 milliseconds, but in this instance I
don't think that's a issue here. All the same, use usleep_range between
10, 15 milliseconds.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/i2c/rdacm20.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index cda3e6372ea9..1ed928c4ca70 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -488,9 +488,9 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
 	 * MAX9271 GPIO1 and verify communication with the OV10635.
 	 */
 	max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
-	mdelay(10);
+	usleep_range(10000, 15000);
 	max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
-	mdelay(10);
+	usleep_range(10000, 15000);
 
 again:
 	ret = ov10635_read16(dev, OV10635_PID);
-- 
2.25.1

Re: [PATCH v9.2 0/9] GMSL fixups ready for v10.

[Kieran Bingham]

Hi Kieran,

You forgot to mention that the previous 3 patches posted as v9.1 have
already been merged into your GMSL branch and these are based upon that...:

     - Use the same default mbus_fmt everywhere
     - Don't provide GPIO names
     - Fix dev->of_node refcnting

--
Kieran


On 10/06/2020 13:46, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Unfortunately we missed the 5.8 window with reviews that came in really
> late in the merge window, but hey  ... more time to do more fixups to
> GMSL....
> 
> Jacopo has done various updates to fix the DT Yaml validation, though
> there is probably some scope there that means we might need a
> meta-schema for I2C-Muxes ... but that gets more complicated and
> probably a bit out of scope for now.
> 
> I've taken more review comments into consideration and handled more
> fixes for the drivers, which we expect to get to a point that these can
> now be merged for the next release.
> 
> Sakari has at least provisionalyl given us his blessing - so lets hope
> v10 is the last - and we can finally see max9286/rdacm20 get upstream.
> 
> --
> Kieran
> 
> 
> Jacopo Mondi (1):
>   fixes! [max9286-dt]: Fix dt-validation
> 
> Kieran Bingham (8):
>   fixes! [max9286]: Fix whitespace indent
>   fixes! [max9286]: Validate link formats
>   fixes! [max9286]: Use single sample per pixel
>   fixes! [max9286]: Remove redundant DPHY check
>   fixes! [max9286]: Remove redundant call
>   fixes! [max9286-dt]: Add GPIO controller support
>   fixes! [max9286-dt]: Correctly match the hex camera node reg
>   fixes! [rdacm20]: Use usleep_range over mdelay(10)
> 
>  .../bindings/media/i2c/maxim,max9286.yaml     | 91 +++++++++++++++++--
>  drivers/media/i2c/max9286.c                   | 45 ++++-----
>  drivers/media/i2c/rdacm20.c                   |  4 +-
>  3 files changed, 107 insertions(+), 33 deletions(-)
> 

Re: [PATCH v9.2 9/9] fixes! [rdacm20]: Use usleep_range over mdelay(10)

[Jacopo Mondi]

Hi Kieran,

On Wed, Jun 10, 2020 at 01:46:23PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> Prefer usleep_range rather than the busy looping mdelay for 10ms waits.
>
> msleep( n < 20 ) may sleep up to 20 milliseconds, but in this instance I
> don't think that's a issue here. All the same, use usleep_range between
> 10, 15 milliseconds.

I think a rather conservative sleep interval like this one is fine, as
I don't have a characterization of the resest delay for this chip. The
10ms came from BSp code, and I'm not sure that was an empirical
measurement of was supported by some document.

Acked-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/rdacm20.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index cda3e6372ea9..1ed928c4ca70 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -488,9 +488,9 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>  	 * MAX9271 GPIO1 and verify communication with the OV10635.
>  	 */
>  	max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> -	mdelay(10);
> +	usleep_range(10000, 15000);
>  	max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> -	mdelay(10);
> +	usleep_range(10000, 15000);
>
>  again:
>  	ret = ov10635_read16(dev, OV10635_PID);
> --
> 2.25.1
>

[PATCH] fixup! dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286

[Jacopo Mondi]

---
 .../devicetree/bindings/media/i2c/maxim,max9286.yaml   | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 34e0431d0bc1..8307c41f2cae 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -176,6 +176,8 @@ properties:
                 description: The remote device compatible string.

               reg:
+                minItems: 2
+                maxItems: 3
                 description: |
                   The I2C addresses to be assigned to the remote devices through
                   address reprogramming. The number of entries depends on the
@@ -294,7 +296,7 @@ examples:

             camera@51 {
               compatible = "imi,rdacm20";
-              reg = <0x51 0x61>;
+              reg = <0x51>, <0x61>;

               port {
                 rdacm20_out0: endpoint {
@@ -312,7 +314,7 @@ examples:

             camera@52 {
               compatible = "imi,rdacm20";
-              reg = <0x52 0x62>;
+              reg = <0x52>, <0x62>;

               port {
                 rdacm20_out1: endpoint {
@@ -329,7 +331,7 @@ examples:

             camera@53 {
               compatible = "imi,rdacm20";
-              reg = <0x53 0x63>;
+              reg = <0x53>, <0x63>;

               port {
                 rdacm20_out2: endpoint {
@@ -346,7 +348,7 @@ examples:

             camera@54 {
               compatible = "imi,rdacm20";
-              reg = <0x54 0x64>;
+              reg = <0x54>, <0x64>;

               port {
                 rdacm20_out3: endpoint {
--
2.27.0

Re: [PATCH v9.2 0/9] GMSL fixups ready for v10.

[Jacopo Mondi]

Hi Kieran,

On Wed, Jun 10, 2020 at 01:46:14PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> Unfortunately we missed the 5.8 window with reviews that came in really
> late in the merge window, but hey  ... more time to do more fixups to
> GMSL....
>
> Jacopo has done various updates to fix the DT Yaml validation, though
> there is probably some scope there that means we might need a
> meta-schema for I2C-Muxes ... but that gets more complicated and
> probably a bit out of scope for now.

I've replied with a patch that fixes the dt-schema validation warning we had.

The example in the schema was actually wrong.

If you agree with the patch, please squash it in forthcoming v10.

Thanks
  j

>
> I've taken more review comments into consideration and handled more
> fixes for the drivers, which we expect to get to a point that these can
> now be merged for the next release.
>
> Sakari has at least provisionalyl given us his blessing - so lets hope
> v10 is the last - and we can finally see max9286/rdacm20 get upstream.
>
> --
> Kieran
>
>
> Jacopo Mondi (1):
>   fixes! [max9286-dt]: Fix dt-validation
>
> Kieran Bingham (8):
>   fixes! [max9286]: Fix whitespace indent
>   fixes! [max9286]: Validate link formats
>   fixes! [max9286]: Use single sample per pixel
>   fixes! [max9286]: Remove redundant DPHY check
>   fixes! [max9286]: Remove redundant call
>   fixes! [max9286-dt]: Add GPIO controller support
>   fixes! [max9286-dt]: Correctly match the hex camera node reg
>   fixes! [rdacm20]: Use usleep_range over mdelay(10)
>
>  .../bindings/media/i2c/maxim,max9286.yaml     | 91 +++++++++++++++++--
>  drivers/media/i2c/max9286.c                   | 45 ++++-----
>  drivers/media/i2c/rdacm20.c                   |  4 +-
>  3 files changed, 107 insertions(+), 33 deletions(-)
>
> --
> 2.25.1
>

Re: [PATCH v9.2 2/9] fixes! [max9286]: Validate link formats

[Jacopo Mondi]

Hi Kieran,
  a few small nits.

The patch is fine, feel free to squash it in v10.

On Wed, Jun 10, 2020 at 01:46:16PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> Disallow setting a format on the source link, but enable link validation
> by returning the format of the first bound source when getting the
> format of the source pad.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index ef824d2b26b8..7e59391a5736 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -711,7 +711,11 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	struct max9286_priv *priv = sd_to_max9286(sd);
>  	struct v4l2_mbus_framefmt *cfg_fmt;
>
> -	if (format->pad >= MAX9286_SRC_PAD)
> +	/* TODO:
> +	 * Multiplexed Stream Support: Prevent setting the format on the shared
> +	 * multiplexed bus.
> +	 */

I am not sure I would mention multiplexed stream support, and this is
not a TODO item. Simply, max9286 does not allow any format conversion
on its source pad, so the format is propagated from one of its sink to
the source (assuming all sinks have the same format applied).

Quite a common thing, isn't it ?

> +	if (format->pad == MAX9286_SRC_PAD)
>  		return -EINVAL;
>
>  	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
> @@ -743,11 +747,18 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
>  	struct v4l2_mbus_framefmt *cfg_fmt;
> +	unsigned int pad = format->pad;
>
> -	if (format->pad >= MAX9286_SRC_PAD)

I was about to comment we'ra nowe allowing pads > MAX9286_SRC_PAD, but the
core actually checks that for us.

> -		return -EINVAL;
> +	/* TODO:
> +	 * Multiplexed Stream Support: Support link validation by returning the
> +	 * format of the first bound link. All links must have the same format,
> +	 * as we do not support mixing, and matching of cameras connected to

nit: is the comma in 'mixing,' intentional ?
Same comment about multiplexed stream support in comment.

Theoretically, a set_fmt on a sink should propagate the format to the
source pad, and you could access it through
priv->fmts[MAX9286_SRC_PAD] and drop this check.

The result is actually the same, so it's up to you.

Thanks
  j

> +	 * the max9286.
> +	 */
> +	if (pad == MAX9286_SRC_PAD)
> +		pad = __ffs(priv->bound_sources);
>
> -	cfg_fmt = max9286_get_pad_format(priv, cfg, format->pad, format->which);
> +	cfg_fmt = max9286_get_pad_format(priv, cfg, pad, format->which);
>  	if (!cfg_fmt)
>  		return -EINVAL;
>
> --
> 2.25.1
>

Re: [PATCH v9.2 3/9] fixes! [max9286]: Use single sample per pixel

[Jacopo Mondi]

Hi Kieran,

On Wed, Jun 10, 2020 at 01:46:17PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> MBUS formats for a serial bus should be specified as a single sample
> point.
>
> This change requires updating user-space test scripts to configure as
> 1x16 accordingly:
>
> -    media-ctl -d $mdev -V "'$CSI':$IDX [fmt:UYVY8_2X8/1280x800 field:none]"
> +    media-ctl -d $mdev -V "'$CSI':$IDX [fmt:UYVY8_1X16/1280x800 field:none]"
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

FWIW I just sent a patch to update vin tests to use the new format.

Patch is fine, please squash in.

Thanks
  j

> ---
>  drivers/media/i2c/max9286.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7e59391a5736..807024a9a149 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -684,7 +684,7 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->pad || code->index > 0)
>  		return -EINVAL;
>
> -	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +	code->code = MEDIA_BUS_FMT_UYVY8_1X16;
>
>  	return 0;
>  }
> @@ -720,13 +720,13 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>
>  	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
>  	switch (format->format.code) {
> -	case MEDIA_BUS_FMT_UYVY8_2X8:
> -	case MEDIA_BUS_FMT_VYUY8_2X8:
> -	case MEDIA_BUS_FMT_YUYV8_2X8:
> -	case MEDIA_BUS_FMT_YVYU8_2X8:
> +	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_2X8;
> +		format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
>  		break;
>  	}
>
> @@ -788,7 +788,7 @@ static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
>  {
>  	fmt->width		= 1280;
>  	fmt->height		= 800;
> -	fmt->code		= MEDIA_BUS_FMT_UYVY8_2X8;
> +	fmt->code		= MEDIA_BUS_FMT_UYVY8_1X16;
>  	fmt->colorspace		= V4L2_COLORSPACE_SRGB;
>  	fmt->field		= V4L2_FIELD_NONE;
>  	fmt->ycbcr_enc		= V4L2_YCBCR_ENC_DEFAULT;
> --
> 2.25.1
>

Re: [PATCH v9.2 6/9] fixes! [max9286-dt]: Add GPIO controller support

[Jacopo Mondi]

Hi Kieran

On Wed, Jun 10, 2020 at 01:46:20PM +0100, Kieran Bingham wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> The MAX9286 exposes a GPIO controller to control the two GPIO out pins
> of the chip.
>
> These can in some configurations be used to control the power of the
> cameras, and in the case of the V3M, it is used to enable power up
> of the GMSL PoC regulator.
>
> The regulator can not (currently) be moddelled as a regulator due to
> probe time issues, and instead are controlled through the use of a
> gpio-hog.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

I have missed if this should be a required property or not..


> ---
>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml         | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index f9d3e5712c59..7d75c3b63c0b 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -46,6 +46,11 @@ properties:
>      description: GPIO connected to the \#PWDN pin with inverted polarity
>      maxItems: 1
>
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +      const: 2
> +
>    ports:
>      type: object
>      description: |
> --
> 2.25.1
>

Re: [PATCH] fixup! dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286

[Kieran Bingham]

Hi Jacopo,

Thanks - this looks good to me, and fixes the validation.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Thanks, will integrate into the current patches.


On 10/06/2020 15:44, Jacopo Mondi wrote:
> ---
>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml   | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index 34e0431d0bc1..8307c41f2cae 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -176,6 +176,8 @@ properties:
>                  description: The remote device compatible string.
> 
>                reg:
> +                minItems: 2
> +                maxItems: 3
>                  description: |
>                    The I2C addresses to be assigned to the remote devices through
>                    address reprogramming. The number of entries depends on the
> @@ -294,7 +296,7 @@ examples:
> 
>              camera@51 {
>                compatible = "imi,rdacm20";
> -              reg = <0x51 0x61>;
> +              reg = <0x51>, <0x61>;
> 
>                port {
>                  rdacm20_out0: endpoint {
> @@ -312,7 +314,7 @@ examples:
> 
>              camera@52 {
>                compatible = "imi,rdacm20";
> -              reg = <0x52 0x62>;
> +              reg = <0x52>, <0x62>;
> 
>                port {
>                  rdacm20_out1: endpoint {
> @@ -329,7 +331,7 @@ examples:
> 
>              camera@53 {
>                compatible = "imi,rdacm20";
> -              reg = <0x53 0x63>;
> +              reg = <0x53>, <0x63>;
> 
>                port {
>                  rdacm20_out2: endpoint {
> @@ -346,7 +348,7 @@ examples:
> 
>              camera@54 {
>                compatible = "imi,rdacm20";
> -              reg = <0x54 0x64>;
> +              reg = <0x54>, <0x64>;
> 
>                port {
>                  rdacm20_out3: endpoint {
> --
> 2.27.0
> 

Re: [PATCH v9.2 6/9] fixes! [max9286-dt]: Add GPIO controller support

[Kieran Bingham]

On 10/06/2020 16:16, Jacopo Mondi wrote:
> Hi Kieran
> 
> On Wed, Jun 10, 2020 at 01:46:20PM +0100, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> The MAX9286 exposes a GPIO controller to control the two GPIO out pins
>> of the chip.
>>
>> These can in some configurations be used to control the power of the
>> cameras, and in the case of the V3M, it is used to enable power up
>> of the GMSL PoC regulator.
>>
>> The regulator can not (currently) be moddelled as a regulator due to
>> probe time issues, and instead are controlled through the use of a
>> gpio-hog.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> I have missed if this should be a required property or not..

Hrm... I'm not sure. It isn't 'required' ... but the device does expose
gpio pins, which the driver provides access to (and is needed to be able
to expose a gpio-hog).

If the device isn't marked as a gpio-controller, then the gpio-hog
framework doesn't work.

But the gpio pins will ...

Do you think I should add gpio-controller to the required section as well?:

--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -220,6 +220,7 @@ required:
   - reg
   - ports
   - i2c-mux
+  - gpio-controller

 additionalProperties: false


As it's only required for making gpio-hogs, I guess it's optional, and
doesn't need to be listed...

But the *hardware* has gpios... which are controllable...

--
Kieran


> 
> 
>> ---
>>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml         | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>> index f9d3e5712c59..7d75c3b63c0b 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>> @@ -46,6 +46,11 @@ properties:
>>      description: GPIO connected to the \#PWDN pin with inverted polarity
>>      maxItems: 1
>>
>> +  gpio-controller: true
>> +
>> +  '#gpio-cells':
>> +      const: 2
>> +
>>    ports:
>>      type: object
>>      description: |
>> --
>> 2.25.1
>>

Re: [PATCH v9.2 6/9] fixes! [max9286-dt]: Add GPIO controller support

[Kieran Bingham]

On 12/06/2020 13:35, Kieran Bingham wrote:
> On 10/06/2020 16:16, Jacopo Mondi wrote:
>> Hi Kieran
>>
>> On Wed, Jun 10, 2020 at 01:46:20PM +0100, Kieran Bingham wrote:
>>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>
>>> The MAX9286 exposes a GPIO controller to control the two GPIO out pins
>>> of the chip.
>>>
>>> These can in some configurations be used to control the power of the
>>> cameras, and in the case of the V3M, it is used to enable power up
>>> of the GMSL PoC regulator.
>>>
>>> The regulator can not (currently) be moddelled as a regulator due to
>>> probe time issues, and instead are controlled through the use of a
>>> gpio-hog.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> I have missed if this should be a required property or not..
> 
> Hrm... I'm not sure. It isn't 'required' ... but the device does expose
> gpio pins, which the driver provides access to (and is needed to be able
> to expose a gpio-hog).
> 
> If the device isn't marked as a gpio-controller, then the gpio-hog
> framework doesn't work.
> 
> But the gpio pins will ...
> 
> Do you think I should add gpio-controller to the required section as well?:
> 
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -220,6 +220,7 @@ required:
>    - reg
>    - ports
>    - i2c-mux
> +  - gpio-controller
> 
>  additionalProperties: false
> 
> 
> As it's only required for making gpio-hogs, I guess it's optional, and
> doesn't need to be listed...
> 
> But the *hardware* has gpios... which are controllable...

And of course adding to the requried properties, means the example needs
ot be updated too, so it's actually:

Which I'll also add to v10, if you don't object.



Author: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Date:   Fri Jun 12 13:36:28 2020 +0100

    make gpio-controller non-optional

    Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

diff --git
a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index 8307c41f2cae..c632a10a753a 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -220,6 +220,7 @@ required:
   - reg
   - ports
   - i2c-mux
+  - gpio-controller

 additionalProperties: false

@@ -239,6 +240,9 @@ examples:
         poc-supply = <&camera_poc_12v>;
         enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;

+        gpio-controller;
+        #gpio-cells = <2>;
+
         ports {
           #address-cells = <1>;
           #size-cells = <0>;



> 
> --
> Kieran
> 
> 
>>
>>
>>> ---
>>>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml         | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>>> index f9d3e5712c59..7d75c3b63c0b 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>>> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
>>> @@ -46,6 +46,11 @@ properties:
>>>      description: GPIO connected to the \#PWDN pin with inverted polarity
>>>      maxItems: 1
>>>
>>> +  gpio-controller: true
>>> +
>>> +  '#gpio-cells':
>>> +      const: 2
>>> +
>>>    ports:
>>>      type: object
>>>      description: |
>>> --
>>> 2.25.1
>>>
> 

Re: [PATCH v9.2 6/9] fixes! [max9286-dt]: Add GPIO controller support

[Jacopo Mondi]

Hi Kieran,

On Fri, Jun 12, 2020 at 01:47:58PM +0100, Kieran Bingham wrote:
> On 12/06/2020 13:35, Kieran Bingham wrote:
> > On 10/06/2020 16:16, Jacopo Mondi wrote:
> >> Hi Kieran
> >>
> >> On Wed, Jun 10, 2020 at 01:46:20PM +0100, Kieran Bingham wrote:
> >>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>>
> >>> The MAX9286 exposes a GPIO controller to control the two GPIO out pins
> >>> of the chip.
> >>>
> >>> These can in some configurations be used to control the power of the
> >>> cameras, and in the case of the V3M, it is used to enable power up
> >>> of the GMSL PoC regulator.
> >>>
> >>> The regulator can not (currently) be moddelled as a regulator due to
> >>> probe time issues, and instead are controlled through the use of a
> >>> gpio-hog.
> >>>
> >>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> >>
> >> I have missed if this should be a required property or not..
> >
> > Hrm... I'm not sure. It isn't 'required' ... but the device does expose
> > gpio pins, which the driver provides access to (and is needed to be able
> > to expose a gpio-hog).
> >
> > If the device isn't marked as a gpio-controller, then the gpio-hog
> > framework doesn't work.
> >
> > But the gpio pins will ...
> >
> > Do you think I should add gpio-controller to the required section as well?:
> >
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -220,6 +220,7 @@ required:
> >    - reg
> >    - ports
> >    - i2c-mux
> > +  - gpio-controller
> >
> >  additionalProperties: false
> >
> >
> > As it's only required for making gpio-hogs, I guess it's optional, and
> > doesn't need to be listed...

As you off-line explained me, this mean that if cameras power is
controlled by the gpio pins exposed by the max9286, without this
property we would not be able to control it.

I would then make it mandatory

> >
> > But the *hardware* has gpios... which are controllable...
>
> And of course adding to the requried properties, means the example needs
> ot be updated too, so it's actually:

oh yes :)

>
> Which I'll also add to v10, if you don't object.
>
>
>
> Author: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Date:   Fri Jun 12 13:36:28 2020 +0100
>
>     make gpio-controller non-optional
>
>     Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> diff --git
> a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index 8307c41f2cae..c632a10a753a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -220,6 +220,7 @@ required:
>    - reg
>    - ports
>    - i2c-mux
> +  - gpio-controller
>
>  additionalProperties: false
>
> @@ -239,6 +240,9 @@ examples:
>          poc-supply = <&camera_poc_12v>;
>          enable-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
>
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +

Looks good to me!

Thanks
  j

>          ports {
>            #address-cells = <1>;
>            #size-cells = <0>;
>
>
>
> >
> > --
> > Kieran
> >
> >
> >>
> >>
> >>> ---
> >>>  .../devicetree/bindings/media/i2c/maxim,max9286.yaml         | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> >>> index f9d3e5712c59..7d75c3b63c0b 100644
> >>> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> >>> @@ -46,6 +46,11 @@ properties:
> >>>      description: GPIO connected to the \#PWDN pin with inverted polarity
> >>>      maxItems: 1
> >>>
> >>> +  gpio-controller: true
> >>> +
> >>> +  '#gpio-cells':
> >>> +      const: 2
> >>> +
> >>>    ports:
> >>>      type: object
> >>>      description: |
> >>> --
> >>> 2.25.1
> >>>
> >
>

Re: [PATCH v9.2 2/9] fixes! [max9286]: Validate link formats

[Kieran Bingham]

Hi Jacopo,

On 10/06/2020 16:02, Jacopo Mondi wrote:
> Hi Kieran,
>   a few small nits.
> 
> The patch is fine, feel free to squash it in v10.

Thanks,

> 
> On Wed, Jun 10, 2020 at 01:46:16PM +0100, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> Disallow setting a format on the source link, but enable link validation
>> by returning the format of the first bound source when getting the
>> format of the source pad.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/i2c/max9286.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
>> index ef824d2b26b8..7e59391a5736 100644
>> --- a/drivers/media/i2c/max9286.c
>> +++ b/drivers/media/i2c/max9286.c
>> @@ -711,7 +711,11 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>>  	struct max9286_priv *priv = sd_to_max9286(sd);
>>  	struct v4l2_mbus_framefmt *cfg_fmt;
>>
>> -	if (format->pad >= MAX9286_SRC_PAD)
>> +	/* TODO:
>> +	 * Multiplexed Stream Support: Prevent setting the format on the shared
>> +	 * multiplexed bus.
>> +	 */
> 
> I am not sure I would mention multiplexed stream support, and this is
> not a TODO item. Simply, max9286 does not allow any format conversion
> on its source pad, so the format is propagated from one of its sink to
> the source (assuming all sinks have the same format applied).
> 
> Quite a common thing, isn't it ?

Ok - I'll just drop the comment then.


> 
>> +	if (format->pad == MAX9286_SRC_PAD)
>>  		return -EINVAL;
>>
>>  	/* Refuse non YUV422 formats as we hardcode DT to 8 bit YUV422 */
>> @@ -743,11 +747,18 @@ static int max9286_get_fmt(struct v4l2_subdev *sd,
>>  {
>>  	struct max9286_priv *priv = sd_to_max9286(sd);
>>  	struct v4l2_mbus_framefmt *cfg_fmt;
>> +	unsigned int pad = format->pad;
>>
>> -	if (format->pad >= MAX9286_SRC_PAD)
> 
> I was about to comment we'ra nowe allowing pads > MAX9286_SRC_PAD, but the
> core actually checks that for us.
> 
>> -		return -EINVAL;
>> +	/* TODO:
>> +	 * Multiplexed Stream Support: Support link validation by returning the
>> +	 * format of the first bound link. All links must have the same format,
>> +	 * as we do not support mixing, and matching of cameras connected to
> 
> nit: is the comma in 'mixing,' intentional ?

No, it should be removed.

> Same comment about multiplexed stream support in comment.
> 
> Theoretically, a set_fmt on a sink should propagate the format to the
> source pad, and you could access it through
> priv->fmts[MAX9286_SRC_PAD] and drop this check.
> 
> The result is actually the same, so it's up to you.

I'll stick with what we've got then, and just remove those comments, if
you think they get in the way.



> 
> Thanks
>   j
> 
>> +	 * the max9286.
>> +	 */
>> +	if (pad == MAX9286_SRC_PAD)
>> +		pad = __ffs(priv->bound_sources);
>>
>> -	cfg_fmt = max9286_get_pad_format(priv, cfg, format->pad, format->which);
>> +	cfg_fmt = max9286_get_pad_format(priv, cfg, pad, format->which);
>>  	if (!cfg_fmt)
>>  		return -EINVAL;
>>
>> --
>> 2.25.1
>>