[PATCH v2 0/4] media: adv7180: make VPP handling more flexible

[PATCH v2 0/4] media: adv7180: make VPP handling more flexible

[Michael Tretter]

The adv7280 and adv7280-m have a Video Post Processor (VPP) unit that is
able to de-interlace the video signal.

This series makes the handling of the VPP more flexible. Furthermore, it
fixes the reported frame interval if the VPP is enabled.

Patches 1 and 2 add the device tree bindings and driver handling to
specify the (programmable) I2C address of the VPP (and CSI) slave device
via device tree.

Patch 3 exposes the registers of the adv via CONFIG_VIDEO_ADV_DEBUG to
user space for improved debugging capabilities.

Patch 4 fixes the frame interval that is reported by the driver if the
de-interlacing is active.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
Changes in v2:
- fix and rewrite device tree bindings definition
- simplify calculation of frame interval in progressive mode
- Link to v1: https://patch.msgid.link/20251111-b4-adv7180-vpp-sub-device-v1-0-9877fe9f709b@pengutronix.de

---
Michael Tretter (1):
      media: dt-bindings: adi,adv7180: add VPP and CSI register maps

Thorsten Schmelzer (3):
      media: adv7180: add support for ancillary devices
      media: adv7180: implement g_register and s_register
      media: adv7180: fix frame interval in progressive mode

 .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 93 +++++++++++++++++++++-
 drivers/media/i2c/adv7180.c                        | 55 +++++++++++--
 2 files changed, 139 insertions(+), 9 deletions(-)
---
base-commit: d363bdfa0ec6b19a4f40b572cec70430d5b13ad6
change-id: 20251111-b4-adv7180-vpp-sub-device-bcd357a7a491

Best regards,
-- 
Michael Tretter <m.tretter@pengutronix.de>

[PATCH v2 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Michael Tretter]

Different variants of the ADV7280 chip have up to three register maps.
The availability of the CSI and VPP register maps depends on the chip
variant. The address of the additional register maps depends on the
board design and other chips on the I2C but. They may be programmed via
registers in the main register map.

Allow to specify the addresses of the VPP and CSI register maps in the
device tree to solve I2C address conflicts on a board level.

The CSI and VPP register maps are always optional to allow backwards
compatibility with existing device trees which may rely on the default
address.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
Changes in v2:
- document backwards compatibility in commit message
- extend register maps in top level and define limits per variant
- define additional register maps for all variants
- document, why the programmable addresses are hardware description
---
 .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 93 +++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
index dee8ce7cb7ba..dbbbe76291bc 100644
--- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
@@ -30,7 +30,27 @@ properties:
           - adi,adv7282-m
 
   reg:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: main register map
+      - description: CSI register map
+      - description: VPP register map
+    description:
+      The ADV7180 family may have up to three register maps. All chips have
+      the main register map. The availability of the CSI and VPP register maps
+      depends on the chip variant.
+
+      The addresses of the CSI and VPP register maps are programmable by
+      software. They depend on the board layout and other devices on the I2C
+      bus and are determined by the hardware designer to avoid address
+      conflicts on the I2C bus.
+
+  reg-names:
+    minItems: 1
+    items:
+      - const: main
+      - enum: [ csi, vpp ]
+      - enum: [ csi, vpp ]
 
   powerdown-gpios:
     maxItems: 1
@@ -138,6 +158,58 @@ allOf:
       required:
         - ports
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,adv7180
+              - adi,adv7180cp
+              - adi,adv7180st
+              - adi,adv7182
+    then:
+      properties:
+        reg:
+          maxItems: 1
+
+        reg-names:
+          maxItems: 1
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,adv7281
+              - adi,adv7281-m
+              - adi,adv7281-ma
+    then:
+      properties:
+        reg:
+          maxItems: 2
+
+        reg-names:
+          items:
+            - const: main
+            - enum: [ csi ]
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,adv7280
+              - adi,adv7282
+    then:
+      properties:
+        reg:
+          maxItems: 2
+
+        reg-names:
+          items:
+            - const: main
+            - enum: [ vpp ]
+
 examples:
   - |
     i2c {
@@ -187,3 +259,22 @@ examples:
                     };
             };
     };
+
+  - |
+    i2c {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            composite-in@20 {
+                    compatible = "adi,adv7280-m";
+                    reg = <0x20>, <0x42>, <0x44>;
+                    reg-names = "main", "vpp", "csi";
+
+                    port {
+                            adv7280_out: endpoint {
+                                    bus-width = <8>;
+                                    remote-endpoint = <&vin1ep>;
+                            };
+                    };
+            };
+    };

