| Message ID | 20241029172354.4027886-1-michael.nemanov@ti.com |
|---|---|
| Headers | show |
| Series | wifi: cc33xx: Add driver for new TI CC33xx wireless device family | expand |
On 29/10/2024 18:23, Michael Nemanov wrote: > diff --git a/drivers/net/wireless/ti/Makefile b/drivers/net/wireless/ti/Makefile > index 05ee016594f8..9e028a91ec30 100644 > --- a/drivers/net/wireless/ti/Makefile > +++ b/drivers/net/wireless/ti/Makefile > @@ -3,3 +3,4 @@ obj-$(CONFIG_WLCORE) += wlcore/ > obj-$(CONFIG_WL12XX) += wl12xx/ > obj-$(CONFIG_WL1251) += wl1251/ > obj-$(CONFIG_WL18XX) += wl18xx/ > +obj-$(CONFIG_CC33XX) += cc33xx/ > \ No newline at end of file Patch error. 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