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

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 7/14/2009 6:54 PM, Mark Brown wrote:
> 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.
> 

Thank you for your review.

> 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.
> 

OK, i will add it.

>> +static const char *max9877_osc_mode[] = {
>> +	"1176KHz",
>> +	"1100KHz",
>> +	"700KHz",
>> +};
> 
> Would this be better as platform data for the device?
> 

I'm not sure about this, but if this would be platform data, does it
means the osc_mode should not be changed through a control?

>> +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?
> 

I think these would be a SOC_DOUBLE_R_EXT_TLV() because HP Left register 
and HP Right register are different. If this is right, i will send a 
patch for this.

>> +/* 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.
> 

OK, i will add it.