On Tue, Jun 15, 2010 at 10:29:14AM +1000, Stuart Longland wrote:
> > Please post to the mailing list if you want review - the standard
> > workflow is to provide comments by replying to the patch.
> Attached :- sound/soc/codecs/tlv320aic3204.c
Please post patches in the normal fashion, it's really much easier.
This looks mostly good, but a few comments below.
> #ifndef DEBUG
> #define DEBUG
> #endif
Hrm.
> if ( reg ) {
> /* Refresh cache */
scripts/checkpatch.pl is your friend.
> #if 0
> dev_dbg( &codec->dev, "%s: pg %d reg %d[%04x] => %02x\n",
> __func__, reg >> 7, reg & 0xff, reg, value[0] );
> #endif
Can probably just leave dev_dbg() unconditionally, it won't display in
production builds.
> static inline int aic3204_mod( struct snd_soc_codec *codec, unsigned int reg,
> u8 and_mask, u8 or_mask )
snd_soc_update_bits().
> * This is a bit misleading; xshift refers to the bit one bit *past*
> * the most significant bit. Or at least that's what it appears to be
> * doing the math. Mask needs to select the last 6 bits; which is the
> * signed bitfield specifiying the gain in dB.
If that's the case it's buggy and should be fixed.
> static const struct snd_soc_dapm_route intercon[] = {
> /* Data Connections */
> {"Left Data Out", NULL, "Left ADC"},
> {"Right Data Out", NULL, "Right ADC"},
> {"Left DAC", NULL, "Left Data In"},
> {"Right DAC", NULL, "Right Data In"},
> /* Line Output */
Some blanks in this table might not hurt.
> #ifndef ENABLE_PLL
> /* If PLL is disabled; always bypass the PLL */
> bypass_pll = 1;
> #endif
If you've got the PLL working just drop the #defines, there shouldn't be
build time configuration for things like this. If configuration is
needed make it an API call or platform data.
> static int aic3204_set_bias_level(struct snd_soc_codec *codec,
> enum snd_soc_bias_level level)
> {
> #if 0
> switch (level) {
> case SND_SOC_BIAS_ON:
If there's really nothing needed then no need to supply the function.
> /* Reset the CODEC */
> aic3204_write(codec, AIC3204_RESET, AIC3204_RESET_SOFT);
>
> /* Read in the cache */
> for( reg=0; reg < AIC3204_CACHEREGNUM; reg++ ) {
> u8 temp;
> aic3204_read( codec, reg, &temp );
> }
You should be able to have a table of defaults.
> return aic3204_register(codec);
>
> err_enable:
> regulator_bulk_free(ARRAY_SIZE(aic3204->supplies), aic3204->supplies);
The split between the registration function and things like the
regulators is a bit odd here - probably as well to just have one
function unless you have both I2C and SPI options (in which case merge
the GPIO and regulator stuff into register()).
> #else
> static inline void aic3204_i2c_init(void) { }
> static inline void aic3204_i2c_exit(void) { }
> #endif
Not needed unless you have multiple buses.
> if (setup) {
> struct aic3204_priv *aic3204 = snd_soc_codec_get_drvdata(codec);
>
> /* Run setup script; if any */
> if ( setup->init_reg_list ) {
This shouldn't be needed - if it were needed it should be part of the
core.
> /* Copy over channel information */
> aic3204->channels = setup->channels;
Platform data?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
