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@0/bus@30800000/mmc@30b40000/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@xxxxxxx> > --- > .../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@xxxxxxx> I wouldn't mind adding: - Brian Norris <briannorris@xxxxxxxxxxxx> > + > +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>; > + }; > + }; > +
