diff mbox

[v2] ASoC: rt5670: add clock source selection controls

Message ID 1409549878-27304-1-git-send-email-bardliao@realtek.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bard Liao Sept. 1, 2014, 5:37 a.m. UTC 
From: Bard Liao <bardliao@realtek.com>

We can select the clock source of some digital widgets in rt5670.
This patch adds the controls of those selections.

Signed-off-by: Bard Liao <bardliao@realtek.com>
---
Mark,

I write the changelog at the beginning of rt5670.c. But I am not
sure if it is a regular way to make a changelog.
---
 sound/soc/codecs/rt5670.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

Comments

Mark Brown Sept. 1, 2014, 3:34 p.m. UTC | #1 
On Mon, Sep 01, 2014 at 01:37:58PM +0800, bardliao@realtek.com wrote:

> I write the changelog at the beginning of rt5670.c. But I am not
> sure if it is a regular way to make a changelog.

No, please don't do this - in the kernel we just use git for the
changelog, there's no need for additional changelogs in files.

> +static const char * const rt5670_asrc_clk_source[] = {
> +	"clk_sysy_div_out", "clk_i2s1_track", "clk_i2s2_track",
> +	"clk_i2s3_track", "clk_i2s4_track", "clk_sys2", "clk_sys3",
> +	"clk_sys4", "clk_sys5"
> +};

Every time I see this patch I raise the same concern about the clocking
usually being controlled in the kernel since other clocks are controlled
within the kernel.  I've not seen a response to this yet - it's possible
there is a good reason for putting this under userspace control but you
need to make the case for doing that.
Bard Liao Sept. 2, 2014, 5:22 a.m. UTC | #2 
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Monday, September 01, 2014 11:35 PM
> To: Bard Liao
> Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; lars@metafoo.de;
> Flove; Oder Chiou
> Subject: Re: [PATCH v2] ASoC: rt5670: add clock source selection controls
> 
> On Mon, Sep 01, 2014 at 01:37:58PM +0800, bardliao@realtek.com
> wrote:
> 
> > +static const char * const rt5670_asrc_clk_source[] = {
> > +	"clk_sysy_div_out", "clk_i2s1_track", "clk_i2s2_track",
> > +	"clk_i2s3_track", "clk_i2s4_track", "clk_sys2", "clk_sys3",
> > +	"clk_sys4", "clk_sys5"
> > +};
> 
> Every time I see this patch I raise the same concern about the clocking
> usually being controlled in the kernel since other clocks are controlled
> within the kernel.  I've not seen a response to this yet - it's possible there
> is a good reason for putting this under userspace control but you need to
> make the case for doing that.

Which clock source should be selected mainly decided by the sampling
rate and the audio routing path. However, there are some exceptions.
For example, if the MCLK and I2S clock are asynchronous, we should
always select "clk_i2s_track" as the clock source. And In some cases,
we can choice "clk_i2s_track" or "clk_sysy_div_out" as the clock source.
Users can choose what they want to use.

Even if we ignore all the exceptions and make choice for users, it is still
hard to implement it in the driver since we need to read a lot of registers
to decide the clock source. It will be easier if we open it to user space
so user can configure it with the audio routing path.

> 
> ------Please consider the environment before printing this e-mail.
Mark Brown Sept. 4, 2014, 7:06 p.m. UTC | #3 
On Tue, Sep 02, 2014 at 05:22:49AM +0000, Bard Liao wrote:

> Which clock source should be selected mainly decided by the sampling
> rate and the audio routing path. However, there are some exceptions.
> For example, if the MCLK and I2S clock are asynchronous, we should
> always select "clk_i2s_track" as the clock source. And In some cases,
> we can choice "clk_i2s_track" or "clk_sysy_div_out" as the clock source.
> Users can choose what they want to use.

> Even if we ignore all the exceptions and make choice for users, it is still
> hard to implement it in the driver since we need to read a lot of registers
> to decide the clock source. It will be easier if we open it to user space
> so user can configure it with the audio routing path.

The choice is usually deferred to the machine driver rather than made
entirely automatically within the CODEC driver - where manual control is
required typically the machine driver is then able to offer a bigger,
system-wide choice to the user that controls several settings and makes
sure that any required coordination occurs.
diff mbox

Patch

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index ba9d9b4..87131a1 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -7,6 +7,10 @@ 
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
+ *
+ * Changelog:
+ *
+ * - Add clock source selections for some digital widgets.
  */
 
 #include <linux/module.h>
