Re: [PATCH v1 1/2] ASoC: dt-bindings: nuvoton,nau8310: Add NAU83G10

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ASoC: dt-bindings: nuvoton,nau8310: inverted clock-det-disable description
- [Medium] ASoC: dt-bindings: nuvoton,nau8310: raw register values instead of units
- [Medium] ASoC: dt-bindings: nuvoton,nau8310: unconstrained nau8320 variant properties
--

commit 01b8fdb80988a85278fec4d71b7b4a47411ed6ee
Author: Neo Chang <YLCHANG2@nuvoton.com>

ASoC: dt-bindings: nuvoton,nau8310: Add NAU83G10

This commit adds the device tree binding schema in
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml for the
Nuvoton mono class-d audio amplifier family.

> diff --git a/Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml b/Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml
> new file mode 100644
> index 0000000000000..5a5142a583ed1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml
> @@ -0,0 +1,200 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/nuvoton,nau8310.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton NAU83G10/20 Mono Class-D Amplifier with DSP
> +
> +description:
> +  Mono Class-D Amplifier with DSP and I/V-sense.
> +  This device supports I2C.
> +
> +maintainers:
> +  - Neo Chang <YLCHANG2@nuvoton.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nuvoton,nau8310
> +      - nuvoton,nau8320

[Severity: Medium]
Should Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml restrict
the boost converter properties when compatible is set to nuvoton,nau8320?

In sound/soc/codecs/nau8310.c, nau8310_read_device_properties() only reads
nuvoton,boost-delay, nuvoton,boost-convert-enable,
nuvoton,boost-target-limit, and nuvoton,boost-target-margin for
nuvoton,nau8310 and ignores them on nuvoton,nau8320:

    if (nau8310->silicon_id == NAU8310_REG_SI_REV_G10) {
        ret = device_property_read_u32(dev, "nuvoton,boost-delay",
            &nau8310->boost_delay);
        ...
    }

Without an allOf if/then block setting those four properties to false for
nuvoton,nau8320, devicetree nodes for nuvoton,nau8320 with boost converter
properties will pass schema validation even though the driver ignores them.
Should the commit message also mention nuvoton,nau8320 alongside
nuvoton,nau8310?

[ ... ]

> +  nuvoton,vref-impedance:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      VREF Impedance Selection.
> +    enum:
> +      - 0 # Open
> +      - 1 # 25 kOhm
> +      - 2 # 125 kOhm
> +      - 3 # 2.5 kOhm
> +    default: 0
> +
> +  nuvoton,dac-vref:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      DAC Reference Voltage Setting.
> +    enum:
> +      - 0 # Voltage source directly from VDDA pin
> +      - 1 # VDDA * 1.5/1.8V
> +      - 2 # VDDA * 1.6/1.8V
> +      - 3 # VDDA * 1.7/1.8V
> +    default: 1
> +
> +  nuvoton,sar-voltage:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      SAR Gain Tracking Selection.
> +    enum:
> +      - 0 # VDDA
> +      - 1 # VDDA * 1.0
> +      - 2 # VDDA * 1.1
> +      - 3 # VDDA * 1.2
> +      - 4 # VDDA * 1.3
> +      - 5 # VDDA * 1.4
> +      - 6 # VDDA * 1.54
> +      - 7 # VDDA * 3.12
> +    default: 0
> +
> +  nuvoton,sar-compare-time:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      SAR compare time.
> +    enum:
> +      - 0 # 500 ns
> +      - 1 # 1 us
> +      - 2 # 2 us
> +      - 3 # 4 us
> +    default: 1
> +
> +  nuvoton,sar-sampling-time:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      SAR sampling time.
> +    enum:
> +      - 0 # 2 us
> +      - 1 # 4 us
> +      - 2 # 8 us
> +      - 3 # 16 us
> +    default: 1
> +
> +  nuvoton,clock-det-disable:
> +    type: boolean
> +    description:
> +      Enable Clock Detection Module.

[Severity: Medium]
Is the description for nuvoton,clock-det-disable in
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml inverted
relative to the property name and driver behavior?

In sound/soc/codecs/nau8310.c, nau8310_read_device_properties() and
nau8310_init_regs() disable clock detection when nuvoton,clock-det-disable
is present in the devicetree node:

    nau8310->clock_detection =
        !device_property_read_bool(dev, "nuvoton,clock-det-disable");

    if (!nau8310->clock_detection)
        regmap_update_bits(nau8310->regmap, NAU8310_REG_CLK_DET,
            NAU8310_CLKPWRUP_DIS, NAU8310_CLKPWRUP_DIS);

Also, Documentation/devicetree/bindings/sound/nuvoton,nau8325.yaml already
defines nuvoton,clock-detection-disable for this feature. Could this binding
reuse nuvoton,clock-detection-disable and state that it disables the clock
detection module?

> +
> +  nuvoton,clock-det-data:
> +    type: boolean
> +    description:
> +      Clock detection non-zero samples configuration for power up signal.
> +
> +  nuvoton,boost-delay:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Boost Control Hold Timer in decrement (msec).
> +    enum:
> +      - 0 # 0 msec
> +      - 1 # 1 msec
> +      - 2 # 2 msec
> +      - 3 # 4 msec
> +      - 4 # 8 msec
> +      - 5 # 16 msec
> +      - 6 # 32 msec
> +      - 7 # 64 msec
> +      - 8 # 128 msec
> +    default: 0
> +
> +  nuvoton,boost-convert-enable:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Boost Convert Enable.
> +    enum:
> +      - 0 # BOOST CONVERTER disabled
> +      - 1 # BOOST CONVERTER enable gated by POWERUP signal
> +      - 2 # BOOST CONVERTER disabled
> +      - 3 # BOOST CONVERTER enabled
> +    default: 0
> +
> +  nuvoton,boost-target-limit:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Boost Control Target Value Limit.
> +      The value ranges from 0 to 63, representing 0V to 12V with a 0.19V step.
> +    minimum: 0
> +    maximum: 63
> +    default: 63
> +
> +  nuvoton,boost-target-margin:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Boost Margin Value between Boost Target Voltage and peak output level.
> +      The value ranges from 0 to 63, representing 0V to 12V with a 0.19V step.
> +    minimum: 0
> +    maximum: 63
> +    default: 0

[Severity: Medium]
Could these vendor properties in
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml use standard
unit suffixes and physical values instead of raw register encodings?

Specifically:
- nuvoton,vref-impedance and nuvoton,dac-vref use raw register indices 0..3,
  whereas Documentation/devicetree/bindings/sound/nuvoton,nau8325.yaml
  already defines nuvoton,vref-impedance-ohms and nuvoton,dac-vref-microvolt.
- nuvoton,sar-compare-time, nuvoton,sar-sampling-time, nuvoton,boost-delay,
  nuvoton,boost-target-limit, and nuvoton,boost-target-margin expose raw
  register indices or step counts (0..3, 0..8, 0..63) rather than physical
  values with -ns, -us, -ms, and -microvolt suffixes.
- The schema default values for nuvoton,vref-impedance (0),
  nuvoton,boost-delay (0), nuvoton,boost-target-limit (63), and
  nuvoton,boost-target-margin (0) do not match the defaults applied in
  nau8310_read_device_properties() (2, 0x8, 0x32, and 0x2).
- nuvoton,boost-convert-enable has a boolean-style -enable suffix for a
  uint32 register field where both 0 and 2 mean disabled, and
  nuvoton,sar-voltage exposes duplicate register values 0 and 1 for the same
  1.0 gain setting.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022084037.259983-1-YLCHANG2@nuvoton.com?part=1