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

[Mark Brown]

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150602/8aae46ff/attachment.sig>