-- 
2.47.3

[PATCH v2 2/4] media: adv7180: add support for ancillary devices

[Michael Tretter]

From: Thorsten Schmelzer <tschmelzer@topcon.com>

Depending on other devices on the i2c bus, using a non-default address
for the CSI and VPP devices may be necessary.

Replace calls to i2c_new_dummy_device with i2c_new_ancillary_device,
which can directly use an i2c address from the device tree.

Program the actual addresses of the sub-devices when configuring the
chip.

Signed-off-by: Thorsten Schmelzer <tschmelzer@topcon.com>
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
Changes in v2:
- None
---
 drivers/media/i2c/adv7180.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 378f4e6af12c..4152f2049a6d 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -1066,13 +1066,13 @@ static int adv7180_select_input(struct adv7180_state *state, unsigned int input)
 
 static int adv7182_init(struct adv7180_state *state)
 {
-	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2)
+	if (state->csi_client)
 		adv7180_write(state, ADV7180_REG_CSI_SLAVE_ADDR,
-			ADV7180_DEFAULT_CSI_I2C_ADDR << 1);
+			      state->csi_client->addr << 1);
 
-	if (state->chip_info->flags & ADV7180_FLAG_I2P)
+	if (state->vpp_client)
 		adv7180_write(state, ADV7180_REG_VPP_SLAVE_ADDR,
-			ADV7180_DEFAULT_VPP_I2C_ADDR << 1);
+			      state->vpp_client->addr << 1);
 
 	if (state->chip_info->flags & ADV7180_FLAG_V2) {
 		/* ADI recommended writes for improved video quality */
@@ -1443,15 +1443,17 @@ static int adv7180_probe(struct i2c_client *client)
 		state->force_bt656_4 = true;
 
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
-		state->csi_client = i2c_new_dummy_device(client->adapter,
-				ADV7180_DEFAULT_CSI_I2C_ADDR);
+		state->csi_client =
+			i2c_new_ancillary_device(client, "csi",
+						 ADV7180_DEFAULT_CSI_I2C_ADDR);
 		if (IS_ERR(state->csi_client))
 			return PTR_ERR(state->csi_client);
 	}
 
 	if (state->chip_info->flags & ADV7180_FLAG_I2P) {
-		state->vpp_client = i2c_new_dummy_device(client->adapter,
-				ADV7180_DEFAULT_VPP_I2C_ADDR);
+		state->vpp_client =
+			i2c_new_ancillary_device(client, "vpp",
+						 ADV7180_DEFAULT_VPP_I2C_ADDR);
 		if (IS_ERR(state->vpp_client)) {
 			ret = PTR_ERR(state->vpp_client);
 			goto err_unregister_csi_client;

-- 
2.47.3

[PATCH v2 3/4] media: adv7180: implement g_register and s_register

[Michael Tretter]

From: Thorsten Schmelzer <tschmelzer@topcon.com>

The g_register and s_register callbacks are useful for debugging the
adv7180.

Implement the callbacks to expose the register debugging to userspace.

Signed-off-by: Thorsten Schmelzer <tschmelzer@topcon.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
Changes in v2:
- None
---
 drivers/media/i2c/adv7180.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 4152f2049a6d..d289cbc2eefd 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -969,6 +969,32 @@ static int adv7180_subscribe_event(struct v4l2_subdev *sd,
 	}
 }
 
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int adv7180_g_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct adv7180_state *state = to_state(sd);
+	int ret;
+
+	ret = adv7180_read(state, reg->reg);
+	if (ret < 0)
+		return ret;
+
+	reg->val = ret;
+	reg->size = 1;
+
+	return 0;
+}
+
+static int adv7180_s_register(struct v4l2_subdev *sd,
+			      const struct v4l2_dbg_register *reg)
+{
+	struct adv7180_state *state = to_state(sd);
+
+	return adv7180_write(state, reg->reg, reg->val);
+}
+#endif
+
 static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 	.s_std = adv7180_s_std,
 	.g_std = adv7180_g_std,
