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

[Yang Li]

On 2024/7/7 21:08, Krzysztof Kozlowski wrote:
> [你通常不会收到来自 krzk@xxxxxxxxxx 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要;]
>
> [ EXTERNAL EMAIL ]
>
> On 05/07/2024 13:20, 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  | 62 ++++++++++++++++++++++
> >   1 file changed, 62 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..d59e3206af62
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/bluetooth/amlogic,w155s2-bt.yaml
> > @@ -0,0 +1,62 @@
> > +# 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.
> Do not say that binding describes a binding. It is not helpful. Describe
> the hardware instead.
Well do.
> > +
> > +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
> > +
> > +  amlogic,wcn-pwrseq:
> ??
I will remove this line, add regolator.
> > +    default: 0
> > +    description: specify the power sequence used to power on Bluetooth
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> What? I have no clue what is this and have no clue what to say here. Drop.
will do.
> > +
> > +  amlogic,firmware:
> > +    description: specify the path of firmware bin to load
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> Srsly, it's third case in your bindings. Do not re-invent the wheel but
> use existing, common properties. How to find them? Well, git grep or ask
> your colleagues...
I will change "amlogic,firmware" to " firmware-name".
> > +
> > +  amlogic,antenna-number:
> > +    default: 1
> > +    description: number of antenna
> Useless description. Do not repeat the property name but explain what is
> it and how it is used.
>
> Or drop the property.
I will change the description.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  amlogic,a2dp-sink-enable:
> > +    default: 0
> > +    description: enable sink mode with controller
> Again, not possible to figure out.
>
> NAK
Well do.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +required:
> > +  - compatible
> > +  - amlogic,wcn-pwrseq
> > +  - amlogic,firmware
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    serial {
> Drop
Will do.
> > +        bluetooth {
> > +            compatible = "amlogic,w155s2-bt";
> > +            amlogic,wcn-pwrseq = <1>;
> Joking, right?
Well, I will remove it.
> Best regards,
> Krzysztof