Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/02/2025 10:26, Sasha Finkelstein via B4 Relay wrote:
> +  reg-names:
> +    items:
> +      - const: coproc
> +      - const: mbox
> +      - const: gpio
> +      - const: mbox2
> +
> +  iommus:
> +    description: All 3 must be kept in sync
> +    minItems: 3


Drop minItems

> +    maxItems: 3
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    minItems: 1
> +    maxItems: 20
> +    description: All necessary power domains. Driver will enable them in order
> +
> +  memory-region:
> +    maxItems: 1
> +
> +  apple,dart-vm-size:
> +    description: Supported device memory range
> +    $ref: /schemas/types.yaml#/definitions/uint64


That's deducible from comaptible.

> +
> +  apple,platform-id:
> +    description: Platform id for firmware
> +    $ref: /schemas/types.yaml#/definitions/uint32


No, use firmware-name.

> +
> +  apple,temporal-filter:
> +    description: Whether temporal filter should be enabled in firmware
> +    $ref: /schemas/types.yaml#/definitions/uint32

And why is this not enabled always? Why this is board specific?


You miss here ports or port. ISP usually gets signal from some camera or
other block.


> +
> +  sensor-presets:
> +    additionalProperties: false
> +
> +    patternProperties:
> +      '^preset[0-9]+$':
> +        type: object
> +
> +        additionalProperties: false
> +
> +        properties:
> +          apple,config-index:
> +            description: Firmware config index
> +            $ref: /schemas/types.yaml#/definitions/uint32


No duplicated indices. You have reg for this, assuming this is index.

> +
> +          apple,input-size:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            minItems: 2
> +            maxItems: 2
> +            description: Raw sensor size
> +
> +          apple,output-size:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            minItems: 2
> +            maxItems: 2
> +            description: Cropped and scaled image size
> +
> +          apple,crop:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            minItems: 4
> +            maxItems: 4
> +            description: Area to crop


All these do not look like hardware properties but rather configuration
of sensor which should be done runtime by OS, not by DT.

Best regards,
Krzysztof