RE: [PATCH v2 2/2] ASoC: codecs: Add driver for Iron Device MA5311
From: Hyun-Uk Son
Date: Tue Nov 10 2026 - 03:43:36 EST
Hello Mark,
If you don't mind, I'd like to share my proposed changes with
you before submitting the next revision of the patch.
Please let me know if you would prefer to continue the discussion
in the review thread for the next patch revision instead.
I’d like to start by addressing the feedback on v2.
[Feedback 1]
> > +static bool ma5311_adc_volatile_register(struct device *dev, unsigned
> > +int reg) {
> > + return false;
> > +}
>
> Like I said on v1 this is the default so the function can be dropped.
I removed the code based on your feedback.
[Feedback 2]
> > +static int ma5311_reset(struct snd_soc_component *component) {
>
> > + ma5311->adc1_vol_left = 0xc0;
> > + ma5311->adc1_vol_right = 0xc0;
> > + ma5311->adc2_vol_left = 0xc0;
> > + ma5311->adc2_vol_right = 0xc0;
> > + ma5311->adc3_vol_left = 0xc0;
> > + ma5311->adc3_vol_right = 0xc0;
> > + ma5311->adc4_vol_left = 0xc0;
> > + ma5311->adc4_vol_right = 0xc0;
> > + ma5311->adc1_pga_left = 0x10;
> > + ma5311->adc1_pga_right = 0x10;
> > + ma5311->adc2_pga_left = 0x10;
> > + ma5311->adc2_pga_right = 0x10;
> > + ma5311->adc3_pga_left = 0x10;
> > + ma5311->adc3_pga_right = 0x10;
> > + ma5311->adc4_pga_left = 0x10;
> > + ma5311->adc4_pga_right = 0x10;
>
> Like I said on v1 we usually leave the device with the hardware defaults
> to avoid having to decide on a specific use case in the kernel -
> presumably the defaults in the hardware were chosen for a reason.
I added a register initialization function because the factory-default
register settings are not suitable for practical use.
Since the driver applies a fixed set of custom default register values,
I also needed to initialize the corresponding software private data
to keep the software and hardware states consistent.
static void ma5311_priv_reset(struct ma5311_priv *ma5311)
{
ma5311->amp_power_status = false;
ma5311->adc_power_status = false;
ma5311->last_bclk = 0;
ma5311->amp_mode = MA5311_MONO_BOTH_MODE;
ma5311->num_of_pll_matches = ARRAY_SIZE(ma5311_pll_matches);
ma5311->pll_matches = ma5311_pll_matches;
ma5311->format = SND_SOC_DAIFMT_I2S;
ma5311->sys_clk_id = MA5311_PLL_CLKIN_BCLK;
ma5311->frame_size = 0;
ma5311->tdm_slot = TDM01;
ma5311->capture_mode = I2S_ADC1;
}
static const struct regmap_config ma5311_adc_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.max_register = MA5311_ADC_CH4_3F_OUTPUT_FORMAT,
.readable_reg = ma5311_adc_readable_register,
.writeable_reg = ma5311_adc_writeable_register,
.cache_type = REGCACHE_FLAT,
.reg_defaults = ma5311_adc_reg_def,
.num_reg_defaults = ARRAY_SIZE(ma5311_adc_reg_def),
};
Since each regmap maintains its own cache, I removed software caching code,
as shown below.
static int ma5311_adc_pga_control_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
const struct soc_mixer_control *mc =
(const struct soc_mixer_control *)kcontrol->private_value;
long input = ucontrol->value.integer.value[0];
unsigned int val;
int ret;
bool changed;
if (input < 0 || input > MA5311_ADC_PGA_CTRL_MASK)
return -EINVAL;
val = input;
ret = regmap_update_bits_check(ma5311->adc_regmap, mc->reg,
MA5311_ADC_PGA_CTRL_MASK, val, &changed);
if (ret)
return ret;
return changed;
}
[Feedback 3]
> > + ma5311->capture_mode = I2S_ADC1;
>
> What about playback_mode and dapm_aif_in?
Defined a structure for private data initialization.
(To synchronize the value with reset value)
static void ma5311_priv_reset(struct ma5311_priv *ma5311)
{
ma5311->amp_power_status = false;
ma5311->adc_power_status = false;
ma5311->last_bclk = 0;
ma5311->amp_mode = MA5311_MONO_BOTH_MODE;
ma5311->num_of_pll_matches = ARRAY_SIZE(ma5311_pll_matches);
ma5311->pll_matches = ma5311_pll_matches;
ma5311->format = SND_SOC_DAIFMT_I2S;
ma5311->sys_clk_id = MA5311_PLL_CLKIN_BCLK;
ma5311->frame_size = 0;
ma5311->tdm_slot = TDM01;
ma5311->capture_mode = I2S_ADC1;
}
I renamed playback_mode to tdm_slot based on your feedback,
as this mixer control is only used in TDM mode, not in I2S mode.
Also removed the dapm_aif_in member, replacing the custom software cache
with standard ASoC helpers, such as dapm_kcontrol_get_value().
[Feedback 4]
> > +static int ma5311_adc_volume_control_get(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + const struct soc_mixer_control *mc =
> > + (const struct soc_mixer_control *)kcontrol->private_value;
> > + unsigned int *cache = ma5311_adc_volume_control_cache(ma5311, mc-
> >reg);
> > + unsigned int val;
> > + int ret;
> > +
> > + if (!cache)
> > + return -EINVAL;
> > +
> > + ret = regmap_read(ma5311->adc_regmap, mc->reg, &val);
> > + if (ret)
> > + return ret;
> > +
> > + ucontrol->value.integer.value[0] = val & MA5311_ADC_VOL_CTRL_MASK;
> > + *cache = ucontrol->value.integer.value[0];
> > +
> > + return 0;
> > +}
>
> I still don't understand why we have these open coded caches rather than
> just using a regmap cache and the standard helpers, it'd be a lot less
> code.
I'm now using regmap's built-in caching mechanism, as mentioned in Feedback 2.
I didn't use the standard ASoC helpers for the ADC controls because they access
the component's default regmap (ma5311->ctrl_regmap),whereas the ADC controls
require access to a separate regmap (ma5311->adc_regmap).
[Feedback 5]
> > +static int ma5311_playback_mode_put(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int val = ucontrol->value.enumerated.item[0];
> > +
> > + if (val >= ARRAY_SIZE(ma5311_playback_mode_text)) {
> > + dev_err(ma5311->dev, "%s: Out of range\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (ma5311->playback_mode == val)
> > + return 0;
> > +
> > + switch (val) {
> > + case TDM01:
> > + case TDM23:
> > + case TDM45:
> > + case TDM67:
> > + }
> > +
> > + ma5311->playback_mode = val;
>
> This doesn't handle the I2S option so we won't reset if the user flips
> back to that. More generally this feels like it ought to just be a
> set_tdm_slot() operation?
I renamed the function because it's no longer related to I2S operation.
static int ma5311_rx_tdm_slot_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
/*
Purpose of the mixer is at controling speaker output dynamically.
This doesn't affect I2S operation.
*/
struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
unsigned int val = ucontrol->value.enumerated.item[0];
if (val >= ARRAY_SIZE(ma5311_tdm_rx_slot_text)) {
dev_err(ma5311->dev, "%s: Out of range\n", __func__);
return -EINVAL;
}
if (ma5311->tdm_slot == val)
return 0;
switch (val) {
case TDM01:
case TDM23:
case TDM45:
case TDM67:
ma5311->tdm_slot = val;
return 1;
}
Also, this control is essential because it allows the speaker output
channels to be dynamically assigned to different playback channels
during PCM playback.
[Feedback 6]
> > +static int ma5311_reset_control_get(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > + struct ma5311_priv *ma5311 =
> > +snd_soc_component_get_drvdata(component);
> > +
> > + if (!ma5311->reset_gpio)
> > + return -ENODEV;
>
> As I said on v1 it'd be better to just not register this control if we
> don't have the GPIO.
I moved the reset control into a separate control array, as shown below.
static const struct snd_kcontrol_new ma5311_reset_controls[] = {
SOC_ENUM_EXT(MA5311_RESET_CTRL_NAME, ma5311_reset_enum,
ma5311_reset_control_get, ma5311_reset_control_put),
};
I also updated the component probe function to register the reset control
only when the reset GPIO is available.
static int ma5311_probe(struct snd_soc_component *component)
{
struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
int ret;
if (ma5311->reset_gpio) {
ret = snd_soc_add_component_controls(component,
ma5311_reset_controls,
ARRAY_SIZE(ma5311_reset_controls));
if (ret)
return ret;
}
return ma5311_reset(component);
}
[Feedback 7]
> > +static int ma5311_reset_control_put(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
>
> > + ret = ma5311_reset(component);
>
> This might break the machine driver if it's relying on setting the DAI up
> during probe()...
With internal verification, I figured out that synchronizing
private data to reset hardware register setting is enough,
because The driver does not rely on probe() for any essential
configuration and dai_ops callback functions always set
the essential settings including PLL, TDM slot, etc..
Only one limitation of reset control is that every PCM
operation needs to be off and removed from buffer, however
I think it's enough because user can remove existing PCM
operation with additional custom sequence to utilize this control.
Based on internal testing, I found that synchronizing the
driver's private data with the hardware register defaults
is sufficient for our current configuration.
On our target platform, the required DAI settings,
including PLL and TDM slot configuration, are reapplied
when the PCM streams are reconfigured after a reset.
The reset control is restricted to cases where no
PCM streams are active and both playback and capture
paths are powered down. Otherwise, the control returns -EBUSY.
Although this introduces a limitation, I believe it
is acceptable for our intended use case, as userspace can
stop and close existing PCM streams before requesting a reset,
then reopen them as part of a controlled recovery sequence.
static int ma5311_reset_control_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
struct snd_soc_dapm_context *dapm =
snd_soc_component_get_dapm(component);
unsigned int val = ucontrol->value.enumerated.item[0];
int ret = 0;
if (!val)
return 0;
if (!ma5311->reset_gpio)
return -ENODEV;
if (val >= ARRAY_SIZE(ma5311_reset_text))
return -EINVAL;
snd_soc_dpcm_mutex_lock(component->card);
snd_soc_dapm_mutex_lock(dapm);
if (ma5311->amp_power_status || ma5311->adc_power_status) {
dev_warn(component->dev,
"Reset denied: playback or capture path is powered\n");
ret = -EBUSY;
goto unlock;
}
if (snd_soc_component_active(component)) {
dev_warn(component->dev,
"Reset denied: pcm stream is in active state\n");
ret = -EBUSY;
goto unlock;
}
gpiod_set_value_cansleep(ma5311->reset_gpio, 1);
msleep(MA5311_RESET_ASSERT_MS);
gpiod_set_value_cansleep(ma5311->reset_gpio, 0);
msleep(MA5311_RESET_RECOVERY_MS);
ret = ma5311_reset(component);
unlock:
snd_soc_dapm_mutex_unlock(dapm);
snd_soc_dpcm_mutex_unlock(component->card);
return ret;
}
static const struct snd_soc_dai_ops ma5311_dai_ops_amp = {
.hw_params = ma5311_dai_hw_params,
.set_fmt = ma5311_dai_set_fmt,
.set_sysclk = ma5311_dai_set_sysclk,
.set_tdm_slot = ma5311_dai_set_tdm_slot,
.mute_stream = ma5311_dai_mute_stream,
};
[Feedback 8]
> > +static int ma5311_dapm_aif_in_put(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_dapm_context *dapm =
> > + snd_soc_dapm_kcontrol_to_dapm(kcontrol);
> > + struct snd_soc_component *component =
> snd_soc_dapm_to_component(dapm);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int val = ucontrol->value.enumerated.item[0];
> > + unsigned int old_val = ma5311->dapm_aif_in;
> > + int ret;
> > +
> > + if (val >= ARRAY_SIZE(ma5311_aif_in_source_text)) {
> > + dev_err(ma5311->dev, "%s: Out of range\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (ma5311->dapm_aif_in == val)
> > + return 0;
> > +
> > + ma5311->dapm_aif_in = val;
>
> DAPM is already storing the value - you could just read it with
> dapm_kcontrol_get_value().
static const struct snd_kcontrol_new ma5311_aif_in_source_control = {
.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
.name = MA5311_AIF_IN_NAME,
.info = snd_soc_info_enum_double,
.get = snd_soc_dapm_get_enum_double,
.put = ma5311_dapm_aif_in_put,
.private_value = (unsigned long)&ma5311_aif_in_source_enum,
};
static int ma5311_dapm_aif_in_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct snd_soc_dapm_context *dapm =
snd_soc_dapm_kcontrol_dapm(kcontrol);
struct snd_soc_component *component = snd_soc_dapm_to_component(dapm);
struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
unsigned int val = ucontrol->value.enumerated.item[0];
unsigned int old_val = dapm_kcontrol_get_value(kcontrol);
if (val >= ARRAY_SIZE(ma5311_aif_in_source_text)) {
dev_err(ma5311->dev, "%s: Out of range\n", __func__);
return -EINVAL;
}
if (old_val == val)
return 0;
if (ma5311->amp_power_status) {
dev_warn(ma5311->dev,
"Cannot change AIF IN Source during playback\n");
return -EBUSY;
}
return snd_soc_dapm_put_enum_double(kcontrol, ucontrol);
}
I updated the code based on your suggestion and now use
dapm_kcontrol_get_value() to retrieve the current control value.
I also replaced the custom get callback with the standard
ASoC helper snd_soc_dapm_get_enum_double().
[Feedback 9]
> > +static int ma5311_adc_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);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int mux = ma5311->capture_mode;
> > +
> > + switch (event) {
> > + case SND_SOC_DAPM_PRE_PMU:
> > + regmap_update_bits(ma5311->ctrl_regmap,
> MA5311_DAC_A2_TOP_MAN2,
> > + MA5311_PLL_MASK, MA5311_PLL_ON);
> > +
> > + regmap_update_bits(ma5311->ctrl_regmap,
> > + MA5311_DAC_00_SYSTEM_CTRL1,
> > + MA5311_POWER_MASK, MA5311_POWER_ON);
> > +
> > + switch (mux) {
> > + case I2S_ADC1:
> > + regmap_update_bits(ma5311->adc_regmap,
> > + MA5311_ADC_CH1_00_GENERAL_SETTING,
> > + MA5311_ADC_POWER_MASK,
> > + MA5311_ADC_POWER_ON);
> > + regmap_update_bits(ma5311->adc_regmap,
> > + MA5311_ADC_CH1_0E_MUTE_DATA_FLOW,
> > + MA5311_ADC_MUTE_BOTH_MASK,
> > + MA5311_ADC_BOTH_DEMUTE);
> > + break;
> > + case I2S_ADC2:
> > + regmap_update_bits(ma5311->adc_regmap,
> > + MA5311_ADC_CH1_00_GENERAL_SETTING,
> > + MA5311_ADC_POWER_MASK,
> > + MA5311_ADC_POWER_ON);
> > + regmap_update_bits(ma5311->adc_regmap,
> > + MA5311_ADC_CH1_0E_MUTE_DATA_FLOW,
> > + MA5311_ADC_MUTE_BOTH_MASK,
> > + MA5311_ADC_BOTH_DEMUTE);
>
> Could use a comment about ADC2 enabling ADC1 as well (similarly for ADC4
> and ADC3).
static int ma5311_adc_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);
struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
unsigned int mux = ma5311->capture_mode;
switch (event) {
case SND_SOC_DAPM_PRE_PMU:
regmap_update_bits(ma5311->ctrl_regmap, MA5311_DAC_A2_TOP_MAN2,
MA5311_PLL_MASK, MA5311_PLL_ON);
regmap_update_bits(ma5311->ctrl_regmap,
MA5311_DAC_00_SYSTEM_CTRL1,
MA5311_POWER_MASK, MA5311_POWER_ON);
switch (mux) {
case I2S_ADC1:
regmap_update_bits(ma5311->adc_regmap,
MA5311_ADC_CH1_00_GENERAL_SETTING,
MA5311_ADC_POWER_MASK,
MA5311_ADC_POWER_ON);
regmap_update_bits(ma5311->adc_regmap,
MA5311_ADC_CH1_0E_MUTE_DATA_FLOW,
MA5311_ADC_MUTE_BOTH_MASK,
MA5311_ADC_BOTH_DEMUTE);
break;
case I2S_ADC2:
/* ADC2 requires ADC1 to remain powered on. */
regmap_update_bits(ma5311->adc_regmap,
MA5311_ADC_CH1_00_GENERAL_SETTING,
MA5311_ADC_POWER_MASK,
MA5311_ADC_POWER_ON);
regmap_update_bits(ma5311->adc_regmap,
MA5311_ADC_CH2_10_GENERAL_SETTING,
MA5311_ADC_POWER_MASK,
MA5311_ADC_POWER_ON);
regmap_update_bits(ma5311->adc_regmap,
MA5311_ADC_CH2_1E_MUTE_DATA_FLOW,
MA5311_ADC_MUTE_BOTH_MASK,
MA5311_ADC_BOTH_DEMUTE);
I added a comment explaining that ADC1 must be powered on for ADC2 to operate,
and similarly, ADC3 must be powered on for ADC4 to operate.
[Feedback 10]
> > + SND_SOC_DAPM_OUT_DRV_E("AMP Power", SND_SOC_NOPM, 0, 0, NULL, 0,
> > + ma5311_power_event,
> > + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD |
> > + SND_SOC_DAPM_POST_PMU),
>
> The function doesn't need all the events?
SND_SOC_DAPM_OUT_DRV_E("AMP Power", SND_SOC_NOPM, 0, 0, NULL, 0,
ma5311_power_event,
SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
I removed `SND_SOC_DAPM_PRE_PMD` because this event is no longer used.
[Feedback 11]
> > +static const struct regmap_config ma5311_ctrl_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > +
> > + .max_register = MA5311_DAC_FF_VERSION,
> > + .readable_reg = ma5311_ctrl_readable_register,
> > + .writeable_reg = ma5311_ctrl_writeable_register,
> > + .volatile_reg = ma5311_ctrl_volatile_register,
> > +
> > + .cache_type = REGCACHE_NONE,
>
> As I said above and on v1 you should probably enable caches here, it'd
> simplify the rest of the code.
The update is explained in Feedback 2.
[Feedback 12]
> > + dev_dbg(component->dev, "Estimated TDM BCLK: %u Hz\n", bclk);
> > + } else if (ma5311->format == SND_SOC_DAIFMT_I2S) {
> > + bclk = params_rate(params) * params_physical_width(params) *
> > + params_channels(params);
>
> Note that I2S always needs to clock two channels even for mono streams.
I updated the BCLK calculation for I2S mode to account for mono streams,
as I2S always requires two channel slots.
else if (ma5311->format == SND_SOC_DAIFMT_I2S) {
bclk = params_rate(params) * params_physical_width(params) * 2;
dev_dbg(component->dev, "Estimated I2S BCLK: %u Hz\n", bclk);
}
[Feedback 13]
> > +static int ma5311_dai_set_tdm_slot(struct snd_soc_dai *dai,
> > + unsigned int tx_mask, unsigned int rx_mask,
> > + int slots, int slot_width)
> > +{
>
> rx_mask should be what the playback mode is doing I think.
I explained why I implemented ma5311_rx_tdm_slot_put() in my response
to Feedback 5. I also added logic to initialize the TDM slot setting
based on the `rx_mask` value, as shown below.
switch (rx_mask) {
case 0:
break;
case BIT(0) | BIT(1):
rx_val = MA5311_TDM_SLOT0_RX_POS_0 |
MA5311_TDM_SLOT1_RX_POS_1;
tdm_slot = TDM01;
break;
case BIT(2) | BIT(3):
rx_val = MA5311_TDM_SLOT0_RX_POS_2 |
MA5311_TDM_SLOT1_RX_POS_3;
tdm_slot = TDM23;
break;
case BIT(4) | BIT(5):
rx_val = MA5311_TDM_SLOT0_RX_POS_4 |
MA5311_TDM_SLOT1_RX_POS_5;
tdm_slot = TDM45;
break;
case BIT(6) | BIT(7):
rx_val = MA5311_TDM_SLOT0_RX_POS_6 |
MA5311_TDM_SLOT1_RX_POS_7;
tdm_slot = TDM67;
break;
default:
dev_err(component->dev, "Unsupported RX mask: %#x\n",
rx_mask);
return -EINVAL;
}
ret = regmap_update_bits(ma5311->ctrl_regmap,
MA5311_DAC_37_TDM1,
MA5311_TDM_SLOT0_RX_POS_MASK |
MA5311_TDM_SLOT1_RX_POS_MASK,
rx_val);
[Feedback 14]
> > +static int ma5311_dai_mute_stream(struct snd_soc_dai *dai, int mute,
> > +int stream) {
> > + struct snd_soc_component *component = dai->component;
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int val;
> > +
> > + if (stream == SNDRV_PCM_STREAM_CAPTURE)
> > + return 0;
> > +
> > + if (mute || ma5311->force_mute_status) {
>
> Does anything set force_mute_status?
I removed the `force_mute_status` member because it is no longer needed.
Below are responses to the feedback on v1.
[Feedback 15]
> > +static int ma5311_capture_mode_put(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int val = ucontrol->value.enumerated.item[0];
> > +
> > + if (val >= ARRAY_SIZE(ma5311_capture_mode_text)) {
> > + dev_err(ma5311->dev, "%s: Out of range\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (ma5311->capture_mode == val)
> > + return 0;
> > +
> > + if (ma5311->adc_power_status) {
> > + dev_err(ma5311->dev, "Try after capture is complete.\n");
> > + return -EBUSY;
> > + }
>
> There's no locking between this and DAPM so we could race with power up
> here. Taking the DAPM mutex ought to do enough.
I added DAPM mutex locking with goto-based cleanup.
static int ma5311_capture_mode_put(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
struct snd_soc_dapm_context *dapm =
snd_soc_component_get_dapm(component);
unsigned int val = ucontrol->value.enumerated.item[0];
int ret = 0;
if (val >= ARRAY_SIZE(ma5311_capture_mode_text)) {
dev_err(ma5311->dev, "%s: Out of range\n", __func__);
return -EINVAL;
}
snd_soc_dapm_mutex_lock(dapm);
if (ma5311->capture_mode == val)
goto unlock;
if (ma5311->adc_power_status) {
dev_warn(ma5311->dev, "Try after capture process is done.\n");
ret = -EBUSY;
goto unlock;
}
switch (val) {
case I2S_ADC1:
// Register Update
break;
case I2S_ADC2:
// Register Update
break;
case I2S_ADC3:
// Register Update
break;
case I2S_ADC4:
// Register Update
break;
case TDM_8CH:
// Register Update
break;
}
if (val != TDM_8CH) {
regmap_update_bits(ma5311->ctrl_regmap, MA5311_DAC_39_DATA_PATH,
MA5311_DAC_SDO_DATA_PATH_MASK, val << 2);
regmap_update_bits(ma5311->ctrl_regmap, MA5311_DAC_91_ADC_DAC_MIX,
MA5311_DAC_ADC_DAC_MIX_CH_MASK, val);
}
ma5311->capture_mode = val;
ret = 1;
unlock:
snd_soc_dapm_mutex_unlock(dapm);
return ret;
}
[Feedback 16]
> > + switch (params_width(params)) {
> > + case 16:
> > + case 24:
> > + case 32:
> > + break;
> > + default:
> > + dev_err(component->dev, "%s: unsupported PCM format: %d\n",
> > + __func__, params_format(params));
> > + return -EINVAL;
> > + }
>
> Don't we need to configure anything in the hardware for clock provider
> mode here?
Since the frame size is already calculated in ma5311_dai_set_tdm_slot(),
I don't think we need to configure the clock provider mode here.
[Feedback 17]
> > +#define MA5311_RATES_PLAYBACK SNDRV_PCM_RATE_8000_96000 #define
> > +MA5311_RATES_CAPTURE SNDRV_PCM_RATE_8000_96000 #define MA5311_FORMATS
> > +(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE)
>
> These don't seem to match what hw_params() accepts.
I updated the supported sample rates to match the restriction in hw_params(),
as shown below. Based on our internal evaluation, we concluded that operating
playback and capture simultaneously at 48 kHz is preferable. Therefore, the
current driver supports only 48 kHz.
#define MA5311_RATES_PLAYBACK SNDRV_PCM_RATE_48000
#define MA5311_RATES_CAPTURE SNDRV_PCM_RATE_48000
#define MA5311_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE)
static int ma5311_dai_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 ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
unsigned int bclk = 0;
int ret;
switch (params_rate(params)) {
case 48000:
break;
[Feedback 18]
> > +static struct snd_soc_dai_driver ma5311_dai[] = {
> > + {
> > + .name = "ma5311-amplifier",
> > + .id = 0,
> > + .playback = {
> > + .stream_name = "Playback",
> > + .channels_min = 1,
> > + .channels_max = 2,
> > + .rates = MA5311_RATES_PLAYBACK,
> > + .formats = MA5311_FORMATS,
> > + },
> > + .capture = {
> > + .stream_name = "Capture",
> > + .channels_min = 1,
> > + .channels_max = 8,
> > + .rates = MA5311_RATES_CAPTURE,
> > + .formats = MA5311_FORMATS,
> > + },
> > + .ops = &ma5311_dai_ops_amp,
> > + },
> > +};
>
> I think the device needs symmetric rate and widths too?
static struct snd_soc_dai_driver ma5311_dai[] = {
{
.name = "ma5311-amplifier",
.id = 0,
.playback = {
.stream_name = "Playback",
.channels_min = 1,
.channels_max = 8,
.rates = MA5311_RATES_PLAYBACK,
.formats = MA5311_FORMATS,
},
.capture = {
.stream_name = "Capture",
.channels_min = 1,
.channels_max = 8,
.rates = MA5311_RATES_CAPTURE,
.formats = MA5311_FORMATS,
},
.ops = &ma5311_dai_ops_amp,
.symmetric_rate = 1,
.symmetric_channels = 1,
.symmetric_sample_bits = 1,
},
};
I added `symmetric_rate` and `symmetric_sample_bits` to enforce
matching sample rates and sample bit widths for playback and capture.
[Feedback 19]
> > +static irqreturn_t ma5311_irq_thread(int irq, void *data) {
> > + struct ma5311_priv *ma5311 = data;
> > + unsigned int dac_status;
> > + unsigned int dac_ocp_status;
> > + unsigned int dac_scp_status;
> > + int ret = 0;
> > +
> > + ret |= regmap_read(ma5311->ctrl_regmap,
> > + MA5311_DAC_EA_DAC_STATUS, &dac_status);
> > +
> > + ret |= regmap_read(ma5311->ctrl_regmap,
> > + MA5311_DAC_EB_DAC_OCP_STATUS, &dac_ocp_status);
> > +
> > + ret |= regmap_read(ma5311->ctrl_regmap,
> > + MA5311_DAC_EC_DAC_SCP_STATUS, &dac_scp_status);
> > +
> > + if (ret) {
> > + dev_crit(ma5311->dev,
> > + "Error Occurred During Interrupt Handling\n");
> > + return ret;
>
> This should be IRQ_NONE, we're returning irqreturn_t not an error code.
The IRQ handler now returns `IRQ_NONE` if an I2C register read fails.
static irqreturn_t ma5311_irq_thread(int irq, void *data)
{
struct ma5311_priv *ma5311 = data;
unsigned int dac_status;
int ret = 0;
ret = regmap_read(ma5311->ctrl_regmap,
MA5311_DAC_EA_DAC_STATUS, &dac_status);
if (ret) {
dev_err_ratelimited(ma5311->dev,
"I2C Read Failed During Interrupt Handling\n");
return IRQ_NONE;
}
if (!(dac_status & MA5311_DAC_EA_FAULT_MASK))
return IRQ_NONE;
dev_err_ratelimited(ma5311->dev, "FAULT Asserted\n");
return IRQ_HANDLED;
}
Lastly, do I need to address all feedback from the Sashiko bot, or are only
the "High Priority" and "Medium Priority" comments mandatory, with the
remaining comments serving as recommendations?
Best Regards.
Hyunuk Son
The information in this email and any attachment is confidential and may be legally protected. It is intended solely for the addressee. Access to this email by anyone else is unauthorised. If you are not the intended recipient, any disclosure or actions taken
as a result of the information in this email is prohibited and may be unlawful. If you are not the intended recipient, please notify us immediately and then delete this e-mail and any attachment. Thank you.
> -----Original Message-----
> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: Wednesday, October 28, 2026 8:25 PM
> To: Hyun-Uk Son <hyunuk.son@xxxxxxxxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>; Liam Girdwood <lgirdwood@xxxxxxxxx>;
> Ki-Seok Jo <kiseok.jo@xxxxxxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Jaroslav Kysela <perex@xxxxxxxx>; Takashi Iwai
> <tiwai@xxxxxxxx>; linux-sound@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/2] ASoC: codecs: Add driver for Iron Device
> MA5311
>
> > Add an ASoC codec driver for the Iron Device MA5311.
> >
> > The driver supports audio playback and capture through I2S and TDM
> > interfaces. It provides ALSA mixer controls for audio configuration
> > and uses regmap for register access.
>
> > +static const struct reg_default ma5311_ctrl_reg_def[] = {
> > + { 0x00, 0x88 },
> > + { 0x01, 0x00 },
> > + { 0x02, 0x00 },
> > + { 0x03, 0x01 },
> > + { 0x04, 0x17 },
> > + { 0x05, 0x01 },
> > + { 0x09, 0x00 },
> > + { 0x0A, 0x30 },
> > + { 0x0B, 0x30 },
> > + { 0x0E, 0xAF },
> > + { 0x0F, 0x0F },
> > + { 0x10, 0x0C },
> > + { 0x11, 0x00 },
> > + { 0x12, 0x01 },
> > + { 0x14, 0x5B },
> > + { 0x15, 0x01 },
> > + { 0x16, 0x0F },
> > + { 0x17, 0x0F },
> > + { 0x18, 0x0F },
> > + { 0x19, 0x00 },
> > + { 0x1A, 0x00 },
> > + { 0x1B, 0x00 },
> > + { 0x23, 0x01 },
> > + { 0x24, 0x00 },
> > + { 0x25, 0x00 },
> > + { 0x26, 0x00 },
> > + { 0x2B, 0x07 },
> > + { 0x2C, 0x0C },
> > + { 0x2D, 0x0C },
> > + { 0x2E, 0x0C },
> > + { 0x2F, 0x0C },
> > + { 0x30, 0x0C },
> > + { 0x33, 0x00 },
> > + { 0x34, 0x00 },
> > + { 0x35, 0x00 },
> > + { 0x36, 0x92 },
> > + { 0x37, 0x40 },
> > + { 0x38, 0xFF },
> > + { 0x39, 0x00 },
> > + { 0x3A, 0x00 },
> > + { 0x3B, 0x00 },
> > + { 0x3C, 0x00 },
> > + { 0x3D, 0x00 },
> > + { 0x40, 0x00 },
> > + { 0x41, 0x00 },
> > + { 0x42, 0x00 },
> > + { 0x43, 0x00 },
> > + { 0x44, 0x00 },
> > + { 0x45, 0x00 },
> > + { 0x46, 0x20 },
> > + { 0x47, 0x00 },
> > + { 0x48, 0x00 },
> > + { 0x49, 0x00 },
> > + { 0x4A, 0x00 },
> > + { 0x4B, 0x00 },
> > + { 0x4C, 0x00 },
> > + { 0x4D, 0x00 },
> > + { 0x4E, 0x00 },
> > + { 0x4F, 0x00 },
> > + { 0x50, 0x00 },
> > + { 0x51, 0x00 },
> > + { 0x52, 0x00 },
> > + { 0x53, 0x00 },
> > + { 0x54, 0x00 },
> > + { 0x55, 0x20 },
> > + { 0x56, 0x00 },
> > + { 0x57, 0x00 },
> > + { 0x58, 0x00 },
> > + { 0x59, 0x00 },
> > + { 0x5A, 0x00 },
> > + { 0x5B, 0x00 },
> > + { 0x5C, 0x00 },
> > + { 0x5D, 0x00 },
> > + { 0x5E, 0x00 },
> > + { 0x5F, 0x00 },
> > + { 0x60, 0x00 },
> > + { 0x61, 0x00 },
> > + { 0x62, 0x00 },
> > + { 0x63, 0x00 },
> > + { 0x64, 0x20 },
> > + { 0x65, 0x00 },
> > + { 0x66, 0x00 },
> > + { 0x67, 0x00 },
> > + { 0x68, 0x00 },
> > + { 0x69, 0x00 },
> > + { 0x6A, 0x00 },
> > + { 0x6B, 0x00 },
> > + { 0x6C, 0x00 },
> > + { 0x6D, 0x00 },
> > + { 0x6E, 0x00 },
> > + { 0x6F, 0x00 },
> > + { 0x70, 0x00 },
> > + { 0x71, 0x00 },
> > + { 0x72, 0x00 },
> > + { 0x73, 0x20 },
> > + { 0x74, 0x00 },
> > + { 0x75, 0x00 },
> > + { 0x76, 0x00 },
> > + { 0x77, 0x00 },
> > + { 0x78, 0x00 },
> > + { 0x79, 0x00 },
> > + { 0x7A, 0x00 },
> > + { 0x7B, 0x00 },
> > + { 0x7C, 0x00 },
> > + { 0x7D, 0x00 },
> > + { 0x7E, 0x00 },
> > + { 0x7F, 0x00 },
> > + { 0x80, 0x00 },
> > + { 0x81, 0x00 },
> > + { 0x82, 0x20 },
> > + { 0x83, 0x00 },
> > + { 0x84, 0x00 },
> > + { 0x85, 0x00 },
> > + { 0x86, 0x00 },
> > + { 0x87, 0x00 },
> > + { 0x88, 0x00 },
> > + { 0x89, 0x00 },
> > + { 0x91, 0x00 },
> > + { 0x8A, 0x00 },
> > + { 0x8B, 0x07 },
> > + { 0x8C, 0x70 },
> > + { 0x8D, 0x00 },
> > + { 0x8E, 0x00 },
> > + { 0x8F, 0x02 },
> > + { 0xA1, 0x40 },
> > + { 0xA2, 0x08 },
> > + { 0xA3, 0x20 },
> > + { 0xA4, 0x00 },
> > + { 0xA5, 0x1A },
> > + { 0xA6, 0x52 },
> > + { 0xA7, 0x00 },
> > + { 0xA8, 0x28 },
> > + { 0xA9, 0x25 },
> > + { 0xAA, 0x44 },
> > + { 0xAB, 0x62 },
> > + { 0xAC, 0x04 },
> > + { 0xAD, 0x00 },
> > + { 0xAE, 0x00 },
> > + { 0xAF, 0x00 },
> > + { 0xB0, 0x00 },
> > + { 0xB1, 0x00 },
> > + { 0xB2, 0x00 },
> > + { 0xB3, 0x04 },
> > + { 0xB4, 0x00 },
> > + { 0xB5, 0x00 },
> > + { 0xB7, 0x20 },
> > + { 0xB8, 0x7C },
> > + { 0xB9, 0x0B },
> > + { 0xBB, 0x42 },
> > + { 0xBC, 0x42 },
> > + { 0xBD, 0x61 },
> > + { 0xBE, 0x67 },
> > + { 0xBF, 0x02 },
> > +};
> > +
> > +static const struct reg_default ma5311_adc_reg_def[] = {
> > + { 0x00, 0xA5 },
> > + { 0x01, 0x6C },
> > + { 0x02, 0x07 },
> > + { 0x03, 0x00 },
> > + { 0x04, 0xFF },
> > + { 0x05, 0xFF },
> > + { 0x06, 0x00 },
> > + { 0x07, 0xC0 },
> > + { 0x08, 0xC0 },
> > + { 0x09, 0x10 },
> > + { 0x0A, 0x10 },
> > + { 0x0B, 0xFF },
> > + { 0x0C, 0x6C },
> > + { 0x0D, 0x6C },
> > + { 0x0E, 0x00 },
> > + { 0x0F, 0x02 },
> > + { 0x10, 0xA5 },
> > + { 0x11, 0x6C },
> > + { 0x12, 0x07 },
> > + { 0x13, 0x00 },
> > + { 0x14, 0xFF },
> > + { 0x15, 0xFF },
> > + { 0x16, 0x00 },
> > + { 0x17, 0xC0 },
> > + { 0x18, 0xC0 },
> > + { 0x19, 0x10 },
> > + { 0x1A, 0x10 },
> > + { 0x1B, 0xFF },
> > + { 0x1C, 0x6C },
> > + { 0x1D, 0x6C },
> > + { 0x1E, 0x00 },
> > + { 0x1F, 0x02 },
> > + { 0x20, 0xA5 },
> > + { 0x21, 0x6C },
> > + { 0x22, 0x07 },
> > + { 0x23, 0x00 },
> > + { 0x24, 0xFF },
> > + { 0x25, 0xFF },
> > + { 0x26, 0x00 },
> > + { 0x27, 0xC0 },
> > + { 0x28, 0xC0 },
> > + { 0x29, 0x10 },
> > + { 0x2A, 0x10 },
> > + { 0x2B, 0xFF },
> > + { 0x2C, 0x6C },
> > + { 0x2D, 0x6C },
> > + { 0x2E, 0x00 },
> > + { 0x2F, 0x02 },
> > + { 0x30, 0xA5 },
> > + { 0x31, 0x6C },
> > + { 0x32, 0x07 },
> > + { 0x33, 0x00 },
> > + { 0x34, 0xFF },
> > + { 0x35, 0xFF },
> > + { 0x36, 0x00 },
> > + { 0x37, 0xC0 },
> > + { 0x38, 0xC0 },
> > + { 0x39, 0x10 },
> > + { 0x3A, 0x10 },
> > + { 0x3B, 0xFF },
> > + { 0x3C, 0x6C },
> > + { 0x3D, 0x6C },
> > + { 0x3E, 0x00 },
> > + { 0x3F, 0x02 },
> > +};
>
> > +static bool ma5311_adc_volatile_register(struct device *dev, unsigned
> > +int reg) {
> > + return false;
> > +}
>
> Like I said on v1 this is the default so the function can be dropped.
>
> > +static int ma5311_reset(struct snd_soc_component *component) {
>
> > + ma5311->adc1_vol_left = 0xc0;
> > + ma5311->adc1_vol_right = 0xc0;
> > + ma5311->adc2_vol_left = 0xc0;
> > + ma5311->adc2_vol_right = 0xc0;
> > + ma5311->adc3_vol_left = 0xc0;
> > + ma5311->adc3_vol_right = 0xc0;
> > + ma5311->adc4_vol_left = 0xc0;
> > + ma5311->adc4_vol_right = 0xc0;
> > + ma5311->adc1_pga_left = 0x10;
> > + ma5311->adc1_pga_right = 0x10;
> > + ma5311->adc2_pga_left = 0x10;
> > + ma5311->adc2_pga_right = 0x10;
> > + ma5311->adc3_pga_left = 0x10;
> > + ma5311->adc3_pga_right = 0x10;
> > + ma5311->adc4_pga_left = 0x10;
> > + ma5311->adc4_pga_right = 0x10;
>
> Like I said on v1 we usually leave the device with the hardware defaults
> to avoid having to decide on a specific use case in the kernel -
> presumably the defaults in the hardware were chosen for a reason.
>
> > + ma5311->capture_mode = I2S_ADC1;
>
> What about playback_mode and dapm_aif_in?
>
> > +static int ma5311_adc_volume_control_get(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + const struct soc_mixer_control *mc =
> > + (const struct soc_mixer_control *)kcontrol->private_value;
> > + unsigned int *cache = ma5311_adc_volume_control_cache(ma5311, mc-
> >reg);
> > + unsigned int val;
> > + int ret;
> > +
> > + if (!cache)
> > + return -EINVAL;
> > +
> > + ret = regmap_read(ma5311->adc_regmap, mc->reg, &val);
> > + if (ret)
> > + return ret;
> > +
> > + ucontrol->value.integer.value[0] = val & MA5311_ADC_VOL_CTRL_MASK;
> > + *cache = ucontrol->value.integer.value[0];
> > +
> > + return 0;
> > +}
>
> I still don't understand why we have these open coded caches rather than
> just using a regmap cache and the standard helpers, it'd be a lot less
> code.
>
> > +static int ma5311_playback_mode_put(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int val = ucontrol->value.enumerated.item[0];
> > +
> > + if (val >= ARRAY_SIZE(ma5311_playback_mode_text)) {
> > + dev_err(ma5311->dev, "%s: Out of range\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (ma5311->playback_mode == val)
> > + return 0;
> > +
> > + switch (val) {
> > + case TDM01:
> > + case TDM23:
> > + case TDM45:
> > + case TDM67:
> > + }
> > +
> > + ma5311->playback_mode = val;
>
> This doesn't handle the I2S option so we won't reset if the user flips
> back to that. More generally this feels like it ought to just be a
> set_tdm_slot() operation?
>
> > +static int ma5311_reset_control_get(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> > + struct ma5311_priv *ma5311 =
> > +snd_soc_component_get_drvdata(component);
> > +
> > + if (!ma5311->reset_gpio)
> > + return -ENODEV;
>
> As I said on v1 it'd be better to just not register this control if we
> don't have the GPIO.
>
> > +static int ma5311_reset_control_put(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
>
> > + ret = ma5311_reset(component);
>
> This might break the machine driver if it's relying on setting the DAI up
> during probe()...
>
> > +static int ma5311_dapm_aif_in_put(struct snd_kcontrol *kcontrol,
> > + struct snd_ctl_elem_value *ucontrol) {
> > + struct snd_soc_dapm_context *dapm =
> > + snd_soc_dapm_kcontrol_to_dapm(kcontrol);
> > + struct snd_soc_component *component =
> snd_soc_dapm_to_component(dapm);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int val = ucontrol->value.enumerated.item[0];
> > + unsigned int old_val = ma5311->dapm_aif_in;
> > + int ret;
> > +
> > + if (val >= ARRAY_SIZE(ma5311_aif_in_source_text)) {
> > + dev_err(ma5311->dev, "%s: Out of range\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + if (ma5311->dapm_aif_in == val)
> > + return 0;
> > +
> > + ma5311->dapm_aif_in = val;
>
> DAPM is already storing the value - you could just read it with
> dapm_kcontrol_get_value().
>
> > +static int ma5311_adc_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);
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int mux = ma5311->capture_mode;
> > +
> > + switch (event) {
> > + case SND_SOC_DAPM_PRE_PMU:
> > + regmap_update_bits(ma5311->ctrl_regmap,
> MA5311_DAC_A2_TOP_MAN2,
> > + MA5311_PLL_MASK, MA5311_PLL_ON);
> > +
> > + regmap_update_bits(ma5311->ctrl_regmap,
> > + MA5311_DAC_00_SYSTEM_CTRL1,
> > + MA5311_POWER_MASK, MA5311_POWER_ON);
> > +
> > + switch (mux) {
> > + case I2S_ADC1:
> > + regmap_update_bits(ma5311->adc_regmap,
> > + MA5311_ADC_CH1_00_GENERAL_SETTING,
> > + MA5311_ADC_POWER_MASK,
> > + MA5311_ADC_POWER_ON);
> > + regmap_update_bits(ma5311->adc_regmap,
> > + MA5311_ADC_CH1_0E_MUTE_DATA_FLOW,
> > + MA5311_ADC_MUTE_BOTH_MASK,
> > + MA5311_ADC_BOTH_DEMUTE);
> > + break;
> > + case I2S_ADC2:
> > + regmap_update_bits(ma5311->adc_regmap,
> > + MA5311_ADC_CH1_00_GENERAL_SETTING,
> > + MA5311_ADC_POWER_MASK,
> > + MA5311_ADC_POWER_ON);
> > + regmap_update_bits(ma5311->adc_regmap,
> > + MA5311_ADC_CH1_0E_MUTE_DATA_FLOW,
> > + MA5311_ADC_MUTE_BOTH_MASK,
> > + MA5311_ADC_BOTH_DEMUTE);
>
> Could use a comment about ADC2 enabling ADC1 as well (similarly for ADC4
> and ADC3).
>
> > + SND_SOC_DAPM_OUT_DRV_E("AMP Power", SND_SOC_NOPM, 0, 0, NULL, 0,
> > + ma5311_power_event,
> > + SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD |
> > + SND_SOC_DAPM_POST_PMU),
>
> The function doesn't need all the events?
>
> > +static const struct regmap_config ma5311_ctrl_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > +
> > + .max_register = MA5311_DAC_FF_VERSION,
> > + .readable_reg = ma5311_ctrl_readable_register,
> > + .writeable_reg = ma5311_ctrl_writeable_register,
> > + .volatile_reg = ma5311_ctrl_volatile_register,
> > +
> > + .cache_type = REGCACHE_NONE,
>
> As I said above and on v1 you should probably enable caches here, it'd
> simplify the rest of the code.
>
> > + dev_dbg(component->dev, "Estimated TDM BCLK: %u Hz\n", bclk);
> > + } else if (ma5311->format == SND_SOC_DAIFMT_I2S) {
> > + bclk = params_rate(params) * params_physical_width(params) *
> > + params_channels(params);
>
> Note that I2S always needs to clock two channels even for mono streams.
>
> > +static int ma5311_dai_set_tdm_slot(struct snd_soc_dai *dai,
> > + unsigned int tx_mask, unsigned int rx_mask,
> > + int slots, int slot_width)
> > +{
>
> rx_mask should be what the playback mode is doing I think.
>
> > +static int ma5311_dai_mute_stream(struct snd_soc_dai *dai, int mute,
> > +int stream) {
> > + struct snd_soc_component *component = dai->component;
> > + struct ma5311_priv *ma5311 =
> snd_soc_component_get_drvdata(component);
> > + unsigned int val;
> > +
> > + if (stream == SNDRV_PCM_STREAM_CAPTURE)
> > + return 0;
> > +
> > + if (mute || ma5311->force_mute_status) {
>
> Does anything set force_mute_status?
>
> --
> Mark Brown <broonie@xxxxxxxxxx>