Re: [PATCH v2 1/2] dt: binding: Add Qualcomm WCNSS control binding

[Rob Herring]

+dt list

On Thu, May 12, 2016 at 6:17 PM, Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> wrote:
> This binding describes the control interface for the Qualcomm WCNSS.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> ---
>
> Changes since v1:
> - Introduce reference to wcnss block node for register block definition
> - Use wcnss block node compatible for hw version detection (riva vs pronto)
> - Clean up compatible name for bt node
>
>  .../devicetree/bindings/soc/qcom/qcom,wcnss.txt    | 115 +++++++++++++++++++++
>  1 file changed, 115 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> new file mode 100644
> index 000000000000..5413098a1e1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,wcnss.txt
> @@ -0,0 +1,115 @@
> +Qualcomm WCNSS Binding
> +
> +This binding describes the Qualcomm WCNSS hardware. It consists of control
> +block and a BT, WiFi and FM radio block, all useing SMD as command channels.

s/useing/using/

> +
> +- compatible:
> +       Usage: required
> +       Value type: <string>
> +       Definition: must be: "qcom,wcnss",
> +
> +- qcom,smd-channel:
> +       Usage: required
> +       Value type: <string>
> +       Definition: standard SMD property specifying the SMD channel used for
> +                   communication with the WiFi firmware

Is the string value defined somewhere?

> +
> +- qcom,mmio:
> +       Usage: required
> +       Value type: <prop-encoded-array>
> +       Definition: reference to a node specifying the wcnss "ccu" and "dxe"
> +                   register blocks. The node must be compatible with one of
> +                   the following:
> +                   "qcom,riva",
> +                   "qcom,pronto"
> +
> += SUBNODES
> +The subnodes of the wcnss node are optional and describe the individual blocks in
> +the WCNSS.
> +
> +== Bluetooth
> +The following properties are defined to the bluetooth node:
> +
> +- compatible:
> +       Usage: required
> +       Value type: <string>
> +       Definition: must be:
> +                   "qcom,wcnss-bt"
> +
> +== WiFi
> +The following properties are defined to the WiFi node:
> +
> +- compatible:
> +       Usage: required
> +       Value type: <string>
> +       Definition: must be one of:
> +                   "qcom,wcnss-wlan",
> +
> +- interrupts:
> +       Usage: required
> +       Value type: <prop-encoded-array>
> +       Definition: should specify the "rx" and "tx" interrupts
> +
> +- interrupt-names:
> +       Usage: required
> +       Value type: <stringlist>
> +       Definition: must contain "rx" and "tx"
> +
> +- qcom,state:

This is kind of generic.

> +       Usage: required
> +       Value type: <prop-encoded-array>
> +       Definition: should reference the tx-enable and tx-rings-empty SMEM states
> +
> +- qcom,state-names:
> +       Usage: required
> +       Value type: <stringlist>
> +       Definition: must contain "tx-enable" and "tx-rings-empty"
> +
> += EXAMPLE
> +The following example represents a SMD node, with one edge representing the
> +"pronto" subsystem, with the wcnss device and its wcn3680 BT and WiFi blocks
> +described; as found on the 8974 platform.

So somewhere in here I'd expect to see 8974 in a compatible string.

> +
> +smd {
> +       compatible = "qcom,smd";
> +
> +       pronto {
> +               interrupts = <0 142 1>;
> +
> +               qcom,ipc = <&apcs 8 17>;
> +               qcom,smd-edge = <6>;
> +
> +               wcnss {
> +                       compatible = "qcom,wcnss";
> +                       qcom,smd-channels = "WCNSS_CTRL";
> +
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +
> +                       qcom,wcnss = <&pronto>;

Not documented and poorly named. The property name should reflect what
this is for or points too.

> +
> +                       bt {
> +                               compatible = "qcom,wcnss-bt";
> +                       };
> +
> +                       wlan {
> +                               compatible = "qcom,wcnss-wlan";
> +
> +                               interrupts = <0 145 0>, <0 146 0>;
> +                               interrupt-names = "tx", "rx";
> +
> +                               qcom,state = <&apps_smsm 10>, <&apps_smsm 9>;
> +                               qcom,state-names = "tx-enable", "tx-rings-empty";
> +                       };
> +               };
> +       };
> +};
> +
> +soc {
> +       pronto: pronto {

2 pronto nodes? That's confusing...

> +               compatible = "qcom,pronto";
> +
> +               reg = <0xfb204000 0x2000>, <0xfb202000 0x1000>, <0xfb21b000 0x3000>;
> +               reg-names = "ccu", "dxe", "pmu";
> +       };
> +};
> --
> 2.5.0
>