Re: [PATCH v2 1/3] dt-bindings: net: bluetooth: Add support for Amlogic Bluetooth

[Yang Li <yang.li@amlogic.com>]

Dear Krzysztof

Thanks.

On 2024/7/18 19:40, Krzysztof Kozlowski wrote:
> On 18/07/2024 09:42, Yang Li via B4 Relay wrote:
>> From: Yang Li <yang.li@amlogic.com>
>>
>> Add binding document for Amlogic Bluetooth chipsets attached over UART.
>>
>> Signed-off-by: Yang Li <yang.li@amlogic.com>
>> ---
>>   .../bindings/net/bluetooth/amlogic,w155s2-bt.yaml  | 66 ++++++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
>> new file mode 100644
>> index 000000000000..2e433d5692ff
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
>> @@ -0,0 +1,66 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +# Copyright (C) 2024 Amlogic, Inc. All rights reserved
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/bluetooth/amlogic,w155s2-bt.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic Bluetooth chips
>> +
>> +description:
>> +  This binding describes UART-attached Amlogic bluetooth chips.
> <form letter>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
> </form letter>

Apologies for the earlier omission. I have amended the description of the

UART-attached Amlogic Bluetooth chips in the patch:

"This binding describes Amlogic Bluetooth chips connected via UART,

which function as dual-radio devices supporting Wi-Fi and Bluetooth.

It operates on the H4 protocol over a 4-wire UART, with RTS and CTS lines

used for firmware download. It supports Bluetooth and Wi-Fi coexistence."

>> +
>> +maintainers:
>> +  - Yang Li <yang.li@amlogic.com>
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - const: amlogic,w155s2-bt
>> +      - items:
>> +          - enum:
>> +              - amlogic,w265s1-bt
>> +              - amlogic,w265p1-bt
>> +              - amlogic,w265s2-bt
>> +          - const: amlogic,w155s2-bt
>> +
>> +  bt-enable-gpios:
> enable-gpios
will do.
>
>> +    maxItems: 1
>> +    description: gpio specifier used to enable BT
> Drop, redundant.
will do.
>
>> +
>> +  bt-supply:
> It's called "bt" in schematics or datasheet? Feels unusual. Please list
> all the pins if you claim that's a real name.
>
Yes, you are correct, the actual name is 'vddio-supply.' I initially 
intended to

differentiate it from WiFi, but it seems unnecessary. I will change it 
to 'vddio-supply'.

>
>> +    description: bluetooth chip 3.3V supply regulator handle
>> +
>> +  clocks:
>> +    maxItems: 1
>> +    description: clock provided to the controller (32.768KHz)
>> +
>> +  antenna-number:
>> +    default: 1
>> +    description: device supports up to two antennas
> Keep it consistent - either descriptions are the last property or
> somewhere else. Usually the last.
>
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> And what does it mean? What happens if BT uses antenna number 2, not 1?
> What is connected to the other antenna? It really feels useless to say
> which antenna is connected to hardware.

Sorry, the antenna description was incorrect, it should specify whether

Bluetooth and WiFi coexist. I will change it as below:

     aml,work-mode:
     type: boolean
     description: specifywhether Bluetooth and WiFi coexist.
>> +
>> +  firmware-name:
>> +    description: specify the path of firmware bin to load
> Missing maxItems
will do.
>
>> +    $ref: /schemas/types.yaml#/definitions/string-array
> That's redundant, drop.
will do.
>
>> +
>
> Best regards,
> Krzysztof
>