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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
