Re: [alsa-devel] [PATCH] ASoC: rt5645: Add codec driver

On Fri, Apr 11, 2014 at 06:34:19PM +0800, Oder Chiou wrote:

This patch adds the Realtek ALC5645 codec driver. It is the base version that because the jack detect function is not implemented to it, the headphone and AMIC1 are not workable. We will fill up the further functions later.

This looks pretty good, a few things below but none of them too big.

+static bool rt5645_readable_register(struct device *dev, unsigned int reg) +{

• int i;
• for (i = 0; i < ARRAY_SIZE(rt5645_ranges); i++)

• if ((reg >= rt5645_ranges[i].window_start &&
  

•      reg <= rt5645_ranges[i].window_start +
  

•      rt5645_ranges[i].window_len) ||
  

•     (reg >= rt5645_ranges[i].range_min &&
  

•      reg <= rt5645_ranges[i].range_max))
  

• 	return true;
  

Please include some braces in these loops for legibility.

+static int rt5645_set_dmic1_event(struct snd_soc_dapm_widget *w,

• struct snd_kcontrol *kcontrol, int event)

+{

• struct snd_soc_codec *codec = w->codec;
• switch (event) {
• case SND_SOC_DAPM_PRE_PMU:

• snd_soc_update_bits(codec, RT5645_GPIO_CTRL1,
  

• 	RT5645_GP2_PIN_MASK, RT5645_GP2_PIN_DMIC1_SCL);
  

• snd_soc_update_bits(codec, RT5645_DMIC1, RT5645_DMIC_1_DP_MASK,
  

• 	RT5645_DMIC_1_DP_IN2N);
  

• break;
  

For rt5640 we've got this configued by platform data - why not do the same here? In general I'm seeing some similarities again, though I didn't check to see if the same driver can be used yet. Is that the case?

+static const struct snd_kcontrol_new hp_r_vol_control =

• SOC_DAPM_SINGLE_AUTODISABLE("Switch", RT5645_HP_VOL,

• RT5645_R_MUTE_SFT, 1, 1);
  

+static void hp_amp_power(struct snd_soc_codec *codec, int on)

Coding style, your blank line is in the wrong place.

+static int rt5645_hp_event(struct snd_soc_dapm_widget *w,

• struct snd_kcontrol *kcontrol, int event)

+{

• struct snd_soc_codec *codec = w->codec;
• switch (event) {
• case SND_SOC_DAPM_POST_PMU:

• rt5645_pmu_depop(codec);
  

• break;
  

• case SND_SOC_DAPM_PRE_PMD:

• rt5645_pmd_depop(codec);
  

• break;
  

May as well just inline the power up/down sequences?

• case SND_SOC_DAPM_POST_PMU:

• regmap_write(rt5645->regmap, RT5645_PR_BASE + 0x1c, 0xfd20);
  

• regmap_write(rt5645->regmap, RT5645_PR_BASE + 0x20, 0x611f);
  

• regmap_write(rt5645->regmap, RT5645_PR_BASE + 0x21, 0x4040);
  

• regmap_write(rt5645->regmap, RT5645_PR_BASE + 0x23, 0x0004);
  

• snd_soc_update_bits(codec, RT5645_PWR_DIG1,
  

• 	RT5645_PWR_CLS_D | RT5645_PWR_CLS_D_R |
  

Either use snd_soc_ or regmap_ - either is fine but be consistent.

• 	RT5645_PWR_CLS_D_L,
  

• 	RT5645_PWR_CLS_D | RT5645_PWR_CLS_D_R |
  

• 	RT5645_PWR_CLS_D_L);
  

• break;
  

• case SND_SOC_DAPM_PRE_PMD:

• snd_soc_update_bits(codec, RT5645_PWR_DIG1,
  

• 	RT5645_PWR_CLS_D | RT5645_PWR_CLS_D_R |
  

• 	RT5645_PWR_CLS_D_L, 0);
  

It's a bit odd that the first bank of writes done with regmap_write() isn't undone here, can it be done once on init?

+static int rt5645_pdm1_l_event(struct snd_soc_dapm_widget *w,

• struct snd_kcontrol *kcontrol, int event)

+{

• struct snd_soc_codec *codec = w->codec;
• switch (event) {
• case SND_SOC_DAPM_POST_PMU:

• snd_soc_update_bits(codec, RT5645_PDM_OUT_CTRL,
  

• 	RT5645_M_PDM1_L, 0);
  

• break;
  

Why are these done by an event - I'd expect this to be done using the normal DAPM register update support?

• bclk_ms = frame_size > 32 ? 1 : 0;

bclk_ms = frame_size > 32.

+static int rt5645_set_bias_level(struct snd_soc_codec *codec,

• 	enum snd_soc_bias_level level)
  

+{

• switch (level) {
• case SND_SOC_BIAS_STANDBY:

• if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
  

• 	snd_soc_update_bits(codec, RT5645_PWR_ANLG1,
  

• 		RT5645_PWR_VREF1 | RT5645_PWR_MB |
  

• 		RT5645_PWR_BG | RT5645_PWR_VREF2,
  

• 		RT5645_PWR_VREF1 | RT5645_PWR_MB |
  

• 		RT5645_PWR_BG | RT5645_PWR_VREF2);
  

• 	mdelay(10);
  

• 	snd_soc_update_bits(codec, RT5645_PWR_ANLG1,
  

• 		RT5645_PWR_FV1 | RT5645_PWR_FV2,
  

• 		RT5645_PWR_FV1 | RT5645_PWR_FV2);
  

• 	snd_soc_update_bits(codec, RT5645_GEN_CTRL1,
  

• 		RT5645_DIG_GATE_CTRL, RT5645_DIG_GATE_CTRL);
  

• }
  

• break;
  

Since the device can power up and down very quickly it should make sense to set idle_bias_off for a power saving when idle.

+#ifdef CONFIG_PM +static int rt5645_suspend(struct snd_soc_codec *codec) +{

• rt5645_set_bias_level(codec, SND_SOC_BIAS_OFF);
• return 0;

+}

+static int rt5645_resume(struct snd_soc_codec *codec) +{

• return 0;

+} +#else +#define rt5645_suspend NULL +#define rt5645_resume NULL +#endif

If you use idle_bias_off you can remove this.

+#if defined(CONFIG_OF) +static const struct of_device_id rt5645_of_match[] = {

• { .compatible = "realtek,rt5645", },
• {},

+}; +MODULE_DEVICE_TABLE(of, rt5645_of_match); +#endif

We need device tree binding documentation for any device with DT bindings.

+static int rt5645_i2c_remove(struct i2c_client *i2c) +{

• snd_soc_unregister_codec(&i2c->dev);
• kfree(i2c_get_clientdata(i2c));
• return 0;

+}

You used devm_kzalloc(), no need to free.

+void rt5645_i2c_shutdown(struct i2c_client *client) +{

• struct rt5645_priv *rt5645 = i2c_get_clientdata(client);
• struct snd_soc_codec *codec = rt5645->codec;
• if (codec != NULL)

• rt5645_set_bias_level(codec, SND_SOC_BIAS_OFF);
  

+}

The ASoC framework will do this for you.