On 17/05/2023 21:38, Krzysztof Kozlowski wrote: > On 17/05/2023 16:52, Alexandre Bailon wrote: >> This adds the device tree bindings for the APU DRM driver. >> >> Signed-off-by: Alexandre Bailon <abailon@xxxxxxxxxxxx> >> Reviewed-by: Julien Stephan <jstephan@xxxxxxxxxxxx> > > There are so many errors in this patch... that for sure it was not > tested. Reduced review, except what was already said: > >> --- >> .../devicetree/bindings/gpu/mtk,apu-drm.yaml | 38 +++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> >> diff --git a/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> new file mode 100644 >> index 000000000000..6f432d3ea478 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml >> @@ -0,0 +1,38 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/gpu/mediatek,apu-drm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: AI Processor Unit DRM >> + >> +properties: >> + compatible: >> + const: mediatek,apu-drm > > drm is not hardware. Drop everywhere or explain the acronym. If you > explain it like Linux explains, then: drm is not hardware. > >> + >> + remoteproc: >> + maxItems: 2 >> + description: >> + Handle to remoteproc devices controlling the APU > > Missing type/ref. Does not look like generic property, so missing vendor > prefix. > >> + >> + iova: >> + maxItems: 1 >> + description: >> + Address and size of virtual memory that could used by the APU > > So it is a reg? > >> + >> +required: >> + - compatible >> + - remoteproc >> + - iova >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + apu@0 { > > Where is reg? @0 says you have it... > >> + compatible = "mediatek,apu-drm"; >> + remoteproc = <&vpu0>, <&vpu1>; >> + iova = <0 0x60000000 0 0x10000000>; > > Why would you store virtual address, not real, in DT? Let's say you have > some randomization like KASLR. How is it going to work? Drop, it is not > hardware property. Actually RANDOMIZE_BASE. KASLR randomizes the physical. Best regards, Krzysztof
