| Message ID | 20241029172354.4027886-2-michael.nemanov@ti.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: cc33xx: Add driver for new TI CC33xx wireless device family | expand |
On 29/10/2024 18:23, Michael Nemanov wrote: > Add device-tree bindings for the CC33xx family. > > Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> > --- > .../bindings/net/wireless/ti,cc33xx.yaml | 59 +++++++++++++++++++ > 1 file changed, 59 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..12a0a2f52f44 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml > @@ -0,0 +1,59 @@ > +# 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: > + The CC33xx is a family of IEEE 802.11ax chips from Texas Instruments. > + These chips must be connected via SDIO and support in-band / out-of-band IRQ. > + > +properties: > + $nodename: > + pattern: "^wifi@2" This wasn't here, please drop. > + > + compatible: > + oneOf: Why oneOf appeared? Do you plan to grow it? > + - items: > + - enum: > + - ti,cc3300 > + - ti,cc3301 > + - ti,cc3350 > + - ti,cc3351 > + - const: ti,cc33xx And how cc33xx could appear? That's a no. Generic compatibles are not allowed. Please do not introduce some completely different changes than asked for. Your changelog does not explain these three. "Fixed compatibility" is way too vague, especially that you do not fix anything here. > + > + reg: > + const: 2 > + Best regards, Krzysztof
On 10/29/2024 7:28 PM, Krzysztof Kozlowski wrote: > On 29/10/2024 18:23, Michael Nemanov wrote: >> Add device-tree bindings for the CC33xx family. >> >> Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> >> --- >> .../bindings/net/wireless/ti,cc33xx.yaml | 59 +++++++++++++++++++ >> 1 file changed, 59 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..12a0a2f52f44 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml >> @@ -0,0 +1,59 @@ >> +# 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: >> + The CC33xx is a family of IEEE 802.11ax chips from Texas Instruments. >> + These chips must be connected via SDIO and support in-band / out-of-band IRQ. >> + >> +properties: >> + $nodename: >> + pattern: "^wifi@2" > > This wasn't here, please drop. > In the previous patch you noted there was a mismatch between the reg address in the schema (const: 2) and the used in the example (wifi@1). The dt_binding_check did not flag this because SDIO is not a simple bus. Using this regex seemed like a good alternative. Still drop it? > >> + >> + compatible: >> + oneOf: > > Why oneOf appeared? Do you plan to grow it? > >> + - items: >> + - enum: >> + - ti,cc3300 >> + - ti,cc3301 >> + - ti,cc3350 >> + - ti,cc3351 >> + - const: ti,cc33xx > > And how cc33xx could appear? That's a no. Generic compatibles are not > allowed. Please do not introduce some completely different changes than > asked for. > > Your changelog does not explain these three. "Fixed compatibility" is > way too vague, especially that you do not fix anything here. > I was trying to address the feedback from previous patch. You said: >>>> +static const struct of_device_id cc33xx_sdio_of_match_table[] = { >>>> + { .compatible = "ti,cc3300", .data = &cc33xx_data }, >>>> + { .compatible = "ti,cc3301", .data = &cc33xx_data }, >>>> + { .compatible = "ti,cc3350", .data = &cc33xx_data }, >>>> + { .compatible = "ti,cc3351", .data = &cc33xx_data }, >>>> + { } >>>> +}; >>> >>> >>> Eh? What happened here? So devices are compatibles thus make them >>> compatible in the bindings. >>> >> >> I thought this is the right way to do it (originally taken from [1]). >> How can I solve it via DT bindings? > > It's all over the bindings (also example-schema). Use fallback and oneOf. > Looking at [2] and [3] as an example I tried to do the same (make cc33xx driver compatible with all chip variants). How should have I done it? Regards, Michael. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/sdio.c#n204 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/watchdog/qcom-wdt.yaml [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/watchdog/qcom-wdt.c
On 30/10/2024 11:59, Nemanov, Michael wrote: >> >> Your changelog does not explain these three. "Fixed compatibility" is >> way too vague, especially that you do not fix anything here. >> > > I was trying to address the feedback from previous patch. You said: > >>>>> +static const struct of_device_id cc33xx_sdio_of_match_table[] = { >>>>> + { .compatible = "ti,cc3300", .data = &cc33xx_data }, >>>>> + { .compatible = "ti,cc3301", .data = &cc33xx_data }, >>>>> + { .compatible = "ti,cc3350", .data = &cc33xx_data }, >>>>> + { .compatible = "ti,cc3351", .data = &cc33xx_data }, >>>>> + { } >>>>> +}; >>>> >>>> >>>> Eh? What happened here? So devices are compatibles thus make them >>>> compatible in the bindings. >>>> >>> >>> I thought this is the right way to do it (originally taken from [1]). >>> How can I solve it via DT bindings? >> >> It's all over the bindings (also example-schema). Use fallback and oneOf. >> > > Looking at [2] and [3] as an example I tried to do the same (make cc33xx > driver compatible with all chip variants). > How should have I done it? qcom-wdt is quite a different device. It's true you should have here oneOf, but for a purpose. oneOf without purpose does not make sense, right? I think other TI bindings would serve you as an example. Or this one: https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31 Best regards, Krzysztof
On 10/30/2024 1:09 PM, Krzysztof Kozlowski wrote: > On 30/10/2024 11:59, Nemanov, Michael wrote: >>> >>> Your changelog does not explain these three. "Fixed compatibility" is >>> way too vague, especially that you do not fix anything here. >>> >> >> I was trying to address the feedback from previous patch. You said: >> >>>>>> +static const struct of_device_id cc33xx_sdio_of_match_table[] = { >>>>>> + { .compatible = "ti,cc3300", .data = &cc33xx_data }, >>>>>> + { .compatible = "ti,cc3301", .data = &cc33xx_data }, >>>>>> + { .compatible = "ti,cc3350", .data = &cc33xx_data }, >>>>>> + { .compatible = "ti,cc3351", .data = &cc33xx_data }, >>>>>> + { } >>>>>> +}; >>>>> >>>>> >>>>> Eh? What happened here? So devices are compatibles thus make them >>>>> compatible in the bindings. >>>>> >>>> >>>> I thought this is the right way to do it (originally taken from [1]). >>>> How can I solve it via DT bindings? >>> >>> It's all over the bindings (also example-schema). Use fallback and oneOf. >>> >> >> Looking at [2] and [3] as an example I tried to do the same (make cc33xx >> driver compatible with all chip variants). >> How should have I done it? > > qcom-wdt is quite a different device. It's true you should have here > oneOf, but for a purpose. oneOf without purpose does not make sense, right? > > I think other TI bindings would serve you as an example. Or this one: > > https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31 > > > Best regards, > Krzysztof > OK. So I should make one of the variants the base and declare others as compatible? i.e: --Bindings-- compatible: oneOf: - const: ti,cc3300 - items: - enum: - ti,cc3301 - ti,cc3350 - ti,cc3351 - const: ti,cc3300 --Driver-- static const struct of_device_id cc33xx_sdio_of_match_table[] = { { .compatible = "ti,cc3300", .data = &cc33xx_data }, { } };
On 30/10/2024 13:14, Nemanov, Michael wrote: > On 10/30/2024 1:09 PM, Krzysztof Kozlowski wrote: >> On 30/10/2024 11:59, Nemanov, Michael wrote: >>>> >>>> Your changelog does not explain these three. "Fixed compatibility" is >>>> way too vague, especially that you do not fix anything here. >>>> >>> >>> I was trying to address the feedback from previous patch. You said: >>> >>>>>>> +static const struct of_device_id cc33xx_sdio_of_match_table[] = { >>>>>>> + { .compatible = "ti,cc3300", .data = &cc33xx_data }, >>>>>>> + { .compatible = "ti,cc3301", .data = &cc33xx_data }, >>>>>>> + { .compatible = "ti,cc3350", .data = &cc33xx_data }, >>>>>>> + { .compatible = "ti,cc3351", .data = &cc33xx_data }, >>>>>>> + { } >>>>>>> +}; >>>>>> >>>>>> >>>>>> Eh? What happened here? So devices are compatibles thus make them >>>>>> compatible in the bindings. >>>>>> >>>>> >>>>> I thought this is the right way to do it (originally taken from [1]). >>>>> How can I solve it via DT bindings? >>>> >>>> It's all over the bindings (also example-schema). Use fallback and oneOf. >>>> >>> >>> Looking at [2] and [3] as an example I tried to do the same (make cc33xx >>> driver compatible with all chip variants). >>> How should have I done it? >> >> qcom-wdt is quite a different device. It's true you should have here >> oneOf, but for a purpose. oneOf without purpose does not make sense, right? >> >> I think other TI bindings would serve you as an example. Or this one: >> >> https://elixir.bootlin.com/linux/v6.3-rc6/source/Documentation/devicetree/bindings/sound/nvidia,tegra210-ope.yaml#L31 >> >> >> Best regards, >> Krzysztof >> > > OK. > So I should make one of the variants the base and declare others as > compatible? i.e: > Yes Best regards, Krzysztof
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..12a0a2f52f44 --- /dev/null +++ b/Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml @@ -0,0 +1,59 @@ +# 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: + The CC33xx is a family of IEEE 802.11ax chips from Texas Instruments. + These chips must be connected via SDIO and support in-band / out-of-band IRQ. + +properties: + $nodename: + pattern: "^wifi@2" + + compatible: + oneOf: + - items: + - enum: + - ti,cc3300 + - ti,cc3301 + - ti,cc3350 + - ti,cc3351 + - const: ti,cc33xx + + reg: + const: 2 + + interrupts: + description: + The out-of-band interrupt line. + Can be IRQ_TYPE_EDGE_RISING or IRQ_TYPE_LEVEL_HIGH. + If property is omitted, SDIO in-band IRQ will be used. + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + + mmc { + #address-cells = <1>; + #size-cells = <0>; + + wifi@2 { + compatible = "ti,cc3300", "ti,cc33xx"; + reg = <2>; + interrupts = <19 IRQ_TYPE_EDGE_RISING>; + }; + };
Add device-tree bindings for the CC33xx family. Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> --- .../bindings/net/wireless/ti,cc33xx.yaml | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml