Re: [PATCH] ASoC: Add ssm2518 support

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1721 bytes --]

On Wed, May 22, 2013 at 07:00:13PM +0200, Lars-Peter Clausen wrote:
> This patch adds a ASoC CODEC driver for the SSM2516. The SSM2516 is a stereo
> Class-D audio amplifier with an I2S interface for audio in and a built-in
> dynamic range control processor.

I'll apply this but 

> +	if (slots == 0) {
> +		return regmap_update_bits(ssm2518->regmap,
> +			SSM2518_REG_SAI_CTRL1, SSM2518_SAI_CTRL1_SAI_MASK,
> +			SSM2518_SAI_CTRL1_SAI_I2S);
> +	}

You've got quite a few single statement if () blocks with { } which
shouldn't be there.

> +	codec->control_data = ssm2518->regmap;
> +	ret = snd_soc_codec_set_cache_io(codec, 0, 0, SND_SOC_REGMAP);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(ssm2518->regmap, SSM2518_REG_POWER2,
> +				SSM2518_POWER2_APWDN, 0x00);
> +	if (ret)
> +		return ret;
> +
> +	return ssm2518_set_bias_level(codec, SND_SOC_BIAS_OFF);

The power down stuff shouild probably be done in the I2C probe so that
the device is in a low power state if unused.

> +	switch (freq) {
> +	case 2048000:

Looks like the user can't select 0 for the SYSCLK, I'd expect that to be
possible for systems that can reprogram the clock so that they can avoid
having constraints set when they don't need them.

> +static bool ssm2518_register_volatile(struct device *dev, unsigned int reg)
> +{
> +	return false;
> +}

Hrm, this should be the default...

> +	} else if (i2c->dev.of_node) {
> +		ssm2518->enable_gpio = of_get_gpio(i2c->dev.of_node, 0);
> +		if (ssm2518->enable_gpio == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;

Why are other errors being ignored here?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]