Re: [PATCH v2 1/3] dt-bindings: media: Add bindings for the RZ/V2H IVC block

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/06/2025 14:35, Daniel Scally wrote:
> The RZ/V2H SoC has a block called the Input Video Control block which
> feeds image data into the Image Signal Processor. Add dt bindings to
> describe the IVC.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Changes in v2:
> 
> 	- compatible matches filename

This is not what I asked.

> 	- Added power-domains
> 	- Aligned clock and reset entries on opening "<"
> 	- Removed status = "okay"; from example
> ---
>  .../bindings/media/renesas,rzv2h-ivc.yaml          | 103 +++++++++++++++++++++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,rzv2h-ivc.yaml b/Documentation/devicetree/bindings/media/renesas,rzv2h-ivc.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..ee0849eeeaba840cf43c81d69449c631ad1c6a6a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/renesas,rzv2h-ivc.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/renesas,rzv2h-ivc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/V2H Input Video Control Block
> +
> +maintainers:
> +  - Daniel Scally <dan.scally@ideasonboard.com>
> +
> +description:
> +  The IVC block is a module that takes video frames from memory and feeds them
> +  to the Image Signal Processor for processing.
> +
> +properties:
> +  compatible:
> +    const: renesas,rzv2h-ivc

No, you cannot use generic compatibles. See writing bindings (or
basically every review from DT maintainers).

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Input Video Control block register access clock
> +      - description: Video input data AXI bus clock
> +      - description: ISP system clock
> +
> +  clock-names:
> +    items:
> +      - const: pclk
> +      - const: vin_aclk

aclk

vin is redundant. This cannot be anything else than vin.

> +      - const: sclk
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    items:
> +      - description: Input Video Control block register access reset
> +      - description: Video input data AXI bus reset
> +      - description: ISP core reset
> +
> +  reset-names:
> +    items:
> +      - const: presetn
> +      - const: vin_aresetn
> +      - const: sresetn


s/reset// because it is redundant... which would point to pretty
pointless names. Look at your description - one is core or registers,
second is vin and other is isp-core... although then I wonder why ISP
input resets ISP core?



Best regards,
Krzysztof