| Message ID | 20231002182454.211165-2-ayushdevel1325@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | greybus: Add BeaglePlay Greybus Driver | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 02/10/2023 20:24, Ayush Singh wrote: > Add DT bindings for BeaglePlay CC1352 co-processor. > Thank you for your patch. There is something to discuss/improve. Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. For example: dt-bindings: net: > The BeaglePlay has a CC1352 co-processor. This co-processor is connected > to the main AM62 (running Linux) over UART. In the BeagleConnect > Technology, CC1352 is responsible for handling 6LoWPAN communication > with beagleconnect freedom nodes as well as their discovery > > This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus A nit: I pointed you to the documentation explaining not to use "This commit adds". It's v6 and the wording is back. Instead drop both sentences - they are pointless in this context. First one repeats previous text, second describes driver, but we do not talk here about drivers. > driver. > > Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> > --- > .../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ > MAINTAINERS | 6 +++ > 2 files changed, 54 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > > diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > new file mode 100644 > index 000000000000..57bc2c43e5b1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Texas Instruments Simplelink CC1352P7 wireless MCU > + > +description: > + The cc1352p7 mcu can be connected via SPI or UART. If over SPI, then the binding is incomplete. This is fine for now, I guess. > + > +maintainers: > + - Ayush Singh <ayushdevel1325@gmail.com> > + > +properties: > + compatible: > + const: ti,cc1352p7 > + > + clocks: > + maxItems: 2 Instead please list the items like: clocks: items: - description: High-Foo-bar - description: Low-Foo-bar > + > + reset-gpios: > + maxItems: 1 > + > + power-gpios: > + maxItems: 3 > + description: > + The device has three power rails that are exposed on external pins VDDS, > + VDDR and DCOUPL. No, power rails are not GPIOs. You need supplies, so: vdds-supply: true vddr-supply: true dcoupl-supply: true Look also at: Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml which explicitly allows only one powerdown GPIO. > + Best regards, Krzysztof
On 23:54-20231002, Ayush Singh wrote: > Add DT bindings for BeaglePlay CC1352 co-processor. > > The BeaglePlay has a CC1352 co-processor. This co-processor is connected > to the main AM62 (running Linux) over UART. In the BeagleConnect > Technology, CC1352 is responsible for handling 6LoWPAN communication > with beagleconnect freedom nodes as well as their discovery > > This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus > driver. > > Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> > --- > .../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ > MAINTAINERS | 6 +++ > 2 files changed, 54 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > > diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > new file mode 100644 > index 000000000000..57bc2c43e5b1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Texas Instruments Simplelink CC1352P7 wireless MCU > + > +description: > + The cc1352p7 mcu can be connected via SPI or UART. > + > +maintainers: > + - Ayush Singh <ayushdevel1325@gmail.com> > + > +properties: > + compatible: > + const: ti,cc1352p7 > + > + clocks: > + maxItems: 2 I would suggest clock-names and description for it. > + > + reset-gpios: > + maxItems: 1 > + > + power-gpios: > + maxItems: 3 > + description: > + The device has three power rails that are exposed on external pins VDDS, > + VDDR and DCOUPL. Shouldn't these be regulators? The power rails are input to the MCU, correct? The properties should be something like: vdds-supply vddr-supply dcoupl-supply ? (not sure what dcoupl is, but description should provide that info). the gpio controls for those can be modelled by regulator-gpio ? > + > + > +required: > + - compatible > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + serial { > + mcu { > + compatible = "ti,cc1352p7"; > + clocks = <&sclk_hf 0>, <&sclk_lf 25>; > + reset-gpios = <&pio 35 GPIO_ACTIVE_LOW>; > + power-gpios = <&pio 1 GPIO_ACTIVE_HIGH>, <&pio 2 GPIO_ACTIVE_HIGH>, <&pio 3 GPIO_ACTIVE_HIGH>; > + }; > + }; > diff --git a/MAINTAINERS b/MAINTAINERS > index 37b9626ee654..5467669d7963 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8969,6 +8969,12 @@ F: drivers/staging/greybus/sdio.c > F: drivers/staging/greybus/spi.c > F: drivers/staging/greybus/spilib.c > > +GREYBUS BEAGLEPLAY DRIVERS > +M: Ayush Singh <ayushdevel1325@gmail.com> > +L: greybus-dev@lists.linaro.org (moderated for non-subscribers) > +S: Maintained > +F: Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > + > GREYBUS SUBSYSTEM > M: Johan Hovold <johan@kernel.org> > M: Alex Elder <elder@kernel.org> > -- > 2.41.0 >
>> driver. >> >> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> >> --- >> .../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ >> MAINTAINERS | 6 +++ >> 2 files changed, 54 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml >> new file mode 100644 >> index 000000000000..57bc2c43e5b1 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml >> @@ -0,0 +1,48 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Texas Instruments Simplelink CC1352P7 wireless MCU >> + >> +description: >> + The cc1352p7 mcu can be connected via SPI or UART. > If over SPI, then the binding is incomplete. This is fine for now, I guess. > > Best regards, > Krzysztof Well, I added the line about SPI because the data sheet states that CC1352P7 can be connected over SPI or UART when used as wireless MCU. But yes, I do not have much knowledge about SPI itself, so the bindings might be incomplete for SPI usage. Should I remove it or leave it be? Sincerely, Ayush Singh
>> + >> + reset-gpios: >> + maxItems: 1 >> + >> + power-gpios: >> + maxItems: 3 >> + description: >> + The device has three power rails that are exposed on external pins VDDS, >> + VDDR and DCOUPL. > Shouldn't these be regulators? The power rails are input to the MCU, > correct? > The properties should be something like: > vdds-supply > vddr-supply > dcoupl-supply ? (not sure what dcoupl is, but description should provide > that info). > > the gpio controls for those can be modelled by regulator-gpio ? I picked up power lines from "CC13xx/CC26xx Hardware Configuration and PCB Design Considerations Application Report" present under "8.14 Network Processor" of CC1352P7 data sheet. But now looking closer, it doesn't seem like DCOUPL can be supplied externally for CC1352P7 and thus should probably be removed. Also, it seems like for CC1352P7, VDDR must always be supplied internally (The data sheet states: "Internal supply, must be powered from the internal DC/DC converter or the internal LDO"). Thus, it should be safe to remove VDDR as well. That means only VDDS needs to be present for power line. CC13xx/CC26xx Hardware Configuration and PCB Design Considerations Application Report: https://www.ti.com/lit/pdf/swra640 CC1352P7 Data sheet: https://www.ti.com/lit/gpn/CC1352P7 Sincerely, Ayush Singh
On 18:17-20231003, Ayush Singh wrote: > > > + > > > + reset-gpios: > > > + maxItems: 1 > > > + > > > + power-gpios: > > > + maxItems: 3 > > > + description: > > > + The device has three power rails that are exposed on external pins VDDS, > > > + VDDR and DCOUPL. > > Shouldn't these be regulators? The power rails are input to the MCU, > > correct? > > The properties should be something like: > > vdds-supply > > vddr-supply > > dcoupl-supply ? (not sure what dcoupl is, but description should provide > > that info). > > > > the gpio controls for those can be modelled by regulator-gpio ? > > I picked up power lines from "CC13xx/CC26xx Hardware Configuration and PCB > Design Considerations Application Report" present under "8.14 Network > Processor" of CC1352P7 data sheet. > > But now looking closer, it doesn't seem like DCOUPL can be supplied > externally for CC1352P7 and thus should probably be removed. > > Also, it seems like for CC1352P7, VDDR must always be supplied internally > (The data sheet states: "Internal supply, must be powered from the internal > DC/DC converter or the internal LDO"). Thus, it should be safe to remove > VDDR as well. From Figure 3-1. CC1312R 7x7 RF Part Schematic Overview (app report you point out below) Typical usage is vdds-supply supplying: VDDS (pin 44) VDDS2 (pin 13) VDDS3 (pin 22) VDDS_DCDC (pin 34) And DCDC_SW (pin 33) supplies vddr supplying: VDDR(pin 45) VDDR_RF (Pin 48) > > > That means only VDDS needs to be present for power line. I agree that that would be the typical supply model. So, just vdds-supply > > > CC13xx/CC26xx Hardware Configuration and PCB Design Considerations > Application Report: https://www.ti.com/lit/pdf/swra640 > > CC1352P7 Data sheet: https://www.ti.com/lit/gpn/CC1352P7
On 17:39-20231003, Ayush Singh wrote: > > > driver. > > > > > > Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> > > > --- > > > .../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ > > > MAINTAINERS | 6 +++ > > > 2 files changed, 54 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > > > new file mode 100644 > > > index 000000000000..57bc2c43e5b1 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml > > > @@ -0,0 +1,48 @@ > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Texas Instruments Simplelink CC1352P7 wireless MCU > > > + > > > +description: > > > + The cc1352p7 mcu can be connected via SPI or UART. > > If over SPI, then the binding is incomplete. This is fine for now, I guess. > > > > Best regards, > > Krzysztof > > Well, I added the line about SPI because the data sheet states that CC1352P7 > can be connected over SPI or UART when used as wireless MCU. But yes, I do > not have much knowledge about SPI itself, so the bindings might be > incomplete for SPI usage. Should I remove it or leave it be? I'd suggest to leave it for now, we can expand as there is a need.
diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml new file mode 100644 index 000000000000..57bc2c43e5b1 --- /dev/null +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Texas Instruments Simplelink CC1352P7 wireless MCU + +description: + The cc1352p7 mcu can be connected via SPI or UART. + +maintainers: + - Ayush Singh <ayushdevel1325@gmail.com> + +properties: + compatible: + const: ti,cc1352p7 + + clocks: + maxItems: 2 + + reset-gpios: + maxItems: 1 + + power-gpios: + maxItems: 3 + description: + The device has three power rails that are exposed on external pins VDDS, + VDDR and DCOUPL. + + +required: + - compatible + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + serial { + mcu { + compatible = "ti,cc1352p7"; + clocks = <&sclk_hf 0>, <&sclk_lf 25>; + reset-gpios = <&pio 35 GPIO_ACTIVE_LOW>; + power-gpios = <&pio 1 GPIO_ACTIVE_HIGH>, <&pio 2 GPIO_ACTIVE_HIGH>, <&pio 3 GPIO_ACTIVE_HIGH>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 37b9626ee654..5467669d7963 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8969,6 +8969,12 @@ F: drivers/staging/greybus/sdio.c F: drivers/staging/greybus/spi.c F: drivers/staging/greybus/spilib.c +GREYBUS BEAGLEPLAY DRIVERS +M: Ayush Singh <ayushdevel1325@gmail.com> +L: greybus-dev@lists.linaro.org (moderated for non-subscribers) +S: Maintained +F: Documentation/devicetree/bindings/net/ti,cc1352p7.yaml + GREYBUS SUBSYSTEM M: Johan Hovold <johan@kernel.org> M: Alex Elder <elder@kernel.org>
Add DT bindings for BeaglePlay CC1352 co-processor. The BeaglePlay has a CC1352 co-processor. This co-processor is connected to the main AM62 (running Linux) over UART. In the BeagleConnect Technology, CC1352 is responsible for handling 6LoWPAN communication with beagleconnect freedom nodes as well as their discovery This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus driver. Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com> --- .../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ MAINTAINERS | 6 +++ 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml