2018-03-14 12:40:40

by Katsuhiro Suzuki

[permalink] [raw]
Subject: [PATCH] ASoC: uniphier: evea: add switch for changing source of line-in

This patch adds mixer switch for changing audio source of line-in.
We can choose one of LIN1, 2, 3, default is LIN1.

Signed-off-by: Katsuhiro Suzuki <[email protected]>
---
sound/soc/uniphier/evea.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/sound/soc/uniphier/evea.c b/sound/soc/uniphier/evea.c
index 439f14f91b23..73fd6730095c 100644
--- a/sound/soc/uniphier/evea.c
+++ b/sound/soc/uniphier/evea.c
@@ -18,6 +18,8 @@

#define AADCPOW(n) (0x0078 + 0x04 * (n))
#define AADCPOW_AADC_POWD BIT(0)
+#define ALINSW1 0x0088
+#define ALINSW1_SEL1_SHIFT 3
#define AHPOUTPOW 0x0098
#define AHPOUTPOW_HP_ON BIT(4)
#define ALINEPOW 0x009c
@@ -278,7 +280,16 @@ static int evea_set_switch_hp(struct snd_kcontrol *kcontrol,
return evea_update_switch_hp(evea);
}

+static const char * const linsw1_sel1_text[] = {
+ "LIN1", "LIN2", "LIN3"
+};
+
+static SOC_ENUM_SINGLE_DECL(linsw1_sel1_enum,
+ ALINSW1, ALINSW1_SEL1_SHIFT,
+ linsw1_sel1_text);
+
static const struct snd_kcontrol_new evea_controls[] = {
+ SOC_ENUM("Line Capture Source", linsw1_sel1_enum),
SOC_SINGLE_BOOL_EXT("Line Capture Switch", 0,
evea_get_switch_lin, evea_set_switch_lin),
SOC_SINGLE_BOOL_EXT("Line Playback Switch", 0,
--
2.16.1



2018-03-14 16:29:02

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] ASoC: uniphier: evea: add switch for changing source of line-in

On Wed, Mar 14, 2018 at 09:39:00PM +0900, Katsuhiro Suzuki wrote:
> This patch adds mixer switch for changing audio source of line-in.
> We can choose one of LIN1, 2, 3, default is LIN1.

I'll apply for now but this should really be a DAPM control so that we
can power down things connected to the disconnected line inputs when
recording.


Attachments:
(No filename) (346.00 B)
signature.asc (499.00 B)
Download all attachments

2018-03-14 16:41:09

by Mark Brown

[permalink] [raw]
Subject: Applied "ASoC: uniphier: evea: add switch for changing source of line-in" to the asoc tree

The patch

ASoC: uniphier: evea: add switch for changing source of line-in

has been applied to the asoc tree at

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 90e0fb05e5c1b1cf6a59c4f888f500e2b1feffc4 Mon Sep 17 00:00:00 2001
From: Katsuhiro Suzuki <[email protected]>
Date: Wed, 14 Mar 2018 21:39:00 +0900
Subject: [PATCH] ASoC: uniphier: evea: add switch for changing source of
line-in

This patch adds mixer switch for changing audio source of line-in.
We can choose one of LIN1, 2, 3, default is LIN1.

Signed-off-by: Katsuhiro Suzuki <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
sound/soc/uniphier/evea.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/sound/soc/uniphier/evea.c b/sound/soc/uniphier/evea.c
index 439f14f91b23..73fd6730095c 100644
--- a/sound/soc/uniphier/evea.c
+++ b/sound/soc/uniphier/evea.c
@@ -18,6 +18,8 @@

#define AADCPOW(n) (0x0078 + 0x04 * (n))
#define AADCPOW_AADC_POWD BIT(0)
+#define ALINSW1 0x0088
+#define ALINSW1_SEL1_SHIFT 3
#define AHPOUTPOW 0x0098
#define AHPOUTPOW_HP_ON BIT(4)
#define ALINEPOW 0x009c
@@ -278,7 +280,16 @@ static int evea_set_switch_hp(struct snd_kcontrol *kcontrol,
return evea_update_switch_hp(evea);
}

+static const char * const linsw1_sel1_text[] = {
+ "LIN1", "LIN2", "LIN3"
+};
+
+static SOC_ENUM_SINGLE_DECL(linsw1_sel1_enum,
+ ALINSW1, ALINSW1_SEL1_SHIFT,
+ linsw1_sel1_text);
+
static const struct snd_kcontrol_new evea_controls[] = {
+ SOC_ENUM("Line Capture Source", linsw1_sel1_enum),
SOC_SINGLE_BOOL_EXT("Line Capture Switch", 0,
evea_get_switch_lin, evea_set_switch_lin),
SOC_SINGLE_BOOL_EXT("Line Playback Switch", 0,
--
2.16.2


2018-03-19 04:36:18

by Katsuhiro Suzuki

[permalink] [raw]
Subject: Re: [PATCH] ASoC: uniphier: evea: add switch for changing source of line-in

Hello Mark,

> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: Thursday, March 15, 2018 1:26 AM
> To: Suzuki, Katsuhiro <[email protected]>
> Cc: [email protected]; Masami Hiramatsu
<[email protected]>;
> Jassi Brar <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [PATCH] ASoC: uniphier: evea: add switch for changing source of
line-in
>
> On Wed, Mar 14, 2018 at 09:39:00PM +0900, Katsuhiro Suzuki wrote:
> > This patch adds mixer switch for changing audio source of line-in.
> > We can choose one of LIN1, 2, 3, default is LIN1.
>
> I'll apply for now but this should really be a DAPM control so that we
> can power down things connected to the disconnected line inputs when
> recording.

Thanks a lot for your suggestion. I tried to change the implementation to DAPM
control as follows:

----------
static const char * const linsw1_sel1_text[] = {
"LIN1", "LIN2", "LIN3"
};

static SOC_ENUM_SINGLE_DECL(linsw1_sel1_enum,
ALINSW1, ALINSW1_SEL1_SHIFT,
linsw1_sel1_text);

static const struct snd_kcontrol_new linesw1_mux =
SOC_DAPM_ENUM("Line In 1 Source", linsw1_sel1_enum);

static const struct snd_soc_dapm_widget evea_widgets[] = {
SND_SOC_DAPM_ADC("ADC", NULL, SND_SOC_NOPM, 0, 0),
SND_SOC_DAPM_MUX("Line In 1 Mux", SND_SOC_NOPM, 0, 0, &linesw1_mux),
SND_SOC_DAPM_INPUT("LIN1_LP"),
SND_SOC_DAPM_INPUT("LIN1_RP"),
SND_SOC_DAPM_INPUT("LIN2_LP"),
SND_SOC_DAPM_INPUT("LIN2_RP"),
SND_SOC_DAPM_INPUT("LIN3_LP"),
SND_SOC_DAPM_INPUT("LIN3_RP"),

SND_SOC_DAPM_DAC("DAC HP", NULL, SND_SOC_NOPM, 0, 0),
SND_SOC_DAPM_DAC("DAC LO1", NULL, SND_SOC_NOPM, 0, 0),
SND_SOC_DAPM_DAC("DAC LO2", NULL, SND_SOC_NOPM, 0, 0),
SND_SOC_DAPM_OUTPUT("HP1_L"),
SND_SOC_DAPM_OUTPUT("HP1_R"),
SND_SOC_DAPM_OUTPUT("LO2_L"),
SND_SOC_DAPM_OUTPUT("LO2_R"),
};

static const struct snd_soc_dapm_route evea_routes[] = {
{ "Line In 1", NULL, "ADC" },
{ "ADC", NULL, "Line In 1 Mux" },
{ "Line In 1 Mux", NULL, "LIN1_LP" },
{ "Line In 1 Mux", NULL, "LIN1_RP" },
{ "Line In 1 Mux", NULL, "LIN2_LP" },
{ "Line In 1 Mux", NULL, "LIN2_RP" },
{ "Line In 1 Mux", NULL, "LIN3_LP" },
{ "Line In 1 Mux", NULL, "LIN3_RP" },

{ "DAC HP", NULL, "Headphone 1" },
{ "DAC LO1", NULL, "Line Out 1" },
{ "DAC LO2", NULL, "Line Out 2" },
{ "HP1_L", NULL, "DAC HP" },
{ "HP1_R", NULL, "DAC HP" },
{ "LO2_L", NULL, "DAC LO2" },
{ "LO2_R", NULL, "DAC LO2" },
};
----------

I can see the value of ALINSW1 register at 'Line In 1 Mux',0 using
amixer get 'Line In 1 Mux',0

But I can't change the value.
amixer set 'Line In 1 Mux',0 LIN2
Simple mixer control 'Line In 1 Mux',0
Capabilities: enum
Items: 'LIN1' 'LIN2' 'LIN3'
Item0: 'LIN1'

Would you tell me what is wrong...


Regards,
--
Katsuhiro Suzuki




2018-03-20 02:06:33

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] ASoC: uniphier: evea: add switch for changing source of line-in

On Mon, Mar 19, 2018 at 01:19:10PM +0900, Katsuhiro Suzuki wrote:

> > I'll apply for now but this should really be a DAPM control so that we
> > can power down things connected to the disconnected line inputs when
> > recording.

> Thanks a lot for your suggestion. I tried to change the implementation to DAPM
> control as follows:

> I can see the value of ALINSW1 register at 'Line In 1 Mux',0 using
> amixer get 'Line In 1 Mux',0

> But I can't change the value.
> amixer set 'Line In 1 Mux',0 LIN2
> Simple mixer control 'Line In 1 Mux',0
> Capabilities: enum
> Items: 'LIN1' 'LIN2' 'LIN3'
> Item0: 'LIN1'

> Would you tell me what is wrong...

Ugh, I *have* run into that before but I can't remember what triggers it
and your code doesn't have any mistakes I can spot. Unfortunately I'm
at Linaro Connect this week and don't have a test system I can poke at
with me to remind myself, and I'm still travelling next week
unfortunately.

I'd add some trace to the set code path to make sure everything is being
called as expected. It's somemthing really small that's hard to make a
warning for in the code IIRC.


Attachments:
(No filename) (1.14 kB)
signature.asc (499.00 B)
Download all attachments

2018-03-20 02:37:09

by Katsuhiro Suzuki

[permalink] [raw]
Subject: Re: [PATCH] ASoC: uniphier: evea: add switch for changing source of line-in

Hello Mark,

> -----Original Message-----
> From: Mark Brown [mailto:[email protected]]
> Sent: Tuesday, March 20, 2018 10:12 AM
> To: Suzuki, Katsuhiro <[email protected]>
> Cc: [email protected]; Masami Hiramatsu
<[email protected]>;
> Jassi Brar <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [PATCH] ASoC: uniphier: evea: add switch for changing source of
line-in
>
> On Mon, Mar 19, 2018 at 01:19:10PM +0900, Katsuhiro Suzuki wrote:
>
> > > I'll apply for now but this should really be a DAPM control so that we
> > > can power down things connected to the disconnected line inputs when
> > > recording.
>
> > Thanks a lot for your suggestion. I tried to change the implementation to
DAPM
> > control as follows:
>
> > I can see the value of ALINSW1 register at 'Line In 1 Mux',0 using
> > amixer get 'Line In 1 Mux',0
>
> > But I can't change the value.
> > amixer set 'Line In 1 Mux',0 LIN2
> > Simple mixer control 'Line In 1 Mux',0
> > Capabilities: enum
> > Items: 'LIN1' 'LIN2' 'LIN3'
> > Item0: 'LIN1'
>
> > Would you tell me what is wrong...
>
> Ugh, I *have* run into that before but I can't remember what triggers it
> and your code doesn't have any mistakes I can spot. Unfortunately I'm

Thank you for reviewing.


> at Linaro Connect this week and don't have a test system I can poke at
> with me to remind myself, and I'm still travelling next week
> unfortunately.
>

I see, no problem. Have a nice trip!


> I'd add some trace to the set code path to make sure everything is being
> called as expected. It's somemthing really small that's hard to make a
> warning for in the code IIRC.

I traced some DAPM codes. The 'get' function as follows:

int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
//...

mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
if (e->reg != SND_SOC_NOPM && dapm_kcontrol_is_powered(kcontrol)) {
int ret = soc_dapm_read(dapm, e->reg, &reg_val);

The soc_dapm_read() reads real value of register. It's simple.


But 'put' function is mysterious for me...

int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)
{
//...

change = dapm_kcontrol_set_value(kcontrol, val);

if (e->reg != SND_SOC_NOPM)
reg_change = soc_dapm_test_bits(dapm, e->reg, mask, val);

dapm_kcontrol_set_value() has stored value into dapm_kcontrol_data. And
soc_dapm_test_bits() has just checked value of a register and returned need
update or not. It seems anyone does not update a register in this function.

I tried to change soc_dapm_test_bits() -> soc_dapm_update_bits(), so I can
change value of register using amixer. But I feel this change was wrong. And I
found dapm_seq_run_coalesced() calls soc_dapm_update_bits(). Unfortunately
this function has not been called even if I run amixer.

Anyway, I'll continue to study about DAPM codes.


Regards,
--
Katsuhiro Suzuki