[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8024aa1c-5bd1-40d8-b0c3-14b5fcd992e2@ti.com>
Date: Wed, 30 Oct 2024 12:59:50 +0200
From: "Nemanov, Michael" <michael.nemanov@...com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 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>, <linux-wireless@...r.kernel.org>,
<netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
CC: Sabeeh Khan <sabeeh-khan@...com>
Subject: Re: [PATCH v4 01/17] dt-bindings: net: wireless: cc33xx: Add
ti,cc33xx.yaml
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@...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@...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
Powered by blists - more mailing lists