| Message ID | 20220907142124.2532620-1-luca.ceresoli@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for the internal RK3308 audio codec | expand |
On Wed, Sep 07, 2022 at 04:21:16PM +0200, luca.ceresoli@bootlin.com wrote: > Luca Ceresoli (8): > ASoC: rockchip: rk3308: add internal audio codec bindings > ASoC: rockchip: rk3308: add audio card bindings > arm64: dts: rockchip: add i2s_8ch_2 and i2s_8ch_3 > arm64: dts: rockchip: add the internal audio codec > ASoC: rockchip: i2s-tdm: Fix clk_id usage in .set_sysclk() > ASoC: audio-graph: let dai_link->init be overridable > ASoC: codecs: Add RK3308 internal audio codec driver > ASoC: rockchip: add new RK3308 sound card Please pay attention to the ordering of your serieses when posting: generally any bug fixes should come first so that they can be easily sent as fixes, and normally DTS updates are at the very end of the series rather than mixed in the middle since they go via the platform maintainer tree normally rather than with everything else.
Hello Mark, thanks for the quick feedback! On Wed, 7 Sep 2022 16:41:44 +0100 Mark Brown <broonie@kernel.org> wrote: > On Wed, Sep 07, 2022 at 04:21:16PM +0200, luca.ceresoli@bootlin.com wrote: > > > Luca Ceresoli (8): > > ASoC: rockchip: rk3308: add internal audio codec bindings > > ASoC: rockchip: rk3308: add audio card bindings > > arm64: dts: rockchip: add i2s_8ch_2 and i2s_8ch_3 > > arm64: dts: rockchip: add the internal audio codec > > ASoC: rockchip: i2s-tdm: Fix clk_id usage in .set_sysclk() > > ASoC: audio-graph: let dai_link->init be overridable > > ASoC: codecs: Add RK3308 internal audio codec driver > > ASoC: rockchip: add new RK3308 sound card > > Please pay attention to the ordering of your serieses when posting: > generally any bug fixes should come first so that they can be easily > sent as fixes, and normally DTS updates are at the very end of the > series rather than mixed in the middle since they go via the platform > maintainer tree normally rather than with everything else. Sorry about that. I've reordered the patches in my branch for the next iteration. Best regards, Luca
On 07/09/2022 16:21, luca.ceresoli@bootlin.com wrote: > From: Luca Ceresoli <luca.ceresoli@bootlin.com> > > The RK3308 has a built-in audio codec that connects internally to i2s_8ch_2 > or i2s_8ch_3. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > arch/arm64/boot/dts/rockchip/rk3308.dtsi | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3308.dtsi b/arch/arm64/boot/dts/rockchip/rk3308.dtsi > index 093b70563b23..221cde49dc98 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3308.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3308.dtsi > @@ -808,6 +808,20 @@ cru: clock-controller@ff500000 { > assigned-clock-rates = <32768>; > }; > > + acodec: acodec@ff560000 { Node names should be generic. https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Best regards, Krzysztof
On Wed, Sep 07, 2022 at 04:21:23PM +0200, luca.ceresoli@bootlin.com wrote: > +static const char *offon_text[2] = { > + [0] = "Off", > + [1] = "On", > +}; Simple on/off controls should be normal switches, not enums. > +/* ALC AGC Switch */ > +static const struct soc_enum rk3308_agc_enum_array[] = { > + SOC_ENUM_SINGLE(0, 0, ARRAY_SIZE(offon_text), offon_text), > + SOC_ENUM_SINGLE(0, 1, ARRAY_SIZE(offon_text), offon_text), Do not index into arrays of enums with magic numbers, this is hard to read and maintain. Use named variables for the enums like other drivers do. > +static void rk3308_codec_update_zerocross(struct rk3308_codec_priv *rk3308) > +{ > + unsigned int agc_on, value; > + int grp; > + > + /* 14: enable zero-cross detection if AGC enabled, else AGC won't work */ > + for (grp = 0; grp < rk3308->used_adc_grps; grp++) { If you're going to force zero cross on then you need to generate an event on the zero cross control so that UIs get updated, however it might be a bit more idiomatic to either just leave zero cross always on and not offer user control or return an error if the user tries to enable AGC without zero cross (or tries to disable zero cross without AGC). Given that there's very few use cases for disabling zero cross with actual audio usage it may be best to just force it on and worry about offering control for those other use cases later if someone actually needs that, it's probably more trouble than it's worth to handle. > +static int rk3308_codec_agc_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); > + struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component); > + > + } > + > + rk3308_codec_update_zerocross(rk3308); > + > + return 0; > +} _put() operations need to return 1 if the value changed, please run the mixer-test selftest on a system with your driver0 - it'll catch issues like this for you. > + if (any_micbias_enabled) { > + regmap_update_bits(rk3308->regmap, RK3308_ADC_ANA_CON07(0), > + RK3308_ADC_LEVEL_RANGE_MICBIAS_MSK, > + rk3308->micbias_avdd_mult); > + > + /* Wait until the VCMH keep stable */ > + msleep(20); /* estimated value */ > + > + regmap_set_bits(rk3308->regmap, RK3308_ADC_ANA_CON08(0), > + RK3308_ADC_MICBIAS_CURRENT_EN); > + } This should probably be modelled as a SUPPLY widget for the micbiases rather than open coding. > +static int rk3308_codec_micbias_put(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); > + struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component); There should be no reason to offer userspace control over micbiases, they should be modelled as supply widgets like with other drivers. If there's some system specific reason for offering control then the machine driver can handle it. > +static int rk3308_codec_hpout_l_get_tlv(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + return snd_soc_get_volsw_range(kcontrol, ucontrol); > +} Just use the generic function directly, no need for this wrapper. > +static int rk3308_codec_hpout_r_put_tlv(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); > + struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component); > + unsigned int dgain = ucontrol->value.integer.value[0]; > + > + rk3308->hpout_r_dgain = dgain; > + > + return snd_soc_put_volsw_range(kcontrol, ucontrol); > +} Just model the headphone output as a stereo control. > + * There are 8 ADCs and use the same SCLK and LRCK internal for master > + * mode, We need to make sure that they are in effect at the same time, > + * otherwise they will cause the abnormal clocks. > + */ > + if (is_master) > + regmap_clear_bits(rk3308->regmap, RK3308_GLB_CON, RK3308_ADC_DIG_WORK); > + > + for (grp = 0; grp < ADC_LR_GROUP_MAX; grp++) { > + regmap_update_bits(rk3308->regmap, RK3308_ADC_DIG_CON01(grp), > + RK3308_ADC_I2S_LRC_POL_REVERSAL | > + RK3308_ADC_I2S_MODE_MSK, > + adc_aif1); > + regmap_update_bits(rk3308->regmap, RK3308_ADC_DIG_CON02(grp), > + RK3308_ADC_IO_MODE_MASTER | > + RK3308_ADC_MODE_MASTER | > + RK3308_ADC_I2S_BIT_CLK_POL_REVERSAL, > + adc_aif2); > + } > + > + /* Hold ADC Digital registers end at master mode */ > + if (is_master) > + regmap_set_bits(rk3308->regmap, RK3308_GLB_CON, RK3308_ADC_DIG_WORK); This looks like it might glitch other active streams, you should probably have a check to see if the configuration is actually being changed and skip updates entirely if not. > +static int rk3308_mute_stream(struct snd_soc_dai *dai, int mute, int stream) > +{ > + struct snd_soc_component *component = dai->component; > + struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component); > + > + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { > + int dgain; > + > + if (mute) { > + for (dgain = 0x2; dgain <= 0x7; dgain++) { > + /* > + * Keep the max -> min digital CIC interpolation > + * filter gain step by step. > + * > + * loud: 0x2; whisper: 0x7 > + */ > + regmap_update_bits(rk3308->regmap, > + RK3308_DAC_DIG_CON04, > + RK3308_DAC_CIC_IF_GAIN_MSK, > + dgain); > + usleep_range(200, 300); /* estimated value */ > + } I'm not clear why a volume ramp is required here? Generally when the mute operation happens there's no audio running so we'd just be ramping up the noise floor, it's for masking glitches from the controller. > +static int rk3308_codec_digital_fadein(struct rk3308_codec_priv *rk3308) > +{ > + unsigned int dgain, dgain_ref; > + > + if (rk3308->hpout_l_dgain != rk3308->hpout_r_dgain) { > + pr_warn("HPOUT l_dgain: 0x%x != r_dgain: 0x%x\n", > + rk3308->hpout_l_dgain, rk3308->hpout_r_dgain); > + dgain_ref = min(rk3308->hpout_l_dgain, rk3308->hpout_r_dgain); > + } else { > + dgain_ref = rk3308->hpout_l_dgain; > + } Does the device actively need this for masking some issue with clean startup or is this just a nice to have? It should be easy enough to handle asymmatric volumes, you can just ramp by one step at once and just stop ramping on whichever channel needs fewer steps whenever it hits target. If there's some other reason the gains absolutely must be identical just offer a mono control. > + /* > + * We'd better change the gain of the left and right channels > + * at the same time to avoid different listening > + */ Looks like it is working around a power up issue so it's fine to have this. > +static int rk3308_codec_dac_enable(struct rk3308_codec_priv *rk3308) > +{ This looks way, way too open coded - you should be implementing a DAPM graph for the device and splitting things up. > + /* > + * 1. Set the ACODEC_DAC_ANA_CON0[0] to 0x1, to enable the current > + * source of DAC > + */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON00, > + RK3308_DAC_CURRENT_EN); > + > + usleep_range(20, 40); This should be a DAC widget. > + > + /* > + * 2. Set the ACODEC_DAC_ANA_CON1[6] and ACODEC_DAC_ANA_CON1[2] to 0x1, > + * to enable the reference voltage buffer > + */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON01, > + RK3308_DAC_BUF_REF_L_EN | > + RK3308_DAC_BUF_REF_R_EN); > + > + /* Waiting the stable reference voltage */ > + mdelay(1); This should be a supply. > + /* Step 03 */ > + regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON01, > + RK3308_DAC_HPOUT_POP_SOUND_L_MSK | > + RK3308_DAC_HPOUT_POP_SOUND_R_MSK, > + RK3308_DAC_HPOUT_POP_SOUND_L_WORK | > + RK3308_DAC_HPOUT_POP_SOUND_R_WORK); > + > + usleep_range(20, 40); > + You can probably push preparatory work like this into a fake supply widget, wm8994 has some stuff like that. > + /* Step 05 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON13, > + RK3308_DAC_L_HPMIX_EN | > + RK3308_DAC_R_HPMIX_EN); > + > + /* Waiting the stable HPMIX */ > + mdelay(1); > + > + /* Step 06. Reset HPMIX and recover HPMIX gains */ > + regmap_clear_bits(rk3308->regmap, RK3308_DAC_ANA_CON13, > + RK3308_DAC_L_HPMIX_WORK | > + RK3308_DAC_R_HPMIX_WORK); > + usleep_range(50, 100); > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON13, > + RK3308_DAC_L_HPMIX_WORK | > + RK3308_DAC_R_HPMIX_WORK); > + > + usleep_range(20, 40); These are mixers. > + if (rk3308->dac_output == DAC_LINEOUT || > + rk3308->dac_output == DAC_LINEOUT_HPOUT) { > + /* Step 07 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON04, > + RK3308_DAC_L_LINEOUT_EN | > + RK3308_DAC_R_LINEOUT_EN); > + > + usleep_range(20, 40); > + } > + > + if (rk3308->dac_output == DAC_HPOUT || > + rk3308->dac_output == DAC_LINEOUT_HPOUT) { > + /* Step 08 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON03, > + RK3308_DAC_L_HPOUT_EN | > + RK3308_DAC_R_HPOUT_EN); > + > + usleep_range(20, 40); > + > + /* Step 09 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON03, > + RK3308_DAC_L_HPOUT_WORK | > + RK3308_DAC_R_HPOUT_WORK); > + > + usleep_range(20, 40); > + } > + > + if (rk3308->codec_ver == ACODEC_VERSION_B) { > + /* Step 10 */ > + regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON15, > + RK3308_DAC_LINEOUT_POP_SOUND_L_MSK | > + RK3308_DAC_LINEOUT_POP_SOUND_R_MSK, > + RK3308_DAC_L_SEL_LINEOUT_FROM_INTERNAL | > + RK3308_DAC_R_SEL_LINEOUT_FROM_INTERNAL); > + > + usleep_range(20, 40); > + } > + > + /* Step 11 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON02, > + RK3308_DAC_L_REF_EN | RK3308_DAC_R_REF_EN); > + > + usleep_range(20, 40); > + > + /* Step 12 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON02, > + RK3308_DAC_L_CLK_EN | RK3308_DAC_R_CLK_EN); > + > + usleep_range(20, 40); > + > + /* Step 13 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON02, > + RK3308_DAC_L_DAC_EN | RK3308_DAC_R_DAC_EN); > + > + usleep_range(20, 40); > + > + /* Step 14 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON02, > + RK3308_DAC_L_DAC_WORK | RK3308_DAC_R_DAC_WORK); > + > + usleep_range(20, 40); > + > + /* Step 15 */ > + regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON12, > + RK3308_DAC_L_HPMIX_SEL_MSK | > + RK3308_DAC_R_HPMIX_SEL_MSK, > + RK3308_DAC_L_HPMIX_I2S | > + RK3308_DAC_R_HPMIX_I2S); > + > + usleep_range(20, 40); > + > + /* Step 16 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON13, > + RK3308_DAC_L_HPMIX_UNMUTE | RK3308_DAC_R_HPMIX_UNMUTE); > + > + usleep_range(20, 40); > + > + /* Step 17: Put configuration HPMIX Gain */ > + > + if (rk3308->dac_output == DAC_HPOUT || > + rk3308->dac_output == DAC_LINEOUT_HPOUT) { > + /* Step 18 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON03, > + RK3308_DAC_L_HPOUT_UNMUTE | RK3308_DAC_R_HPOUT_UNMUTE); > + > + usleep_range(20, 40); > + } These are all output drivers. > + > + if (rk3308->dac_output == DAC_LINEOUT || > + rk3308->dac_output == DAC_LINEOUT_HPOUT) { > + /* Step 19 */ > + regmap_set_bits(rk3308->regmap, RK3308_DAC_ANA_CON04, > + RK3308_DAC_L_LINEOUT_UNMUTE | RK3308_DAC_R_LINEOUT_UNMUTE); > + usleep_range(20, 40); > + } > + > + /* Step 20, put configuration HPOUT gain control */ > + /* Step 21, put configuration LINEOUT gain control */ > + > + if (rk3308->dac_output == DAC_HPOUT || > + rk3308->dac_output == DAC_LINEOUT_HPOUT) { > + /* Just for HPOUT */ > + rk3308_codec_digital_fadein(rk3308); > + } Use a _POST widget for this. > +static int rk3308_codec_power_on(struct rk3308_codec_priv *rk3308) > +{ > + unsigned int v; > + > +static int rk3308_codec_power_off(struct rk3308_codec_priv *rk3308) > +{ > + unsigned int v; This would normally be in set_bias_level() or a DAPM supply, probably set_bias_level() as there's a few moderate delays. > +static int rk3308_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + struct snd_soc_component *component = dai->component; > + struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component); > + int ret; > + > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > + /* DAC only supports 2 channels */ > + rk3308_codec_dac_mclk_enable(rk3308); > + rk3308_codec_dac_enable(rk3308); > + rk3308_codec_dac_dig_config(rk3308, params); > + } else { > + rk3308_codec_micbias_apply(rk3308); > + rk3308_codec_adc_mclk_enable(rk3308); > + ret = rk3308_codec_update_adc_grps(rk3308, params); > + if (ret < 0) > + return ret; > + > + rk3308_codec_open_capture(rk3308); > + rk3308_codec_agc_dig_config(rk3308, params); > + rk3308_codec_adc_dig_config(rk3308, params); > + } Note that hw_params() can be called repeatedly before actually starting the audio, you shouldn't be doing any refcounted operations. There is an open operation or again lots of this looks like powerup stuff which could be moved to DAPM. > +static int rk3308_codec_default_gains(struct rk3308_codec_priv *rk3308) > +{ Use the hardware defaults, don't open code settings for things like gains. Your settings may not be appropriate for other systems. > +static int rk3308_codec_prepare(struct rk3308_codec_priv *rk3308) > +{ > + /* Clear registers for ADC and DAC */ > + rk3308_codec_dac_disable(rk3308); > + rk3308_codec_adc_ana_disable(rk3308, ADC_LR_GROUP_MAX); > + rk3308_codec_default_gains(rk3308); > + rk3308_codec_llp_down(rk3308); > + rk3308_codec_controls_prepare(rk3308); This applies to settings in general, the exceptions are generally things like zero cross where it's so overwhelmingly clear what makes sense and the setting is more of a chicken bit than actually useful.
On Wed, Sep 07, 2022 at 04:21:21PM +0200, luca.ceresoli@bootlin.com wrote: > -static int rockchip_i2s_tdm_set_sysclk(struct snd_soc_dai *cpu_dai, int stream, > +static int rockchip_i2s_tdm_set_sysclk(struct snd_soc_dai *cpu_dai, int clk_id, > unsigned int freq, int dir) > { > struct rk_i2s_tdm_dev *i2s_tdm = to_info(cpu_dai); > @@ -978,15 +981,18 @@ static int rockchip_i2s_tdm_set_sysclk(struct snd_soc_dai *cpu_dai, int stream, > if (i2s_tdm->clk_trcm) { > i2s_tdm->mclk_tx_freq = freq; > i2s_tdm->mclk_rx_freq = freq; > + > + dev_dbg(i2s_tdm->dev, "mclk freq: %u", freq); > } else { > - if (stream == SNDRV_PCM_STREAM_PLAYBACK) > + if (clk_id == CLK_IDX_MCLK_TX) > i2s_tdm->mclk_tx_freq = freq; > - else > + else if (clk_id == CLK_IDX_MCLK_RX) > i2s_tdm->mclk_rx_freq = freq; > - } > + else > + return -ENOTSUPP; This should be a switch statement for clarity and exensibility.
Hello Mark,
On Thu, 8 Sep 2022 17:27:36 +0100
Mark Brown <broonie@kernel.org> wrote:
> On Wed, Sep 07, 2022 at 04:21:23PM +0200, luca.ceresoli@bootlin.com wrote:
Thank you for taking the time to review my patch in such detail! This
is my first contribution to ALSA, and it was not clear to me which
parts of the existing vendor driver needed even more cleanups than I
have already done. I will probably get back to you with specific
questions later on, while addressing your comments.
Best regards,
Luca
From: Luca Ceresoli <luca.ceresoli@bootlin.com> This series of patches adds support to use the internal audio codec of the Rockchip RK3308 SoC. This codec is internally connected to the I2S peripherals on the same chip, and it has some peculiarities arising from that interconnection. For proper bidirectional operation with the internal codec, the I2S peripheral needs two clock sources (tx and rx), while connection with an external codec commonly needs only one. Since v5.16 there is a driver for the I2S in sound/soc/rockchip/rockchip_i2s_tdm.c, but it does not correctly handle receiving those two clocks via the .set_sysclk op. Patch 5 fixes that. However the simple-audio-card and audio-graph-card sound card drivers do not support calling .set_sysclk twice, thus patch 6 makes the .init op of struct asoc_simple_priv overridable by a driver in order to be able to call .set_sysclk twice and thus configure both clocks. Patch 7 adds the codec driver and patch 8 builds on top of all the above by implementing a simple RK3308-specific audio card, based on audio-graph-card. This card sets all the I2S input clocks. Patches 1-2 add DT bindings for the codec and the card. Patches 3-4 add to the SoC DT file two I2S controllers (those which are internally connected to the internal codec) and the codec itself. Luca Luca Ceresoli (8): ASoC: rockchip: rk3308: add internal audio codec bindings ASoC: rockchip: rk3308: add audio card bindings arm64: dts: rockchip: add i2s_8ch_2 and i2s_8ch_3 arm64: dts: rockchip: add the internal audio codec ASoC: rockchip: i2s-tdm: Fix clk_id usage in .set_sysclk() ASoC: audio-graph: let dai_link->init be overridable ASoC: codecs: Add RK3308 internal audio codec driver ASoC: rockchip: add new RK3308 sound card .../rockchip,rk3308-audio-graph-card.yaml | 50 + .../bindings/sound/rockchip,rk3308-codec.yaml | 102 + MAINTAINERS | 14 + arch/arm64/boot/dts/rockchip/rk3308.dtsi | 68 + .../dt-bindings/sound/rockchip,rk3308-codec.h | 15 + include/sound/simple_card_utils.h | 1 + sound/soc/codecs/Kconfig | 11 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/rk3308_codec.c | 2122 +++++++++++++++++ sound/soc/codecs/rk3308_codec.h | 648 +++++ sound/soc/generic/audio-graph-card.c | 2 + sound/soc/rockchip/Kconfig | 14 + sound/soc/rockchip/Makefile | 1 + sound/soc/rockchip/rockchip_i2s_tdm.c | 18 +- sound/soc/rockchip/rockchip_rk3308_card.c | 97 + 15 files changed, 3159 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/sound/rockchip,rk3308-audio-graph-card.yaml create mode 100644 Documentation/devicetree/bindings/sound/rockchip,rk3308-codec.yaml create mode 100644 include/dt-bindings/sound/rockchip,rk3308-codec.h create mode 100644 sound/soc/codecs/rk3308_codec.c create mode 100644 sound/soc/codecs/rk3308_codec.h create mode 100644 sound/soc/rockchip/rockchip_rk3308_card.c