[PATCH 2/2] ASoC: rt1011: ADCDAT pin config modification

[PATCH 2/2] ASoC: rt1011: ADCDAT pin config modification

[shumingf]

From: Shuming Fan <shumingf@realtek.com>

ADCDAT pin config sets to output mode in default.
And the driver creates the kcontrol to control ADCDAT pin configuration.

Signed-off-by: Shuming Fan <shumingf@realtek.com>
---
 sound/soc/codecs/rt1011.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt1011.c b/sound/soc/codecs/rt1011.c
index f9235764ad32..52c17450d5a0 100644
--- a/sound/soc/codecs/rt1011.c
+++ b/sound/soc/codecs/rt1011.c
@@ -1029,6 +1029,14 @@ static SOC_ENUM_SINGLE_DECL(rt1011_tdm_adc1_1_enum,	RT1011_TDM1_SET_3, 6,
 static SOC_ENUM_SINGLE_DECL(rt1011_tdm_adc2_1_enum,	RT1011_TDM1_SET_3, 4,
 	rt1011_tdm_adc_swap_select);
 
+static const char * const rt1011_adcdat_pin_config_select[] = {
+	"Output", "Input"
+};
+static SOC_ENUM_SINGLE_DECL(rt1011_adcdat1_pin_config_enum, RT1011_TDM_TOTAL_SET, 4,
+	rt1011_adcdat_pin_config_select);
+static SOC_ENUM_SINGLE_DECL(rt1011_adcdat2_pin_config_enum, RT1011_TDM_TOTAL_SET, 3,
+	rt1011_adcdat_pin_config_select);
+
 static void rt1011_reset(struct regmap *regmap)
 {
 	regmap_write(regmap, RT1011_RESET, 0);
@@ -1343,6 +1351,8 @@ static const struct snd_kcontrol_new rt1011_snd_controls[] = {
 	SOC_ENUM("I2S ADC DOUT Mode", rt1011_adc_dout_mode_enum),
 	SOC_ENUM("TDM1 DOUT Length", rt1011_tdm1_dout_len_enum),
 	SOC_ENUM("TDM2 DOUT Length", rt1011_tdm2_dout_len_enum),
+	SOC_ENUM("ADCDAT1 Pin Config", rt1011_adcdat1_pin_config_enum),
+	SOC_ENUM("ADCDAT2 Pin Config", rt1011_adcdat2_pin_config_enum),
 
 	/* Speaker/Receiver Mode */
 	SOC_SINGLE_EXT("RECV SPK Mode", SND_SOC_NOPM, 0, 1, 0,
@@ -1859,9 +1869,6 @@ static int rt1011_set_tdm_slot(struct snd_soc_dai *dai,
 		RT1011_TDM_I2S_DOCK_EN_1_MASK,	tdm_en);
 	snd_soc_component_update_bits(component, RT1011_TDM2_SET_2,
 		RT1011_TDM_I2S_DOCK_EN_2_MASK,	tdm_en);
-	snd_soc_component_update_bits(component, RT1011_TDM_TOTAL_SET,
-		RT1011_ADCDAT1_PIN_CONFIG | RT1011_ADCDAT2_PIN_CONFIG,
-		RT1011_ADCDAT1_OUTPUT | RT1011_ADCDAT2_OUTPUT);
 
 	snd_soc_dapm_mutex_unlock(dapm);
 	return ret;
@@ -2207,6 +2214,11 @@ static void rt1011_calibration_work(struct work_struct *work)
 
 	/* initial */
 	rt1011_reg_init(component);
+
+	/* set ADCDAT to output mode */
+	snd_soc_component_update_bits(component, RT1011_TDM_TOTAL_SET,
+		RT1011_ADCDAT1_PIN_CONFIG | RT1011_ADCDAT2_PIN_CONFIG,
+		RT1011_ADCDAT1_OUTPUT | RT1011_ADCDAT2_OUTPUT);
 }
 
 static int rt1011_i2c_probe(struct i2c_client *i2c,
-- 
2.23.0

Re: [PATCH 2/2] ASoC: rt1011: ADCDAT pin config modification

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 360 bytes --]

On Mon, Aug 26, 2019 at 05:01:20PM +0800, shumingf@realtek.com wrote:
> From: Shuming Fan <shumingf@realtek.com>
> 
> ADCDAT pin config sets to output mode in default.
> And the driver creates the kcontrol to control ADCDAT pin configuration.

Why would this change at runtime?  I'd expect this to be controlled by
the machine driver or a DT property.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ASoC: rt1011: ADCDAT pin config modification

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 511 bytes --]

On Wed, Aug 28, 2019 at 11:01:21AM +0000, Shuming [范書銘] wrote:

> > > ADCDAT pin config sets to output mode in default.
> > > And the driver creates the kcontrol to control ADCDAT pin configuration.

> > Why would this change at runtime?  I'd expect this to be controlled by the
> > machine driver or a DT property.

> It will not change at runtime in the normal case. 
> I think we may use it to test the feedback signal in factory mode.

So really this is setting up a loopback for testing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2/2] ASoC: rt1011: ADCDAT pin config modification

[Shuming [范書銘]]

> > > > ADCDAT pin config sets to output mode in default.
> > > > And the driver creates the kcontrol to control ADCDAT pin configuration.
> 
> > > Why would this change at runtime?  I'd expect this to be controlled
> > > by the machine driver or a DT property.
> 
> > It will not change at runtime in the normal case.
> > I think we may use it to test the feedback signal in factory mode.
> 
> So really this is setting up a loopback for testing?

We would not like to change ADCDAT pin to input mode in normal case.
That's why the driver enables ADCDAT pin to the output mode in default.
The rt1011 supports the feedback signal which could be playback data or I/V data, etc.
If the system wants the AEC reference data, rt1011 could feedback the playback data.

The product could connect 2/4/6/8 rt1011 chips on the same I2S bus.
In a test or debug mode, we could toggle ADCDAT pin to input mode that also
helps HW engineer check the slot of feedback signal for each rt1011.

> ------Please consider the environment before printing this e-mail.

Re: [PATCH 2/2] ASoC: rt1011: ADCDAT pin config modification

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1209 bytes --]

