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

On Fri, May 29, 2015 at 01:32:05PM -0700, Anish Kumar wrote:

This looks mostly good, there's a few things below but they're mostly pretty small and should be easy to update.

+static const char *const max98926_hpf_cutoff_txt[] = {

• "hpf_disable", "hpf_dc_block", "hpf_enable_100",
• "hpf_enable_200", "hpf_enable_400", "hpf_enable_800",

+};

Please write these like other ALSA controls write user visible strings - "Disable", "DC Block", "100Hz" and so on.

+static const char *const max98926_input_txt[] = {

• "Pcm", "Analog",

+};

PCM.

+static struct reg_default max98926_reg[] = {

• { 0x00, 0x00 }, /* Battery Voltage Data */
• { 0x01, 0x00 }, /* Boost Voltage Data */
• { 0x02, 0x00 }, /* Live Status0 */
• { 0x03, 0x00 }, /* Live Status1 */
• { 0x04, 0x00 }, /* Live Status2 */

These and some of the other registers sound like volatile registers which shouldn't have defaults since they shouldn't be in the cache at all but rather read from the device - and you do in fact have a function marking them as volatile further down.

+static int max98926_dac_event(struct snd_soc_dapm_widget *w,

• struct snd_kcontrol *kcontrol, int event)
  

+{

• struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
• struct max98926_priv *max98926 = snd_soc_codec_get_drvdata(codec);
• switch (event) {
• case SND_SOC_DAPM_PRE_PMU:

• regmap_update_bits(max98926->regmap,
  

• 	MAX98926_BLOCK_ENABLE, MAX98926_BST_EN_MASK |
  

• 	MAX98926_ADC_IMON_EN_MASK |
  

• 	MAX98926_ADC_VMON_EN_MASK,
  

• 	MAX98926_BST_EN_MASK |
  

• 	MAX98926_ADC_IMON_EN_MASK |
  

• 	MAX98926_ADC_VMON_EN_MASK);
  

• break;
  

Why not just use DAPM widgets for these? BST looks like a boost amplifier so should probably be a PGA, IMON and VMON can probably be represented usefully as supplies (or could be PGAs routed through when recording the signal, presumably users could save a little power by not enabling the monitoring if they don't care for some reason).

+static int pdm_ch_zero; +static int pdm_ch_one;

No global variables, store any state you need in driver data.

• 	MAX98926_PDM_CHANNEL_1_MASK);
  

• }
• switch (source) {

Coding style: blank line between blocks.

• case 0:

• /* enable current */
  

• regmap_update_bits(max98926->regmap,
  

• 	reg, MAX98926_PDM_CURRENT_MASK,
  

• 	MAX98926_PDM_CURRENT_MASK);
  

• /* enable source*/
  

• if (channel == 0)
  

• 	regmap_update_bits(max98926->regmap,
  

• 		reg, MAX98926_PDM_SOURCE_0_MASK,
  

• 		MAX98926_PDM_SOURCE_0_MASK);
  

• else
  

• 	regmap_update_bits(max98926->regmap,
  

• 		reg, MAX98926_PDM_SOURCE_1_MASK,
  

• 		MAX98926_PDM_SOURCE_1_MASK);
  

• break;
  

• case 1:

• /* enable voltage */
  

• regmap_update_bits(max98926->regmap,
  

• 	reg, MAX98926_PDM_VOLTAGE_MASK,
  

• 	MAX98926_PDM_VOLTAGE_MASK);
  

• /* enable source*/
  

• if (channel == 0)
  

• 	regmap_update_bits(max98926->regmap,
  

• 		reg, MAX98926_PDM_SOURCE_0_MASK, 0);
  

• else
  

• 	regmap_update_bits(max98926->regmap,
  

• 		reg, MAX98926_PDM_SOURCE_1_MASK, 0);
  

• break;
  

This seems a bit magic numberish (what are 0 and 1 here?).

+static const struct soc_enum max98926_global_enum[] = {

• SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(pdm_text), pdm_text),

+};

Not sure what this control is supposed to do?

• SOC_ENUM("Input Sel", max98926_input_sel),
• SOC_ENUM("DAI Input Sel", max98926_dai_input_sel),

These look like DAPM controls?

• SOC_ENUM_EXT("PDM CH0 Enable", max98926_global_enum[0],

• max98926_get_pdm_ch_zero, max98926_put_pdm_ch_zero),
  

• SOC_ENUM_EXT("PDM CH1 Enable", max98926_global_enum[0],

• max98926_get_pdm_ch_one, max98926_put_pdm_ch_one),
  

There's two different controls mapping to one element of an array?

+static void max98926_set_sense_data(struct max98926_priv *max98926) +{

• if (max98926->intrleave_mode) {

interleave.

• switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
• case SND_SOC_DAIFMT_CBS_CFS:

• max98926_set_sense_data(max98926);
  

• break;
  

• case SND_SOC_DAIFMT_CBM_CFM:
• case SND_SOC_DAIFMT_CBS_CFM:
• case SND_SOC_DAIFMT_CBM_CFS:
• default:

• dev_err(codec->dev, "DAI clock mode unsupported");
  

• return -EINVAL;
  

No need to list all the formats you don't support.

• }
• switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
• case SND_SOC_DAIFMT_I2S:
• case SND_SOC_DAIFMT_LEFT_J:

• break;
  

I'm not seeing anything that handles these diffeerently, they are different formats so you should only list whichever one is actually supported.

• default:

• dev_dbg(codec->dev, "format unsupported %d",
  

• 		params_format(params));
  

• return -EINVAL;
  

• }
• return max98926_set_clock(max98926, params);

May as well just inline set_clock() here - this appears to be the only place it's used.

+static int max98926_dai_set_sysclk(struct snd_soc_dai *dai,

• 		   int clk_id, unsigned int freq, int dir)
  

+{

• struct snd_soc_codec *codec = dai->codec;
• struct max98926_priv *max98926 = snd_soc_codec_get_drvdata(codec);
• max98926->sysclk = freq;
• return 0;

+}

You had error checking on sysclk in hw_params(), it'd bet better to do that here too so people can tell they set a bad clock before they start a stream.

• ret = regmap_read(max98926->regmap,

• 	MAX98926_VERSION, &reg);
  

• if ((ret < 0) ||

• ((reg != MAX98926_CHIP_VERSION) &&
  

• (reg != MAX98926_CHIP_VERSION1))) {
  

• dev_err(codec->dev, "Failed to read: %x\n", reg);
  

• return -EINVAL;
  

• }

You should do this check in the I2C level probe. The error message is also wrong if the register does read but isn't what we expected.

Are you *sure* no new revisions of the device will be made?

• /* It's not the default but we need to set DAI_DLY */
• regmap_write(max98926->regmap,

• 	MAX98926_FORMAT, MAX98926_DAI_DLY_MASK);
  

Sounds like this might be the I2S vs left justified configuration?