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

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

Hi Ben,

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.

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

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

Is this mandatory now or not with maxItems?

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

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

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

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

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

On Tue, Jan 08, 2019 at 05:41:29PM +0900, Ben Whitten wrote:
> 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

A schema binding. Yay!

> 
> 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
> +  demodulating LoRa signals on 8 channels simultaneously.
> +
> +  It is typically paired with two sx125x IQ radios controlled over an
> +  SPI directly from the concentrator.
> +
> +  The concentrator itself it controlled over SPI.

s/it/is/

> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - semtech,sx1301
> +        - semtech,sx1308
> +
> +  reg:
> +    maxItems: 1
> +    description: The chip select on the SPI bus.
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: A connection of the reset gpio line.
> +
> +  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.
> +
> +  radio-spi:

child nodes should have 'type: object'

> +    description: The concentrator has two radios connected which are contained
> +      within the following node.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0

You need the grandchildren here too to define 'reg' should be 0-8. 
You'll need to use 'patternProperties' since there's a unit-address.

You could maybe get rid of the radio-spi node. You don't need it unless 
you wanted to add other types of child nodes.

> +
> +    required:
> +      - '#address-cells'
> +      - '#size-cells'
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - radio-spi
> +
> +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>;
> +        };
> +      };
> +    };
> -- 
> 2.17.1
>

On Mon, Jan 21, 2019 at 01:14:20PM -0600, Rob Herring wrote:
> On Tue, Jan 08, 2019 at 05:41:29PM +0900, Ben Whitten wrote:
> > 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
> 
> A schema binding. Yay!
> 
> > 
> > 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

Also, I'd like new bindings to be dual GPL-2.0 and BSD-2-Clause if you 
don't mind. This will make it easier to move the schema out of the 
kernel if/when we ever do that.

Rob

On Tue, Jan 08, 2019 at 05:41:29PM +0900, Ben Whitten wrote:
> 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
> 
> 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
> +  demodulating LoRa signals on 8 channels simultaneously.
> +
> +  It is typically paired with two sx125x IQ radios controlled over an
> +  SPI directly from the concentrator.
> +
> +  The concentrator itself it controlled over SPI.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +        - semtech,sx1301
> +        - semtech,sx1308
> +
> +  reg:
> +    maxItems: 1
> +    description: The chip select on the SPI bus.
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: A connection of the reset gpio line.
> +
> +  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.
> +
> +  radio-spi:
> +    description: The concentrator has two radios connected which are contained
> +      within the following node.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    required:
> +      - '#address-cells'
> +      - '#size-cells'
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - radio-spi
> +
> +examples:
> +  - |

One more comment. Make sure you "build" the schema with 
'dt_binding_check' target. That builds the examples.

It is also checked by a CI job and the status is added to patchwork[1]. 
(Though it didn't work correctly for this one and I just fixed it.)

> +    concentrator0: lora@0 {

This example doesn't build because there's an expectation of the top 
level nodes being MMIO if there's a reg prop. You just need to wrap this 
in a parent node representing a spi controller:

spi {
  #address-cells = <1>;
  #size-cells = <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>;
> +        };
> +      };
> +    };

[1] https://patchwork.ozlabs.org/patch/1021773/