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

[Ben Whitten <Ben.Whitten@lairdtech.com>]

Hi Andreas,

> Am 08.01.19 um 09:41 schrieb Ben Whitten:
> > 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 <ben.whitten@gmail.com>
> > ---
> >  .../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

> >
> > diff --git a/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> b/Documentation/devicetree/bindings/lora/semtech,sx130x.yaml
> > 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.
> 
> > +  demodulating LoRa signals on 8 channels simultaneously.
> > +
> > +  It is typically paired with two sx125x IQ radios controlled over an
> 
> Ditto, SX125x
> 
> > +  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.

> 
> > +
> > +  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.

> 
> > +
> > +  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.

> > +
> > +  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.

> 
> > +
> > +  radio-spi:
> > +    description: The concentrator has two radios connected which are
> contained
> > +      within the following node.
> 
> "can have"
> 
> > +
> > +    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?

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reset-gpios
> 
> Must be optional.
> 
> > +  - radio-spi
> 
> Should be optional. (Driver needs it today, but that's another problem.)
> 
> > +
> > +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