From: Mark Brown <broonie@kernel.org>
To: jack.yu@realtek.com
Cc: alsa-devel@alsa-project.org, lars@metafoo.de,
kent_chen@realtek.com, kenny_chen@realtek.com,
lgirdwood@gmail.com, mingjane_hsieh@realtek.com,
flove@realtek.com
Subject: Re: [alsa-devel] [PATCH] ASoC: rt1015: add rt1015 amplifier driver
Date: Tue, 14 Jan 2020 16:55:40 +0000 [thread overview]
Message-ID: <20200114165540.GB3897@sirena.org.uk> (raw)
In-Reply-To: <20200114025628.4241-1-jack.yu@realtek.com>
[-- Attachment #1.1: Type: text/plain, Size: 2542 bytes --]
On Tue, Jan 14, 2020 at 10:56:28AM +0800, jack.yu@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?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2020-01-14 16:56 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-14 2:56 [alsa-devel] [PATCH] ASoC: rt1015: add rt1015 amplifier driver jack.yu
2020-01-14 16:55 ` Mark Brown [this message]
2020-02-25 22:08 ` Pierre-Louis Bossart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200114165540.GB3897@sirena.org.uk \
--to=broonie@kernel.org \
--cc=alsa-devel@alsa-project.org \
--cc=flove@realtek.com \
--cc=jack.yu@realtek.com \
--cc=kenny_chen@realtek.com \
--cc=kent_chen@realtek.com \
--cc=lars@metafoo.de \
--cc=lgirdwood@gmail.com \
--cc=mingjane_hsieh@realtek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).