Message-ID: <c84b783a-0118-43d8-8f03-a98fdf5bd8c5@kernel.org>
Date: Tue, 3 Sep 2024 20:47:38 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Alexis Lothoré <alexis.lothore@...tlin.com>,
 Marek Vasut <marex@...x.de>, linux-wireless@...r.kernel.org
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 "David S. Miller" <davem@...emloft.net>,
 Adham Abozaeid <adham.abozaeid@...rochip.com>,
 Ajay Singh <ajay.kathat@...rochip.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>, Conor Dooley
 <conor+dt@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Kalle Valo <kvalo@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH v4 1/5] dt-bindings: wireless: wilc1000: Document WILC3000
 compatible string

On 03/09/2024 18:09, Alexis Lothoré wrote:
> Hello everyone,
> 
> On 8/29/24 02:44, Marek Vasut wrote:
>> Document compatible string for the WILC3000 chip. The chip is similar
>> to WILC1000, except that the register layout is slightly different and
>> it does not support WPA3/SAE.
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
>> Signed-off-by: Marek Vasut <marex@...x.de>
> 
> [...]
> 
>>  .../bindings/net/wireless/microchip,wilc1000.yaml           | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> index 2460ccc082371..5d40f22765bb6 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> @@ -16,7 +16,11 @@ description:
>>  
>>  properties:
>>    compatible:
>> -    const: microchip,wilc1000
>> +    oneOf:
>> +      - items:
>> +          - const: microchip,wilc3000
>> +          - const: microchip,wilc1000
>> +      - const: microchip,wilc1000
>>  
>>    reg: true
> 
> Following this series first revision, I have been taking a look at how to
> implement bluetooth feature for wilc3000 (the chip supports Bluetooth LE through
> a separated UART, see [1]), and I am facing some constraints. I feel like the
> possible solutions would conflict with this new binding, so even if I am a bit
> late to the party, I would like to expose the issue before the binding is merged
> in case we can find something which would allow to add bluetooth support without
> too much pain after the wlan part.
> 
> Downstream driver currently does not implement bluetooth as a standard bluetooth
> driver (module in drivers/bluetooth, registering a HCI device) but only performs
> a minimal set of operations directly in the wlan part ([2]). Getting a version
> valid for upstream would need the following points to be addressed:
> 1. despite being controlled from a serial port for nominal operations, the
> bluetooth part also depends on the "wlan" bus (spi or sdio) for initialization
> 2. yet init steps are not performed on any kind of subsystem ops but through
> writes to a custom chardev
> 3. the driver does not register itself a hci interface, it is expected to be
> done by userspace (hciattach).
> 
> It is only after those 3 steps that the chip can be used with standard hci
> commands over serial port. IMHO 1 is the biggest point, because it means that
> **a bluetooth driver for wilc3000 needs access to the bus used by wlan part**
> (so only describing the bluetooth part of the chip as a child node of an uart
> controller is not enough). Aside from bus access, I also expect some
> interactions between bluetooth and wifi (eg: power management, sleep/wakeup)
> 
> After considering multiple solutions to try to share this bus between existing
> wlan driver and a new bt driver (mfd device, auxiliary bus, device link + some

Driver design should not have impact on bindings.

> handles, etc), my current best guess is to convert wilc driver to a MFD driver
> for wilc3000. I guess some work can be done so that the driver can still be
> shared between wilc1000 and wilc3000 _while_ remaining compatible with current
> wilc1000 description, but it would impact the DT description for wilc3000, which
> would need to switch from this:
> 
>   spi {
>     wifi@0 {
>       compatible = "microchip,wilc3000";
>       [...]
>     };
>   };
> 
> To something like this:
> 
>   spi {
>     wilc@0 {
>       compatible = "microchip,wilc3000"; /* mfd driver */

I do not see any reason why... or rather: What is MFD here? MFD is Linux
stuff and we talk about hardware.

>       wifi {
>         compatible = "microchip,wilc3000-wlan";

Why? Just merge it to parent...

>         [...]
>       };
>       bt {
>         compatible = "microchip,wilc3000-bt";
>         XXXX; /* some link to the uart controller connected to the chip */

That's not how we represent UART devices. I don't understand why do you
need these - if for power sequencing, then use power sequencing
framework and describe associated hardware (there are some talks coming
about it in 2 weeks). If for something else, then for what?

>         [...]
>       };
>     };
>   };
> 
> (and similar thing when wilc is driven over a sdio bus)
> 
> Any opinion on this ? Would it make sense to describe wilc3000 chip that way ?
> 

You described drivers, not wilc3000 chip...

Best regards,
Krzysztof

Powered by blists - more mailing lists