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.