Re: [PATCH] ASoC: MAX9877: add MAX9877 amp driver

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

On Tue, Jul 14, 2009 at 03:39:37PM +0900, Joonyoung Shim wrote:

Overall this looks great, thanks.  Clearly it's not an ideal approach
but it gets the job done and it can be adapted to use core features as
they're added.

A couple of relatively nitpicky issues below:

> +++ b/sound/soc/codecs/Kconfig
> @@ -188,3 +188,7 @@ config SND_SOC_WM9712
>  
>  config SND_SOC_WM9713
>  	tristate
> +
> +# Amp
> +config SND_SOC_MAX9877
> +	tristate

I'd add it to SND_SOC_ALL_CODECS too - while it's not strictly a CODEC
the point of the Kconfig option is to get build coverage and this driver
will benefit from that as much as others.

> +static const char *max9877_osc_mode[] = {
> +	"1176KHz",
> +	"1100KHz",
> +	"700KHz",
> +};

Would this be better as platform data for the device?

> +static const struct snd_kcontrol_new max9877_controls[] = {
> +	SOC_SINGLE_EXT_TLV("MAX9877 Amp HP Left Playback Volume",
> +			MAX9877_HPL_VOLUME, 0, 31, 0,
> +			max9877_get_reg, max9877_set_reg, max9877_output_tlv),
> +	SOC_SINGLE_EXT_TLV("MAX9877 Amp HP Right Playback Volume",
> +			MAX9877_HPR_VOLUME, 0, 31, 0,
> +			max9877_get_reg, max9877_set_reg, max9877_output_tlv),

Ideally these would be a SOC_DOUBLE_EXT_TLV() but we don't have that yet
- would you mind sending a patch for that?

> +/* This function is called from ASoC machine driver */
> +int max9877_add_controls(struct snd_soc_codec *codec)
> +{
> +	return snd_soc_add_controls(codec, max9877_controls,
> +			ARRAY_SIZE(max9877_controls));
> +}
> +

This should have an EXPORT_SYMBOL_GPL() to allow it to be used from
machine drivers which are built modular.