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

[Péter Ujfalusi]

Hi jack,

On 01/12/2021 04:36, Jack Yu wrote:
> Hi Peter,
> 
>> 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...
>>
> This patch is specific for device which uses I2S format and need the reference data, 
> if the device uses TDM format, it doesn't need to call "I2S Reference ".
> 
> If device runs TDM mode, machine driver sets TDM slot and DAI format, but no need to call " I2S reference ".
> If device runs I2S mode, machine driver sets I2S DAI format, and ucm sets "I2S reference" (it's optional, only if device needs reference signal )

But then this should not be called as 'I2S Reference'
'I2S Reference' is _one_ bit.
This is something like drop dai_fmt and all configuration and
set ADCDAT1/2 pin to output
set I2S, 24 bit for rx/tx
set undocumented BIT12 in RT1011_TDM1_SET_1
set 'TDM1 DOUT Source' to ALC

What happens if one starts 16bit audio playback after triggering this
macro? It is going to change some of the settings and things might brake
here and there?

What I'm saying is that the implementation is misleading and it is
covertly reconfigures the codec to some static config

> 
>>>
>>> Signed-off-by: Jack Yu <jack.yu at 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
>> ------Please consider the environment before printing this e-mail.

-- 
Péter