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

[Yang Li]

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@xxxxxxxxxxx>
> >
> > Add binding document for Amlogic Bluetooth chipsets attached over UART.
> >
> > Signed-off-by: Yang Li <yang.li@xxxxxxxxxxx>
> > ---
> >   .../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@xxxxxxxxxxx>
> > +
> > +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