| Message ID | 20240609182102.2950457-18-michael.nemanov@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: cc33xx: Add driver for new TI CC33xx wireless device family | expand |
On 09/06/2024 20:21, michael.nemanov@ti.com wrote: > From: Michael Nemanov <michael.nemanov@ti.com> > Missing commit msg (explain the hardware), missing SoB. Please run scripts/checkpatch.pl and fix reported warnings. Then please run `scripts/checkpatch.pl --strict` and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > --- > .../bindings/net/wireless/ti,cc33xx.yaml | 60 +++++++++++++++++++ > 1 file changed, 60 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml > > diff --git a/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml > new file mode 100644 > index 000000000000..08ab2ed93dba > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml > @@ -0,0 +1,60 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/wireless/ti,cc33xx.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Texas Instruments CC33xx Wireless LAN Controller > + > +maintainers: > + - Michael Nemanov <michael.nemanov@ti.com> > + > +description: > + These are dt entries for the IEEE 802.11ax chips CC33xx from Texas Instruments. Describe hardware not "dt entries, regardless of what "dt entries" are. > + Currently, these chips must be connected via SDIO. > + > +properties: > + compatible: > + enum: > + - ti,cc3300 > + - ti,cc3301 > + - ti,cc3350 > + - ti,cc3351 > + > + reg: > + description: > + For WLAN communication, <reg> must be set to 2. And for other cases it can be something else? What else could it be if not a WLAN controller (description says WLAN) > + maxItems: 1 > + > + interrupts: > + description: The interrupt line. Can be IRQ_TYPE_EDGE_RISING or IRQ_TYPE_LEVEL_HIGH. > + When SDIO is used, the "in-band" interrupt provided by the SDIO bus is used > + unless an interrupt is defined in the Device Tree. Does not look wrapped at 80 (see coding style). I don't understand the comment. You describe SDIO interface here, so how it could be conditional "when SDIO is used"? What is the other case? This binding has so many weird statements. Describe *fully* hardware, not your drivers. > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +unevaluatedProperties: false additionalProperties instead... or this is somehow incomplete (properties, $ref?). > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + > + // SDIO example: > + mmc3 { mmc > + vmmc-supply = <&wlan_en_reg>; > + bus-width = <4>; > + cap-power-off-card; > + keep-power-in-suspend; Drop all these, not relevant. > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + cc33xx: cc33xx@0 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Drop unused label > + compatible = "ti,cc3300"; > + reg = <2>; > + interrupts = <19 IRQ_TYPE_EDGE_RISING>; > + }; > + }; Best regards, Krzysztof
On 10/06/2024 10:16, Krzysztof Kozlowski wrote: > On 09/06/2024 20:21, michael.nemanov@ti.com wrote: >> From: Michael Nemanov <michael.nemanov@ti.com> >> > > Missing commit msg (explain the hardware), missing SoB. And now I found that you actually received such feedback and you just ignored it. Go back to v1 and respond to each feedback you got. Best regards, Krzysztof
On 10/6/2024 11:17 AM, Krzysztof Kozlowski wrote: >> On 09/06/2024 20:21, michael.nemanov@ti.com wrote: >>> From: Michael Nemanov <michael.nemanov@ti.com> >>> >> >> Missing commit msg (explain the hardware), missing SoB. > > And now I found that you actually received such feedback and you just > ignored it. > > Go back to v1 and respond to each feedback you got. > > Best regards, > Krzysztof > Oversight on my part, will fix.
diff --git a/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml new file mode 100644 index 000000000000..08ab2ed93dba --- /dev/null +++ b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml @@ -0,0 +1,60 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/wireless/ti,cc33xx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Texas Instruments CC33xx Wireless LAN Controller + +maintainers: + - Michael Nemanov <michael.nemanov@ti.com> + +description: + These are dt entries for the IEEE 802.11ax chips CC33xx from Texas Instruments. + Currently, these chips must be connected via SDIO. + +properties: + compatible: + enum: + - ti,cc3300 + - ti,cc3301 + - ti,cc3350 + - ti,cc3351 + + reg: + description: + For WLAN communication, <reg> must be set to 2. + maxItems: 1 + + interrupts: + description: The interrupt line. Can be IRQ_TYPE_EDGE_RISING or IRQ_TYPE_LEVEL_HIGH. + When SDIO is used, the "in-band" interrupt provided by the SDIO bus is used + unless an interrupt is defined in the Device Tree. + maxItems: 1 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + + // SDIO example: + mmc3 { + vmmc-supply = <&wlan_en_reg>; + bus-width = <4>; + cap-power-off-card; + keep-power-in-suspend; + + #address-cells = <1>; + #size-cells = <0>; + + cc33xx: cc33xx@0 { + compatible = "ti,cc3300"; + reg = <2>; + interrupts = <19 IRQ_TYPE_EDGE_RISING>; + }; + };
From: Michael Nemanov <michael.nemanov@ti.com> --- .../bindings/net/wireless/ti,cc33xx.yaml | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml