On Fri, Nov 14, 2008 at 11:55 AM, Mark Brown broonie@sirena.org.uk wrote:
On Thu, Nov 13, 2008 at 04:41:30PM +0100, Christian Pellegrin wrote:
A couple of minor procedural points:
- Normally the patches in a series are numbered starting from 1 with
mail 0 being a cover letter ("This series does..." or similar).
- Information like the "Generated on" should go after the --- with the
diffstat so that it doesn't go in the commit message.
ack
+++ b/include/sound/uda134x.h
+extern struct snd_soc_dai uda134x_dai; +extern struct snd_soc_codec_device soc_codec_dev_uda134x;
At least this should be in a header sound/soc/codecs - as I said previously that is the idiom used by codec drivers. However...
ack
...putting this in a separate file in include/sound is a good idea.
Is having UDA1340 as zero a good idea - that'll mean that if someone forgets to initialise the model it'll come out as UDA1340? It might be better to start the model numbers from 1 so that it'll be obvious if that happens. Might also be an idea to use an enum rather than the series of #defines?
good point, ack
--- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -8,6 +8,8 @@ snd-soc-tlv320aic23-objs := tlv320aic23.o snd-soc-tlv320aic26-objs := tlv320aic26.o snd-soc-tlv320aic3x-objs := tlv320aic3x.o snd-soc-twl4030-objs := twl4030.o +snd-soc-l3-objs := l3.o +snd-soc-uda134x-objs := uda134x.o
Please keep this and the other build stuff sorted (it helps merges).
I'll sort alphabetically
if (reg >= UDA134X_REGS_NUM) {printk(KERN_ERR "%s unkown register: reg: %d",__func__, reg);Could use pr_error() here and elsewhere but doesn't make much difference either way - up to you.
didn't know about it. The next driver will use it because it's shorter.
pr_debug("%s costrining to %d bits at %d\n", __func__,constraining.
ack
codec->write(codec, i, *cache++);/* ADC, DAC on */reg = uda134x_read_reg_cache(codec, UDA134X_STATUS1);uda134x_write(codec, UDA134X_STATUS1, reg | 0x03);break;This should probably be done when going into SND_SOC_BIAS_STANDBY or at least _PREPARE. The codec will be brought up to _ON whenever a playback or record is active, spending most of the rest of the time at _STANDBY. The result will be that you're syncing the register cache to the codec every time playback starts, even if the codec is already on. _ON is expected to be very quick.
ack, I was thinking to put it in prepare but I wasn't sure.
+EXPORT_SYMBOL(uda134x_dai);
Note that since ASoC core is all EXPORT_SYMBOL_GPL() it'll be difficult for anyone to actually use this from non-GPLed code.
ack
printk(KERN_ERR "UDA134X SoC codec: ""unsupported model\n");return -EINVAL;Probably worth printing out what the model was set to if it's not recognised.
ack
The #else should be with the definition of the functions so there's no need for a conditional here.
ack