Re: [PATCH v2 17/17] dt-bindings: net: wireless: cc33xx: Add ti,cc33xx.yaml
flat view
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: 2024-06-10 08:16:27
Also in:
linux-devicetree, linux-wireless, lkml
On 09/06/2024 20:21, michael.nemanov@ti.com wrote:
From: Michael Nemanov <redacted>
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.
quoted hunk ↗ jump to hunk
--- .../bindings/net/wireless/ti,cc33xx.yaml | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yamldiff --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.
quoted hunk ↗ jump to hunk
+ 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)
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+ maxItems: 1 + +required: + - compatible + - reg + +unevaluatedProperties: false
additionalProperties instead... or this is somehow incomplete (properties, $ref?).
quoted hunk ↗ jump to hunk
+ +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + + // SDIO example: + mmc3 {
mmc
quoted hunk ↗ jump to hunk
+ vmmc-supply = <&wlan_en_reg>; + bus-width = <4>; + cap-power-off-card; + keep-power-in-suspend;
Drop all these, not relevant.
quoted hunk ↗ jump to hunk
+ + #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
quoted hunk ↗ jump to hunk
+ compatible = "ti,cc3300"; + reg = <2>; + interrupts = <19 IRQ_TYPE_EDGE_RISING>; + }; + };
Best regards, Krzysztof