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

[PATCH 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 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>
---
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 | 44 +++++++++++++++++
 drivers/media/i2c/adv7180.c                        | 55 ++++++++++++++++++----
 2 files changed, 89 insertions(+), 10 deletions(-)
---
base-commit: b4fbb13db86adb0bae1d7f968b61ea8dc9635e33
change-id: 20251111-b4-adv7180-vpp-sub-device-bcd357a7a491

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

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

[Michael Tretter]

The ADV7280 and ADV7280-M have up to three register maps for the VPP and
CSI. The address of these register maps may be programmed via registers
in the main register map.

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

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
index dee8ce7cb7ba..4bbdc812442b 100644
--- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
@@ -138,6 +138,31 @@ allOf:
       required:
         - ports
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,adv7280
+              - adi,adv7280-m
+    then:
+      properties:
+        reg:
+          minItems: 1
+          maxItems: 3
+          description:
+            The ADV7280 and ADV7280-M have up to three register maps, which can be
+            accessed via the I2C port. The main register map, the VPP register map,
+            and the CSI register map. The main register map is mandatory. The other
+            register maps are optional and the default is used unless specified.
+
+        reg-names:
+          minItems: 1
+          items:
+            - const: main
+            - enum: [ csi, vpp ]
+            - enum: [ csi, vpp ]
+
 examples:
   - |
     i2c {
@@ -187,3 +212,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

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

[Rob Herring (Arm)]

On Tue, 11 Nov 2025 15:36:14 +0100, Michael Tretter wrote:
> The ADV7280 and ADV7280-M have up to three register maps for the VPP and
> CSI. The address of these register maps may be programmed via registers
> in the main register map.
> 
> Allow to specify the addresses of the VPP and CSI in the device tree to
> solve address conflicts on a board level.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/adi,adv7180.example.dtb: composite-in@20 (adi,adv7280-m): 'reg-names' does not match any of the regexes: '^pinctrl-[0-9]+$'
	from schema $id: http://devicetree.org/schemas/media/i2c/adi,adv7180.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/adi,adv7180.example.dtb: composite-in@20 (adi,adv7280-m): reg: [[32], [66], [68]] is too long
	from schema $id: http://devicetree.org/schemas/media/i2c/adi,adv7180.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20251111-b4-adv7180-vpp-sub-device-v1-1-9877fe9f709b@pengutronix.de

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 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Michael Tretter]

On Tue, 11 Nov 2025 09:33:05 -0600, Rob Herring (Arm) wrote:
> On Tue, 11 Nov 2025 15:36:14 +0100, Michael Tretter wrote:
> > The ADV7280 and ADV7280-M have up to three register maps for the VPP and
> > CSI. The address of these register maps may be programmed via registers
> > in the main register map.
> > 
> > Allow to specify the addresses of the VPP and CSI in the device tree to
> > solve address conflicts on a board level.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/adi,adv7180.example.dtb: composite-in@20 (adi,adv7280-m): 'reg-names' does not match any of the regexes: '^pinctrl-[0-9]+$'
> 	from schema $id: http://devicetree.org/schemas/media/i2c/adi,adv7180.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/i2c/adi,adv7180.example.dtb: composite-in@20 (adi,adv7280-m): reg: [[32], [66], [68]] is too long
> 	from schema $id: http://devicetree.org/schemas/media/i2c/adi,adv7180.yaml

I assumed that I could conditionally override the properties and use the
existing definition as default. It seems that I have to explicitly set
the default in the else path.

I'll fix this in v2.

> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20251111-b4-adv7180-vpp-sub-device-v1-1-9877fe9f709b@pengutronix.de
> 
> 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 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Dave Stevenson]

Hi Michael

On Tue, 11 Nov 2025 at 14:50, Michael Tretter <m.tretter@pengutronix.de> wrote:
>
> The ADV7280 and ADV7280-M have up to three register maps for the VPP and
> CSI. The address of these register maps may be programmed via registers
> in the main register map.

