RE: [PATCH lora-next 1/4] dt-bindings: lora: sx130x: add basic documentation

Hi Andreas,

Am 08.01.19 um 09:41 schrieb Ben Whitten:

quoted

Add basic documentation in YAML format for the sx130x series concentrators
from Semtech.
Required is; the location on the SPI bus, the reset gpio and the node for
downstream IQ radios, typically sx125x.

Signed-off-by: Ben Whitten <redacted>
---
 .../bindings/lora/semtech,sx130x.yaml         | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/lora/semtech,sx130x.yaml

Patch 3/4 moves this to net/lora/, which I think is more appropriate.

Agreed, I think it was a change merged into the wrong commits by mistake

quoted

diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml

b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml

quoted

new file mode 100644
index 000000000000..ad263bc4e60d

--- /dev/null
+++ b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml

@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/lora/semtech,sx130x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Semtech LoRa concentrator
+
+maintainers:
+  - Andreas Färber <afaerber@suse.de>
+  - Ben Whitten <ben.whitten@gmail.com>
+
+description: |
+  Semtech LoRa concentrator sx130x digital baseband chip is capable of

SX130x or SX1301/SX1308, to distinguish from driver name sx130x and to
avoid ambiguities of which x is a wildcard.

quoted

+  demodulating LoRa signals on 8 channels simultaneously.
+
+  It is typically paired with two sx125x IQ radios controlled over an

Ditto, SX125x

quoted

+  SPI directly from the concentrator.
+
+  The concentrator itself it controlled over SPI.
+
+properties:
+  compatible:
+    items:
+      - enum:
+        - semtech,sx1301
+        - semtech,sx1308

We should only list both if we expect differences between the two
models. Otherwise SX1308 can reuse the SX1301 compatible. If we want to
mark it up just in case then rearranging the above to be a sequence of
"semtech,sx1308", "semtech,sx1301" would be an alternative.

It was my understanding that we should name each device that is compatible,
avoiding wildcard 'x' in compatible names. This allows the device tree to be
more accurate to the hardware that it is describing.

I do not expect there to be much difference, but there may be some that I 
am unaware of.

Not sure I follow here, do you wish for the order to be flipped if we do want
to state every device? I see that example-schema does indeed have entries
in reverse.

quoted

+
+  reg:
+    maxItems: 1
+    description: The chip select on the SPI bus.

Is this mandatory now or not with maxItems?

min/maxItems is implied if you have a list but for our chipselect we have no
list. I followed child-node-example.yaml in yaml-bindings and
trivial-devices.yaml in being explicit and stating it be one element and making
reg required.

quoted

+
+  reset-gpios:
+    maxItems: 1
+    description: A connection of the reset gpio line.

This needs to be optional, which I think the maxItems syntax says unlike
the commit message?
On an mPCIe card you won't have such a GPIO, for instance. We do a Soft
Reset, so it's not functionally mandatory.

I'll drop this from required and from the commit message.

quoted

+
+  spi-max-frequency:
+    maximum: 10000000
+    default: 8000000
+    description: The frequency of the SPI communication to the concentrator,
+      in Hz. Maximum SPI frequency is 10MHz although 8MHz is typically used
+      on a number of cards.

Do we really need to describe this here? It should be covered in the
common SPI bindings, and only applies to SPI bus, not USB picoGW.

True, I'll drop this.

quoted

+
+  radio-spi:
+    description: The concentrator has two radios connected which are

contained

quoted

+      within the following node.

"can have"

quoted

+
+    properties:
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
+    required:
+      - '#address-cells'
+      - '#size-cells'

I'm pretty sure that Rob would like to have a compatible here, even if
unneeded in our Linux driver?

BTW if someone has better naming suggestions than "radio-spi"... I just
wanted to avoid having it in the main node directly, in case we need
other sub-nodes, too.

Just like ahb and apb it makes sense to separate the bus from the device
An alternative could be "transceiver-bus" or "radio-bus" as the fact its
spi is masked away anyway.
What would it's compatible string be, match the node name?
Would the sx130x_radio bus type match against it?

quoted

+
+required:
+  - compatible
+  - reg
+  - reset-gpios

Must be optional.

quoted

+  - radio-spi

Should be optional. (Driver needs it today, but that's another problem.)

quoted

+
+examples:
+  - |
+    concentrator0: lora@0 {
+      compatible = "semtech,sx1301";
+      reg = <0>;
+      reset-gpios = <&pioB 27 GPIO_ACTIVE_HIGH>;
+      spi-max-frequency = <8000000>;
+
+      radio-spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        radio0: lora@0 {
+          compatible = "semtech,sx1257";
+          reg = <0>;
+        };
+
+        radio1: lora@1 {
+          compatible = "semtech,sx1257";
+          reg = <1>;
+        };
+      };
+    };

Thanks for looking into this,

Andreas

Thanks,
Ben Whitten