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

[Mark Brown]

On Tue, Dec 01, 2015 at 10:31:19AM -0800, anish kumar wrote:

> Changes since v1:
> Made some widgets as supply widgets
> Changed the names a bit
> Moved some registers to volatile struct
> Some coding style changes
> added the print for diagnostics
> 
> Signed-off-by: anish kumar <yesanishhere at gmail.com>
> ---

Please use the patch submission format covered in SubmittingPatches -
the inter-version changelog should be after the --- so it doesn't end up
in the final changelog.

>  	select SND_SOC_AK4641 if I2C
>  	select SND_SOC_AK4642 if I2C
> +	select SND_SOC_MAX98926 if I2C
>  	select SND_SOC_AK4671 if I2C
>  	select SND_SOC_AK5386
>  	select SND_SOC_ALC5623 if I2C

Please keep the Kconfig and Makefile sorted.

> +static const struct snd_kcontrol_new max98926_mixer_controls[] = {
> +	SOC_DAPM_SINGLE("PCM", MAX98926_SPK_AMP,
> +		MAX98926_INSELECT_MODE_SHIFT, 0, 0),
> +	SOC_DAPM_SINGLE("PDM", MAX98926_SPK_AMP,
> +		MAX98926_INSELECT_MODE_SHIFT, 1, 0),
> +};

On/off switches should have names ending in " Single".

> +	SOC_DAPM_SINGLE("LeftRightDiv2", MAX98926_GAIN,
> +		MAX98926_DAC_IN_SEL_SHIFT, 3, 0),

Please give this a more human readable name, something like "(Left +
Right) / 2 Switch".

> +static int max98926_probe(struct snd_soc_codec *codec)
> +{
> +	struct max98926_priv *max98926 = snd_soc_codec_get_drvdata(codec);
> +
> +	max98926->codec = codec;
> +	codec->control_data = max98926->regmap;
> +	regmap_write(max98926->regmap, MAX98926_GLOBAL_ENABLE, 0x00);
> +	regmap_write(max98926->regmap, MAX98926_TDM_SLOT_SELECT, 0xC8);
> +	regmap_write(max98926->regmap, MAX98926_DOUT_HIZ_CFG1, 0xFF);
> +	regmap_write(max98926->regmap, MAX98926_DOUT_HIZ_CFG2, 0xFF);
> +	regmap_write(max98926->regmap, MAX98926_DOUT_HIZ_CFG3, 0xFF);
> +	regmap_write(max98926->regmap, MAX98926_DOUT_HIZ_CFG4, 0xF0);
> +	regmap_write(max98926->regmap, MAX98926_FILTERS, 0xD8);
> +	regmap_write(max98926->regmap, MAX98926_ALC_CONFIGURATION, 0xF8);
> +	regmap_write(max98926->regmap, MAX98926_CONFIGURATION, 0xF0);

Lots of random magic numbers - what's going on here?

> +	/* Disable ALC muting */
> +	regmap_write(max98926->regmap, MAX98926_BOOST_LIMITER, 0xF8);

Shouldn't this be a control?

> +	ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_max98926,
> +			max98926_dai, ARRAY_SIZE(max98926_dai));
> +	if (ret < 0)
> +		dev_err(&i2c->dev,
> +				"Failed to register codec: %d\n", ret);
> +	ret = regmap_read(max98926->regmap,
> +			MAX98926_VERSION, &reg);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Failed to read: %x\n", reg);
> +		return -EINVAL;
> +	}
> +	dev_info(&i2c->dev, "device version: %x\n", reg);

It's more normal to read the device ID information before doing anything
else - part of what we're doing is working out if we've got working
register access to the device, if that fails then there's no point in
registering.  Please also pass back error codes rather than substituting
new codes unless there's a strong reason.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20151208/e03d82c2/attachment-0001.sig>