On Tue, Jun 15, 2010 at 03:20:50PM +0100, Mark Brown wrote:
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.
Yes, I know, it'll be gone in the patch.
if ( reg ) { /* Refresh cache */
scripts/checkpatch.pl is your friend.
I take it this checks for coding style? I'll have a look.
#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.
Yep... at the moment I've got a couple of #ifdefs around that statement now; basically so that certain types of debug message can be enabled. In the initial phase when I was reading every register into cache, having this uncommented lead to a _lot_ of data, but it was handy in the beginning to check things were working as I expected.
I'll get rid of those now.
static inline int aic3204_mod( struct snd_soc_codec *codec, unsigned int reg, u8 and_mask, u8 or_mask )
snd_soc_update_bits().
Yep, I've done that in the latest revision.
* 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.
By the sounds of the comments made elsewhere in this thread, it could be a case of round pegs in square holes. My analysis of the functions behaviour lead me to the above comment.
The dB scale comes out correct, but there is the funny overlap between mute and volume level as per my earlier posts.
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.
All noted.
#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.
Indeed... it was used before I got the PLL working; and the PLL has been working well lately, so I may just ditch those #ifdefs and make it the default.
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.
Yep, will get rid of it then. I wasn't quite sure what it was doing, or what was needed. I'm undecided as to whether I should use this to power the PLL down, or whether that'd be better done elsewhere.
I'm giving some thought to handling the bit and word clocks -- and think since this is related, we can do something about the PLL in the same place.
/* 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.
Yep, I've done this in the latest version (although it's a big long table).
Funny thing, one of the I²C devices on the same bus as the CODEC is a PSoC-based sensor controller which had a polled-interrupt I²C slave implementation that would stretch the clock when it was busy doing its other tasks. This caused the machine to appear to "lock up" at this point -- later calculations revealed that the effective bitrate would have meant this step would have taken about 10 minutes. My test board didn't have one of these devices onboard, hence the problem wasn't noticed.
I've since used the above ifdef'd dev_dbg statement to capture the data out of the registers after reset and did some vim voodoo to translate that into C initialisation data in a static const array.
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.
The TLV320AIC3204 is a dual I²C/SPI device, if you tie one of its pins to Vdd, it runs I²C, if you tie that pin to Vss, it runs SPI.
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?
This has all now been replaced; in the early days I had it there as a means of setting up the CODEC, and had thought it'd be a useful way to set custom parameters like GPIO pin functions, interrupts and filter settings... but I knew full well that I'd have a difficult job getting an ugly hack like that accepted into the kernel.
I've now condensed this down to what I need to get things working for now, and made this into a platform data struct. The channel configuration is also obsolete now; this is set up through the mixer interface (in the end it wasn't even being used).
Next time around there'll be a patch on its way, since it now looks as if the driver is getting to the point where it's in viable shape to be contributed to the Linux community proper. :-)