ASoC: Add Richtek RT5512 Speaker Amp Driver
Subsystems (4)
PATCH 1/1 ASoC: Add Richtek RT5512 Speaker Amp Driver
From: Jeff <[email protected]>
The RT5512 is a boosted class-D amplifier with V/I sensing.
A built-in DC-DC step-up converter is used to provide efficient power for class-D amplifier with multi-level class-H operation. The digital audio interface supports I2S, left-justified, right-justified and TDM format for audio in with a data out used for chip information like voltage sense and current sense, which are able to be monitored via DATAO pin through proper register setting.
--- .../bindings/sound/richtek,rt5512.yaml | 50 + sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/rt5512.c | 860 ++++++++++++++++++ 4 files changed, 922 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/richtek,rt5512.yaml create mode 100644 sound/soc/codecs/rt5512.c
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +50 -0
@@ -0,0 +1,50 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/richtek,rt5512.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Richtek RT5512 speaker amplifier + +maintainers: + - SHIH CHIA CHANG <[email protected]> + +description: | + The RT5512B is a boosted class-D amplifier with V/I sensing. + A built-in DC-DC step-up converter is used to provide efficient power for + class-D amplifier with multi-level class-H operation. The digital audio + interface supports I2S, left-justified, right-justified and TDM format for + audio in with a data out used for chip information like voltage sense and + current sense, which are able to be monitored via DATAO pin through proper + register setting. + +properties: + compatible: + enum: + - richtek,rt5150 + + reg: + description: I2C device address + maxItems: 1 + + '#sound-dai-cells': + const: 0 + +required: + - compatible + - reg + - '#sound-dai-cells' + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + rt5512@5c { + compatible = "richtek,rt5512"; + reg = <0x5c>; + #sound-dai-cells = <0>; + }; + };
sound/soc/codecs/Kconfig +10 -0
@@ -176,6 +176,7 @@ config SND_SOC_ALL_CODECS imply SND_SOC_RT1019 imply SND_SOC_RT1305 imply SND_SOC_RT1308 + imply SND_SOC_RT5512 imply SND_SOC_RT5514 imply SND_SOC_RT5616 imply SND_SOC_RT5631
@@ -1311,6 +1312,15 @@ config SND_SOC_RT1316_SDW depends on SOUNDWIRE select REGMAP_SOUNDWIRE +config SND_SOC_RT5512 + tristate "Mediatek RT5512 speaker amplifier" + depends on I2C + help + Config of RT5512. It is a Smart power + amplifier which contain speaker protection, + multi-band DRC, equalizer function. It would + be set to yes if device with RT5512 on it. + config SND_SOC_RT5514 tristate depends on I2C
sound/soc/codecs/Makefile +2 -0
@@ -199,6 +199,7 @@ snd-soc-rt1316-sdw-objs := rt1316-sdw.o snd-soc-rt274-objs := rt274.o snd-soc-rt286-objs := rt286.o snd-soc-rt298-objs := rt298.o +snd-soc-rt5512-objs := rt5512.o snd-soc-rt5514-objs := rt5514.o snd-soc-rt5514-spi-objs := rt5514-spi.o snd-soc-rt5616-objs := rt5616.o
@@ -554,6 +555,7 @@ obj-$(CONFIG_SND_SOC_RT1316_SDW) += snd-soc-rt1316-sdw.o obj-$(CONFIG_SND_SOC_RT274) += snd-soc-rt274.o obj-$(CONFIG_SND_SOC_RT286) += snd-soc-rt286.o obj-$(CONFIG_SND_SOC_RT298) += snd-soc-rt298.o +obj-$(CONFIG_SND_SOC_RT5512) += snd-soc-rt5512.o obj-$(CONFIG_SND_SOC_RT5514) += snd-soc-rt5514.o obj-$(CONFIG_SND_SOC_RT5514_SPI) += snd-soc-rt5514-spi.o obj-$(CONFIG_SND_SOC_RT5514_SPI_BUILTIN) += snd-soc-rt5514-spi.o
sound/soc/codecs/rt5512.c new file +860 -0
860 lines not on this page · raw message
Signature · 1 line
2.17.1
Rob Herring
maintainerQuoted 11 lines · Jeff Chang
From: Jeff <[email protected]>
The RT5512 is a boosted class-D amplifier with V/I sensing.
A built-in DC-DC step-up converter is used to provide efficient power for class-D amplifier with multi-level class-H operation. The digital audio interface supports I2S, left-justified, right-justified and TDM format for audio in with a data out used for chip information like voltage sense and current sense, which are able to be monitored via DATAO pin through proper register setting.Signed-off-by SHIH CHIA CHANG <[email protected]>
Quoted 9 lines
--- .../bindings/sound/richtek,rt5512.yaml | 50 + sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/rt5512.c | 860 ++++++++++++++++++ 4 files changed, 922 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/richtek,rt5512.yaml create mode 100644 sound/soc/codecs/rt5512.c
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:Documentation/devicetree/bindings/sound/richtek,rt5512.example.dtb:0:0: /example-0/i2c/rt5512@5c: failed to match any schema with compatible: ['richtek,rt5512']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/
This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit.
Mark Brown
maintainerQuoted 5 lines · Jeff Chang
From: Jeff <[email protected]>
The RT5512 is a boosted class-D amplifier with V/I sensing.
A built-in DC-DC step-up converter is used to provide efficient power for class-D amplifier with multi-level class-H operation. The digital audio
There's a few issues to clean up here but they're mostly stylistic for the upstream kernel rather than major structural things I think.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml
Quoted 2 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +0
-0
The DT bindings seem fine but normally they'd be sent as a separate patch so it's easier for the DT reviewers to find them.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml
Quoted 2 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +2
-0+config SND_SOC_RT5512 + tristate "Mediatek RT5512 speaker amplifier"
Looks like there's some Richtek/Mediatek branding confusion with this - it's a bit unclear. It's all the same company in the end I guess so it doesn't matter.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 4
Quoted 5 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +4
-0@@ -0,0 +1,860 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2021 Mediatek Inc. + */
Please make the entire comment a C++ one so things look more intentional.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 16
Quoted 12 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +12
-0+static int rt5512_codec_dac_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_component *component = + snd_soc_dapm_to_component(w->dapm); + int ret = 0; + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + /* un-mute */ + ret = snd_soc_component_update_bits(component, 0x03, 0x0002, 0); + break;
I'm not seeing what turns the mute on? I'm also not loving all the magic numbers in code here, these don't look like things that I'd expect to be undocumented and it's an issue through the code.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 21
Quoted 5 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +5
-0+ 0xf9fc); + mdelay(11); + + dev_info(component->dev, "%s rt5512 update bst mode %d\n", + __func__, chip->bst_mode);
All these dev_info() prints during normal operation are going to be way too noisy, at most they should be dev_dbg().
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 34
Quoted 13 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +13
-0+static int rt5512_set_bstmode(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = + snd_soc_kcontrol_component(kcontrol); + struct rt5512_chip *chip = snd_soc_component_get_drvdata(component); + int ret = 0; + + chip->bst_mode = ucontrol->value.integer.value[0]; + dev_info(component->dev, "%s, bst_mode = %d\n", __func__, + chip->bst_mode); + return ret; +}
You should run the ALSA kselftests on a system with this driver loaded - this should tell you that the driver needs to return a 1 when the value is changed so that events are generated and will also identify a bunch of other issues.
I'd also expect to see some validation of the value set, I'm guessing that there's only a limited set of modes?
Quoted 12 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +12
-0+static int rt5512_codec_put_istcbypass(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = + snd_soc_kcontrol_component(kcontrol); + int ret; + + pm_runtime_get_sync(component->dev); + if (ucontrol->value.integer.value[0]) { + ret = snd_soc_component_update_bits(component, + RT5512_REG_PATH_BYPASS, + 0x0004, 0x0004);
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 56
Quoted 10 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +10
-0+ } else { + ret = snd_soc_component_update_bits(component, + RT5512_REG_PATH_BYPASS, + 0x0004, 0x0000); + } + if (ret) + dev_err(component->dev, "%s set CC Max Failed\n", __func__); + pm_runtime_put_sync(component->dev); + return ret; +}
Why is this a custom control? It looks like a normal mux.
Quoted 4 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +4
-0+static int rt5512_codec_put_volsw(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component =
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 70
Quoted 10 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +10
-0+ snd_soc_kcontrol_component(kcontrol); + int put_ret = 0; + + pm_runtime_get_sync(component->dev); + put_ret = snd_soc_put_volsw(kcontrol, ucontrol); + if (put_ret < 0) + dev_err(component->dev, "%s put volsw fail\n", __func__); + pm_runtime_put_sync(component->dev); + return put_ret; +}
Same here. The runtime PM stuff here and in all the controls looks wasteful as well, why would you need to power up the device here?
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 75
Quoted 5 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +5
-0+static const DECLARE_TLV_DB_SCALE(vol_ctl_tlv, -1155, 5, 0); +static const struct snd_kcontrol_new rt5512_component_snd_controls[] = { + SOC_SINGLE_EXT_TLV("Volume_Ctrl", RT5512_REG_VOL_CTRL, 0, 255, + 1, rt5512_codec_get_volsw, rt5512_codec_put_volsw, + vol_ctl_tlv),
Volume controls should end in Volume as per control-names.rst, most of the controls here don't seem to follow the ALSA naming coventions at all.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 85
Quoted 10 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +10
-0+static int rt5512_component_setting(struct snd_soc_component *component) +{ + struct rt5512_chip *chip = snd_soc_component_get_drvdata(component); + int ret = 0; + + ret = snd_soc_component_read(component, 0x4d); + if (ret < 0) + return -EIO; + chip->ff_gain = ret; + if (chip->chip_rev == RT5512_REV_A) {
It would be more idiomatic to do a switch statement here.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 86
Quoted 1 line
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +1
-0+ .idle_bias_on = false,
No need to explicitly set things to false, that's the defualt for statics.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 94
Quoted 8 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +8
-0+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + dev_info(dai->dev, "format: 0x%08x, rate: 0x%08x, word_len: %d, aud_bit: %d\n", + params_format(hw_params), params_rate(hw_params), word_len, + aud_bit); + if (word_len > 32 || word_len < 16) { + dev_err(dai->dev, "not supported word length\n"); + return -ENOTSUPP; + }
This could use some blank lines between blocks (as could a lot of the rest), and it's not clear why there's a check for playback here (especially not without reporting an error).
Quoted 6 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +6
-0+static int rt5512_component_aif_hw_free(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_soc_dapm_context *dapm = + snd_soc_component_get_dapm(dai->component); + int ret = 0;
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 110
Quoted 10 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +10
-0+ char *tmp = "SPK"; + + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) + return 0; + dev_info(dai->dev, "%s successfully start\n", __func__); + + ret = snd_soc_dapm_disable_pin(dapm, tmp); + if (ret < 0) + return ret; + return snd_soc_dapm_sync(dapm);
The driver should be doing no DAPM management in PCM operations, the core will manage DAPM for the PCM itself.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 121
Quoted 11 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +11
-0+static inline int _rt5512_chip_sw_reset(struct rt5512_chip *chip) +{ + int ret; + u8 data[2] = {0x00, 0x00}; + u8 reg_data[2] = {0x00, 0x80}; + + /* turn on main pll first, then trigger reset */ + ret = i2c_smbus_write_i2c_block_data(chip->i2c, RT5512_REG_SYSTEM_CTRL, + 2, data); + if (ret < 0) + return ret;
Why is this bypassing regmap?
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 127
Quoted 6 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +6
-0+ chip->chip_rev = id[1]; + if (chip->chip_rev != RT5512_REV_A && chip->chip_rev != RT5512_REV_B) { + dev_err(chip->dev, "%s chip rev not match, rev = %d\n", + __func__, chip->chip_rev); + return -ENODEV; + }
Use a switch statement for exensibility here.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 134
Quoted 7 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +7
-0+static inline int rt5512_component_register(struct rt5512_chip *chip) +{ + return devm_snd_soc_register_component(chip->dev, + &rt5512_component_driver, + rt5512_codec_dai, + ARRAY_SIZE(rt5512_codec_dai)); +}
This may as well just be inlined.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 137
Quoted 3 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +3
-0+ /* register qos to prevent deep idle during transfer */ + cpu_latency_qos_add_request(&chip->rt5512_qos_request, + PM_QOS_DEFAULT_VALUE);
This looks like it's trying to work around some bug in the I2C controller driver perhaps? In any case it needs a lot more explanation if it's sensible to have in a CODEC driver, there's nothing unusual here.
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml line 142
Quoted 5 lines
Documentation/devicetree/bindings/sound/richtek,rt5512.yaml new file +5
-0+ ret = rt5512_component_register(chip); + if (!ret) { + pm_runtime_set_active(chip->dev); + pm_runtime_enable(chip->dev); + }
The driver uses runtime PM operations so you should enable runtime PM before registering the component, it might be used immediately.
Quoted 7 lines · Mark Brown
sound/soc/codecs/Kconfig config SND_SOC_RT1316_SDW · line 1316
Quoted 2 lines · Jeff Chang
sound/soc/codecs/Kconfig +2
-0+config SND_SOC_RT5512 + tristate "Mediatek RT5512 speaker amplifier"Looks like there's some Richtek/Mediatek branding confusion with this - it's a bit unclear. It's all the same company in the end I guess so it doesn't matter.
Interesting, the naming of RT5512 looks like a Realtek codec. ^^
Mark Brown
maintainerQuoted 11 lines · Bard
Quoted 7 lines · Mark Brown
sound/soc/codecs/Kconfig config SND_SOC_RT1316_SDW · line 1316
Quoted 2 lines · Jeff Chang
sound/soc/codecs/Kconfig +2
-0+config SND_SOC_RT5512 + tristate "Mediatek RT5512 speaker amplifier"Looks like there's some Richtek/Mediatek branding confusion with this - it's a bit unclear. It's all the same company in the end I guess so it doesn't matter.
Interesting, the naming of RT5512 looks like a Realtek codec. ^^
Yeah, exactly.
Quoted 10 lines · Jeff Chang
From: Jeff <[email protected]>
The RT5512 is a boosted class-D amplifier with V/I sensing.
A built-in DC-DC step-up converter is used to provide efficient power for class-D amplifier with multi-level class-H operation. The digital audio interface supports I2S, left-justified, right-justified and TDM format for audio in with a data out used for chip information like voltage sense and current sense, which are able to be monitored via DATAO pin through proper register setting.
Quoted 3 lines
Signed-off-by SHIH CHIA CHANG <[email protected]>--- .../bindings/sound/richtek,rt5512.yaml | 50 +
Bindings are separate patches.
Best regards,
Krzysztof