AIUI all the ADV728x devices have at least 2 addresses, signified by
the ADV7180_FLAG_MIPI_CSI2 and ADV7180_FLAG_I2P flags in their
adv7180_chip_info structures.
Is there a reason that you've restricted this to just the two chips?

adv7281 - CSI
adv7281-m - CSI
adv7281-ma - CSI
adv7282 - VPP
adv7282-m - VPP and CSI

The adv7180 and adv7182 families are the only two which only have the
single I2C address.

Thanks.
  Dave

> Allow to specify the addresses of the VPP and CSI in the device tree to
> solve address conflicts on a board level.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> index dee8ce7cb7ba..4bbdc812442b 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> @@ -138,6 +138,31 @@ allOf:
>        required:
>          - ports
>
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,adv7280
> +              - adi,adv7280-m
> +    then:
> +      properties:
> +        reg:
> +          minItems: 1
> +          maxItems: 3
> +          description:
> +            The ADV7280 and ADV7280-M have up to three register maps, which can be
> +            accessed via the I2C port. The main register map, the VPP register map,
> +            and the CSI register map. The main register map is mandatory. The other
> +            register maps are optional and the default is used unless specified.
> +
> +        reg-names:
> +          minItems: 1
> +          items:
> +            - const: main
> +            - enum: [ csi, vpp ]
> +            - enum: [ csi, vpp ]
> +
>  examples:
>    - |
>      i2c {
> @@ -187,3 +212,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
>
>

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

[Michael Tretter]

Hi Dave,

On Tue, 11 Nov 2025 16:02:26 +0000, Dave Stevenson wrote:
> On Tue, 11 Nov 2025 at 14:50, Michael Tretter <m.tretter@pengutronix.de> wrote:
> >
> > The ADV7280 and ADV7280-M have up to three register maps for the VPP and
> > CSI. The address of these register maps may be programmed via registers
> > in the main register map.
> 
> AIUI all the ADV728x devices have at least 2 addresses, signified by
> the ADV7180_FLAG_MIPI_CSI2 and ADV7180_FLAG_I2P flags in their
> adv7180_chip_info structures.
> Is there a reason that you've restricted this to just the two chips?

I only have an ADV7280-M for testing. Thus, I restricted it to the chip
that I could test and the variant that's the same except for the CSI.

> 
> adv7281 - CSI
> adv7281-m - CSI
> adv7281-ma - CSI
> adv7282 - VPP
> adv7282-m - VPP and CSI
> 
> The adv7180 and adv7182 families are the only two which only have the
> single I2C address.

Are you suggesting to extend the binding to be able to specify the
addresses for the CSI and VPP for any of these chips?

Michael

> 
> Thanks.
>   Dave
> 
> > Allow to specify the addresses of the VPP and CSI in the device tree to
> > solve address conflicts on a board level.
> >
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > index dee8ce7cb7ba..4bbdc812442b 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > @@ -138,6 +138,31 @@ allOf:
> >        required:
> >          - ports
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,adv7280
> > +              - adi,adv7280-m
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 1
> > +          maxItems: 3
> > +          description:
> > +            The ADV7280 and ADV7280-M have up to three register maps, which can be
> > +            accessed via the I2C port. The main register map, the VPP register map,
> > +            and the CSI register map. The main register map is mandatory. The other
> > +            register maps are optional and the default is used unless specified.
> > +
> > +        reg-names:
> > +          minItems: 1
> > +          items:
> > +            - const: main
> > +            - enum: [ csi, vpp ]
> > +            - enum: [ csi, vpp ]
> > +
> >  examples:
> >    - |
> >      i2c {
> > @@ -187,3 +212,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
> >
> >
> 

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

[Dave Stevenson]

Hi Michael

On Tue, 11 Nov 2025 at 17:39, Michael Tretter <m.tretter@pengutronix.de> wrote:
>
> Hi Dave,
>
> On Tue, 11 Nov 2025 16:02:26 +0000, Dave Stevenson wrote:
> > On Tue, 11 Nov 2025 at 14:50, Michael Tretter <m.tretter@pengutronix.de> wrote:
> > >
> > > The ADV7280 and ADV7280-M have up to three register maps for the VPP and
> > > CSI. The address of these register maps may be programmed via registers
> > > in the main register map.
> >
> > AIUI all the ADV728x devices have at least 2 addresses, signified by
> > the ADV7180_FLAG_MIPI_CSI2 and ADV7180_FLAG_I2P flags in their
> > adv7180_chip_info structures.
> > Is there a reason that you've restricted this to just the two chips?
>
> I only have an ADV7280-M for testing. Thus, I restricted it to the chip
> that I could test and the variant that's the same except for the CSI.
>
> >
> > adv7281 - CSI
> > adv7281-m - CSI
> > adv7281-ma - CSI
> > adv7282 - VPP
> > adv7282-m - VPP and CSI
> >
> > The adv7180 and adv7182 families are the only two which only have the
> > single I2C address.
>
> Are you suggesting to extend the binding to be able to specify the
> addresses for the CSI and VPP for any of these chips?

Seeing as the driver change implements that, IMHO the binding ought to match.
I can test adv7282-m for you if you want. I'd be very surprised if it
didn't work with the entire family, and as setting the additional
addresses is optional it's unlikely to cause an issue anyway.

  Dave

> Michael
>
> >
> > Thanks.
> >   Dave
> >
> > > Allow to specify the addresses of the VPP and CSI in the device tree to
> > > solve address conflicts on a board level.
> > >
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > > index dee8ce7cb7ba..4bbdc812442b 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > > @@ -138,6 +138,31 @@ allOf:
> > >        required:
> > >          - ports
> > >
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - adi,adv7280
> > > +              - adi,adv7280-m
> > > +    then:
> > > +      properties:
> > > +        reg:
> > > +          minItems: 1
> > > +          maxItems: 3
> > > +          description:
> > > +            The ADV7280 and ADV7280-M have up to three register maps, which can be
> > > +            accessed via the I2C port. The main register map, the VPP register map,
> > > +            and the CSI register map. The main register map is mandatory. The other
> > > +            register maps are optional and the default is used unless specified.
> > > +
> > > +        reg-names:
> > > +          minItems: 1
> > > +          items:
> > > +            - const: main
> > > +            - enum: [ csi, vpp ]
> > > +            - enum: [ csi, vpp ]
> > > +
> > >  examples:
> > >    - |
> > >      i2c {
> > > @@ -187,3 +212,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
> > >
> > >
> >

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

[Krzysztof Kozlowski]

On Tue, Nov 11, 2025 at 03:36:14PM +0100, Michael Tretter wrote:
> The ADV7280 and ADV7280-M have up to three register maps for the VPP and
> CSI. The address of these register maps may be programmed via registers
> in the main register map.
> 
> Allow to specify the addresses of the VPP and CSI in the device tree to
> solve address conflicts on a board level.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> index dee8ce7cb7ba..4bbdc812442b 100644
> --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> @@ -138,6 +138,31 @@ allOf:
>        required:
>          - ports
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,adv7280
> +              - adi,adv7280-m
> +    then:
> +      properties:
> +        reg:
> +          minItems: 1
> +          maxItems: 3

This does not match top-level.

> +          description:
> +            The ADV7280 and ADV7280-M have up to three register maps, which can be
> +            accessed via the I2C port. The main register map, the VPP register map,
> +            and the CSI register map. The main register map is mandatory. The other
> +            register maps are optional and the default is used unless specified.
> +
> +        reg-names:
> +          minItems: 1
> +          items:
> +            - const: main
> +            - enum: [ csi, vpp ]
> +            - enum: [ csi, vpp ]

List must not be flexible. What does it mean "optional"? The device has
them always or you disable them via some sort of efuse/OTP?

Best regards,
Krzysztof

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

[Michael Tretter]

On Wed, 12 Nov 2025 09:05:14 +0100, Krzysztof Kozlowski wrote:
> On Tue, Nov 11, 2025 at 03:36:14PM +0100, Michael Tretter wrote:
> > The ADV7280 and ADV7280-M have up to three register maps for the VPP and
> > CSI. The address of these register maps may be programmed via registers
> > in the main register map.
> > 
> > Allow to specify the addresses of the VPP and CSI in the device tree to
> > solve address conflicts on a board level.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > index dee8ce7cb7ba..4bbdc812442b 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> > @@ -138,6 +138,31 @@ allOf:
> >        required:
> >          - ports
> >  
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,adv7280
> > +              - adi,adv7280-m
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 1
> > +          maxItems: 3
> 
> This does not match top-level.

Ack. If I understand the error message by dt_bindings_check correctly, I
have to move the default from the top-level to the else branch of this
conditional expression. Right?

> 
> > +          description:
> > +            The ADV7280 and ADV7280-M have up to three register maps, which can be
> > +            accessed via the I2C port. The main register map, the VPP register map,
> > +            and the CSI register map. The main register map is mandatory. The other
> > +            register maps are optional and the default is used unless specified.
> > +
> > +        reg-names:
> > +          minItems: 1
> > +          items:
> > +            - const: main
> > +            - enum: [ csi, vpp ]
> > +            - enum: [ csi, vpp ]
> 
> List must not be flexible. What does it mean "optional"? The device has
> them always or you disable them via some sort of efuse/OTP?

The flexible list is inspired by the binding for adi,adv748x.yaml. It
also has a main device and several optional slave devices, which remain
at the default address if not specified in the device tree.

The optional register maps depend on the variant of the chip, and the
driver may or may not use them. In case of the adv7280-m, the driver
currently always programs a hard-coded default address into the
hardware. Making the csi and vpp register map addresses as optional
would allow to preserve the current driver behavior in these cases.

I think this is necessary for backward compatibility.

Michael

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

[Krzysztof Kozlowski]

On 12/11/2025 10:39, Michael Tretter wrote:
> On Wed, 12 Nov 2025 09:05:14 +0100, Krzysztof Kozlowski wrote:
>> On Tue, Nov 11, 2025 at 03:36:14PM +0100, Michael Tretter wrote:
>>> The ADV7280 and ADV7280-M have up to three register maps for the VPP and
>>> CSI. The address of these register maps may be programmed via registers
>>> in the main register map.
>>>
>>> Allow to specify the addresses of the VPP and CSI in the device tree to
>>> solve address conflicts on a board level.
>>>
>>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>>> ---
>>>  .../devicetree/bindings/media/i2c/adi,adv7180.yaml | 44 ++++++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>> index dee8ce7cb7ba..4bbdc812442b 100644
>>> --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>> +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
>>> @@ -138,6 +138,31 @@ allOf:
>>>        required:
>>>          - ports
>>>  
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - adi,adv7280
>>> +              - adi,adv7280-m
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 1
>>> +          maxItems: 3
>>
>> This does not match top-level.
> 
> Ack. If I understand the error message by dt_bindings_check correctly, I
> have to move the default from the top-level to the else branch of this
> conditional expression. Right?

Top-level should have widest constraints and in if:then: please narrow
them per variant.

> 
>>
>>> +          description:
>>> +            The ADV7280 and ADV7280-M have up to three register maps, which can be
>>> +            accessed via the I2C port. The main register map, the VPP register map,
>>> +            and the CSI register map. The main register map is mandatory. The other
>>> +            register maps are optional and the default is used unless specified.

BTW, here should be list with description, instead of min/maxItems and
above.

>>> +
>>> +        reg-names:
>>> +          minItems: 1
>>> +          items:
>>> +            - const: main
>>> +            - enum: [ csi, vpp ]
>>> +            - enum: [ csi, vpp ]
>>
>> List must not be flexible. What does it mean "optional"? The device has
>> them always or you disable them via some sort of efuse/OTP?
> 
> The flexible list is inspired by the binding for adi,adv748x.yaml. It
> also has a main device and several optional slave devices, which remain
> at the default address if not specified in the device tree.
> 
> The optional register maps depend on the variant of the chip, and the
> driver may or may not use them. In case of the adv7280-m, the driver
> currently always programs a hard-coded default address into the
> hardware. Making the csi and vpp register map addresses as optional
> would allow to preserve the current driver behavior in these cases.

You can keep them optional for backwards compatibility and this should
be explained in the commit msg. However description must speak about
hardware and your last sentence felt as not speaking that - "register
maps are optional and the default is used unless specified."

If the device always has three register maps, then the three register
maps shall be always defined. See also writing bindings doc.

Best regards,
Krzysztof

[PATCH 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>
---
 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 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>
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 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

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

[Niklas Söderlund]

Hi Michael and Thorsten,

Thanks for your work.

On 2025-11-11 15:36:16 +0100, Michael Tretter wrote:
> 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>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  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
> 

-- 
Kind Regards,
Niklas Söderlund

[PATCH 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 at the full refresh rate of 50 FPS (PAL) or 60 FPS
(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>
---
 drivers/media/i2c/adv7180.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index d289cbc2eefd..3a5a0818bc5f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -491,6 +491,7 @@ static int adv7180_get_frame_interval(struct v4l2_subdev *sd,
 				      struct v4l2_subdev_frame_interval *fi)
 {
 	struct adv7180_state *state = to_state(sd);
+	bool progressive;
 
 	/*
 	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
@@ -499,12 +500,14 @@ static int adv7180_get_frame_interval(struct v4l2_subdev *sd,
 	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
 		return -EINVAL;
 
+	progressive = (state->field == V4L2_FIELD_NONE);
+
 	if (state->curr_norm & V4L2_STD_525_60) {
 		fi->interval.numerator = 1001;
-		fi->interval.denominator = 30000;
+		fi->interval.denominator = progressive ? 60000 : 30000;
 	} else {
 		fi->interval.numerator = 1;
-		fi->interval.denominator = 25;
+		fi->interval.denominator = progressive ? 50 : 25;
 	}
 
 	return 0;

-- 
2.47.3

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

[Niklas Söderlund]

Hi Michael and Thorsten,

Thanks for your patch.

On 2025-11-11 15:36:17 +0100, Michael Tretter wrote:
> 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 at the full refresh rate of 50 FPS (PAL) or 60 FPS
> (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>
> ---
>  drivers/media/i2c/adv7180.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
> index d289cbc2eefd..3a5a0818bc5f 100644
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c
> @@ -491,6 +491,7 @@ static int adv7180_get_frame_interval(struct v4l2_subdev *sd,
>  				      struct v4l2_subdev_frame_interval *fi)
>  {
>  	struct adv7180_state *state = to_state(sd);
> +	bool progressive;
>  
>  	/*
>  	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> @@ -499,12 +500,14 @@ static int adv7180_get_frame_interval(struct v4l2_subdev *sd,
>  	if (fi->which != V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return -EINVAL;
>  
> +	progressive = (state->field == V4L2_FIELD_NONE);

No need for ( ) here.

> +
>  	if (state->curr_norm & V4L2_STD_525_60) {
>  		fi->interval.numerator = 1001;
> -		fi->interval.denominator = 30000;
> +		fi->interval.denominator = progressive ? 60000 : 30000;
>  	} else {
>  		fi->interval.numerator = 1;
> -		fi->interval.denominator = 25;
> +		fi->interval.denominator = progressive ? 50 : 25;
>  	}

I wonder if it make sens to instead of the above do?

    /* Interlaced video have half the frame interval period. */
    if (state->field == V4L2_FIELD_NONE)
        fi->interval.denominator *= 2;


>  
>  	return 0;
> 
> -- 
> 2.47.3
> 

-- 
Kind Regards,
Niklas Söderlund