Re: [PATCH v3 2/7] dt-bindings: display: add a device tree supplied boot logo

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: embedding raw image payloads in DT violates design principles
- [Low] dt-bindings: logo-rotation uses non-standard string enum instead of integer
--

commit 7c49827223c484baa96e766e2d19b7e319290579
Author: Max Pedraza <maximpedraza@gmail.com>

dt-bindings: display: add a device tree supplied boot logo

This commit introduces a device tree binding for providing a boot logo from
firmware, placing a "boot-logo-clut224" node under /chosen. It defines
properties for dimensions, color lookup table, image data, positioning, and
rotation.

> diff --git a/Documentation/devicetree/bindings/display/boot-logo-clut224.yaml b/Documentation/devicetree/bindings/display/boot-logo-clut224.yaml
> new file mode 100644
> index 0000000000000..9dc34717639e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/boot-logo-clut224.yaml
> @@ -0,0 +1,135 @@
[ ... ]
> +  clut:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description:
> +      Colour lookup table, as consecutive red, green and blue bytes per entry.
> +      The number of entries is derived from the property length and must not
> +      exceed 224.
> +    minItems: 3
> +    maxItems: 672
> +
> +  data:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description:
> +      One byte per pixel, left to right and top to bottom, each byte being an
> +      index into the colour lookup table. The property length must be equal to
> +      width multiplied by height.

[Severity: Medium]
Does this code introduce a regression by allowing large binary payloads to be
embedded directly in the device tree?

The Device Tree is intended for hardware description and minimal configuration,
not for delivering large binary files. Embedding an uncompressed image directly
via the 'data' property can permanently waste un-reclaimable kernel memory, as
the unflattened DT remains in memory indefinitely. It also risks exceeding
bootloader DTB size limits.

While follow-up work might provide a 'memory-region' alternative, the schema
still explicitly permits and supports embedding the binary payload directly via
this property.

[ ... ]
> +  logo-offset:
> +    $ref: /schemas/types.yaml#/definitions/int32-array
> +    description:
> +      X and Y displacement, in screen pixels, applied after the logo has been
> +      placed.
> +      Mostly useful together with a centred axis, to land the logo somewhere
> +      other than the middle of a panel whose usable area is not its centre.
> +    items:
> +      - description: X displacement
> +        minimum: -65535
> +        maximum: 65535
> +      - description: Y displacement
> +        minimum: -65535
> +        maximum: 65535
> +
> +  logo-rotation:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description:
> +      Rotation applied to the logo before it is drawn. It turns the logo and
> +      not the screen, so a quarter turn swaps how much room the logo takes
> +      up, but logo-position and logo-offset stay in screen pixels either
> +      way.
> +    enum: [none, cw, ccw, ud]
> +    default: none

[Severity: Low]
Is this non-standard string enum a regression from established device tree
conventions?

Using a custom string enum ('none', 'cw', 'ccw', 'ud') violates the established
convention for rotation. Existing bindings like panel-common.yaml and
video-interface-devices.yaml dictate using a standard integer 'rotation'
property with degree values (0, 90, 180, 270). This custom format fragments
parsing logic in drivers.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923201035.51007-1-maximpedraza@gmail.com?part=2