Re: [PATCH v4 01/17] dt-bindings: net: wireless: cc33xx: Add ti,cc33xx.yaml

["Nemanov, Michael" <michael.nemanov@ti.com>]

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