[alsa-devel] [PATCH 5/6] ASoC: new ADAU1761 codec driver

[Mark Brown]

On 7 Aug 2010, at 21:28, Mike Frysinger <vapier at gentoo.org> wrote:

> +config SND_SOC_ADAU1761
> +	tristate
> +	select SIGMA
> +

What is SIGMA?

> +#define CAP_MIC  1
> +#define CAP_LINE 2
> +#define CAPTURE_SOURCE_NUMBER 2
> +#define ADAU1761_DIG_MIC 0

So, it looks like there's an awful lot of similarity between this and some of the other drivers. I'm going to skip a lot of repetitive comments but please assume that points which can be applied to this driver do apply.

> +static const struct snd_kcontrol_new adau1761_right_mixer_controls[] = {
> +SOC_DAPM_SINGLE("LineRight Bypass Switch", ADAU_PLBLOVR-ADAU_FIRSTREG, 1, 1, 0),
> +SOC_DAPM_SINGLE("HPRight Bypass Switch", ADAU_PLBHPVR-ADAU_FIRSTREG, 1, 1, 0),
> +};

Remember that the name of the mixer will be prepended to these - you can probably drop the Rights.

> +	snd_soc_write(codec, ADAU_DSP_RUN, 0x0);
> +	reg = snd_soc_read(codec, ADAU_CLKCTRL);
> +	snd_soc_write(codec, ADAU_CLKCTRL, reg & ~0x1);

I suspect the DSP power may warrant being managed via DAPM.

> +static ssize_t adau1371_dsp_load(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct snd_soc_device *socdev = dev_get_drvdata(dev);
> +	struct snd_soc_codec *codec = socdev->card->codec;
> +	int ret = 0;
> +
> +	ret = adau1761_setprogram(codec);
> +	if (ret)
> +		return ret;
> +	else
> +		return count;
> +}
> +static DEVICE_ATTR(dsp, 0644, NULL, adau1371_dsp_load);

This looks redundant - it doesn't offer any opportunity to configure the firmware to download and you're already (as one one would expect) automatically downloading the firmware. If this is being exposed at all it should be via debugfs.

The permissions also look wrong given that you don't have a read function.