[alsa-devel] [PATCH] ASoC: rt1015: add rt1015 amplifier driver

[Mark Brown]

On Tue, Jan 14, 2020 at 10:56:28AM +0800, jack.yu at realtek.com wrote:

> +++ b/sound/soc/codecs/rt1015.c
> @@ -0,0 +1,1016 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rt1015.c  --  RT1015 ALSA SoC audio amplifier driver
> + *

Please make the entire comment a C++ so things look more
consistent.

> +static const struct reg_sequence init_list[] = {
> +	{ 0x0000, 0x0000 },
> +	{ 0x0104, 0xA213 },
> +	{ 0x010E, 0x0058 },
> +	{ 0x0111, 0x2000 },
> +	{ 0x0322, 0xF7DF },
> +	{ 0x1302, 0x12F7 },
> +	{ 0x0336, 0x0010 },
> +	{ 0x04FC, 0x35CF },
> +	{ 0x0102, 0xed02 },
> +};

What's this doing?  Some of these (all that I checked) are normal
registers not undocumented registers or anything and you're
changing their default values in here as well as doing a reset.
If we're setting values on normal registers we should at least do
it through explicit, documented writes.

> +#define RT1015_INIT_REG_LEN ARRAY_SIZE(init_list)

Not sure this define is worth the effort?

> +	rt1015->boost_mode = ucontrol->value.integer.value[0];
> +	if (rt1015->boost_mode == 0) {
> +		/* Bypass */
> +		snd_soc_component_update_bits(component,
> +			RT1015_SMART_BST_CTRL1, RT1015_ABST_AUTO_EN_MASK |
> +			RT1015_ABST_FIX_TGT_MASK | RT1015_BYPASS_SWR_REG_MASK,
> +			RT1015_ABST_REG_MODE | RT1015_ABST_FIX_TGT_DIS |
> +			RT1015_BYPASS_SWRREG_BYPASS);
> +	} else if (rt1015->boost_mode == 1) {
> +		/* Adaptive */
> +		snd_soc_component_update_bits(component,
> +			RT1015_SMART_BST_CTRL1, RT1015_ABST_AUTO_EN_MASK |
> +			RT1015_ABST_FIX_TGT_MASK | RT1015_BYPASS_SWR_REG_MASK,
> +			RT1015_ABST_AUTO_MODE | RT1015_ABST_FIX_TGT_DIS |
> +			RT1015_BYPASS_SWRREG_PASS);
> +	} else {
> +		/* Fixed Adaptive */
> +		snd_soc_component_update_bits(component,
> +			RT1015_SMART_BST_CTRL1, RT1015_ABST_AUTO_EN_MASK |
> +			RT1015_ABST_FIX_TGT_MASK | RT1015_BYPASS_SWR_REG_MASK,
> +			RT1015_ABST_AUTO_MODE | RT1015_ABST_FIX_TGT_EN |
> +			RT1015_BYPASS_SWRREG_PASS);
> +	}

This should be a switch statement and should reject invalid
values.

> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		if (rt1015->bypass_boost == 0) {
> +			snd_soc_component_write(component,
> +				RT1015_SYS_RST1, 0x05f7);
> +			snd_soc_component_write(component,
> +				RT1015_GAT_BOOST, 0xacfe);
> +			snd_soc_component_write(component,
> +				RT1015_PWR9, 0xaa00);
> +			snd_soc_component_write(component,
> +				RT1015_GAT_BOOST, 0xecfe);
> +		} else {

Do we need some sort of lock out to prevent applications changing
the bypass boost mode while the amp is powered up?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20200114/1a42c054/attachment.sig>