Re: [PATCH 6/8] media: dt-bindings: Add bindings for Raspberry Pi PiSP Back End

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 09/02/2024 17:48, Jacopo Mondi wrote:
> Add bindings for the Raspberry Pi PiSP Back End memory-to-memory image
> signal processor.
> 
> Datasheet:
> https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> 


> +---
> +$id: http://devicetree.org/schemas/media/raspberrypi,pispbe.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Raspberry Pi PiSP Image Signal Processor (ISP) Back End
> +
> +maintainers:
> +  - Dave Stevenson <dave.stevenson@raspberrypi.com>
> +  - David Plowman <david.plowman@raspberrypi.com>
> +  - Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> +  - Naushir Patuck <naush@raspberrypi.com>
> +  - Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>

I assume all folks are fine being here?

> +
> +description: |
> +  The Raspberry Pi PiSP Image Signal Processor (ISP) Back End is an image
> +  processor that fetches images in Bayer or Grayscale format from DRAM memory
> +  in tiles and produces images consumable by applications.
> +
> +  The full ISP documentation is available at:
> +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> +
> +properties:
> +  compatible:
> +    const: raspberrypi,pispbe

Nothing more specific? No model name, no version? It's quite generic
compatible which in general should not be allowed. I would assume that
at least version of Pi could denote some sort of a model... unless
version is detectable?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: isp_be

You can skip clock-names if they have only one entry and it matches
block name. Quite useless.

> +
> +  iommus:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    rpi1 {

soc {

> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        isp: pisp_be@880000  {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
so: isp@

and drop unused label

> +             compatible = "raspberrypi,pispbe";
ds,
Krzysztof