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

[Mark Brown]

On Wed, Feb 18, 2015 at 08:09:46PM -0800, Anish Kumar wrote:

> +config SND_SOC_MAX98925
> +       tristate "Support MAX98925 audio codec"
> +       depends on I2C
> +       help
> +       MAX98925 is a high-efficiency mono
> +       Class DG audio amplifier. Use this
> +       option to enable the codec.
> +

As I said in reply to an earlier version of this patch please format
this using the normal style for Kconfig entries.

> +static const char *const dai_text[] = {
> +	"left", "right", "leftright", "leftrightdiv2",
> +};
> +
> +static const char *const hpf_text[] = {
> +	"disable", "dc_block", "100Hz",	"200Hz", "400Hz", "800Hz",
> +};

As I also said in reply to an earlier version please follow the normal
style for enumeration entries with capitalization, use of spaces and so
on.

> +static struct reg_default max98925_reg[] = {
> +	{ 0x00, 0x00 }, /* Battery Voltage Data */
> +	{ 0x01, 0x00 }, /* Boost Voltage Data */
> +	{ 0x02, 0x00 }, /* Live Status0 */
> +	{ 0x03, 0x00 }, /* Live Status1 */
> +	{ 0x04, 0x00 }, /* Live Status2 */

As I said in reply to a previous version of this patch these seem like
volatile registers so why do they have defaults defined?  

I'm going to glance through the rest of this but not too closely given
that a number of issues from the original review appear to remain
unaddressed and (perhaps more importantly) I'm about to run out of
batter).

> +static int max98925_reg_put(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol, unsigned int reg,
> +		unsigned int mask, unsigned int shift)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct max98925_priv *max98925 = snd_soc_codec_get_drvdata(codec);
> +	unsigned int sel = ucontrol->value.integer.value[0];
> +
> +	regmap_update_bits(max98925->regmap, reg, mask, sel<<shift);
> +	dev_dbg(codec->dev, "%s: register 0x%02X, value 0x%02X\n",
> +				__func__, reg, sel);
> +	return 0;
> +}

This looks like a standard control, why is this open coded?  I'd also
expect some bounds checking on the write.

> +static int max98925_dac_event(struct snd_soc_dapm_widget *w,
> +		struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = w->codec;
> +	struct max98925_priv *max98925 = snd_soc_codec_get_drvdata(codec);
> +
> +	regmap_update_bits(max98925->regmap,
> +		MAX98925_R036_BLOCK_ENABLE,
> +		M98925_BST_EN_MASK |
> +		M98925_ADC_IMON_EN_MASK | M98925_ADC_VMON_EN_MASK,
> +		M98925_BST_EN_MASK |
> +		M98925_ADC_IMON_EN_MASK | M98925_ADC_VMON_EN_MASK);
> +	regmap_write(max98925->regmap,
> +	MAX98925_R038_GLOBAL_ENABLE, M98925_EN_MASK);
> +	return 0;
> +}

It's a bit surprising not to see any of these enables getting turned off
when we power down - won't that waste power?  Looking at the names I'm
wondering if these might make more sense as supply widgets.

> +static int max98925_spk_gain_put(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct max98925_priv *max98925 = snd_soc_codec_get_drvdata(codec);
> +	unsigned int sel = ucontrol->value.integer.value[0];
> +
> +	if (sel < ((1 << M98925_SPK_GAIN_WIDTH) - 1)) {
> +		regmap_update_bits(max98925->regmap, MAX98925_R02D_GAIN,
> +			M98925_SPK_GAIN_MASK, sel << M98925_SPK_GAIN_SHIFT);
> +		max98925->spk_gain = sel;

Why is this not just a normal control?  I can't see any reference to
spk_gain elsewhere...  similar things apply to a lot of these controls.
-------------- 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/20150224/1c35138a/attachment.sig>