@@ -982,6 +1008,10 @@ static const struct v4l2_subdev_video_ops adv7180_video_ops = {
 static const struct v4l2_subdev_core_ops adv7180_core_ops = {
 	.subscribe_event = adv7180_subscribe_event,
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register = adv7180_g_register,
+	.s_register = adv7180_s_register,
+#endif
 };
 
 static const struct v4l2_subdev_pad_ops adv7180_pad_ops = {

-- 
2.47.3

[PATCH v2 4/4] media: adv7180: fix frame interval in progressive mode

[Michael Tretter]

From: Thorsten Schmelzer <tschmelzer@topcon.com>

The ADV7280-M may internally convert interlaced video input to
progressive video. If this mode is enabled, the ADV7280-M delivers
progressive video frames at the field rate of 50 fields per second (PAL)
or 60 fields per second (NTSC).

Fix the reported frame interval if progressive video is enabled.

Signed-off-by: Thorsten Schmelzer <tschmelzer@topcon.com>
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
Changes in v2:
- Simplify and document calculation of frame interval
---
 drivers/media/i2c/adv7180.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index d289cbc2eefd..669b0b3165b1 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -507,6 +507,13 @@ static int adv7180_get_frame_interval(struct v4l2_subdev *sd,
 		fi->interval.denominator = 25;
 	}
 
+	/*
+	 * If the de-interlacer is active, the chip produces full video frames
+	 * at the field rate.
+	 */
+	if (state->field == V4L2_FIELD_NONE)
+		fi->interval.denominator *= 2;
+
 	return 0;
 }
 

-- 
2.47.3

Re: [PATCH v2 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Krzysztof Kozlowski]

On Wed, Nov 19, 2025 at 05:25:51PM +0100, Michael Tretter wrote:
> diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> index dee8ce7cb7ba..dbbbe76291bc 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> @@ -30,7 +30,27 @@ properties:
>            - adi,adv7282-m
>  
>    reg:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: main register map
> +      - description: CSI register map
> +      - description: VPP register map
> +    description:
> +      The ADV7180 family may have up to three register maps. All chips have
> +      the main register map. The availability of the CSI and VPP register maps
> +      depends on the chip variant.
> +
> +      The addresses of the CSI and VPP register maps are programmable by
> +      software. They depend on the board layout and other devices on the I2C
> +      bus and are determined by the hardware designer to avoid address
> +      conflicts on the I2C bus.
> +
> +  reg-names:
> +    minItems: 1
> +    items:
> +      - const: main
> +      - enum: [ csi, vpp ]
> +      - enum: [ csi, vpp ]

Last entry must be:

const: vpp

We do not allow flexible order... but the problem is that your if:then:
does not match above at all. You do not have three items anywhere.


>  
>    powerdown-gpios:
>      maxItems: 1
> @@ -138,6 +158,58 @@ allOf:
>        required:
>          - ports
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,adv7180
> +              - adi,adv7180cp
> +              - adi,adv7180st
> +              - adi,adv7182
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 1
> +
> +        reg-names:
> +          maxItems: 1
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,adv7281
> +              - adi,adv7281-m
> +              - adi,adv7281-ma
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 2
> +
> +        reg-names:
> +          items:
> +            - const: main
> +            - enum: [ csi ]

const

> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,adv7280
> +              - adi,adv7282
> +    then:
> +      properties:
> +        reg:
> +          maxItems: 2
> +
> +        reg-names:
> +          items:
> +            - const: main
> +            - enum: [ vpp ]

const

> +
>  examples:

Best regards,
Krzysztof

Re: [PATCH v2 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Michael Tretter]

On Thu, 20 Nov 2025 09:04:48 +0100, Krzysztof Kozlowski wrote:
> On Wed, Nov 19, 2025 at 05:25:51PM +0100, Michael Tretter wrote:
> > diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > index dee8ce7cb7ba..dbbbe76291bc 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > @@ -30,7 +30,27 @@ properties:
> >            - adi,adv7282-m
> >  
> >    reg:
> > -    maxItems: 1
> > +    minItems: 1
> > +    items:
> > +      - description: main register map
> > +      - description: CSI register map
> > +      - description: VPP register map
> > +    description:
> > +      The ADV7180 family may have up to three register maps. All chips have
> > +      the main register map. The availability of the CSI and VPP register maps
> > +      depends on the chip variant.
> > +
> > +      The addresses of the CSI and VPP register maps are programmable by
> > +      software. They depend on the board layout and other devices on the I2C
> > +      bus and are determined by the hardware designer to avoid address
> > +      conflicts on the I2C bus.
> > +
> > +  reg-names:
> > +    minItems: 1
> > +    items:
> > +      - const: main
> > +      - enum: [ csi, vpp ]
> > +      - enum: [ csi, vpp ]
> 
> Last entry must be:
> 
> const: vpp
> 
> We do not allow flexible order... but the problem is that your if:then:
> does not match above at all. You do not have three items anywhere.

I'm not entirely sure, if I correctly understand that comment.

The adi,adv7280-m and adi,adv7282-m have all three items and don't need
an if:then:. Do I have explicitly define the binding with three items,
too?

The chip has the following variants:

adi,adv7180:     main
adi,adv7180cp:   main
adi,adv7180st:   main
adi,adv7182:     main
adi,adv7280:     main, vpp
adi,adv7280-m:   main, csi, vpp
adi,adv7281:     main, csi
adi,adv7281-m:   main, csi
adi,adv7281-ma:  main, csi
adi,adv7282:     main, vpp
adi,adv7282-m:   main, csi, vpp

If I make the last entry (vpp) const, I allow exactly these variants.

For the adi,adv7280-m compatible, the following combinations would be
valid or invalid:

adi,adv7280-m:   main

is valid, because only main is mandatory. For csi and vpp, the default
addresses are used.

adi,adv7280-m:   main, vpp

is valid, because the second entry may be vpp. For csi, the default
address is used.

adi,adv7280-m:   main, vpp, csi

is invalid, because the entries must be in the defined order, and
flexible order is not possible.

Is this correct and matches the binding definition?

Thanks!

Michael

> 
> 
> >  
> >    powerdown-gpios:
> >      maxItems: 1
> > @@ -138,6 +158,58 @@ allOf:
> >        required:
> >          - ports
> >  
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,adv7180
> > +              - adi,adv7180cp
> > +              - adi,adv7180st
> > +              - adi,adv7182
> > +    then:
> > +      properties:
> > +        reg:
> > +          maxItems: 1
> > +
> > +        reg-names:
> > +          maxItems: 1
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,adv7281
> > +              - adi,adv7281-m
> > +              - adi,adv7281-ma
> > +    then:
> > +      properties:
> > +        reg:
> > +          maxItems: 2
> > +
> > +        reg-names:
> > +          items:
> > +            - const: main
> > +            - enum: [ csi ]
> 
> const
> 
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,adv7280
> > +              - adi,adv7282
> > +    then:
> > +      properties:
> > +        reg:
> > +          maxItems: 2
> > +
> > +        reg-names:
> > +          items:
> > +            - const: main
> > +            - enum: [ vpp ]
> 
> const
> 
> > +
> >  examples:

Re: [PATCH v2 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Krzysztof Kozlowski]

On 20/11/2025 09:53, Michael Tretter wrote:
> On Thu, 20 Nov 2025 09:04:48 +0100, Krzysztof Kozlowski wrote:
>> On Wed, Nov 19, 2025 at 05:25:51PM +0100, Michael Tretter wrote:
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>> index dee8ce7cb7ba..dbbbe76291bc 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>> +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>> @@ -30,7 +30,27 @@ properties:
>>>            - adi,adv7282-m
>>>  
>>>    reg:
>>> -    maxItems: 1
>>> +    minItems: 1
>>> +    items:
>>> +      - description: main register map
>>> +      - description: CSI register map
>>> +      - description: VPP register map
>>> +    description:
>>> +      The ADV7180 family may have up to three register maps. All chips have
>>> +      the main register map. The availability of the CSI and VPP register maps
>>> +      depends on the chip variant.
>>> +
>>> +      The addresses of the CSI and VPP register maps are programmable by
>>> +      software. They depend on the board layout and other devices on the I2C
>>> +      bus and are determined by the hardware designer to avoid address
>>> +      conflicts on the I2C bus.
>>> +
>>> +  reg-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: main
>>> +      - enum: [ csi, vpp ]
>>> +      - enum: [ csi, vpp ]
>>
>> Last entry must be:
>>
>> const: vpp
>>
>> We do not allow flexible order... but the problem is that your if:then:
>> does not match above at all. You do not have three items anywhere.
> 
> I'm not entirely sure, if I correctly understand that comment.
> 
> The adi,adv7280-m and adi,adv7282-m have all three items and don't need
> an if:then:. Do I have explicitly define the binding with three items,
> too?

Which comment? That third item cannot be csi? What is odd here?


> 
> The chip has the following variants:
> 
> adi,adv7180:     main
> adi,adv7180cp:   main
> adi,adv7180st:   main
> adi,adv7182:     main
> adi,adv7280:     main, vpp
> adi,adv7280-m:   main, csi, vpp
> adi,adv7281:     main, csi
> adi,adv7281-m:   main, csi
> adi,adv7281-ma:  main, csi
> adi,adv7282:     main, vpp
> adi,adv7282-m:   main, csi, vpp

So where is csi as third item?

Anyway, you also miss minItems in your if:then: cases.

> 
> If I make the last entry (vpp) const, I allow exactly these variants.
> 
> For the adi,adv7280-m compatible, the following combinations would be
> valid or invalid:
> 
> adi,adv7280-m:   main
> 
> is valid, because only main is mandatory. For csi and vpp, the default
> addresses are used.
> 
> adi,adv7280-m:   main, vpp
> 
> is valid, because the second entry may be vpp. For csi, the default
> address is used.
> 
> adi,adv7280-m:   main, vpp, csi
> 
> is invalid, because the entries must be in the defined order, and
> flexible order is not possible.
> 
> Is this correct and matches the binding definition?

It does not match your code.


Best regards,
Krzysztof

Re: [PATCH v2 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Michael Tretter]

On Thu, 20 Nov 2025 10:05:43 +0100, Krzysztof Kozlowski wrote:
> On 20/11/2025 09:53, Michael Tretter wrote:
> > On Thu, 20 Nov 2025 09:04:48 +0100, Krzysztof Kozlowski wrote:
> >> On Wed, Nov 19, 2025 at 05:25:51PM +0100, Michael Tretter wrote:
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> >>> index dee8ce7cb7ba..dbbbe76291bc 100644
> >>> --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> >>> @@ -30,7 +30,27 @@ properties:
> >>>            - adi,adv7282-m
> >>>  
> >>>    reg:
> >>> -    maxItems: 1
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - description: main register map
> >>> +      - description: CSI register map
> >>> +      - description: VPP register map
> >>> +    description:
> >>> +      The ADV7180 family may have up to three register maps. All chips have
> >>> +      the main register map. The availability of the CSI and VPP register maps
> >>> +      depends on the chip variant.
> >>> +
> >>> +      The addresses of the CSI and VPP register maps are programmable by
> >>> +      software. They depend on the board layout and other devices on the I2C
> >>> +      bus and are determined by the hardware designer to avoid address
> >>> +      conflicts on the I2C bus.
> >>> +
> >>> +  reg-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: main
> >>> +      - enum: [ csi, vpp ]
> >>> +      - enum: [ csi, vpp ]
> >>
> >> Last entry must be:
> >>
> >> const: vpp
> >>
> >> We do not allow flexible order... but the problem is that your if:then:
> >> does not match above at all. You do not have three items anywhere.
> > 
> > I'm not entirely sure, if I correctly understand that comment.
> > 
> > The adi,adv7280-m and adi,adv7282-m have all three items and don't need
> > an if:then:. Do I have explicitly define the binding with three items,
> > too?
> 
> Which comment? That third item cannot be csi? What is odd here?
> 
> 
> > 
> > The chip has the following variants:
> > 
> > adi,adv7180:     main
> > adi,adv7180cp:   main
> > adi,adv7180st:   main
> > adi,adv7182:     main
> > adi,adv7280:     main, vpp
> > adi,adv7280-m:   main, csi, vpp
> > adi,adv7281:     main, csi
> > adi,adv7281-m:   main, csi
> > adi,adv7281-ma:  main, csi
> > adi,adv7282:     main, vpp
> > adi,adv7282-m:   main, csi, vpp
> 
> So where is csi as third item?
> 
> Anyway, you also miss minItems in your if:then: cases.

The minItems in the if:then: cases would be the same as in the top level
definition. Do I have to override the minItems if it's the same?

> 
> > 
> > If I make the last entry (vpp) const, I allow exactly these variants.
> > 
> > For the adi,adv7280-m compatible, the following combinations would be
> > valid or invalid:
> > 
> > adi,adv7280-m:   main
> > 
> > is valid, because only main is mandatory. For csi and vpp, the default
> > addresses are used.
> > 
> > adi,adv7280-m:   main, vpp
> > 
> > is valid, because the second entry may be vpp. For csi, the default
> > address is used.
> > 
> > adi,adv7280-m:   main, vpp, csi
> > 
> > is invalid, because the entries must be in the defined order, and
> > flexible order is not possible.
> > 
> > Is this correct and matches the binding definition?
> 
> It does not match your code.

If I change the last entry to

	- const: csi

to disallow vpp as a third item (which is compliant to the example), the
binding is fine. Correct?

Michael

Re: [PATCH v2 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Krzysztof Kozlowski]

On 20/11/2025 13:01, Michael Tretter wrote:
> On Thu, 20 Nov 2025 10:05:43 +0100, Krzysztof Kozlowski wrote:
>> On 20/11/2025 09:53, Michael Tretter wrote:
>>> On Thu, 20 Nov 2025 09:04:48 +0100, Krzysztof Kozlowski wrote:
>>>> On Wed, Nov 19, 2025 at 05:25:51PM +0100, Michael Tretter wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>>>> index dee8ce7cb7ba..dbbbe76291bc 100644
>>>>> --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>>>> @@ -30,7 +30,27 @@ properties:
>>>>>            - adi,adv7282-m
>>>>>  
>>>>>    reg:
>>>>> -    maxItems: 1
>>>>> +    minItems: 1
>>>>> +    items:
>>>>> +      - description: main register map
>>>>> +      - description: CSI register map
>>>>> +      - description: VPP register map
>>>>> +    description:
>>>>> +      The ADV7180 family may have up to three register maps. All chips have
>>>>> +      the main register map. The availability of the CSI and VPP register maps
>>>>> +      depends on the chip variant.
>>>>> +
>>>>> +      The addresses of the CSI and VPP register maps are programmable by
>>>>> +      software. They depend on the board layout and other devices on the I2C
>>>>> +      bus and are determined by the hardware designer to avoid address
>>>>> +      conflicts on the I2C bus.
>>>>> +
>>>>> +  reg-names:
>>>>> +    minItems: 1
>>>>> +    items:
>>>>> +      - const: main
>>>>> +      - enum: [ csi, vpp ]
>>>>> +      - enum: [ csi, vpp ]
>>>>
>>>> Last entry must be:
>>>>
>>>> const: vpp
>>>>
>>>> We do not allow flexible order... but the problem is that your if:then:
>>>> does not match above at all. You do not have three items anywhere.
>>>
>>> I'm not entirely sure, if I correctly understand that comment.
>>>
>>> The adi,adv7280-m and adi,adv7282-m have all three items and don't need
>>> an if:then:. Do I have explicitly define the binding with three items,
>>> too?
>>
>> Which comment? That third item cannot be csi? What is odd here?
>>
>>
>>>
>>> The chip has the following variants:
>>>
>>> adi,adv7180:     main
>>> adi,adv7180cp:   main
>>> adi,adv7180st:   main
>>> adi,adv7182:     main
>>> adi,adv7280:     main, vpp
>>> adi,adv7280-m:   main, csi, vpp
>>> adi,adv7281:     main, csi
>>> adi,adv7281-m:   main, csi
>>> adi,adv7281-ma:  main, csi
>>> adi,adv7282:     main, vpp
>>> adi,adv7282-m:   main, csi, vpp
>>
>> So where is csi as third item?
>>
>> Anyway, you also miss minItems in your if:then: cases.
> 
> The minItems in the if:then: cases would be the same as in the top level
> definition. Do I have to override the minItems if it's the same?

Yes, you need to make it explicit. All constraints, except min=maxItems,
which you imply here, must be explicit.

> 
>>
>>>
>>> If I make the last entry (vpp) const, I allow exactly these variants.
>>>
>>> For the adi,adv7280-m compatible, the following combinations would be
>>> valid or invalid:
>>>
>>> adi,adv7280-m:   main
>>>
>>> is valid, because only main is mandatory. For csi and vpp, the default
>>> addresses are used.
>>>
>>> adi,adv7280-m:   main, vpp
>>>
>>> is valid, because the second entry may be vpp. For csi, the default
>>> address is used.
>>>
>>> adi,adv7280-m:   main, vpp, csi
>>>
>>> is invalid, because the entries must be in the defined order, and
>>> flexible order is not possible.
>>>
>>> Is this correct and matches the binding definition?
>>
>> It does not match your code.
> 
> If I change the last entry to
> 
> 	- const: csi
> 
> to disallow vpp as a third item (which is compliant to the example), the
> binding is fine. Correct?

Probably, lost context, because I asked you to do that but it felt like
you keep discussing. So if you implement reviewer's suggestion, it is
enough to say "ack" or "ok".


Best regards,
Krzysztof