netdev - Re: [PATCH v2 17/17] dt-bindings: net: wireless: cc33xx: Add ti,cc33xx.yaml

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 10 Jun 2024 10:16:20 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: michael.nemanov@...com, Sabeeh Khan <sabeeh-khan@...com>,
 Kalle Valo <kvalo@...nel.org>, Johannes Berg <johannes.berg@...el.com>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 17/17] dt-bindings: net: wireless: cc33xx: Add
 ti,cc33xx.yaml

On 09/06/2024 20:21, michael.nemanov@...com wrote:
> From: Michael Nemanov <michael.nemanov@...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@...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