ASoC: codecs: nau8310: Add NAU83G10 amplifier
Subsystems (3)
v2 cover ASoC: codecs: nau8310: Add NAU83G10 amplifier
This patch series adds support for the Nuvoton NAU83G10 audio amplifier.
The NAU83G10 is a mono boost Class-D audio amplifier with an integrated DSP and I/V-sense support. The driver supports I2C register access through regmap.
DAPM widgets and routes, TDM interface configuration, and DSP firmware loading.
The series is structured asd follows:
- Patch 1: Adds the YAML device tree bindings documentation.
- Patch 2: Adds the ASoC audio amplifier driver implementation.
---
Changes in v2:
[dt-bindings]
- No changes since v1.
[codec driver]
- Sort SND_SOC_NAU8310 alphabetically in Kconfig and Makefile.
- Fix uninitialized variable 'kcs_setup_comp.set_kcs_offset' warning reported by kernel test robot. Cast 'data[i]' to u32 before bit shifting in payload packing to prevent incorrect sign extension.
- Replace deprecated SND_SOC_DAIFMT_MASTER_MASK with SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK.
- Demote DSP ready status log from dev_info to dev_dbg to reduce log spam.
- Support requesting DSP firmware via 'firmware-name' property defined in device tree.
- Use snd_soc_kcontrol_component() instead of dapm lookup in standard kcontrol handlers.
- Make clock configuration stateless by passing 'fs' and 'mclk' directly as arguments. This prevents internal OSC switching from overwriting active user configurations set via set_sysclk(), ensuring stable state management during suspend/resume.
- Acquire nau8310->lock within the clock event handler to prevent race conditions during DSP command execution and concurrent firmware loading.
- Add NAU8310_CLK_ID_INT to set_sysclk() to properly handle internal ring oscillator configurations.
Neo Chang (2):
ASoC: dt-bindings: nuvoton,nau8310: Add NAU83G10
ASoC: codecs: nau8310: Add support for NAU83G10 amplifier
.../bindings/sound/nuvoton,nau8310.yaml | 200 ++ sound/soc/codecs/Kconfig | 13 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/nau8310-dsp.c | 695 +++++++ sound/soc/codecs/nau8310-dsp.h | 111 + sound/soc/codecs/nau8310.c | 1828 +++++++++++++++++ sound/soc/codecs/nau8310.h | 633 ++++++ 7 files changed, 3482 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml create mode 100644 sound/soc/codecs/nau8310-dsp.c create mode 100644 sound/soc/codecs/nau8310-dsp.h create mode 100644 sound/soc/codecs/nau8310.c create mode 100644 sound/soc/codecs/nau8310.h
Signature · 1 line
2.25.1
PATCH v2 1/2 ASoC: dt-bindings: nuvoton,nau8310: Add NAU83G10
Add device tree bindings documentation for the Nuvoton NAU83G10 audio amplifier.
--- .../bindings/sound/nuvoton,nau8310.yaml | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +200 -0
@@ -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 <[email protected]> + +properties: + compatible: + enum: + - nuvoton,nau8310 + - nuvoton,nau8320 + + reg: + description: The I2C address is determined by the hardware pins. + enum: [0x10, 0x11, 0x40, 0x41] + + "#sound-dai-cells": + const: 0 + + clocks: + maxItems: 1 + + clock-names: + const: mclk + + firmware-name: + items: + - description: + DSP core firmware. + + 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. + + 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 + + nuvoton,temp-compensation: + type: boolean + description: + Enable Temperature Compensation. + + nuvoton,normal-iis-data: + type: boolean + description: + Enable Normal I2S Audio Data mode. The I2S interface + transmits only audio data without SAR ADC data. + +required: + - compatible + - reg + - firmware-name + +allOf: + - $ref: dai-common.yaml# + +unevaluatedProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + codec@10 { + compatible = "nuvoton,nau8310"; + reg = <0x10>; + #sound-dai-cells = <0>; + firmware-name = "NAU83G10.kcs.bin"; + nuvoton,vref-impedance = <2>; + nuvoton,dac-vref = <1>; + nuvoton,sar-voltage = <0>; + nuvoton,sar-compare-time = <1>; + nuvoton,sar-sampling-time = <1>; + nuvoton,clock-det-disable; + nuvoton,clock-det-data; + nuvoton,boost-delay = <0x8>; + nuvoton,boost-convert-enable = <1>; + nuvoton,boost-target-limit = <0x32>; + nuvoton,boost-target-margin = <0x2>; + nuvoton,temp-compensation; + nuvoton,normal-iis-data; + }; + };
Signature · 1 line
2.25.1
[email protected]
AI reviewRe: [PATCH v2 1/2] ASoC: dt-bindings: nuvoton,nau8310: Add NAU83G10
Thank you for your contribution! Sashiko AI review found 3 potential issues to consider: - [Medium] ASoC: dt-bindings: nuvoton,nau8310: inverted clock-det-disable description - [Medium] ASoC…
Thank you for your contribution! Sashiko AI review found 3 potential issues 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 e187fb8f9c949cae2d11b255f2782d69bf853e02
Author: Neo Chang <[email protected]>
ASoC: dt-bindings: nuvoton,nau8310: Add NAU83G10
This commit adds Device Tree binding documentation for the Nuvoton NAU83G10 and NAU83G20 mono Class-D audio amplifiers.
Quoted 16 lines
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +10
-0@@ -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.
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml line 20
Quoted 10 lines
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +10
-0+ This device supports I2C. + +maintainers: + - Neo Chang <[email protected]> + +properties: + compatible: + enum: + - nuvoton,nau8310 + - nuvoton,nau8320
[Severity: Medium]
Should the boost converter properties (nuvoton,boost-delay, nuvoton,boost-convert-enable, nuvoton,boost-target-limit, and nuvoton,boost-target-margin) be disallowed for nuvoton,nau8320 via an allOf if/then constraint?
In the driver, nau8310_read_device_properties() only parses those four properties when silicon_id is NAU8310_REG_SI_REV_G10, so a nuvoton,nau8320 node with boost properties will pass schema validation while the driver ignores them. The commit message also only mentions NAU83G10 and omits NAU83G20.
[ ... ]
Quoted 58 lines
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +58
-0+ 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
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml line 107
Quoted 10 lines
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +10
-0+ + nuvoton,clock-det-disable: + type: boolean + description: + Enable Clock Detection Module. + + nuvoton,clock-det-data: + type: boolean + description: + Clock detection non-zero samples configuration for power up signal.
[Severity: Medium]
Is the description for nuvoton,clock-det-disable inverted relative to its name?
In the driver, nau8310_read_device_properties() sets clock_detection to !device_property_read_bool(dev, "nuvoton,clock-det-disable") and nau8310_init_regs() sets NAU8310_CLKPWRUP_DIS when !clock_detection, so this boolean property disables clock detection rather than enabling it. The example below also sets both nuvoton,clock-det-disable and nuvoton,clock-det-data together. In addition, Documentation/devicetree/bindings/sound/nuvoton,nau8325.yaml already uses nuvoton,clock-detection-disable for this feature.
Quoted 35 lines
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +35
-0+ + 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
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml line 152
Quoted 10 lines
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +10
-0+ 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]
Should these properties use physical units and standard unit suffixes instead of raw register selector values?
Per Documentation/devicetree/bindings/writing-bindings.rst, properties representing physical quantities should use standard unit suffixes (-ohms, -microvolt, -ns, -us, -ms) rather than raw register encodings. Documentation/devicetree/bindings/sound/nuvoton,nau8325.yaml already defines nuvoton,vref-impedance-ohms (enum [0, 25000, 125000, 2500]) and nuvoton,dac-vref-microvolt.
Here, nuvoton,vref-impedance, nuvoton,dac-vref, nuvoton,sar-compare-time, nuvoton,sar-sampling-time, nuvoton,boost-delay, nuvoton,boost-target-limit, and nuvoton,boost-target-margin all expose raw register codes or step counts. nuvoton,boost-convert-enable also uses a boolean-style -enable name for a 2-bit register field where both 0 and 2 mean disabled, and nuvoton,sar-voltage exposes duplicate register values 0 and 1 for 1.0 * VDDA. Finally, the schema defaults for nuvoton,vref-impedance (0), nuvoton,boost-delay (0), nuvoton,boost-target-limit (63), and nuvoton,boost-target-margin (0) do not match the driver defaults in nau8310_read_device_properties() (2, 0x8, 0x32, and 0x2).
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH v2 1/2] ASoC: dt-bindings: nuvoton,nau8310: Add NAU83G10
In reply to Neo ChangQuoted 6 lines · Neo Chang
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +6
-0+ nuvoton,sar-voltage: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + SAR Gain Tracking Selection. + enum: + - 0 # VDDA
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml line 77
Quoted 10 lines
Documentation/devicetree/bindings/sound/nuvoton,nau8310.yaml new file +10
-0+ - 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:
Use proper unit suffix from property-units in dtschema.
Also, Sashiko feedback needs to be addressed.
Best regards,
Krzysztof
PATCH v2 2/2 ASoC: codecs: nau8310: Add support for NAU83G10 amplifier
In reply to Neo ChangAdd support for the Nuvoton NAU83G10 audio amplifier. The NAU83G10 is a mono boost Class-D audio amplifier with integrated DSP and I/V-sense support.
---
Changes in v2:
- Sort SND_SOC_NAU8310 alphabetically in Kconfig and Makefile.
- Fix uninitialized variable 'kcs_setup_comp.set_kcs_offset' warning reported by kernel test robot. Cast 'data[i]' to u32 before bit shifting in payload packing to prevent incorrect sign extension.
- Replace deprecated SND_SOC_DAIFMT_MASTER_MASK with SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK.
- Demote DSP ready status log from dev_info to dev_dbg to reduce log spam.
- Support requesting DSP firmware via 'firmware-name' property defined in device tree.
- Use snd_soc_kcontrol_component() instead of dapm lookup in standard kcontrol handlers.
- Make clock configuration stateless by passing 'fs' and 'mclk' directly as arguments. This prevents internal OSC switching from overwriting active user configurations set via set_sysclk(), ensuring stable state management during suspend/resume.
- Acquire nau8310->lock within the clock event handler to prevent race conditions during DSP command execution and concurrent firmware loading.
- Add NAU8310_CLK_ID_INT to set_sysclk() to properly handle internal ring oscillator configurations.
--- sound/soc/codecs/Kconfig | 13 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/nau8310-dsp.c | 695 ++++++++++++ sound/soc/codecs/nau8310-dsp.h | 111 ++ sound/soc/codecs/nau8310.c | 1828 ++++++++++++++++++++++++++++++++ sound/soc/codecs/nau8310.h | 633 +++++++++++ 6 files changed, 3282 insertions(+) create mode 100644 sound/soc/codecs/nau8310-dsp.c create mode 100644 sound/soc/codecs/nau8310-dsp.h create mode 100644 sound/soc/codecs/nau8310.c create mode 100644 sound/soc/codecs/nau8310.h
sound/soc/codecs/Kconfig +13 -0
@@ -173,6 +173,7 @@ config SND_SOC_ALL_CODECS imply SND_SOC_MT6358 imply SND_SOC_MT6359 imply SND_SOC_MT6660 + imply SND_SOC_NAU8310 imply SND_SOC_NAU8315 imply SND_SOC_NAU8325 imply SND_SOC_NAU8360
@@ -2922,6 +2923,18 @@ config SND_SOC_MT6660 Select N if you don't have MT6660 on board. Select M to build this as module. +config SND_SOC_NAU8310 + tristate "Nuvoton Technology Corporation NAU83G10 Mono Boosted Amplifier" + depends on I2C + select FW_LOADER + help + The NAU83G10 is a mono boosted Class-D amplifier with an integrated + DSP and I/V sense. The DSP is programmed with a speaker excursion + control algorithm that uses real-time voltage and current feedback + to prevent the speaker membrane from exceeding its rated limit. + It has a high-efficiency Class-H boost converter to provide + ample headroom for overall loudness and sound quality. + config SND_SOC_NAU8315 tristate "Nuvoton Technology Corporation NAU8315 CODEC"
sound/soc/codecs/Makefile +2 -0
@@ -198,6 +198,7 @@ snd-soc-mt6358-y := mt6358.o snd-soc-mt6359-y := mt6359.o snd-soc-mt6359-accdet-y := mt6359-accdet.o snd-soc-mt6660-y := mt6660.o +snd-soc-nau8310-y := nau8310.o nau8310-dsp.o snd-soc-nau8315-y := nau8315.o snd-soc-nau8325-y := nau8325.o snd-soc-nau8360-y := nau8360.o nau8360-dsp.o
@@ -642,6 +643,7 @@ obj-$(CONFIG_SND_SOC_MT6358) += snd-soc-mt6358.o obj-$(CONFIG_SND_SOC_MT6359) += snd-soc-mt6359.o obj-$(CONFIG_SND_SOC_MT6359_ACCDET) += mt6359-accdet.o obj-$(CONFIG_SND_SOC_MT6660) += snd-soc-mt6660.o +obj-$(CONFIG_SND_SOC_NAU8310) += snd-soc-nau8310.o obj-$(CONFIG_SND_SOC_NAU8315) += snd-soc-nau8315.o obj-$(CONFIG_SND_SOC_NAU8325) += snd-soc-nau8325.o obj-$(CONFIG_SND_SOC_NAU8360) += snd-soc-nau8360.o
sound/soc/codecs/nau8310-dsp.c new file +695 -0
695 lines not on this page · raw message
sound/soc/codecs/nau8310-dsp.h new file +111 -0
111 lines not on this page · raw message
sound/soc/codecs/nau8310.c new file +1828 -0
1,828 lines not on this page · raw message
sound/soc/codecs/nau8310.h new file +633 -0
327 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 1 line
2.25.1
Part of the diff is not shown here. View the raw message.
[email protected]
AI reviewRe: [PATCH v2 2/2] ASoC: codecs: nau8310: Add support for NAU83G10 amplifier
Thank you for your contribution! Sashiko AI review found 16 potential issues to consider: - [High] ASoC: nau8310: type confusion in nau8310_biq_coeff_put() - [High] ASoC: nau8310: DAPM state desync…
Thank you for your contribution! Sashiko AI review found 16 potential issues to consider:
- [High] ASoC: nau8310: type confusion in nau8310_biq_coeff_put()
- [High] ASoC: nau8310: DAPM state desync and work handling in nau8310_dsp_init()
- [High] ASoC: nau8310: uncanceled load_fw_work on probe error and component remove
- [Medium] ASoC: nau8310: unchecked regmap_read() in nau8310_biq_coeff_get()
- [Medium] ASoC: nau8310: uninitialized kcs_rst on empty reply in nau8310_dsp_kcs_setup()
- [Medium] ASoC: nau8310: races on bias_level and clock in nau8310_dsp_snd_soc_dapm_put()
- [Medium] ASoC: nau8310: unmuting DAC on error in nau8310_dsp_set_kcs_setup()
- [Medium] ASoC: nau8310: broken Sense DAPM pin and ADCDAT_OE lifecycle management
- [Medium] ASoC: nau8310: nau8310_set_tdm_slot() bitwise-ORs encoded TX slot selectors
- [Medium] ASoC: nau8310: broken clock config for 64kHz/96kHz, NAU8310_RATES, and 1x/2x mult
- [Medium] ASoC: nau8310: nau8310_resume() skips firmware load when DSP is bypassed
- [Medium] ASoC: nau8310: race between load_fw_work and DAPM clock events on resume
- [Medium] ASoC: nau8310: nau8310_dac_oversampl_enum exposes reserved register value
- [Low] ASoC: nau8310: broken route cleanup in nau8310_dsp_setup_controls()
- [Low] ASoC: nau8310: off-by-one KCS size limit in NAU8310_DSP_KCS_DAT_LEN_MAX
- [Low] ASoC: nau8310: wrong dB scale unit in adc_vol_tlv definition --
commit b7ad2d10b4218feb399fff7738dd084543f6cdfc
Author: Neo Chang <[email protected]>
ASoC: codecs: nau8310: Add support for NAU83G10 amplifier
This commit adds an ASoC codec driver for the Nuvoton NAU83G10/20 mono boosted Class-D audio amplifier with integrated DSP and I/V-sense support.
Quoted 5 lines
sound/soc/codecs/nau8310-dsp.c new file +0
-0
[ ... ]
Quoted 15 lines
sound/soc/codecs/nau8310-dsp.c new file +15
-0+static int nau8310_reply_from_dsp (struct snd_soc_component *cp, +const struct nau8310_cmd_info *cmd_info,int data_size, +void *data) +{ +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); +struct device *dev = cp->dev; +int ret, frag_len, pad_len_exp, data_count =0 ; + + ret =nau8310_dsp_replied (nau8310, &frag_len); +if (ret) +return ret; +if (!frag_len || !cmd_info->reply_data) +return 0 ; +if (!data || frag_len ==1 ) +return -EINVAL ;
[ ... ]
Quoted 32 lines
sound/soc/codecs/nau8310-dsp.c new file +32
-0+static int nau8310_dsp_kcs_setup (struct snd_soc_component *cp,int offset,int size, +const void *data) +{ +u8 *data_buf = (u8 *)data; +unsigned int kcs_rst; +int retries =0 , ret, data_len, data_rem, addr_offset; + +/* Limit full load of KCS_SETUP data and not beyond 3kB offset. */ +if (!data || size >NAU8310_DSP_KCS_DAT_LEN_MAX || + offset >NAU8310_DSP_KCS_OFFSET_MAX ) +return -EINVAL ; + +/* sending fragments for KCS setup */ + addr_offset = offset; + data_rem = size; + +while (data_rem) { + data_len =min (data_rem,NAU8310_DSP_KCS_TX_MAX ); + + ret =nau8310_dsp_exec_command (cp,NAU8310_DSP_CMD_SET_KCS_SETUP , + addr_offset, data_len, (void *)data_buf,0 , &kcs_rst); +if (ret) { +if (retries++ <NAU8310_DSP_RETRY_MAX ) +continue ; +return ret; + } + + data_buf += (u8 )data_len; + addr_offset += data_len; + data_rem -= data_len; + +/* checking KCS result */
sound/soc/codecs/nau8310-dsp.c line 497
Quoted 10 lines
sound/soc/codecs/nau8310-dsp.c new file +10
-0+ ret =nau8310_dsp_exec_command (cp,NAU8310_DSP_CMD_GET_KCS_RSLTS , +0 ,NAU8310_DSP_DATA_BYTE ,NULL ,NAU8310_DSP_DATA_BYTE , &kcs_rst); +if (ret) +return ret; +if (kcs_rst !=NAU8310_DSP_KCS_RSLTS_SUCCESS ) +return -EINVAL ; + } + +return 0 ; +}
[Severity: Medium]
Can nau8310_dsp_kcs_setup() evaluate an uninitialized kcs_rst if the DSP replies with frag_len == 0? When frag_len is 0, nau8310_reply_from_dsp() returns 0 even when cmd_info->reply_data is true, leaving kcs_rst unmodified on the stack.
Quoted 28 lines
sound/soc/codecs/nau8310-dsp.c new file +28
-0+ +static int nau8310_dsp_snd_soc_dapm_put (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol) +{ +struct snd_soc_dapm_context *dapm =snd_soc_dapm_kcontrol_to_dapm (kcontrol); +struct snd_soc_component *component =snd_soc_dapm_to_component (dapm); +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (component); +struct soc_mixer_control *mc = + (struct soc_mixer_control *)kcontrol->private_value; +unsigned int mask = (1 <<fls (mc->max)) -1 ; +unsigned int val; +int ret; + +if (snd_soc_dapm_get_bias_level (dapm) >SND_SOC_BIAS_STANDBY ) { +dev_warn_ratelimited (nau8310->dev,"changing path is not allowed during playback" ); +return -EBUSY ; + } + + val = (ucontrol->value.integer.value[0 ] & mask); + +mutex_lock (&nau8310->lock); +if (val ==NAU8310_DAC_SRC_DSP && !nau8310->load_fw_done) { +dev_warn_ratelimited (nau8310->dev,"Cannot enable DSP: Firmware not ready or disabled" ); +mutex_unlock (&nau8310->lock); +return -EBUSY ; + } +mutex_unlock (&nau8310->lock); +
sound/soc/codecs/nau8310-dsp.c line 535
Quoted 10 lines
sound/soc/codecs/nau8310-dsp.c new file +10
-0+ ret =snd_soc_dapm_put_volsw (kcontrol, ucontrol); + +if (val ==NAU8310_DAC_SRC_BYPASS ) { +/* Switch the clock source of DSP to MCLK. */ +regmap_update_bits (nau8310->regmap,NAU8310_R04_ENA_CTRL , +NAU8310_DSP_SEL_OSC ,0 ); + } + +return ret; +}
[Severity: Medium]
Can nau8310_dsp_snd_soc_dapm_put() race with stream startup and firmware loading?
CPU0 kworkernau8310_dsp_snd_soc_dapm_put() nau8310_load_fw_work()
snd_soc_dapm_get_bias_level(dapm)
/* val == NAU8310_DAC_SRC_BYPASS */
mutex_lock(&nau8310->lock)
mutex_unlock(&nau8310->lock)
mutex_lock(&nau8310->lock)
nau8310_dsp_init() snd_soc_dapm_put_volsw()
regmap_update_bits(...,
NAU8310_DSP_SEL_OSC, 0) <-- switches DSP clock off OSC
during firmware init
snd_soc_dapm_get_bias_level(dapm) is checked without holding card->dapm_mutex and before waiting on nau8310->lock, so playback can start before snd_soc_dapm_put_volsw() runs. In addition, when val is NAU8310_DAC_SRC_BYPASS, NAU8310_DSP_SEL_OSC is cleared outside nau8310->lock without flushing load_fw_work.
[ ... ]
Quoted 2 lines
sound/soc/codecs/nau8310-dsp.c new file +2
-0+static int nau8310_dsp_set_kcs_setup (struct snd_soc_component *cp) +{
[ ... ]
sound/soc/codecs/nau8310-dsp.c line 653
Quoted 8 lines
sound/soc/codecs/nau8310-dsp.c new file +8
-0+err_loaded: +regmap_update_bits (regmap,NAU8310_R13_MUTE_CTRL ,NAU8310_SOFT_MUTE ,0 ); +if (fw) +release_firmware (fw); +err: + nau8310->load_fw_done = false; +return ret; +}
[Severity: Medium]
As reported by Mark Brown on v1 [1], doesn't clearing NAU8310_SOFT_MUTE to 0 on the err_loaded path unmute the DAC when firmware setup fails? In nau8310_dac_event(), setting NAU8310_SOFT_MUTE mutes the amplifier and 0 unmutes it.
Quoted 5 lines
sound/soc/codecs/nau8310-dsp.c new file +5
-0+ +int nau8310_dsp_init (struct snd_soc_component *cp) +{ +struct regmap *regmap = cp->regmap; +
sound/soc/codecs/nau8310-dsp.c line 668
Quoted 10 lines
sound/soc/codecs/nau8310-dsp.c new file +10
-0+nau8310_sw_reset_chip (regmap); + +/* wait for the power ready */ +msleep (120 ); + +regmap_update_bits (regmap,NAU8310_R1A_DSP_CORE_CTRL2 , +NAU8310_DSP_RUNSTALL |NAU8310_DAC_SEL_DSP_OUT ,NAU8310_DAC_SEL_DSP_OUT ); + +return nau8310_dsp_set_kcs_setup (cp); +}
[Severity: High]
Does setting NAU8310_DAC_SEL_DSP_OUT directly in nau8310_dsp_init() desynchronize the hardware state from DAPM?
During nau8310_codec_probe(), nau8310_dsp_setup_controls() registers the "DAC data from DSP" switch while load_fw_work is sleeping in msleep(120), so dapm_set_mixer_path_status() reads NAU8310_R1A_DSP_CORE_CTRL2 while NAU8310_DAC_SEL_DSP_OUT is still 0 and marks the route disconnected. When nau8310_dsp_init() then sets NAU8310_DAC_SEL_DSP_OUT via regmap_update_bits(), hardware routes the DAC from the DSP, but DAPM leaves the DSP, DSP Clock, Capture Sense, SAR, ADC_I and ADC_V widgets unpowered during playback.
Also, in nau8310_load_fw_work(), nau8310->lock is released before the err label clears NAU8310_DAC_SEL_DSP_OUT and NAU8310_DSP_SEL_OSC on failure, leaving a window where nau8310_dsp_active() is true while load_fw_done is false.
Quoted 17 lines
sound/soc/codecs/nau8310-dsp.c new file +17
-0+ +int nau8310_dsp_setup_controls (struct snd_soc_component *cp) +{ +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); +struct snd_soc_dapm_context *dapm = nau8310->dapm; +int ret; + + ret =snd_soc_dapm_new_controls (dapm, nau8310_dsp_dapm_widgets, +ARRAY_SIZE (nau8310_dsp_dapm_widgets)); +if (ret) { +dev_err (cp->dev,"Add DSP widget fail (%d)" , ret); + goto err; + } + + ret =snd_soc_dapm_add_routes (dapm, nau8310_dsp_dapm_routes, +ARRAY_SIZE (nau8310_dsp_dapm_routes)); +if (ret) {
sound/soc/codecs/nau8310-dsp.c line 695
Quoted 10 lines
sound/soc/codecs/nau8310-dsp.c new file +10
-0+dev_err (cp->dev,"Add DSP route fail (%d)" , ret); + goto err; + } + +return 0 ; +err: +snd_soc_dapm_del_routes (dapm, nau8310_dsp_dapm_routes, +ARRAY_SIZE (nau8310_dsp_dapm_routes)); +return ret; +}
[Severity: Low]
If snd_soc_dapm_new_controls() fails, doesn't jumping to err call snd_soc_dapm_del_routes() on routes that were never added? In addition, nau8310_dsp_dapm_routes contains a route with a control ("Switch"), which snd_soc_dapm_del_route() does not support removing, and ASoC core already cleans up component DAPM widgets and routes when component probe fails.
Quoted 6 lines
sound/soc/codecs/nau8310-dsp.h new file +0
-0@@ -0,0 +1,111 @@
[ ... ]
sound/soc/codecs/nau8310-dsp.h line 21
Quoted 2 lines
sound/soc/codecs/nau8310-dsp.h new file +2
-0+#define NAU8310_DSP_KCS_DAT_LEN_BITS 10 +#define NAU8310_DSP_KCS_DAT_LEN_MAX ((1 << NAU8310_DSP_KCS_DAT_LEN_BITS) - 1)
[Severity: Low]
Should NAU8310_DSP_KCS_DAT_LEN_MAX be (1 << NAU8310_DSP_KCS_DAT_LEN_BITS) (1024) rather than 1023? Defining it as 1023 causes nau8310_dsp_kcs_setup() and nau8310_send_dsp_command() to reject a full 1 KB KCS payload with -EINVAL or -ERANGE.
Quoted 5 lines
sound/soc/codecs/nau8310.c new file +0
-0
[ ... ]
Quoted 12 lines
sound/soc/codecs/nau8310.c new file +12
-0+static int nau8310_biq_coeff_get (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol) +{ +struct snd_soc_component *cp =snd_kcontrol_chip (kcontrol); +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); +struct soc_bytes_ext *params = (void *)kcontrol->private_value; +int i, reg, reg_val; +u16 *val; + + val = (u16 *)ucontrol->value.bytes.data; + reg =NAU8310_R80_BIQ0_COE_1 ; +for (i =0 ; i < params->max /sizeof (u16); i++) {
sound/soc/codecs/nau8310.c line 345
Quoted 10 lines
sound/soc/codecs/nau8310.c new file +10
-0+regmap_read (nau8310->regmap, reg + i, ®_val); +/* conversion of 16-bit integers between native CPU format +* and big endian format +*/ + reg_val =cpu_to_be16 (reg_val); + *(val + i) = reg_val; + } + +return 0 ; +}
[Severity: Medium]
If regmap_read() fails in nau8310_biq_coeff_get(), doesn't this copy the uninitialized stack variable reg_val into ucontrol->value.bytes.data and return 0 to userspace?
sound/soc/codecs/nau8310.c line 356
Quoted 11 lines
sound/soc/codecs/nau8310.c new file +11
-0+ +static int nau8310_biq_coeff_put (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol) +{ +struct snd_soc_dapm_context *dapm =snd_soc_dapm_kcontrol_to_dapm (kcontrol); +struct snd_soc_component *cp =snd_soc_dapm_to_component (dapm); +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); +struct soc_bytes_ext *params = (void *)kcontrol->private_value; +void *data; +u16 *val, value; +int i, reg, ret;
[Severity: High]
Doesn't calling snd_soc_dapm_kcontrol_to_dapm(kcontrol) in nau8310_biq_coeff_put() cause type confusion on kcontrol->private_data and dereference an invalid pointer?
"BIQ Coefficients" is registered in nau8310_snd_controls as a standard component control, so kcontrol->private_data is struct snd_soc_component *, not struct dapm_kcontrol_data *. Writing to this control will crash in snd_soc_dapm_kcontrol_to_dapm(); it should use snd_soc_kcontrol_component(kcontrol) like the other controls.
Also, both nau8310_biq_coeff_get() and nau8310_biq_coeff_put() store and read __be16 values through int reg_val and u16 *val instead of __be16 *, which triggers sparse endianness warnings.
[ ... ]
sound/soc/codecs/nau8310.c line 393
Quoted 6 lines
sound/soc/codecs/nau8310.c new file +6
-0+static const char *const nau8310_dac_oversampl[] = { +"64" ,"256" ,"128" ,"" ,"32" }; + +static const struct soc_enum nau8310_dac_oversampl_enum = +SOC_ENUM_SINGLE (NAU8310_R29_DAC_CTRL1 ,NAU8310_DAC_OVERSAMPLE_SFT , +ARRAY_SIZE (nau8310_dac_oversampl), nau8310_dac_oversampl);
[Severity: Medium]
Does exposing index 3 ("") in nau8310_dac_oversampl_enum allow userspace to write the reserved value 3 to NAU8310_R29_DAC_CTRL1? In osr_dac_sel[], index 3 has osr = 0, which causes nau8310_clock_check() and nau8310_hw_params() to fail with -EINVAL. Using SOC_VALUE_ENUM_SINGLE() would avoid exposing the reserved setting.
[ ... ]
sound/soc/codecs/nau8310.c line 437
Quoted 2 lines
sound/soc/codecs/nau8310.c new file +2
-0+static const DECLARE_TLV_DB_MINMAX(adc_vol_tlv, 0, 24125); +static const DECLARE_TLV_DB_MINMAX(pga_gain_tlv, 0, 1600);
[Severity: Low]
Should the maximum gain in adc_vol_tlv be 2412 (24.12 dB) instead of 24125? DECLARE_TLV_DB_MINMAX() takes values in 0.01 dB units, so 24125 reports 241.25 dB to userspace for 0xc1 steps of 0.125 dB.
[ ... ]
Quoted 8 lines
sound/soc/codecs/nau8310.c new file +8
-0+static int nau8310_clock_event (struct snd_soc_dapm_widget *w, +struct snd_kcontrol *kcontrol,int event) +{ +struct snd_soc_component *cp =snd_soc_dapm_to_component (w->dapm); +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); + +switch (event) { +case SND_SOC_DAPM_PRE_PMU :
sound/soc/codecs/nau8310.c line 536
Quoted 10 lines
sound/soc/codecs/nau8310.c new file +10
-0+/* Switch the clock source of DSP to MCLK. */ +regmap_update_bits (nau8310->regmap,NAU8310_R04_ENA_CTRL , +NAU8310_DSP_SEL_OSC ,0 ); +break ; +default : +return -EINVAL ; + } + +return 0 ; +}
[Severity: Medium]
As reported by Mark Brown on v1 [1], can nau8310_clock_event() and nau8310_dsp_clock_event() race with nau8310_load_fw_work() during system resume?
CPU0 kworkersnd_soc_card_resume_deferred()
nau8310_resume()
nau8310_dsp_fw_load()
/* sets NAU8310_DSP_SEL_OSC */
schedule_work(&load_fw_work)
snd_soc_dapm_sync()
nau8310_clock_event()
regmap_update_bits(...,
NAU8310_DSP_SEL_OSC, 0) nau8310_load_fw_work()
nau8310_dsp_init()
<-- DSP clock switched off
OSC during firmware loadnau8310_clock_event() clears NAU8310_DSP_SEL_OSC without holding nau8310->lock or flushing load_fw_work, and nau8310_dsp_clock_event() does not flush load_fw_work before taking nau8310->lock and clearing NAU8310_DSP_SEL_OSC.
[ ... ]
Quoted 22 lines
sound/soc/codecs/nau8310.c new file +22
-0+static int nau8310_adc_event (struct snd_soc_dapm_widget *w, +struct snd_kcontrol *kcontrol,int event) +{ +struct snd_soc_component *cp =snd_soc_dapm_to_component (w->dapm); +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); +struct regmap *regmap = nau8310->regmap; + +switch (event) { +case SND_SOC_DAPM_POST_PMU : +regmap_update_bits (regmap,NAU8310_R0E_I2S_PCM_CTRL2 , +NAU8310_I2S_ADCDAT_OE_MASK ,NAU8310_I2S_ADCDAT_OE_EN ); +break ; +case SND_SOC_DAPM_POST_PMD : +regmap_update_bits (regmap,NAU8310_R0E_I2S_PCM_CTRL2 , +NAU8310_I2S_ADCDAT_OE_MASK ,NAU8310_I2S_ADCDAT_OE_DIS ); +break ; +default : +return -EINVAL ; + } + +return 0 ; +}
[ ... ]
Quoted 24 lines
sound/soc/codecs/nau8310.c new file +24
-0+static int nau8310_clock_apply (struct nau8310 *nau8310, +const struct nau8310_srate_attr *srate_table, +int n1_sel,int mclk_mult_sel,int n2_sel,int dsp_mult_sel) +{ +struct regmap *regmap = nau8310->regmap; +int mult_sel; +unsigned int ctrl_val =0 ; + +static const unsigned int mclkx_val[] = { +/* multiplier 4x, i.e. 2^2 */ + [2 ] =NAU8310_MCLK4XEN , +/* multiplier 8x, i.e. 2^3 */ + [3 ] =NAU8310_MCLK8XEN |NAU8310_MCLK4XEN , +/* multiplier 16x, i.e. 2^4 */ + [4 ] =NAU8310_MCLK16XEN |NAU8310_MCLK8XEN |NAU8310_MCLK4XEN , + }; + +if (srate_table->adc_div <0 || + dsp_mult_sel <0 || dsp_mult_sel >=ARRAY_SIZE (dsp_src_mult) || + n2_sel <0 || n2_sel >=ARRAY_SIZE (mclk_n2_div) || + n1_sel <0 || n1_sel >=ARRAY_SIZE (mclk_n1_div)) { +dev_err (nau8310->dev,"The fs %d isn't support in DSP." , srate_table->fs); +return -EINVAL ; + }
[ ... ]
Quoted 8 lines
sound/soc/codecs/nau8310.c new file +8
-0+if (mult_sel <2 || mult_sel >4 ) +return -EINVAL ; + +regmap_update_bits (regmap,NAU8310_R68_ANALOG_CONTROL_7 , +NAU8310_MCLKX_MASK , mclkx_val[mult_sel]); + +return 0 ; +}
sound/soc/codecs/nau8310.c line 736
Quoted 10 lines
sound/soc/codecs/nau8310.c new file +10
-0+ +static int nau8310_calc_dsp_clk (int mclk,int fs,int mult_param,int n1_param) +{ +int dsp_clk = (mclk << mult_param) / n1_param; + +if (dsp_clk >= (fs *DSP_CLK_MIN_MULTI_VALUE ) && dsp_clk <=DSP_CLK_MAX ) +return dsp_clk; + +return 0 ; +}
[Severity: Medium]
Can 64 kHz or 96 kHz ever succeed in nau8310_clock_process(), even when the DSP is bypassed?
nau8310_clock_process() unconditionally calls nau8310_clkdsp_choose() -> nau8310_calc_dsp_clk(), which requires fs * 2000 <= dsp_clk <= 102400000; for 64 kHz and 96 kHz, fs * 2000 exceeds 102400000, so it always fails with -EINVAL. For 96 kHz, target_srate_table[] also sets adc_div = -1, which nau8310_clock_apply() unconditionally rejects, and NAU8310_RATES advertises SNDRV_PCM_RATE_8000_192000 even though 11.025, 22.05, 88.2, 176.4 and 192 kHz are absent from target_srate_table[].
In addition, nau8310_clock_apply() rejects mult_sel < 2 (1x and 2x multipliers in mclk_src_mult[] and dsp_src_mult[]) with -EINVAL after already writing NAU8310_R03_CLK_CTRL, NAU8310_R40_CLK_DET_CTRL and NAU8310_R04_ENA_CTRL.
[ ... ]
Quoted 3 lines
sound/soc/codecs/nau8310.c new file +3
-0+static int nau8310_set_tdm_slot (struct snd_soc_dai *dai,unsigned int tx_mask, +unsigned int rx_mask,int slots,int slot_width) +{
[ ... ]
sound/soc/codecs/nau8310.c line 1187
Quoted 6 lines
sound/soc/codecs/nau8310.c new file +6
-0+for (i =0 ; i <NAU8310_TDM_MAX_SLOTS ; i++) +if (tx_mask &BIT (i)) { + ctrl_val |= nau8310_tx_slot_map[i]; +dev_dbg (dev,"NAU8310_ADC_%c_SEL_SLOT%d" , + (i %2 ==0 ) ?'V' :'I' , i); + }
[Severity: Medium]
Doesn't bitwise-ORing nau8310_tx_slot_map[i] across all set bits in tx_mask corrupt NAU8310_ADC_V_SEL and NAU8310_ADC_I_SEL when multiple even or odd slots are set in tx_mask? NAU8310_ADC_V_SEL and NAU8310_ADC_I_SEL are 3-bit binary-encoded fields (0x4..0x7), so ORing slot 2 (0x5) and slot 4 (0x6) produces slot 6 (0x7). Also, slots is a signed int only checked against !slots and slots > NAU8310_TDM_MAX_SLOTS, so negative slots or mask bits >= slots are not rejected.
[ ... ]
Quoted 20 lines
sound/soc/codecs/nau8310.c new file +20
-0+static int nau8310_startup (struct snd_pcm_substream *substream, +struct snd_soc_dai *dai) +{ +struct snd_soc_component *cp = dai->component; +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); + +flush_work (&nau8310->load_fw_work); + +if (nau8310_dsp_active (cp) && !nau8310->load_fw_done) { +dev_warn (nau8310->dev,"DSP firmware is not ready yet!" ); +return -EBUSY ; + } + +if (nau8310_dsp_active (cp)) +snd_soc_dapm_enable_pin (nau8310->dapm,"Sense" ); + +return 0 ; +} + +static void nau8310_shutdown(struct snd_pcm_substream *substream,
sound/soc/codecs/nau8310.c line 1237
Quoted 10 lines
sound/soc/codecs/nau8310.c new file +10
-0+ struct snd_soc_dai *dai) +{ +struct snd_soc_component *cp = dai->component; +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); + +if (!nau8310_dsp_active (cp)) +return ; + +snd_soc_dapm_disable_pin (nau8310->dapm,"Sense" ); +}
[Severity: Medium]
Does the "Sense" pin and NAU8310_I2S_ADCDAT_OE handling work across bypass, full-duplex, and idle states?
1. "Sense" is a SND_SOC_DAPM_SIGGEN widget and is never disabled in nau8310_codec_probe() or nau8310_resume(). Enabling "DAC data from DSP" or "ALC gain selection mode" while idle completes a Sense -> DAC -> Speaker path, powering up the codec and raising bias_level above SND_SOC_BIAS_STANDBY so nau8310_dsp_snd_soc_dapm_put() rejects further path changes with -EBUSY.
2. nau8310_shutdown() disables "Sense" without checking if the other stream direction is still active and without calling snd_soc_dapm_sync().
3. Once nau8310_shutdown() disables "Sense", switching to DSP bypass mode leaves "Sense" disabled because nau8310_startup() only enables "Sense" when nau8310_dsp_active(cp) is true, preventing Capture (ADC_OUT) and ALC sense from powering up.
4. Both "ALC gain selection mode" and "ADC_OUT" share nau8310_adc_event(), so powering down either widget unconditionally sets NAU8310_I2S_ADCDAT_OE_DIS even if the other widget is still active.
[ ... ]
Quoted 14 lines
sound/soc/codecs/nau8310.c new file +14
-0+static int nau8310_codec_probe (struct snd_soc_component *cp) +{ +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); +struct snd_soc_dapm_context *dapm =snd_soc_component_to_dapm (cp); +int ret =0 ; + + nau8310->dapm = dapm; + +/* Loading DSP firmware */ + ret =nau8310_dsp_fw_load (nau8310); +if (ret) { +dev_err (nau8310->dev,"Loading DSP firmware failed(%d)" , ret); + goto err; + }
sound/soc/codecs/nau8310.c line 1318
Quoted 10 lines
sound/soc/codecs/nau8310.c new file +10
-0+ + ret =nau8310_dsp_setup_controls (cp); +if (ret) { +dev_err (nau8310->dev,"DSP setup controls failed(%d)" , ret); + goto err; + } + +err: +return ret; +}
[Severity: High]
Does nau8310_codec_probe() leave nau8310->load_fw_work running if nau8310_dsp_setup_controls() fails or when the component is later removed?
nau8310_dsp_fw_load() schedules load_fw_work before nau8310_dsp_setup_controls() runs, but the error path does not call cancel_work_sync(&nau8310->load_fw_work), and soc_comp_dev_nau8310 has no .remove callback to cancel the work if component probe fails later or the sound card is unbound while the I2C device stays bound.
[ ... ]
Quoted 9 lines
sound/soc/codecs/nau8310.c new file +9
-0+static int __maybe_unused nau8310_resume (struct snd_soc_component *cp) +{ +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); +struct regmap *regmap = nau8310->regmap; +int ret =0 ; + +regcache_cache_only (regmap, false); + ret =regcache_sync (regmap); +if (ret)
sound/soc/codecs/nau8310.c line 1355
Quoted 10 lines
sound/soc/codecs/nau8310.c new file +10
-0+return ret; + +if (nau8310_dsp_active (cp)) { + ret =nau8310_dsp_fw_load (nau8310); +if (ret) +dev_err (nau8310->dev,"Loading DSP firmware failed(%d)" , ret); + } + +return ret; +}
[Severity: Medium]
If the system suspends and resumes while the DSP is bypassed (!nau8310_dsp_active(cp)), doesn't skipping nau8310_dsp_fw_load() leave nau8310->load_fw_done set to true even though the DSP lost its firmware across suspend? If userspace later enables "DAC data from DSP", nau8310_dsp_snd_soc_dapm_put() will see load_fw_done == true and route audio through the uninitialized DSP.
[1] https://lore.kernel.org/r/179304693306.199157.5210648249546181520.b4-review@b4
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v2 2/2] ASoC: codecs: nau8310: Add support for NAU83G10 amplifier
In reply to Neo ChangQuoted 17 lines · Neo Chang
sound/soc/codecs/nau8310.c new file +17
-0+static void nau8310_init_regs (struct nau8310 *nau8310) +{ +struct regmap *regmap = nau8310->regmap; +unsigned int clk_det =0 , ana_ctrl =0 ; + +/* Latch IIC LSB value */ +regmap_write (regmap,NAU8310_R02_I2C_ADDR ,0x0001 ); +/* Block MCLK */ +regmap_update_bits (regmap,NAU8310_R04_ENA_CTRL , +NAU8310_MCLK_SEL_MASK ,0x7 <<NAU8310_MCLK_SEL_SFT ); +/* stall DSP */ +regmap_update_bits (regmap,NAU8310_R1A_DSP_CORE_CTRL2 , +NAU8310_DSP_RUNSTALL ,NAU8310_DSP_RUNSTALL ); +/* Default ADCI from slot1, ADCV from slot 0. */ +regmap_update_bits (regmap,NAU8310_R0C_TDM_CTRL , +NAU8310_ADC_I_SEL_MASK |NAU8310_ADC_V_SEL_MASK , + (0x4 <<NAU8310_ADC_I_SEL_SFT ) |0x4 );
sound/soc/codecs/nau8310.c line 1508
Quoted 10 lines
sound/soc/codecs/nau8310.c new file +10
-0+/* Set I-sense digital gain to +10.25dB */ +regmap_update_bits (regmap,NAU8310_R14_ADC_VOL_CTRL , +NAU8310_ADC_GAIN_L_MASK |NAU8310_ADC_GAIN_R_MASK , + (0x1 <<NAU8310_ADC_GAIN_L_SFT ) |0x62 ); +/* PGA in Class A Mode */ +regmap_update_bits (regmap,NAU8310_R76_BOOST , +NAU8310_STG2_SEL_CLASSA ,NAU8310_STG2_SEL_CLASSA ); +/* Set I-sense PGA Bias Current to minimum, +* Disable PGA Common Mode Lock. +*/
Even netdev does not use that format of comments anymore. And this is not netdev in the first place.
...
Quoted 28 lines
sound/soc/codecs/nau8310.c new file +28
-0+ nau8310 =devm_kzalloc (dev,sizeof (*nau8310),GFP_KERNEL ); +if (!nau8310) +return -ENOMEM ; + +i2c_set_clientdata (i2c, nau8310); +mutex_init (&nau8310->lock); +INIT_WORK (&nau8310->load_fw_work, nau8310_load_fw_work); + + nau8310->regmap =devm_regmap_init (dev,NULL , i2c, &nau8310_regmap_config); +if (IS_ERR (nau8310->regmap)) +return PTR_ERR (nau8310->regmap); + + nau8310->dev = dev; + +nau8310_reset_chip (nau8310->regmap); + ret =regmap_read (nau8310->regmap,NAU8310_R46_I2C_DEVICE_ID , &value); +if (ret) { +dev_err (dev,"Failed to read NAU8310/20 device id: %d" , ret); +return ret; + } + + nau8310->silicon_id = value &NAU8310_REG_SI_REV_MASK ; + ret =nau8310_read_device_properties (nau8310); +if (ret) +return ret; + +nau8310_print_device_properties (nau8310); +nau8310_init_regs (nau8310);
sound/soc/codecs/nau8310.c line 1788
Quoted 10 lines
sound/soc/codecs/nau8310.c new file +10
-0+ +return devm_snd_soc_register_component (dev, &soc_comp_dev_nau8310, + &nau8310_dai,1 ); +} + +static void nau8310_i2c_remove (struct i2c_client *client) +{ +struct nau8310 *nau8310 =i2c_get_clientdata (client); + +cancel_work_sync (&nau8310->load_fw_work);
This looks safe, but still error-prone and discouraged. Use same order of cleanup as order of init/allocation in probe.
See also include/linux/devm-helpers.h
sound/soc/codecs/nau8310.c line 1800
Quoted 12 lines
sound/soc/codecs/nau8310.c new file +12
-0+} + +static const struct i2c_device_id nau8310_i2c_ids[] = { + { .name ="nau8310" }, + { .name ="nau8320" }, + {} +}; +MODULE_DEVICE_TABLE (i2c, nau8310_i2c_ids); + +static const struct of_device_id nau8310_of_ids[] = { + { .compatible ="nuvoton,nau8310" }, + { .compatible ="nuvoton,nau8320" },
This suggests devices are compatible, but was not expressed in the binding (and then second entry is redundant).
Best regards,
Krzysztof
Mark Brown
maintainerRe: [PATCH v2 2/2] ASoC: codecs: nau8310: Add support for NAU83G10 amplifier
In reply to Neo Changsound/soc/codecs/Kconfig config SND_SOC_MT6660 · line 2927
Quoted 6 lines
Add support for the Nuvoton NAU83G10 audio amplifier. The NAU83G10 is a mono boost Class-D audio amplifier with integrated DSP and I/V-sense support.
sound/soc/codecs/Kconfig +2
-0+config SND_SOC_NAU8310 + tristate "Nuvoton Technology Corporation NAU83G10 Mono Boosted Amplifier"
This handles NAU83G20 as well.
sound/soc/codecs/nau8310-dsp.c line 129
Quoted 13 lines
sound/soc/codecs/nau8310-dsp.c new file +13
-0+static int nau8310_dsp_idle (struct snd_soc_component *cp) +{ +struct nau8310 *nau8310 =snd_soc_component_get_drvdata (cp); +struct regmap *regmap = nau8310->regmap; +unsigned int idle_pattern, timeout =NAU8310_DSP_IDLE_RETRY *USEC_PER_MSEC ; +int ret =0 ; + + ret =regmap_read_poll_timeout (regmap, +NAU8310_RF000_DSP_COMM , idle_pattern, + idle_pattern ==NAU8310_DSP_COMM_IDLE_WORD ,USEC_PER_MSEC , timeout); +if (ret) { +dev_err (nau8310->dev,"Timeout waiting for DSP idle state: %d" , ret); +nau8310_sw_reset_chip (regmap);
If you need to reset the chip you should probably also resync the cache, and there's some redundancy here with nau8310_software_reset().
sound/soc/codecs/nau8310-dsp.c line 665
Quoted 11 lines
sound/soc/codecs/nau8310-dsp.c new file +11
-0+int nau8310_dsp_init (struct snd_soc_component *cp) +{ +struct regmap *regmap = cp->regmap; + +nau8310_sw_reset_chip (regmap); + +/* wait for the power ready */ +msleep (120 ); + +regmap_update_bits (regmap,NAU8310_R1A_DSP_CORE_CTRL2 , +NAU8310_DSP_RUNSTALL |NAU8310_DAC_SEL_DSP_OUT ,NAU8310_DAC_SEL_DSP_OUT );
NAU8310_DAC_SEL_DSP_OUT is used by DAPM, you might want to solve this the way nau8360 does.
sound/soc/codecs/nau8310.c line 351
Quoted 5 lines
sound/soc/codecs/nau8310.c new file +5
-0+static int nau8310_biq_coeff_put (struct snd_kcontrol *kcontrol, +struct snd_ctl_elem_value *ucontrol) +{ +struct snd_soc_dapm_context *dapm =snd_soc_dapm_kcontrol_to_dapm (kcontrol); +struct snd_soc_component *cp =snd_soc_dapm_to_component (dapm);
This is the same issue as was fixed in nau8310_clkdet_put() in this version.
sound/soc/codecs/nau8310.c line 456
Quoted 3 lines
sound/soc/codecs/nau8310.c new file +3
-0+/* Boost Control Target Value Limit. 0.16 V per step. */ +SOC_SINGLE ("Boost Target Limit" ,NAU8310_R17_BOOST_CTRL1 , + NAU8310_BSTLIMIT_SFT, NAU8310_BSTLIMIT_MAX, 0),
The binding describes nuvoton,boost-target-limit/-margin as having 0.19V steps - either that or the comment here is wrong.
sound/soc/codecs/nau8310.c line 1074
Quoted 3 lines
sound/soc/codecs/nau8310.c new file +3
-0+ ret =nau8310_clock_config (nau8310, fs, nau8310->mclk); +if (ret) +return -EINVAL ;
Just return ret in case it's more descriptive.
sound/soc/codecs/nau8310.c line 1816
Quoted 5 lines
sound/soc/codecs/nau8310.c new file +5
-0+static struct i2c_driver nau8310_i2c_driver = { + .driver = { + .name ="nau8310" , + .of_match_table =of_match_ptr (nau8310_of_ids), + .acpi_match_table =ACPI_PTR (nau8310_acpi_match),
We don't need the wrappers for these pointers these days.
Signature · 1 line
Mark Brown <[email protected]>