On 3/30/2022 4:45 PM, Raphael-Xu wrote:
Missing commit message in this patch and previous one. Even one sentence
explaining what you are doing and why is better then nothing.
Overall the series looks a lot better now, I still wonder if changing
coding style makes sense, but ultimately it's your code, so as long as
it patches checkpatch I will leave decision to Mark.
And there are few nitpicks, below in this patch.
Signed-off-by: Raphael-Xu <13691752556@xxxxxxx>
---
sound/soc/codecs/tas27xx.c | 378 ++++++++++++++++++++++++++-----------
1 file changed, 263 insertions(+), 115 deletions(-)
diff --git a/sound/soc/codecs/tas27xx.c b/sound/soc/codecs/tas27xx.c
index 8118429bb2f5..bb845d4797ce 100644
--- a/sound/soc/codecs/tas27xx.c
+++ b/sound/soc/codecs/tas27xx.c
...
@@ -146,8 +182,9 @@ static int tas27xx_dac_event(struct snd_soc_dapm_widget *w,
snd_soc_dapm_to_component(w->dapm);
struct tas27xx_priv *tas27xx =
snd_soc_component_get_drvdata(component);
- int ret;
+ int ret = 0;
+ mutex_lock(&tas27xx->codec_lock);
switch (event) {
case SND_SOC_DAPM_POST_PMU:
ret = snd_soc_component_update_bits(component,
@@ -163,13 +200,16 @@ static int tas27xx_dac_event(struct snd_soc_dapm_widget *w,
break;
default:
dev_err(tas27xx->dev, "Unsupported event\n");
- return -EINVAL;
+ ret = -EINVAL;
There seems to be one tab to many here.
}
-
- if (ret < 0)
- return ret;
-
- return 0;
+ if (ret < 0) {
+ pr_err("%s:%u:errCode:0x%0x:PWR_CTRL error\n",
+ __func__, __LINE__, ret);
+ } else {
+ ret = 0;
+ }
+ mutex_unlock(&tas27xx->codec_lock);
+ return ret;
}
static const struct snd_kcontrol_new isense_switch =
@@ -207,55 +247,96 @@ static const struct snd_soc_dapm_route tas27xx_audio_map[] = {
static int tas27xx_mute(struct snd_soc_dai *dai, int mute, int direction)
{
struct snd_soc_component *component = dai->component;
- int ret;
+ struct tas27xx_priv *tas27xx =
+ snd_soc_component_get_drvdata(component);
+ int ret = 0;
+
+ mutex_lock(&tas27xx->codec_lock);
+ if (mute == 0) {
alternatively if (!mute), but you can leave as is
+ ret = snd_soc_component_update_bits(component,
+ TAS27XX_CLK_CFG,
+ TAS27XX_CLK_CFG_MASK,
+ TAS27XX_CLK_CFG_ENABLE);
+ if (ret < 0) {
+ dev_err(tas27xx->dev,
+ "%s:%u: Failed to CLK_CFG_ENABLE\n",
+ __func__, __LINE__);
+ goto EXIT;
+ }
+ usleep_range(2000, 2000);
+ }
ret = snd_soc_component_update_bits(component, TAS27XX_PWR_CTRL,
- TAS27XX_PWR_CTRL_MASK,
- mute ? TAS27XX_PWR_CTRL_MUTE : 0);
-
- if (ret < 0)
- return ret;
+ TAS27XX_PWR_CTRL_MASK,
+ mute ? TAS27XX_PWR_CTRL_MUTE : 0);
+ if (ret >= 0) {
+ tas27xx->mb_power_up = mute?false:true;
+ ret = 0;
+ }
- return 0;
+ if (ret < 0) {
You could probably just use } else { here with above if.
+ pr_err("%s:%u: Failed to set powercontrol\n",
+ __func__, __LINE__);
+ }
+EXIT:
+ mutex_unlock(&tas27xx->codec_lock);
+ return ret;
}
...
static const struct snd_soc_dai_ops tas27xx_dai_ops = {
@@ -495,13 +615,13 @@ static struct snd_soc_dai_driver tas27xx_dai_driver[] = {
},
.capture = {
.stream_name = "ASI1 Capture",
- .channels_min = 0,
+ .channels_min = 1,
.channels_max = 2,
.rates = TAS27XX_RATES,
.formats = TAS27XX_FORMATS,
},
.ops = &tas27xx_dai_ops,
- .symmetric_rate = 1,
+ .symmetric_rates = 1,
I'm pretty sure struct snd_soc_dai_driver uses .symmetric_rate, so this
change would result in build failure?
},
};
@@ -509,7 +629,7 @@ static int tas27xx_codec_probe(struct snd_soc_component *component)
{
struct tas27xx_priv *tas27xx =
snd_soc_component_get_drvdata(component);
- int ret;
+ int ret = 0;
tas27xx->component = component;
...
static DECLARE_TLV_DB_SCALE(tas27xx_digital_tlv, 1100, 50, 0);
@@ -551,8 +685,10 @@ static const struct snd_kcontrol_new tas27xx_snd_controls[] = {
static const struct snd_soc_component_driver soc_component_driver_tas27xx = {
.probe = tas27xx_codec_probe,
+#ifdef CONFIG_PM
.suspend = tas27xx_codec_suspend,
.resume = tas27xx_codec_resume,
+#endif
.set_bias_level = tas27xx_set_bias_level,
.controls = tas27xx_snd_controls,
.num_controls = ARRAY_SIZE(tas27xx_snd_controls),
@@ -590,6 +726,12 @@ static const struct regmap_range_cfg tas27xx_regmap_ranges[] = {
},
};
+static bool tas27xx_volatile(struct device *dev,
+ unsigned int reg)
+{
+ return true;
+}
+
This seems bit weird, are all registers considered volatile?
static const struct regmap_config tas27xx_i2c_regmap = {
.reg_bits = 8,
.val_bits = 8,
@@ -599,6 +741,7 @@ static const struct regmap_config tas27xx_i2c_regmap = {
.ranges = tas27xx_regmap_ranges,
.num_ranges = ARRAY_SIZE(tas27xx_regmap_ranges),
.max_register = 1 * 128,
+ .volatile_reg = tas27xx_volatile,
};
static int tas27xx_parse_dt(struct device *dev, struct tas27xx_priv *tas27xx)
...
