Re: [PATCH] ASoC: rt1011: revert 'I2S Reference' to SOC_ENUM_EXT

["Péter Ujfalusi" <peter.ujfalusi@linux.intel.com>]

Hi Jack,

On 11/11/2021 11:17, Jack Yu wrote:
> Revert 'I2S Reference' to SOC_ENUM_EXT because the settings are specific
> for some platforms, the default setting for 'I2S Reference' does nothing,
> only some SoC platform need to configure it.
> Previous 'I2S Reference' in SOC_ENUM format only toggles one bit of
> RT1011_TDM1_SET_1 register, which isn't enough for specific platform.

This patch again breaks audio but in a less obvious way.
If a user _ever_ touches the "I2S Reference" then audio playback will
never going to work again as instead of changing the i2s reference the
code clears all settings done by set_tdm_slot, dai_fmt to something
which is some product specific setting.
One would think that a reboot helps, but on boot we tend to restore the
saved amixer settings -> audio playback is broken.
So, before reboot one has to set the reference to None and reboot and
hope that on boot the NOP (None) is going to be set which means that the
custom enum code would not overwrite the configuration of the codec.

There is a single bit in RT1011_TDM1_SET_1 (bit 7) which selects the I2S
reference and the reset value is Left (0).

With this custom enum put code you effectively reconfigure the code to
be unusable on most likely in all systems except the one which needs
these settings...

> 
> Signed-off-by: Jack Yu <jack.yu@realtek.com>
> ---
>  sound/soc/codecs/rt1011.c | 55 ++++++++++++++++++++++++++++++++++-----
>  sound/soc/codecs/rt1011.h |  7 +++++
>  2 files changed, 56 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/codecs/rt1011.c b/sound/soc/codecs/rt1011.c
> index 297af7ff824c..b62301a6281f 100644
> --- a/sound/soc/codecs/rt1011.c
> +++ b/sound/soc/codecs/rt1011.c
> @@ -1311,13 +1311,54 @@ static int rt1011_r0_load_info(struct snd_kcontrol *kcontrol,
>  	.put = rt1011_r0_load_mode_put \
>  }
>  
> -static const char * const rt1011_i2s_ref_texts[] = {
> -	"Left Channel", "Right Channel"
> +static const char * const rt1011_i2s_ref[] = {
> +	"None", "Left Channel", "Right Channel"
>  };
>  
> -static SOC_ENUM_SINGLE_DECL(rt1011_i2s_ref_enum,
> -			    RT1011_TDM1_SET_1, 7,
> -			    rt1011_i2s_ref_texts);
> +static SOC_ENUM_SINGLE_DECL(rt1011_i2s_ref_enum, 0, 0,
> +	rt1011_i2s_ref);
> +
> +static int rt1011_i2s_ref_put(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_kcontrol_component(kcontrol);
> +	struct rt1011_priv *rt1011 =
> +		snd_soc_component_get_drvdata(component);
> +
> +	rt1011->i2s_ref = ucontrol->value.enumerated.item[0];
> +	switch (rt1011->i2s_ref) {
> +	case RT1011_I2S_REF_LEFT_CH:
> +		regmap_write(rt1011->regmap, RT1011_TDM_TOTAL_SET, 0x0240);
> +		regmap_write(rt1011->regmap, RT1011_TDM1_SET_2, 0x8);
> +		regmap_write(rt1011->regmap, RT1011_TDM1_SET_1, 0x1022);
> +		regmap_write(rt1011->regmap, RT1011_ADCDAT_OUT_SOURCE, 0x4);
> +		break;
> +	case RT1011_I2S_REF_RIGHT_CH:
> +		regmap_write(rt1011->regmap, RT1011_TDM_TOTAL_SET, 0x0240);
> +		regmap_write(rt1011->regmap, RT1011_TDM1_SET_2, 0x8);
> +		regmap_write(rt1011->regmap, RT1011_TDM1_SET_1, 0x10a2);
> +		regmap_write(rt1011->regmap, RT1011_ADCDAT_OUT_SOURCE, 0x4);
> +		break;
> +	default:
> +		dev_info(component->dev, "I2S Reference: Do nothing\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int rt1011_i2s_ref_get(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +		snd_soc_kcontrol_component(kcontrol);
> +	struct rt1011_priv *rt1011 =
> +		snd_soc_component_get_drvdata(component);
> +
> +	ucontrol->value.enumerated.item[0] = rt1011->i2s_ref;
> +
> +	return 0;
> +}
>  
>  static const struct snd_kcontrol_new rt1011_snd_controls[] = {
>  	/* I2S Data In Selection */
> @@ -1358,7 +1399,8 @@ static const struct snd_kcontrol_new rt1011_snd_controls[] = {
>  	SOC_SINGLE("R0 Temperature", RT1011_STP_INITIAL_RESISTANCE_TEMP,
>  		2, 255, 0),
>  	/* I2S Reference */
> -	SOC_ENUM("I2S Reference", rt1011_i2s_ref_enum),
> +	SOC_ENUM_EXT("I2S Reference", rt1011_i2s_ref_enum,
> +		rt1011_i2s_ref_get, rt1011_i2s_ref_put),
>  };
>  
>  static int rt1011_is_sys_clk_from_pll(struct snd_soc_dapm_widget *source,
> @@ -2017,6 +2059,7 @@ static int rt1011_probe(struct snd_soc_component *component)
>  
>  	schedule_work(&rt1011->cali_work);
>  
> +	rt1011->i2s_ref = 0;
>  	rt1011->bq_drc_params = devm_kcalloc(component->dev,
>  		RT1011_ADVMODE_NUM, sizeof(struct rt1011_bq_drc_params *),
>  		GFP_KERNEL);
> diff --git a/sound/soc/codecs/rt1011.h b/sound/soc/codecs/rt1011.h
> index 68fadc15fa8c..4d6e7492d99c 100644
> --- a/sound/soc/codecs/rt1011.h
> +++ b/sound/soc/codecs/rt1011.h
> @@ -654,6 +654,12 @@ enum {
>  	RT1011_AIFS
>  };
>  
> +enum {
> +	RT1011_I2S_REF_NONE,
> +	RT1011_I2S_REF_LEFT_CH,
> +	RT1011_I2S_REF_RIGHT_CH,
> +};
> +
>  /* BiQual & DRC related settings */
>  #define RT1011_BQ_DRC_NUM 128
>  struct rt1011_bq_drc_params {
> @@ -692,6 +698,7 @@ struct rt1011_priv {
>  	unsigned int r0_reg, cali_done;
>  	unsigned int r0_calib, temperature_calib;
>  	int recv_spk_mode;
> +	int i2s_ref;
>  };
>  
>  #endif		/* end of _RT1011_H_ */
> 

-- 
Péter