Hi Rob, Thanks for the comments! On 11/14/2016 07:04 PM, Rob Herring wrote: > On Mon, Nov 07, 2016 at 07:33:55PM +0200, Stanimir Varbanov wrote: >> Add binding document for Venus video encoder/decoder driver >> >> Cc: Rob Herring <robh+dt@xxxxxxxxxx> >> Cc: Mark Rutland <mark.rutland@xxxxxxx> >> Cc: devicetree@xxxxxxxxxxxxxxx >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx> >> --- >> .../devicetree/bindings/media/qcom,venus.txt | 98 ++++++++++++++++++++++ >> 1 file changed, 98 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt >> >> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt >> new file mode 100644 >> index 000000000000..b2af347fbce4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt >> @@ -0,0 +1,98 @@ >> +* Qualcomm Venus video encode/decode accelerator >> + >> +- compatible: >> + Usage: required >> + Value type: <stringlist> >> + Definition: Value should contain one of: >> + - "qcom,venus-msm8916" >> + - "qcom,venus-msm8996" > > The normal ordering is <vendor>,<soc>-<block> OK. > >> +- reg: >> + Usage: required >> + Value type: <prop-encoded-array> >> + Definition: Register ranges as listed in the reg-names property. >> +- reg-names: >> + Usage: required >> + Value type: <stringlist> >> + Definition: Should contain following entries: >> + - "venus" Venus register base >> +- reg-names: > > I'd prefer these grouped as one entry for reg-names. > >> + Usage: optional for msm8996 > > Why optional? The Venus hw block can work without internal video memory in which case just performance will be impacted. > >> + Value type: <stringlist> >> + Definition: Should contain following entries: >> + - "vmem" Video memory register base >> +- interrupts: >> + Usage: required >> + Value type: <prop-encoded-array> >> + Definition: Should contain interrupts as listed in the interrupt-names >> + property. >> +- interrupt-names: >> + Usage: required >> + Value type: <stringlist> >> + Definition: Should contain following entries: >> + - "venus" Venus interrupt line >> +- interrupt-names: >> + Usage: optional for msm8996 >> + Value type: <stringlist> >> + Definition: Should contain following entries: >> + - "vmem" Video memory interrupt line >> +- clocks: >> + Usage: required >> + Value type: <prop-encoded-array> >> + Definition: A List of phandle and clock specifier pairs as listed >> + in clock-names property. >> +- clock-names: >> + Usage: required >> + Value type: <stringlist> >> + Definition: Should contain the following entries: >> + - "core" Core video accelerator clock >> + - "iface" Video accelerator AHB clock >> + - "bus" Video accelerator AXI clock >> +- clock-names: >> + Usage: required for msm8996 > > Plus the 3 above? Yes, 'required' without 'for xxx' means that the clocks are required for all hw versions (SoCs) and msm8996 needs the extra clocks below. > >> + Value type: <stringlist> >> + Definition: Should contain the following entries: >> + - "subcore0" Subcore0 video accelerator clock >> + - "subcore1" Subcore1 video accelerator clock >> + - "mmssnoc_axi" Multimedia subsystem NOC AXI clock >> + - "mmss_mmagic_iface" Multimedia subsystem MMAGIC AHB clock >> + - "mmss_mmagic_mbus" Multimedia subsystem MMAGIC MAXI clock >> + - "mmagic_video_bus" MMAGIC video AXI clock >> + - "video_mbus" Video MAXI clock >> +- clock-names: >> + Usage: optional for msm8996 > > Clocks shouldn't be optional unless you failed to add in an initial > binding. These clocks are needed by video memory block which I noted as 'optional'. There is another way to model this video memory hw block i.e. by a child node of the venus node. Is that sounds better? <snip> -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
