Re: [PATCH] ASoC: add driver for max9768 amplifier

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Jan 18, 2012 at 04:38:28PM +0100, Wolfram Sang wrote:

> +static int max9768_write(struct snd_soc_codec *codec, unsigned int reg,
> +		unsigned int value)

Use regmap for register I/O please.

> +	data = (reg << 6) | value;
> +	ret = i2c_smbus_write_byte(codec->control_data, data);

You may need to add a new register format, but that shouldn't be too
much work.

> +	if (max9768->flags & MAX9768_FLAG_CLASSIC_PWM) {
> +		ret = snd_soc_write(codec, MAX9768_CTRL, MAX9768_CTRL_PWM);
> +		if (ret)
> +			return ret;
> +	}

Some documentation on what this does would be nice.  Is this something
that might usefully be changed at runtime?

> +	ret = snd_soc_add_controls(codec, max9768_volume,
> +			ARRAY_SIZE(max9768_volume));
> +	if (ret)
> +		return ret;

Use the controls field in the driver.  I'd also expect to see some sort
of DAPM support, even if it's just mapping out the pins.  Since you're
manually controlling the mute it'd be nice to make the unmute be (mute
&& power) to save on pops and clicks from the CODEC being amplified.

> +		err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute");
> +		max9768->mute_gpio = err ?: pdata->mute_gpio;

I really don't like the ternery operator at the best of times...