On Thu, Aug 29, 2019 at 05:54:00AM +0000, Shuming [范書銘] wrote:

> > So really this is setting up a loopback for testing?

> We would not like to change ADCDAT pin to input mode in normal case.
> That's why the driver enables ADCDAT pin to the output mode in default.
> The rt1011 supports the feedback signal which could be playback data or I/V data, etc.
> If the system wants the AEC reference data, rt1011 could feedback the playback data.

> The product could connect 2/4/6/8 rt1011 chips on the same I2S bus.
> In a test or debug mode, we could toggle ADCDAT pin to input mode that also
> helps HW engineer check the slot of feedback signal for each rt1011.

I think this needs more than just a straight userspace control on one
device, these use cases make sense but they'll need to be configured
over multiple chips simultaneously otherwise there's some possibility of
hardware damage (eg, if two chips try to drive the signal at the same
time).  If this really can be usefully varied at runtime then the driver
bit of this should probably be an API that the machine driver can call,
the machine driver can then expose a control that sets all the chips
involved up together.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH 2/2] ASoC: rt1011: ADCDAT pin config modification

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1209 bytes --]

On Thu, Aug 29, 2019 at 05:54:00AM +0000, Shuming [范書銘] wrote:

> > So really this is setting up a loopback for testing?

> We would not like to change ADCDAT pin to input mode in normal case.
> That's why the driver enables ADCDAT pin to the output mode in default.
> The rt1011 supports the feedback signal which could be playback data or I/V data, etc.
> If the system wants the AEC reference data, rt1011 could feedback the playback data.

> The product could connect 2/4/6/8 rt1011 chips on the same I2S bus.
> In a test or debug mode, we could toggle ADCDAT pin to input mode that also
> helps HW engineer check the slot of feedback signal for each rt1011.

I think this needs more than just a straight userspace control on one
device, these use cases make sense but they'll need to be configured
over multiple chips simultaneously otherwise there's some possibility of
hardware damage (eg, if two chips try to drive the signal at the same
time).  If this really can be usefully varied at runtime then the driver
bit of this should probably be an API that the machine driver can call,
the machine driver can then expose a control that sets all the chips
involved up together.

[-- 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

Re: [PATCH 2/2] ASoC: rt1011: ADCDAT pin config modification

[Shuming [范書銘]]

> > > So really this is setting up a loopback for testing?
> 
> > We would not like to change ADCDAT pin to input mode in normal case.
> > That's why the driver enables ADCDAT pin to the output mode in default.
> > The rt1011 supports the feedback signal which could be playback data or I/V
> data, etc.
> > If the system wants the AEC reference data, rt1011 could feedback the
> playback data.
> 
> > The product could connect 2/4/6/8 rt1011 chips on the same I2S bus.
> > In a test or debug mode, we could toggle ADCDAT pin to input mode that
> > also helps HW engineer check the slot of feedback signal for each rt1011.
> 
> I think this needs more than just a straight userspace control on one device,
> these use cases make sense but they'll need to be configured over multiple
> chips simultaneously otherwise there's some possibility of hardware damage
> (eg, if two chips try to drive the signal at the same time).  If this really can be
> usefully varied at runtime then the driver bit of this should probably be an API
> that the machine driver can call, the machine driver can then expose a control
> that sets all the chips involved up together.

I understand your concerns and comments.
In a design-in project, we will provide the proper control settings
to arrange the slot location of ADCDAT signal for each rt1011.
HW engineer will confirm the ADCDAT signal, too.
So, the customer should not make two chips drive the signal
at the same slot and at the same time.
But, there is a chance to happen if the customer sets the wrong control settings.
We will take your suggestion to make it as API call.
The machine driver knows how many rt1011 chips connect and
make pin config change together.
Thanks.

> ------Please consider the environment before printing this e-mail.