Re: [PATCH v1 3/4] ASoC: dt-bindings: Add dt bindings for FS2104/5S audio amplifiers

[Nick Li <nick.li@foursemi.com>]

On Thu, Jul 03, 2025 at 08:36:42AM +0200, Krzysztof Kozlowski wrote:
> On 03/07/2025 05:56, Nick wrote:
> > From: Nick Li <nick.li@foursemi.com>
> > 
> > This patch adds bindings for FourSemi FS2104/5S audio amplifiers
> > which can support both I2S and I2C interface.
> > 
> > Signed-off-by: Nick Li <nick.li@foursemi.com>
> 
> Your patches are not correctly ordered. Vendor prefix is supposed to be
> before is being used. Bindings are before their users.

OK, I will update the orders of patches in next version.

> 
> > ---
> >  .../bindings/sound/foursemi,fs210x.yaml       | 95 +++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml b/Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
> > new file mode 100644
> > index 000000000..3a1aba9b8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
> 
> Choose one compatible as filename.

OK, I will update the file name to foursemi,fs2105s.yaml

> 
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/foursemi,fs210x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: FourSemi FS2104/5S Digital Audio Amplifier
> > +
> > +maintainers:
> > +  - Nick <nick.li@foursemi.com>
> > +
> > +description: |
> > +    The FS2104 is a 15W Inductor-Less, Stereo, Closed-Loop,
> > +    Digital Input Class-D Power Amplifier with Enhanced Signal Processing.
> > +    The FS2105S is a 30W Inductor-Less, Stereo, Closed-Loop,
> > +    Digital Input Class-D Power Amplifier with Enhanced Signal Processing.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - foursemi,fs2104
> > +      - foursemi,fs2105s
> 
> Your driver suggests these are compatible, so express that with fallback.

OK, I will update these code to:
  oneOf:
    - items:
        ...
    - enum:
        ...

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.

OK, I will delete it in next version.

> 
> > +      I2C address of the amplifier. Refer to datasheet for possible values:
> > +      0x68/0x69/0x6A/0x6B
> > +
> > +  clocks:
> > +    items:
> > +      - description: The clock of I2S BCLK
> > +
> > +  clock-names:
> > +    items:
> > +      - const: bclk
> > +
> > +  '#sound-dai-cells':
> > +    const: 0
> > +
> > +  pvdd-supply:
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.

OK, I will delete it in next version.

> 
> 
> > +      Regulator for power supply(PVDD in datasheet).
> > +
> > +  dvdd-supply:
> > +    description: |
> 
> Do not need '|' unless you need to preserve formatting.

OK, I will delete it.

> 
> > +      Regulator for digital supply(DVDD in datasheet).
> > +
> > +  sdz-gpios:
> 
> Use standard properties - see gpio-consumer-common

OK, I will update this property to reset-gpios, and update the driver.

> 
> > +    maxItems: 1
> > +    description: |
> > +      SDZ(Shut Down) pin is active low, it will power down the chip,
> > +      and then reset the chip to shut down state.
> > +
> > +  fs,fwm-name:
> 
> No, use standard properties (see writing bindings)

OK, I will update this property to firmware-name, and update the driver.

> 
> > +    description: |
> > +      The name of firmware that should be loaded for this
> > +      instance. The firmware is gernerated by FourSemi's tuning tool.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +
> > +  fs,dai-name:
> 
> No, it's already in dai-common

OK, delete it in next version, and update the driver.

> 
> > +    description: |
> > +      The name of FS210x DAI, it can be set different name when
> > +      there are multiple FS210x devices in the system.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - sdz-gpios
> > +  - fs,fwm-name
> > +  - '#sound-dai-cells'
> > +
> 
> Missing ref to dai common

OK, reference will be added:
allOf:
  - $ref: dai-common.yaml#

> 
> > +additionalProperties: false
> 
> And this should be unevaluatedProperties instead

OK, I will update it to:
unevaluatedProperties: false

Thanks for your patient response very mush.

Best regards,
Nick

> 
> 
> Best regards,
> Krzysztof
>