Message-ID: <Zr5GnhNm0lSQNBwa@google.com>
Date: Thu, 15 Aug 2024 11:19:10 -0700
From: Brian Norris <briannorris@...omium.org>
To: Frank Li <Frank.Li@....com>
Cc: Kalle Valo <kvalo@...nel.org>, "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>,
	"open list:NETWORKING DRIVERS (WIRELESS)" <linux-wireless@...r.kernel.org>,
	"open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>, imx@...ts.linux.dev
Subject: Re: [PATCH v1 1/1] dt-bindings: net: wireless: convert
 marvel-8xxx.txt to yaml format

Hi Frank,

On Mon, Aug 12, 2024 at 03:44:40PM -0400, Frank Li wrote:
> Convert binding doc marvel-8xxx.txt to yaml format.
> Additional change:
> - Remove marvell,caldata_00_txpwrlimit_2g_cfg_set in example.
> - Remove mmc related property in example.
> 
> Fix below warning:
> arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dtb: /soc@...us@...00000/mmc@...40000/wifi@1:
> failed to match any schema with compatible: ['marvell,sd8997']

Can you make sure to run through `make dtbs_check` and handle any new
issues? For one, I think you might want to include 'wakeup-source'?

arch/arm64/boot/dts/rockchip/rk3399-gru-kevin.dtb: wifi@0,0: 'wakeup-source' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/net/wireless/marvell,8xxx.yaml#


> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
>  .../bindings/net/wireless/marvell,8xxx.yaml   | 96 +++++++++++++++++++
>  .../bindings/net/wireless/marvell-8xxx.txt    | 70 --------------
>  2 files changed, 96 insertions(+), 70 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/wireless/marvell,8xxx.yaml
>  delete mode 100644 Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell,8xxx.yaml b/Documentation/devicetree/bindings/net/wireless/marvell,8xxx.yaml
> new file mode 100644
> index 0000000000000..7b4927cdb7a01
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wireless/marvell,8xxx.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/wireless/marvell,8xxx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell 8787/8897/8978/8997 (sd8787/sd8897/sd8978/sd8997/pcie8997) SDIO/PCIE devices
> +
> +maintainers:
> +  - Frank Li <Frank.Li@....com>

I wouldn't mind adding:

  - Brian Norris <briannorris@...omium.org>

> +
> +description:
> +  This node provides properties for controlling the Marvell SDIO/PCIE wireless device.

Since we're essentially rewriting this doc, might as well tweak a few
things:
Please replace "controlling" with "describing". These bindings are for
hardware description, not for software control (even though they seem
like it sometimes and can be abused for that).

> +  The node is expected to be specified as a child node to the SDIO/PCIE controller that
> +  connects the device to the system.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - marvell,sd8787
> +      - marvell,sd8897
> +      - marvell,sd8978
> +      - marvell,sd8997
> +      - nxp,iw416
> +      - pci11ab,2b42
> +      - pci1b4b,2b42
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  marvell,caldata-txpwrlimit-2g:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: calibration data

Capitalize the first letter ("Calibration"). Same on all your
descriptions.

And maybe expand a little on what this means? Same on the other caldata
properties.

For example, "Calibration data for the 2GHz band."

> +    maxItems: 566

Non-critical question: are these numbers actually correct? The only
instance I see in the upstream tree is
arch/arm/boot/dts/rockchip/rk3288-veyron-jerry.dts, with 526 items. Yes,
that still fits in this "max", but I just wonder whether this is an
actually-correct specification, or an off-by-40 specification. Or, maybe
the structure varies a lot by chip or firmware, and this max just isn't
very meaningful.

Like I said, it's non-critical, so maybe we leave it as-is, if it
doesn't matter much.

> +  marvell,caldata-txpwrlimit-5g-sub0:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: calibration data

Possibly, "Calibration data for sub-band 0 in the 5GHz band."? And even
better if you can describe what sub-band 0 is (e.g., 5.xxx MHz - 5.yyy
MHz). But I'm not familiar.

> +    maxItems: 502
> +
> +  marvell,caldata-txpwrlimit-5g-sub1:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: calibration data
> +    maxItems: 688
> +
> +  marvell,caldata-txpwrlimit-5g-sub2:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: calibration data
> +    maxItems: 750
> +
> +  marvell,caldata-txpwrlimit-5g-sub3:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array
> +    description: calibration data
> +    maxItems: 502
> +
> +  marvell,wakeup-pin:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      a wakeup pin number of wifi chip which will be configured
> +      to firmware. Firmware will wakeup the host using this pin
> +      during suspend/resume.

Optional: this could use a bit of a rewrite to describe the hardware
instead of the software. For example, "Provides the pin number for the
wakeup pin from the device's point of view. The wakeup pin is used for
the device to wake the host system from sleep. This property is only
necessary if the wakeup pin is wired in a non-standard way, such that
the default pin assignments are invalid."

> +
> +  vmmc-supply:
> +    description: a phandle of a regulator, supplying VCC to the card

I believe this vmmc-supply property is actually misplaced. I don't see
any in-tree users, and OTOH all in-tree users specify this in the parent
(e.g., the MMC controller), where it's already properly documented.

> +  mmc-pwrseq:
> +    description:
> +      phandle to the MMC power sequence node. See "mmc-pwrseq-*"
> +      for documentation of MMC power sequence bindings.

Similarly, I think this is misplaced. See its introduction here,
commit e3fffc1f0b47 ("devicetree: document new marvell-8xxx and
pwrseq-sd8787 options"), but the controller docs
(Documentation/devicetree/bindings/mmc/mmc-controller.yaml) specify
these properties for the controller, not the endpoint/card. And
Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.yaml is vague,
but in practice, again, I think everyone uses this only in the
controller.

I'd consider dropping this and vmmc-supply, unless `make dtbs_check`
complains.

Brian

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    mmc {
> +         #address-cells = <1>;
> +         #size-cells = <0>;
> +
> +         wifi@1 {
> +             compatible = "marvell,sd8897";
> +             reg = <1>;
> +             interrupt-parent = <&pio>;
> +             interrupts = <38 IRQ_TYPE_LEVEL_LOW>;
> +             marvell,wakeup-pin = <3>;
> +        };
> +    };
> +

Powered by blists - more mailing lists