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

[Krzysztof Kozlowski]

On 09/06/2024 20:21, michael.nemanov@xxxxxx wrote:
> From: Michael Nemanov <michael.nemanov@xxxxxx>
> 

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