@@ -430,6 +434,105 @@  static SOC_ENUM_SINGLE_DECL(rt5670_if2_dac_enum, RT5670_DIG_INF1_DATA,
 static SOC_ENUM_SINGLE_DECL(rt5670_if2_adc_enum, RT5670_DIG_INF1_DATA,
 				RT5670_IF2_ADC_SEL_SFT, rt5670_data_select);
 
+static const char * const rt5670_asrc_clk_source[] = {
+	"clk_sysy_div_out", "clk_i2s1_track", "clk_i2s2_track",
+	"clk_i2s3_track", "clk_i2s4_track", "clk_sys2", "clk_sys3",
+	"clk_sys4", "clk_sys5"
+};
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_da_sto_asrc_enum, RT5670_ASRC_2,
+				12, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_da_monol_asrc_enum, RT5670_ASRC_2,
+				8, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_da_monor_asrc_enum, RT5670_ASRC_2,
+				4, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_ad_sto1_asrc_enum, RT5670_ASRC_2,
+				0, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_up_filter_asrc_enum, RT5670_ASRC_3,
+				12, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_down_filter_asrc_enum, RT5670_ASRC_3,
+				8, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_ad_monol_asrc_enum, RT5670_ASRC_3,
+				4, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_ad_monor_asrc_enum, RT5670_ASRC_3,
+				0, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_ad_sto2_asrc_enum, RT5670_ASRC_5,
+				12, rt5670_asrc_clk_source);
+
+static const SOC_ENUM_SINGLE_DECL(rt5670_dsp_asrc_enum, RT5670_DSP_CLK,
+				0, rt5670_asrc_clk_source);
+
+static int rt5670_clk_sel_put(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
+	unsigned int u_bit = 0, p_bit = 0;
+	struct soc_enum *em =
+		(struct soc_enum *)kcontrol->private_value;
+
+	switch (em->reg) {
+	case RT5670_ASRC_2:
+		switch (em->shift_l) {
+		case 0:
+			u_bit = 0x8;
+			p_bit = RT5670_PWR_ADC_S1F;
+			break;
+		case 4:
+			u_bit = 0x100;
+			p_bit = RT5670_PWR_DAC_MF_R;
+			break;
+		case 8:
+			u_bit = 0x200;
+			p_bit = RT5670_PWR_DAC_MF_L;
+			break;
+		case 12:
+			u_bit = 0x400;
+			p_bit = RT5670_PWR_DAC_S1F;
+			break;
+		}
+		break;
+	case RT5670_ASRC_3:
+		switch (em->shift_l) {
+		case 0:
+			u_bit = 0x1;
+			p_bit = RT5670_PWR_ADC_MF_R;
+			break;
+		case 4:
+			u_bit = 0x2;
+			p_bit = RT5670_PWR_ADC_MF_L;
+			break;
+		}
+		break;
+	case RT5670_ASRC_5:
+		u_bit = 0x4;
+		p_bit = RT5670_PWR_ADC_S2F;
+		break;
+	}
+
+	if (u_bit || p_bit) {
+		switch (ucontrol->value.integer.value[0]) {
+		case 1 ... 4: /*enable*/
+			if (snd_soc_read(codec, RT5670_PWR_DIG2) & p_bit)
+				snd_soc_update_bits(codec,
+					RT5670_ASRC_1, u_bit, u_bit);
+			break;
+		default: /*disable*/
+			snd_soc_update_bits(codec, RT5670_ASRC_1, u_bit, 0);
+			break;
+		}
+	}
+
+	return snd_soc_put_enum_double(kcontrol, ucontrol);
+}
+
 static const struct snd_kcontrol_new rt5670_snd_controls[] = {
 	/* Headphone Output Volume */
 	SOC_DOUBLE("HP Playback Switch", RT5670_HP_VOL,
@@ -482,6 +585,24 @@  static const struct snd_kcontrol_new rt5670_snd_controls[] = {
 
 	SOC_ENUM("ADC IF2 Data Switch", rt5670_if2_adc_enum),
 	SOC_ENUM("DAC IF2 Data Switch", rt5670_if2_dac_enum),
+
+	SOC_ENUM_EXT("DA STO Clk Sel", rt5670_da_sto_asrc_enum,
+		     snd_soc_get_enum_double, rt5670_clk_sel_put),
+	SOC_ENUM_EXT("DA MONOL Clk Sel", rt5670_da_monol_asrc_enum,
+		     snd_soc_get_enum_double, rt5670_clk_sel_put),
+	SOC_ENUM_EXT("DA MONOR Clk Sel", rt5670_da_monor_asrc_enum,
+		     snd_soc_get_enum_double, rt5670_clk_sel_put),
+	SOC_ENUM_EXT("AD STO1 Clk Sel", rt5670_ad_sto1_asrc_enum,
+		     snd_soc_get_enum_double, rt5670_clk_sel_put),
+	SOC_ENUM_EXT("AD MONOL Clk Sel", rt5670_ad_monol_asrc_enum,
+		     snd_soc_get_enum_double, rt5670_clk_sel_put),
+	SOC_ENUM_EXT("AD MONOR Clk Sel", rt5670_ad_monor_asrc_enum,
+		     snd_soc_get_enum_double, rt5670_clk_sel_put),
+	SOC_ENUM("UP Clk Sel", rt5670_up_filter_asrc_enum),
+	SOC_ENUM("DOWN Clk Sel", rt5670_down_filter_asrc_enum),
+	SOC_ENUM_EXT("AD STO2 Clk Sel", rt5670_ad_sto2_asrc_enum,
+		     snd_soc_get_enum_double, rt5670_clk_sel_put),
+	SOC_ENUM("DSP Clk Sel", rt5670_dsp_asrc_enum),
 };
 
 /**