[PATCH] ASoC: Add new TI TLV320AIC3204 CODEC driver
in [Kernel]
|
Prev: Staging: rtl8192su: ieee80211: fix brace coding style issue and other issues in ieee80211_softmac_wx.c Signed-off-by: Andrea Merello <andreamrl@tiscali.it>
Next: DRM / radeon / PM: Do not evict VRAM during freeze phase of hibernation
From: Stuart Longland on 18 Jun 2010 18:50 On Fri, Jun 18, 2010 at 04:53:23PM +0100, Mark Brown wrote: > On Fri, Jun 18, 2010 at 09:33:35PM +1000, Stuart Longland wrote: > > On Fri, Jun 18, 2010 at 12:01:02PM +0100, Liam Girdwood wrote: > > > On Fri, 2010-06-18 at 13:57 +1000, Stuart Longland wrote: > > > > > The audio interface supports many data bus formats, including I??S > > > > master/slave, DSP, left/right justified and TDM. > > > > Just had a quick check and the register caching needs addressed. > > > > I agree with your comments, I don't think we really want to cache all > > > 16K of codec registers here as we will probably only ever use a handful > > > of them. I think a smaller lookup table containing only the registers > > > that we care about will do. > > > Yeah, I wasn't sure how to go about it. It's inefficient, but it's > > simple, and works. Other options I've considered; > > > (1) Mark's suggestion of using per-page arrays > > (2) Using address translation. that is: > > - Page 0 registers 0-127 stored in cache array elements 0-127 > > - Page 1 registers 0-127 stored in cache array elements 128-255 > > - Page 2..7 are skipped > > - Page 8 registers 0-127 stored in cache array elements 256-383 > > ... etc. > > Option 2 is what others have used here. I do think that anything we do > here really ought to have some sort of page mapping support whatever the > actual implementation it uses - TI in particular have a bunch of chips > which do this so there's a general call for something that can handle > them. > > If you are going to do a lookup table one way of doing this would be to > have the lookup table be a table of range mappings (I've not looked at > the patch, perhaps that's what you've done already) - just so long as > it's got the concept of pages covered somehow. Yeah... for now I've re-sent a new patch... it's on its way... with the register cache in its present form. It's huge and unsightly, but won't otherwise cause a problem. I'd sooner get the driver out there in its known working form, then come up with a solution to fix the problem in a scalable fashion, then quickly try to hack something up here (I'm at home presently) where I have no test hardware to check I haven't broken something... and where the implementation and design will likely be an appauling mess. ;-) > > Another thought regarding register cache, rather than hard-coding it the > > way we presently do, we could also build up the cache on demand... that > > is, we maintain a table of previously read registers; if a register > > value is read for the first time, an actual read is done from the CODEC > > (or the value is copied from a static table). All subsequent reads then > > come from cache. On suspend; if a register has not been touched, it is > > deemed to be left at default settings, and skipped on resume. > > The only problem with this is that for a number of reasons a lot of > devices have no read functionality at all. These need us to supply the > defaults from outside the device, though other devices could use what > you suggest. I'd not be so bothered about the performance here - this > has to be high performance in comparison with doing I/O on a slow bus > such as I2C. If we do run into performance issues we can always work on > substituting in a higher performance algorithm later, if everything is > well encapsulated this should be doable without much disruption. > > It's also useful to have the actual defaults available for allowing > writes to be skipped when powering up the device (eg, on resume) - these > could be cached on first write so it's not insurmountable but it does > need to be considered. Indeed... I don't check if a register has been modified; I just copy it across anyway. I'm aware some devices do not implement reads, that was a comment made about the TLV320AIC3X device driver that I based my driver on... in this situation you'd have no choice but to do it the way it's presently done now. I'll think about this over the next few days... but I'm thinking the basic structure of such a register cache would possibly take the form of something like this: struct soc_reg_cache_pg { /* Page index; for single-page registers, this would be zero */ int index; /* Number of registers cached */ int size; /* Register offset cached */ int offset; /* Pointer to register flags */ const int *flags_data[]; /* Pointer to default values */ const int *default_data[]; /* Pointer to cached values */ int *cached_data[]; /* Pointer to next block of page registers (for sparse maps) */ struct soc_reg_cache_pg *next_block; }; .... then you'd use several of these in an array... one for each block of registers in a page. If your device uses sparse registers, you can use the next_block pointer to point to the next such struct that has the next bank of registers, to save you scanning through *every* struct. The register cache could be an array of pointers, size equal to the total number of pages, if a page is not used; it is set to NULL. If a page is sparse, the other blocks would be accessed using the next_block pointer. The flags_data array would be optional; and could be used to indicate the read/write status, volatile bits, and other information. It'd be a bitfield. Thoughts? Regards, -- Stuart Longland (aka Redhatter, VK4MSL) .'''. Gentoo Linux/MIPS Cobalt and Docs Developer '.'` : .. . . . . . . . . . . . . . . . . . . . . . .'.' http://dev.gentoo.org/~redhatter :.' I haven't lost my mind... ...it's backed up on a tape somewhere. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 18 Jun 2010 21:30 On Sat, Jun 19, 2010 at 08:43:14AM +1000, Stuart Longland wrote: > I'll think about this over the next few days... but I'm thinking the > basic structure of such a register cache would possibly take the form of > something like this: > > struct soc_reg_cache_pg { > /* Page index; for single-page registers, this would be zero */ > int index; > /* Number of registers cached */ > int size; > /* Register offset cached */ > int offset; > /* Pointer to register flags */ > const int *flags_data[]; > /* Pointer to default values */ > const int *default_data[]; This should probably be either unsigned or void (the latter allowing for pluggable register sizes - otherwise you might burn as much data as you save from the sparseness on chips that have small registers). > ... then you'd use several of these in an array... one for each block of > registers in a page. If your device uses sparse registers, you can use > the next_block pointer to point to the next such struct that has the > next bank of registers, to save you scanning through *every* struct. This is going to be painful to write out when writing the data table - if it's useful to do it'd be better to do it at runtime, but I think rather than handling pages it would be at least as easy to munge the page number into the register number like everyone does at the minute and then just optimise plain register lookups in a sparse map. That way way sparse chips of all kinds get the benefit. > The register cache could be an array of pointers, size equal to the > total number of pages, if a page is not used; it is set to NULL. If a > page is sparse, the other blocks would be accessed using the next_block > pointer. Like I say, I think optimising for page based access is not the most general approach - it'd be better to optimise sparse register maps in general and then map page based register maps onto this. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Stuart Longland on 19 Jun 2010 05:50 On Sat, Jun 19, 2010 at 02:12:21AM +0100, Mark Brown wrote: > On Sat, Jun 19, 2010 at 08:24:36AM +1000, Stuart Longland wrote: > > This all looks pretty good, the comments below are fairly minor. > > > + /* AVDD <->DVDD Link enable */ > > + u8 avdd_link_en; > > What's this? It looks like a boolean which makes u8 an odd choice. > > > + /* LDO Enable */ > > + u8 ldo_en; > > Similarly here. Well, I didn't see the point in allocating 32-bits when I'd only be using 1 of them. However, I can change it to an int if need be. I'm not certain that C has a true "bool" type, but I could be wrong. > > + > > + /* Oversampling rate (both ADC and DAC) */ > > + u8 osr; > > Why is this platform data rather than a runtime configurable option? How would the application elect a particular oversampling rate? I'll admit I'm very new to the ALSA framework in general, but I didn't realise there was an oversampling rate setting. I'll have a look at how the other drivers do it. > > +config SND_SOC_TLV320AIC3204 > > + tristate > > + depends on I2C > > + > > Drop the depends, it doesn't actually do anything for selected items. > > > + /* Page 1 */ > > + if (page == 1) { > > + if (reg <= 4) > > + return 1; > > I can't help but think that this'd be more legible with switch () > statements (GCC has an extension for ranges in switch statements which > you could use). I did if statements here for two reasons: (1) Range selection (I was unaware of the GCC extension) (2) I found by experiment that if is slighly more efficient than switch... at least that was the case on MSP430 (using mspgcc). Other platforms could be very different. > > +/* > > + * Pretty-print the value of a register > > + */ > > +static int aic3204_show_reg(struct snd_soc_codec *codec, char *buf, > > + int buf_sz, unsigned int reg) > > +{ > > + return snprintf(buf, buf_sz, "%02x", > > + aic3204_read_reg_cache(codec, reg)); > > +} > > This looks suspiciously close to the standard show_reg() - it seems > wrong that you should need it. Well, the main difference; standard show_reg uses %4x as its format; and therefore wastes two characters printing nothing useful. Given space is already tight for displaying register dumps via debugfs, any little bit helps. :-) Certainly there's no explicit reason why it is necessary and it can go. > > + /* > > + * These registers are not manipulated by us, so we must read them from > > + * the CODEC directly. > > + */ > > Hrm? Also, it strikes me that this code is also used in the WCLK > function and might benefit from being factored out - it's moderately > verbose. The ADC flags register (and the DAC flags registers) are read-only and manipulated by the CODEC when the ADCs and DACs are actually powered up. I could use the flags set elsewhere, but this reflects what *is* the present state, not what ought to be the present state. There is common code between the WCLK and BCLK functions; in fact I did consider making it the one callback, but since they are separate in the registers, I kept them separate in the callbacks. > > +static const char *aic3204_micbias_voltages[] = { > > + "Low", "Med", "High", "V+" > > +}; > > I'd be inclined to spell medium out and write "V+" as "Supply" or > similar unless there's a strong reason to do otherwise - it seems more > legible. Fair enough... I was originally going to try and use some shortened version of what's in the datasheet... but the values of "Low", "Medium" and "High" are dependant on the selected source and the power rails. "Supply" is probably just as descriptive, if not as compact as "V+"... I'll update that. > > +static const char *aic3204_ldac_srcs[] = { > > + "disabled", "left channel", "right channel", "mix" > > +}; > > Capital letters are more idiomatic for enumerations. > > > +/* > > + * DAC digital volumes. From -63.5 to +24 dB in 0.5 dB steps. > > + * Does not mute control. > > + */ > > I'm finding these comments a little too verbose but that's just me - > I stop and read them only to find there's nothing odd going on here. Yeah, some of this is due to the fact that, in the early days, I wasn't too sure if this was the "right way". So I figured I'd describe my intentions, and then if I was doing it wrong, people would at least know what was intended, and could suggest improvements. Now that the mixer is working okay, I could cut this down... but also I hope that others who follow the same path as me, might find this helpful to determine what's going on. > > + /* > > + * Note regarding SOC_DOUBLE_R_SX_TLV... > > + * > > + * 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. > > + */ > > I suspect that fixing the control may break what you're doing here? It > would certainly bitrot the comment. Well, when the problem goes, so can the comment. :-) One question though, the shift value derives the mask for further operations on these registers... if we were to change the shift value so it reflected how other widget types work, how does one define the mask? > > + SOC_SINGLE("Differential Output Switch", AIC3204_DACS2, > > + AIC3204_DACS2_RMOD_INV_SHIFT, 1, 0), > > This feels like platform data - the use of differential outputs is > normally a property of the physical wiring of the board, it's very rare > to be able to vary this usefully at runtime. Arguably, so is the DAC audio path routing. I wasn't sure how to set this up at the time, initially it was CODEC setup data, then I moved it into the mixer. The other consideration here; is the use case where a device with a mono speaker, but a stereo line-out jack... I wasn't sure how to handle these features. > > + /* MICBIAS Options */ > > + SOC_SINGLE("MICBIAS Enable", AIC3204_MICBIAS, 6, 0x1, 0), > > MICBIAS Enable should definitely be DAPM. > > > + SOC_ENUM("MICBIAS Level", aic3204_enum_micbias_voltage), > > + SOC_ENUM("MICBIAS Source", aic3204_enum_micbias_src), > > These I would expect to be managed in kernel - they're normally either a > property of the board or need to be managed in conjunction with jack > detection code which tends to live in kernel. I'll look into this... I was aware of the MICBIAS widget, but will have to do some further research here. As to the others... I wasn't sure how they should be handled. > > +static const struct snd_soc_dapm_widget aic3204_dapm_widgets[] = { > > + /* Driver/Amplifier Selection */ > > + SND_SOC_DAPM_SWITCH("HPL Playback Switch", AIC3204_OUTDRV, > > + AIC3204_OUTDRV_HPL_UP_SHIFT, 0, > > + &aic3204_hpl_switch), > > + SND_SOC_DAPM_SWITCH("HPR Playback Switch", AIC3204_OUTDRV, > > + AIC3204_OUTDRV_HPR_UP_SHIFT, 0, > > + &aic3204_hpr_switch), > > A lot of these SWITCH controls feel like they ought to be PGAs and the > switch controls mutes on those. When muting an output you normally > don't want to power up and down the path since that is slow and tends to > trigger audible issues, you'd normally control the power for path > endpoints and elements that affect routing only. They are PGAs connecting the driver to its mixer... I wasn't sure how the PGAs worked in DAPM. I knew this did work however, the only catch is that DAPM won't power up the DACs unless you switch them through via these mixers to one of the output drivers. I'll have a closer look at the PGAs and see what they're doing. > > + SND_SOC_DAPM_AIF_IN("Left Data In", "Left Playback", > > + 0, SND_SOC_NOPM, 0, 0), > > + SND_SOC_DAPM_AIF_IN("Right Data In", "Right Playback", > > + 1, SND_SOC_NOPM, 0, 0), > > + SND_SOC_DAPM_AIF_OUT("Left Data Out", "Left Capture", > > + 0, SND_SOC_NOPM, 0, 0), > > + SND_SOC_DAPM_AIF_OUT("Right Data Out", "Right Capture", > > + 1, SND_SOC_NOPM, 0, 0), > > You have these AIF widgets but at least the DAC input selection was > being done in the regular controls rather than in the DAPM routing. Indeed, DAC inputs are selectable, the ADC is not. I should probably making the DAC inputs a MUX... I'll give this a re-think too. > > + /* > > + * Set BCLK and WCLK sources... > > + * > > + * Despite DAPM; this is still needed, as DAPM doesn't yet set the > > + * source at the right time. > > + * > > + * TODO: See if we can change the order of initialisation so this > > + * selection is made after bringing up the ADCs/DACs. Alternative: > > + * see if we can find out ahead of time which one to use. > > It surprises me that the ordering matters too much here; worst case you > leave the interface declocked a little longer when you need to switch > sources but that doesn't seem like the end of the world? Well, what I found is that if I didn't do it there... it would call the WCLK and BCLK functions before powering up the DACs/ADCs. Since they have no idea whether recording or playback is in progress (I wasn't sure how to extract this information), the functions rely on the flags registers to determine this. Therefore, it only works *after* DAPM has turned on the respective parts. Without manually switching it here, the functions see that neither ADC nor DAC is active, and therefore don't do any switching of the BCLK and WCLK. End result; no clocks, DMA times out and the user is left with deafening silence. This, is probably one of the biggest areas where the DAPM support in this CODEC driver needs work. I tried to follow what was going on in the other CODEC drivers, and I know what's submitted isn't quite right, but it does at least work. > > + snd_soc_update_bits(codec, AIC3204_AISR3, AIC3204_AISR3_BDIV, > > + (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > > + ? AIC3204_AISR3_BDIV_DAC > > + : AIC3204_AISR3_BDIV_ADC); > > I have to say that I'm really not a fan of the ternery operator for > legibility. > > > +static int aic3204_mute(struct snd_soc_dai *dai, int mute) > > +{ > > ... > > > + aic3204_write(codec, AIC3204_DACS2, dacs2); /* Unmute DAC */ > > ...or possibly mute it :) Yes, obsolete comment I guess. :-) This is also a mixer control... which is better... leave it to ASoC core, or the user to control PCM mute? The user can mute using the line driver controls... so I guess I should ditch the "PCM Playback Switch" control. > > +static int aic3204_resume(struct platform_device *pdev) > > +{ > > + struct snd_soc_device *socdev = platform_get_drvdata(pdev); > > + struct snd_soc_codec *codec = socdev->card->codec; > > + int i; > > + u8 data[2]; > > + u8 *cache = codec->reg_cache; > > + > > + /* Sync reg_cache with the hardware */ > > + for (i = 0; i < AIC3204_CACHEREGNUM; i++) { > > + data[0] = i; > > + data[1] = cache[i]; > > + codec->hw_write(codec->control_data, data, 2); > > Since you've got a register defaults table you could skip rewriting > registers which have their default value. Indeed, suspend/resume isn't tested at all, so no idea if it works or not. I should probably do a forced reset too just to be on the safe side. This is a reminant from the aic3x driver. > > + /* LDO enable, analogue blocks enable */ > > + snd_soc_update_bits(codec, AIC3204_LDO, > > + AIC3204_LDO_ANALOG | AIC3204_LDO_AVDD_UP, > > + AIC3204_LDO_ANALOG_ENABLED | > > + (aic3204->pdata.ldo_en > > + ? AIC3204_LDO_AVDD_UP : 0)); > > This looks a bit fishy since the mask covers more bits than can ever be > enabled - it reads like the other two bits should be unconditionally > cleared. Would I be better breaking that statement up into two statements? I did them as one for efficiency, but I do recognise there is a potential readability issue. Basically, it clears the "analog power-down" bit in the LDO register (yes, this inverse logic irritates me) and optionally turns on the LDO. I figure some users may wish to provide their own AVDD and thus won't want the LDO. I wasn't sure about the link between AVDD and DVDD, so I left that configurable. I'm guessing most will probably want it disabled. This was configured using arbitrary register initialisation scripts, passed in via the CODEC setup data... so at least things are headded in the right direction. :-) Regards, -- Stuart Longland (aka Redhatter, VK4MSL) .'''. Gentoo Linux/MIPS Cobalt and Docs Developer '.'` : .. . . . . . . . . . . . . . . . . . . . . . .'.' http://dev.gentoo.org/~redhatter :.' I haven't lost my mind... ...it's backed up on a tape somewhere. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mark Brown on 19 Jun 2010 07:00
On Sat, Jun 19, 2010 at 07:49:08PM +1000, Stuart Longland wrote: > On Sat, Jun 19, 2010 at 02:12:21AM +0100, Mark Brown wrote: > > On Sat, Jun 19, 2010 at 08:24:36AM +1000, Stuart Longland wrote: > > > + /* LDO Enable */ > > > + u8 ldo_en; > > Similarly here. > Well, I didn't see the point in allocating 32-bits when I'd only be > using 1 of them. However, I can change it to an int if need be. I'm > not certain that C has a true "bool" type, but I could be wrong. C99 introduced a bool type, and there's always been bitfields. Besides, you're microoptimising something that doesn't need it. When you write u8 that means you really need this field to be unsigned and 8 bits which is a bit confusing for something that's actually just a flag. > > > + /* Oversampling rate (both ADC and DAC) */ > > > + u8 osr; > > Why is this platform data rather than a runtime configurable option? > How would the application elect a particular oversampling rate? I'll > admit I'm very new to the ALSA framework in general, but I didn't > realise there was an oversampling rate setting. I'll have a look at how > the other drivers do it. The oversampling rate selection is a performance/power tradeoff. The user may choose to do things like use high performance when listening to music but step back a bit when playing system sounds, for example. There's no specific ALSA control for it, just make it a Switch. > > > +/* > > > + * Pretty-print the value of a register > > > + */ > > > +static int aic3204_show_reg(struct snd_soc_codec *codec, char *buf, > > > + int buf_sz, unsigned int reg) > > > +{ > > > + return snprintf(buf, buf_sz, "%02x", > > > + aic3204_read_reg_cache(codec, reg)); > > > +} > > This looks suspiciously close to the standard show_reg() - it seems > > wrong that you should need it. > Well, the main difference; standard show_reg uses %4x as its format; and > therefore wastes two characters printing nothing useful. Given space is > already tight for displaying register dumps via debugfs, any little bit > helps. :-) > Certainly there's no explicit reason why it is necessary and it can go. If you want to do this it'd be better to add support to the core, for example by keying off a register width supplied by the CODEC. > > > + /* > > > + * These registers are not manipulated by us, so we must read them from > > > + * the CODEC directly. > > > + */ > > Hrm? Also, it strikes me that this code is also used in the WCLK > > function and might benefit from being factored out - it's moderately > > verbose. > The ADC flags register (and the DAC flags registers) are read-only and > manipulated by the CODEC when the ADCs and DACs are actually powered up. > I could use the flags set elsewhere, but this reflects what *is* the > present state, not what ought to be the present state. This strikes me as something that's going to hit a race condition at some point - if the two differ I'd hope that the state the driver has set is going to be the actual CODEC state very soon. > There is common code between the WCLK and BCLK functions; in fact I did > consider making it the one callback, but since they are separate in the > registers, I kept them separate in the callbacks. I was thinking more a utility function that both callbacks use to work out which of the DACs and ADCs are powered. > Now that the mixer is working okay, I could cut this down... but also I > hope that others who follow the same path as me, might find this helpful > to determine what's going on. If we're going to do that we'd need to be doing it for every single TLV control in every single driver... > Well, when the problem goes, so can the comment. :-) One question > though, the shift value derives the mask for further operations on these > registers... if we were to change the shift value so it reflected how > other widget types work, how does one define the mask? I'd intuitively expect it to be relative to the start of the controlled data rather than the start of the register. > > > + SOC_SINGLE("Differential Output Switch", AIC3204_DACS2, > > > + AIC3204_DACS2_RMOD_INV_SHIFT, 1, 0), > > This feels like platform data - the use of differential outputs is > > normally a property of the physical wiring of the board, it's very rare > > to be able to vary this usefully at runtime. > Arguably, so is the DAC audio path routing. I wasn't sure how to set > this up at the time, initially it was CODEC setup data, then I moved it > into the mixer. The DAC routing can be usefully varied at runtime depending on the use case - for example, sometimes people choose to route a mono signal into both DACs to give stereo output. It would be very unusual hardware that provided a way of switching between single ended and differential in a similar fashion. > The other consideration here; is the use case where a device with a mono > speaker, but a stereo line-out jack... I wasn't sure how to handle these > features. Presumably you can make individual outputs differential and single ended then? > > > + SOC_ENUM("MICBIAS Level", aic3204_enum_micbias_voltage), > > > + SOC_ENUM("MICBIAS Source", aic3204_enum_micbias_src), > > These I would expect to be managed in kernel - they're normally either a > > property of the board or need to be managed in conjunction with jack > > detection code which tends to live in kernel. > I'll look into this... I was aware of the MICBIAS widget, but will have > to do some further research here. As to the others... I wasn't sure how > they should be handled. Platform data initially then provide a runtime API for machine drivers if requireed. > > > +static const struct snd_soc_dapm_widget aic3204_dapm_widgets[] = { > > > + /* Driver/Amplifier Selection */ > > > + SND_SOC_DAPM_SWITCH("HPL Playback Switch", AIC3204_OUTDRV, > > > + AIC3204_OUTDRV_HPL_UP_SHIFT, 0, > > > + &aic3204_hpl_switch), > > > + SND_SOC_DAPM_SWITCH("HPR Playback Switch", AIC3204_OUTDRV, > > > + AIC3204_OUTDRV_HPR_UP_SHIFT, 0, > > > + &aic3204_hpr_switch), > > A lot of these SWITCH controls feel like they ought to be PGAs and the > > switch controls mutes on those. When muting an output you normally > > don't want to power up and down the path since that is slow and tends to > > trigger audible issues, you'd normally control the power for path > > endpoints and elements that affect routing only. > They are PGAs connecting the driver to its mixer... I wasn't sure how > the PGAs worked in DAPM. I knew this did work however, the only catch These should definitely be PGA widgets as described above. > is that DAPM won't power up the DACs unless you switch them through via > these mixers to one of the output drivers. I'll have a closer look at > the PGAs and see what they're doing. Not powering up the DACs unless there is a connected output path is the expected behaviour. > > It surprises me that the ordering matters too much here; worst case you > > leave the interface declocked a little longer when you need to switch > > sources but that doesn't seem like the end of the world? > Well, what I found is that if I didn't do it there... it would call the > WCLK and BCLK functions before powering up the DACs/ADCs. Since they > have no idea whether recording or playback is in progress (I wasn't sure > how to extract this information), the functions rely on the flags > registers to determine this. If you track this in the audio path setup/teardown path instead of looking at the DAC power status you should find everything works fine. > > > +static int aic3204_mute(struct snd_soc_dai *dai, int mute) > > > +{ > > ... > > > + aic3204_write(codec, AIC3204_DACS2, dacs2); /* Unmute DAC */ > > ...or possibly mute it :) > Yes, obsolete comment I guess. :-) This is also a mixer control... > which is better... leave it to ASoC core, or the user to control PCM > mute? The user can mute using the line driver controls... so I guess I > should ditch the "PCM Playback Switch" control. Remove the mixer control. The automatic management ensures that no noise that is generated when starting up the audio stream propagates into the output path and that any soft unmute support in the CODEC gets used to stop you having a hard power up with an active signal. > > > + for (i = 0; i < AIC3204_CACHEREGNUM; i++) { > > > + data[0] = i; > > > + data[1] = cache[i]; > > > + codec->hw_write(codec->control_data, data, 2); > > Since you've got a register defaults table you could skip rewriting > > registers which have their default value. > Indeed, suspend/resume isn't tested at all, so no idea if it works or > not. I should probably do a forced reset too just to be on the safe > side. This is a reminant from the aic3x driver. The reset should not be required, either the device will be as you left it or the power will have gone so it will be in the reset state anyway. > > > + /* LDO enable, analogue blocks enable */ > > > + snd_soc_update_bits(codec, AIC3204_LDO, > > > + AIC3204_LDO_ANALOG | AIC3204_LDO_AVDD_UP, > > > + AIC3204_LDO_ANALOG_ENABLED | > > > + (aic3204->pdata.ldo_en > > > + ? AIC3204_LDO_AVDD_UP : 0)); > > This looks a bit fishy since the mask covers more bits than can ever be > > enabled - it reads like the other two bits should be unconditionally > > cleared. > Would I be better breaking that statement up into two statements? I did Yes, it would be much more legible. > I wasn't sure about the link between AVDD and DVDD, so I left that > configurable. I'm guessing most will probably want it disabled. This > was configured using arbitrary register initialisation scripts, passed > in via the CODEC setup data... so at least things are headded in the > right direction. :-) What does this register actually do? Does it describe the hardware configuration to the device? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |