[alsa-devel] Update on TLV320AIC3204 Driver
Hi all, I've managed to get significantly further along with a driver for the TLV320AIC3204, to the point now that I'm starting to look at further things to tidy up for its inclusion into the kernel. Some issues I've faced however:
(1) When using the SOC_DOUBLE_R_SX_TLV controls, I notice there's an odd interaction between the mute switch associated with the control, and its corresponding gain setting. Nothing changes in the actual registers (except the mute bit of course) but the displayed gain shoots up to infinity if the mute is toggled when the gain associated with that mute control is set below 0dB. When the gain is at 0dB or above, toggling the mute has no effect on the displayed gain setting.
Has anyone noticed this before and what would be the cause?
For reference; the controls are defined in lines 395..414 of tlv320aic3204.c.
(2) On the TLV320AIC3204, the clocks used on the audio bus can either be sourced from the ADC clocks, or the DAC clocks. Naturally, before you see any clocks on the bus, you must first power up either the DAC or ADC (or both), and choose the appropriate source. I do this currently in the hw_params callback (right where I *used* to power up the DACs/ADCs, DAPM does this now).
The issue I see, is what happens when a playback stream starts, followed later by a recording stream. All will be fine until the recording stream stops -- if the playback is still going, I fear the clocks will get shut off with the DAC, and the recording will come to a grinding halt. Duplex will be okay since both recording and playback are synchronised events in this case... but I can see the potential for this alternate case causing problems.
Therefore, I ask: Is there a way to control this via DAPM, so that, when the component supplying the bit clocks is shut down, the serial interface clock source is switched automatically to the alternate source? (i.e. if DAC is shut down, switch to using the ADC clocks)
For reference; the switching presently occurs in lines 917..924 of tlv320aic3204.c.
(3) The sysfs interface of my driver still remains. I had a look at using the ASoC debugfs interface, however there are a *lot* of registers on the 'AIC3204 in a sparse memory map. When I try to inspect the registers via /sys/kernel/debug/asoc/tlv320aic3204.0-0018/codec_reg ... I notice there are more registers in the device than are accessible via this interface:
karo ~ # tail /sys/kernel/debug/asoc/tlv320aic3204.0-0018/codec_reg tlv320aic3204-i2c 0-0018: aic3204_write: pg 0 reg 0[0000] <= 01 tlv320aic3204-i2c 0-0018: aic3204_write: pg 0 reg 0[0000] <= 02 tlv320aic3204-i2c 0-0018: aic3204_write: pg 0 reg 0[0000] <= 03 tlv320aic3204-i2c 0-0018: aic3204_write: pg 0 reg 0[0000] <= 00 tlv320aic3204-i2c 0-0018: aic3204_write: pg 0 reg 0[0000] <= 01 tlv320aic3204-i2c 0-0018: aic3204_write: pg 0 reg 0[0000] <= 02 tlv320aic3204-i2c 0-0018: aic3204_write: pg 0 reg 0[0000] <= 03 1a7: 0 1a8: 0 1a9: 0 1aa: 0 1ab: 0 1ac: 0 1ad: 0 1ae: 0 1af: 0 1b0: karo ~ #
Notice it stops at 0x01b0 (corresponding to page 3, register 48), with an incomplete line. There were more registers, than there was buffer space to write the file. Is there a way to increase this buffer? Or alternatively, should I perhaps port my sysfs interface to debugfs, and modify it to augment the existing debugfs interface?
The driver is still using the registration model that was used in kernel 2.6.34. I've managed to backport the ALSA tree to 2.6.28 for our project (since that's what's officially supported by Ka-Ro) and so far, it's working, although things are still quite crude.
Since my last contact I've managed to figure out some aspects of DAPM, for instance the DACs and ADCs are powered up and down by DAPM, whereas I used to do this when setting the PLL manually.
I cache more of the registers now, I tried doing something "clever" and skipping the pages of registers that aren't used and trying to cache just the pages that were meaningful, but I noticed that the rest of ALSA more or less assumed the register address used with cache was the real address. Digging around on the mailing list about how to handle a sparse register map lead me to the solution of just biting the bullet and caching all 80-odd pages. Wasteful on memory, but I won't miss anything.
The driver still has an initialisation script facility, whereby custom register settings can be made at init. I'm still undecided as to whether to keep this or not... it's not the "done thing", but if I leave it there, it allows for custom adaptive filtering coefficients and other parameters to be set by the machine driver, which may be useful in some applications. Is it worth cleaning this up, or should it be ditched before the driver is submitted for inclusion?
The driver is accessible online at http://www.longlandclan.yi.org/~stuartl/asoc/ along with some comments about how to add it to your kernel sources. I'd appreciate any feedback or advice on how the driver can be improved.
Regards,
On Fri, Jun 11, 2010 at 03:55:12PM +1000, Stuart Longland wrote:
(1) When using the SOC_DOUBLE_R_SX_TLV controls, I notice there's an odd interaction between the mute switch associated with the control, and its corresponding gain setting.
Nothing changes in the actual registers (except the mute bit of course) but the displayed gain shoots up to infinity if the mute is toggled when the gain associated with that mute control is set below 0dB. When the gain is at 0dB or above, toggling the mute has no effect on the displayed gain setting.
This is almost always due to having an overlap between the bitfield for the volume and the mute bit.
(2) On the TLV320AIC3204, the clocks used on the audio bus can either be sourced from the ADC clocks, or the DAC clocks. Naturally, before you see any clocks on the bus, you must first power up either the DAC or ADC (or both), and choose the appropriate source. I do this currently in the hw_params callback (right where I *used* to power up the DACs/ADCs, DAPM does this now).
SND_SOC_DAPM_SUPPLY can help you here - you can have a conditional supply.
(3) The sysfs interface of my driver still remains. I had a look at using the ASoC debugfs interface, however there are a *lot* of registers on the 'AIC3204 in a sparse memory map. When I try to inspect the registers via /sys/kernel/debug/asoc/tlv320aic3204.0-0018/codec_reg ... I notice there are more registers in the device than are accessible via this interface:
Provide a display_register() callback which filters out the undefined registers or otherwise improve the standard functionality.
Notice it stops at 0x01b0 (corresponding to page 3, register 48), with an incomplete line. There were more registers, than there was buffer space to write the file. Is there a way to increase this buffer?
No, it's a sysfs limit.
The driver is still using the registration model that was used in kernel 2.6.34. I've managed to backport the ALSA tree to 2.6.28 for our project (since that's what's officially supported by Ka-Ro) and so far, it's working, although things are still quite crude.
If you could convince Ka-Ro to work with mainline that'd be nice :/
I cache more of the registers now, I tried doing something "clever" and skipping the pages of registers that aren't used and trying to cache just the pages that were meaningful, but I noticed that the rest of ALSA more or less assumed the register address used with cache was the real address. Digging around on the mailing list about how to handle a sparse register map lead me to the solution of just biting the bullet and caching all 80-odd pages. Wasteful on memory, but I won't miss anything.
If the registers are mostly clustered at the starts of pages then an array of pointers to per-page arrays might do the trick.
The driver still has an initialisation script facility, whereby custom register settings can be made at init. I'm still undecided as to whether to keep this or not... it's not the "done thing", but if I leave it there, it allows for custom adaptive filtering coefficients and other parameters to be set by the machine driver, which may be useful in some applications. Is it worth cleaning this up, or should it be ditched before the driver is submitted for inclusion?
If the data is stuff that's calculated off-line then it's reasonable to supply specific bits of data via platform data, several existing drivers do this for filter coefficients. Providing completely arbatrary register writes is not suitable for mainline.
The driver is accessible online at http://www.longlandclan.yi.org/~stuartl/asoc/ along with some comments about how to add it to your kernel sources. I'd appreciate any feedback or advice on how the driver can be improved.
Please post to the mailing list if you want review - the standard workflow is to provide comments by replying to the patch.
On Fri, Jun 11, 2010 at 11:18:04AM +0100, Mark Brown wrote:
On Fri, Jun 11, 2010 at 03:55:12PM +1000, Stuart Longland wrote:
(1) When using the SOC_DOUBLE_R_SX_TLV controls, I notice there's an odd interaction between the mute switch associated with the control, and its corresponding gain setting.
Nothing changes in the actual registers (except the mute bit of course) but the displayed gain shoots up to infinity if the mute is toggled when the gain associated with that mute control is set below 0dB. When the gain is at 0dB or above, toggling the mute has no effect on the displayed gain setting.
This is almost always due to having an overlap between the bitfield for the volume and the mute bit.
I'll have a closer look, perhaps there's some misunderstanding on my part as to how the macros work.
I've started documenting some of this stuff on the ALSA Wiki at http://www.alsa-project.org/main/index.php/DAPM. I'm hoping this might be a useful resource for others. Was about to do it on an intranet wiki here, but figured it'd be of benefit to others too.
(2) On the TLV320AIC3204, the clocks used on the audio bus can either be sourced from the ADC clocks, or the DAC clocks. Naturally, before you see any clocks on the bus, you must first power up either the DAC or ADC (or both), and choose the appropriate source. I do this currently in the hw_params callback (right where I *used* to power up the DACs/ADCs, DAPM does this now).
SND_SOC_DAPM_SUPPLY can help you here - you can have a conditional supply.
Is there an example of the supply doing something like this? I'm thinking a mux might be the way to represent it, but it's then a matter of getting DAPM to switch the MUX over to the other source when the current source gets shut down.
The DAC clocks disappear when the DAC powers down. Likewise with the ADC in this case. I'm not sure how to represent this in terms of DAPM widgets, but I'll have a closer look at the other CODEC drivers.
(3) The sysfs interface of my driver still remains. I had a look at using the ASoC debugfs interface, however there are a *lot* of registers on the 'AIC3204 in a sparse memory map. When I try to inspect the registers via /sys/kernel/debug/asoc/tlv320aic3204.0-0018/codec_reg ... I notice there are more registers in the device than are accessible via this interface:
Provide a display_register() callback which filters out the undefined registers or otherwise improve the standard functionality.
I shall look into this... but a thought, is there any merrit in the two-file (register select" and "register data") interface to the CODEC registers? The current interface looks great if you want to quickly get a register dump, but the solution I have is easily used in scripts.
If there's some interest I can port my SYSFS interface over to DebugFS, and move it into the DAPM core where it can be used by others. Basically the interface I had come up with, was meant to directly replace i2cset and i2cget which become unusable once a driver claims control of an I²C device.
The driver is still using the registration model that was used in kernel 2.6.34. I've managed to backport the ALSA tree to 2.6.28 for our project (since that's what's officially supported by Ka-Ro) and so far, it's working, although things are still quite crude.
If you could convince Ka-Ro to work with mainline that'd be nice :/
Indeed, I'll start looking into that at a later date, as it was a major thorn in our side not being able to grab the latest mainline kernel and have it "just work". That said, I can also understand the trouble it might cause from a support point-of-view. Hence the softly-softly approach. :-)
I have a patch for 2.6.34 that adds support for the Ka-Ro TX27, but I'd like to run it past Ka-Ro before I make it publically available ... to prevent them getting a barrage of queries about a kernel they don't support.
In any case, the move from 2.6.34 to git HEAD is not a big one, and I'll do that as one of the final steps before submitting a patch of the driver.
I cache more of the registers now, I tried doing something "clever" and skipping the pages of registers that aren't used and trying to cache just the pages that were meaningful, but I noticed that the rest of ALSA more or less assumed the register address used with cache was the real address. Digging around on the mailing list about how to handle a sparse register map lead me to the solution of just biting the bullet and caching all 80-odd pages. Wasteful on memory, but I won't miss anything.
If the registers are mostly clustered at the starts of pages then an array of pointers to per-page arrays might do the trick.
As in this?
static u8 aic3204_reg_page0[128]; static u8 aic3204_reg_page1[128]; static u8 aic3204_reg_page8[128]; static u8 aic3204_reg_page9[128]; /* ... */ static u8* aic3204_reg_cache[81] = { aic3204_reg_page0, aic3204_reg_page1, NULL, NULL, NULL, /* ... */ aic3204_reg_page8, aic3204_reg_page9, /* ... */ };
I wasn't sure how compatible that would be with the rest of ALSA, but I might give that a try shortly.
The driver still has an initialisation script facility, whereby custom register settings can be made at init. I'm still undecided as to whether to keep this or not... it's not the "done thing", but if I leave it there, it allows for custom adaptive filtering coefficients and other parameters to be set by the machine driver, which may be useful in some applications. Is it worth cleaning this up, or should it be ditched before the driver is submitted for inclusion?
If the data is stuff that's calculated off-line then it's reasonable to supply specific bits of data via platform data, several existing drivers do this for filter coefficients. Providing completely arbatrary register writes is not suitable for mainline.
I didn't think it would be somehow. :-) I'll have a look for a few examples and see if I can convert this driver across. For what it's worth, I haven't yet looked at the adaptive filtering capabilities of this CODEC -- it does have them.
The driver is accessible online at http://www.longlandclan.yi.org/~stuartl/asoc/ along with some comments about how to add it to your kernel sources. I'd appreciate any feedback or advice on how the driver can be improved.
Please post to the mailing list if you want review - the standard workflow is to provide comments by replying to the patch.
No problems, I'll post the code to the list in a moment. Last time I tried this it got held for moderation because the email was too big. Hence my posting it to a website and linking to it. I'll post the files up in my next email. When things are a little more mature, I'll post the patch for the driver.
On Tue, Jun 15, 2010 at 10:27:34AM +1000, Stuart Longland wrote:
On Fri, Jun 11, 2010 at 11:18:04AM +0100, Mark Brown wrote:
On Fri, Jun 11, 2010 at 03:55:12PM +1000, Stuart Longland wrote:
(1) When using the SOC_DOUBLE_R_SX_TLV controls, I notice there's an odd interaction between the mute switch associated with the control, and its corresponding gain setting.
Nothing changes in the actual registers (except the mute bit of course) but the displayed gain shoots up to infinity if the mute is toggled when the gain associated with that mute control is set below 0dB. When the gain is at 0dB or above, toggling the mute has no effect on the displayed gain setting.
This is almost always due to having an overlap between the bitfield for the volume and the mute bit.
I'll have a closer look, perhaps there's some misunderstanding on my part as to how the macros work.
Okay, I've had a close inspection of how the SOC_DOUBLE_R_SX_TLV widgets are implemented. I couldn't find where in the git trees the control had been added, I wound up applying this patch myself in my tree... it apparently got applied in the official trees, but I cannot find it.
http://permalink.gmane.org/gmane.linux.alsa.devel/72893
Hopefully below is telling everyone what we already know... and I'm just checking that I understand this code properly. If I'm misunderstanding something, please let me know.
So the macro is defined as follows (in include/sound.soc.h): #define SOC_DOUBLE_R_SX_TLV(xname, xreg_left, xreg_right, xshift,\ xmin, xmax, tlv_array) \ { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \ .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \ SNDRV_CTL_ELEM_ACCESS_READWRITE, \ .tlv.p = (tlv_array), \ .info = snd_soc_info_volsw_2r_sx, \ .get = snd_soc_get_volsw_2r_sx, \ .put = snd_soc_put_volsw_2r_sx, \ .private_value = (unsigned long)&(struct soc_mixer_control) \ {.reg = xreg_left, \ .rreg = xreg_right, .shift = xshift, \ .min = xmin, .max = xmax} }
When a read is performed, this structure points to this function as the means for reading the register (defined in sound/soc-core.c):
/** * snd_soc_get_volsw_2r_sx - double with tlv and variable data size * mixer get callback * @kcontrol: mixer control * @uinfo: control element information * * Returns 0 for success. */ int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); unsigned int mask = (1<<mc->shift)-1; int min = mc->min; int val = snd_soc_read(codec, mc->reg) & mask; int valr = snd_soc_read(codec, mc->rreg) & mask;
ucontrol->value.integer.value[0] = ((val & 0xff)-min); ucontrol->value.integer.value[1] = ((valr & 0xff)-min); return 0; } EXPORT_SYMBOL_GPL(snd_soc_get_volsw_2r_sx);
Now for my data, almost all the driver gain registers have the following format:
Bit 7 6 5 4 3 2 1 0 | | \ / Resvd Mute '------Signed Gain setting in dB------'
Therefore; I want the 'mask' variable in the above function to select that bottom 6 bits; a hex value of 0x3f... So working backwards to derive mc->shift...
mask = (1 << mc->shift) - 1 = 0011 1111b 1 << mc->shift = 0100 0000b mc->shift = 6
Thus, the macro needs to specify xshift=6. As for the other settings... my gain range is -6dB through to 29dB, so those specify the xmin and xmax as being -6 and 29 respectively.
As for the mute; we define this using the SOC_DOUBLE_R macro; bit 6 is the mute bit as we can see above. It's also inverted; turning it on, turns the output driver off... and it's a single-bit value.
I therefore define the line output driver volume, and its corresponding mute switch as follows:
/* Left Line Output Gain Setting Register */ #define AIC3204_LOLGAIN AIC3204_PGREG(1, 18) /* Left Line Output Mute */ #define AIC3204_LOLGAIN_MUTE_SHIFT (6) /* Left Line Output Gain Mask */ #define AIC3204_LOLGAIN_MASK (0x3f)
/* Right Line Output Gain Setting Register */ #define AIC3204_LORGAIN AIC3204_PGREG(1, 19) /* Right Line Output Mute */ #define AIC3204_LORGAIN_MUTE_SHIFT (6) /* Right Line Output Gain Mask */ #define AIC3204_LORGAIN_MASK (0x3f)
... SOC_DOUBLE_R_SX_TLV("Line Output Playback Volume", AIC3204_LOLGAIN, AIC3204_LORGAIN, 6, -6, 29, output_stage_tlv), SOC_DOUBLE_R("Line Output Playback Switch", AIC3204_LOLGAIN, AIC3204_LORGAIN, AIC3204_LOLGAIN_MUTE_SHIFT, 0x01, 1),
Now, that *mostly* works, except when the gain is set below 0dB, then strange things occur. Did I miss something or is the bug elsewhere?
Regards,
On Tue, Jun 15, 2010 at 03:11:10PM +1000, Stuart Longland wrote:
On Tue, Jun 15, 2010 at 10:27:34AM +1000, Stuart Longland wrote:
On Fri, Jun 11, 2010 at 11:18:04AM +0100, Mark Brown wrote:
On Fri, Jun 11, 2010 at 03:55:12PM +1000, Stuart Longland wrote:
(1) When using the SOC_DOUBLE_R_SX_TLV controls, I notice there's an odd interaction between the mute switch associated with the control, and its corresponding gain setting.
Nothing changes in the actual registers (except the mute bit of course) but the displayed gain shoots up to infinity if the mute is toggled when the gain associated with that mute control is set below 0dB. When the gain is at 0dB or above, toggling the mute has no effect on the displayed gain setting.
This is almost always due to having an overlap between the bitfield for the volume and the mute bit.
I'll have a closer look, perhaps there's some misunderstanding on my part as to how the macros work.
Okay, I've had a close inspection of how the SOC_DOUBLE_R_SX_TLV widgets are implemented. I couldn't find where in the git trees the control had been added, I wound up applying this patch myself in my tree... it apparently got applied in the official trees, but I cannot find it.
Okay... further digging, found it at http://git.kernel.org/?p=linux/kernel/git/lrg/asoc-2.6.git;a=patch;h=b6f4bb383d69cac46f17e2305720f9a3d426c5ed
Looks like what I got is pretty much identical.
On Tue, Jun 15, 2010 at 03:23:51PM +1000, Stuart Longland wrote:
On Tue, Jun 15, 2010 at 03:11:10PM +1000, Stuart Longland wrote:
Okay, I've had a close inspection of how the SOC_DOUBLE_R_SX_TLV widgets are implemented. I couldn't find where in the git trees the control had been added, I wound up applying this patch myself in my tree... it apparently got applied in the official trees, but I cannot find it.
Okay... further digging, found it at http://git.kernel.org/?p=linux/kernel/git/lrg/asoc-2.6.git;a=patch;h=b6f4bb383d69cac46f17e2305720f9a3d426c5ed
Looks like what I got is pretty much identical.
As documented in MAINTAINERS ASoC git for ASoC is:
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git
which gets fed into ALSA git:
git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git
Hi,
On Tuesday 15 June 2010 08:11:10 ext Stuart Longland wrote:
Okay, I've had a close inspection of how the SOC_DOUBLE_R_SX_TLV widgets are implemented. I couldn't find where in the git trees the control had been added, I wound up applying this patch myself in my tree... it apparently got applied in the official trees, but I cannot find it.
I'm not really sure what the SOC_DOUBLE_R_SX_TLV is for... But would a simple SOC_DOUBLE_R_TLV enough for the aic3204 codec? I have not looked at the datasheet (if it is available), but I'll try to find it. Well looking at the _2r_sx functions in soc-core.c does not really clears the intention, but at least I can see several things in the snd_soc_put_volsw_2r_sx function, which can cause problems: - in error case it returns 0, - instead of updating the bits, it rewrites the register, which I think will erase bits, which it should not touch. - it has the ret variable, but it is not really used, and when it got assigned, than it gets 1 in the success case. But luckily the return at the end of the function is not using the ret, but 0. - I'm not really sure if the 0xff masking is needed at all in the _sx functions, since the values are masked with the mask anyways.
So I do not really follow what these suppose to do... Can someone give an example of HW register, and the SX combination with the tlv struct?
On Tue, Jun 15, 2010 at 04:42:32PM +0300, Peter Ujfalusi wrote:
On Tuesday 15 June 2010 08:11:10 ext Stuart Longland wrote:
Okay, I've had a close inspection of how the SOC_DOUBLE_R_SX_TLV widgets are implemented. I couldn't find where in the git trees the control had been added, I wound up applying this patch myself in my tree... it apparently got applied in the official trees, but I cannot find it.
I'm not really sure what the SOC_DOUBLE_R_SX_TLV is for...
It's for value ranges which have normal unsigned integer mapping but where the zero point is not at the lowest value so you need to wrap around. Which isn't actually what Stuart is looking for...
But would a simple SOC_DOUBLE_R_TLV enough for the aic3204 codec? I have not looked at the datasheet (if it is available), but I'll try to find it. Well looking at the _2r_sx functions in soc-core.c does not really clears the intention, but at least I can see several things in the snd_soc_put_volsw_2r_sx
...and this won't work since Stuart is looking for something that does signed values which I don't think we have already, the standard stuff all wants unsigned quantities.
function, which can cause problems:
- in error case it returns 0,
Yup, will fix that just now.
- instead of updating the bits, it rewrites the register, which I think will
erase bits, which it should not touch.
It's just open coding the update bits so there's no actual problem here.
- it has the ret variable, but it is not really used, and when it got assigned,
than it gets 1 in the success case. But luckily the return at the end of the function is not using the ret, but 0.
Yeah, ret is a waste of time.
- I'm not really sure if the 0xff masking is needed at all in the _sx functions,
since the values are masked with the mask anyways.
It's not really needed due to mask, yes.
So I do not really follow what these suppose to do... Can someone give an example of HW register, and the SX combination with the tlv struct?
The only user is cs42l451.
On Tuesday 15 June 2010 16:53:23 ext Mark Brown wrote:
On Tue, Jun 15, 2010 at 04:42:32PM +0300, Peter Ujfalusi wrote:
On Tuesday 15 June 2010 08:11:10 ext Stuart Longland wrote:
Okay, I've had a close inspection of how the SOC_DOUBLE_R_SX_TLV widgets are implemented. I couldn't find where in the git trees the control had been added, I wound up applying this patch myself in my tree... it apparently got applied in the official trees, but I cannot find it.
I'm not really sure what the SOC_DOUBLE_R_SX_TLV is for...
It's for value ranges which have normal unsigned integer mapping but where the zero point is not at the lowest value so you need to wrap around. Which isn't actually what Stuart is looking for...
I found the CS42L51 datasheet, and now it is clear what the _SX is doing.
But would a simple SOC_DOUBLE_R_TLV enough for the aic3204 codec? I have not looked at the datasheet (if it is available), but I'll try to find it. Well looking at the _2r_sx functions in soc-core.c does not really clears the intention, but at least I can see several things in the snd_soc_put_volsw_2r_sx
...and this won't work since Stuart is looking for something that does signed values which I don't think we have already, the standard stuff all wants unsigned quantities.
Yeah, the aic3204 needs signed values. There is this _s8 things, which does signed, but never used, nor really checked it.
- instead of updating the bits, it rewrites the register, which I think
will erase bits, which it should not touch.
It's just open coding the update bits so there's no actual problem here.
It might be so that I have overlooked something, but... If the register has: bit # Function 0 .. 6 : Gain 7 : Mute
Let say that the mute is set to one, and the user modifies the gain, than in the function:
int snd_soc_put_volsw_2r_sx(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); unsigned int mask = (1<<mc->shift)-1;
/* * mc->shift is 7, whiche means seven bits for the gain bit array * mask == 0x7f (b0111 1111): it only covers the gain area */
int min = mc->min; int ret; unsigned int val, valr, oval, ovalr;
val = ((ucontrol->value.integer.value[0]+min) & 0xff); val &= mask; valr = ((ucontrol->value.integer.value[1]+min) & 0xff); valr &= mask;
/* * Here we make sure that the coming new value has only bits set withing the * gain area, so we have b0xxx xxxx for the val, and valr */
oval = snd_soc_read(codec, mc->reg) & mask; ovalr = snd_soc_read(codec, mc->rreg) & mask;
/* * We read the content of the register, and taking only the bits, which is * within the gain area: b0xxx xxxx */ ret = 0; if (oval != val) {
/* Only update, if the gain in the chip is different than the new one */
ret = snd_soc_write(codec, mc->reg, val);
/* * Write the val to the chip. * Now, if the mute was set before it is going to be cleared, since the code has * masked it out, the val only contains the updated bits for the gain part, the * rest is 0. * So if the user changes the volume, the mute bit is going to be erased. */
if (ret < 0) return 0; ret = 1; } if (ovalr != valr) { ret = snd_soc_write(codec, mc->rreg, valr); if (ret < 0) return 0; ret = 1; }
return 0; }
Have I missed something?
On Wed, Jun 16, 2010 at 08:33:12AM +0300, Peter Ujfalusi wrote:
Have I missed something?
No, it's failing to do the shifts. Looking at the cs42l51 driver it looks like all the registers on that chip are fully occupied by the volume controls so it's not an issue for the only current user.
On Tue, Jun 15, 2010 at 04:42:32PM +0300, Peter Ujfalusi wrote:
Hi,
On Tuesday 15 June 2010 08:11:10 ext Stuart Longland wrote:
Okay, I've had a close inspection of how the SOC_DOUBLE_R_SX_TLV widgets are implemented. I couldn't find where in the git trees the control had been added, I wound up applying this patch myself in my tree... it apparently got applied in the official trees, but I cannot find it.
I'm not really sure what the SOC_DOUBLE_R_SX_TLV is for... But would a simple SOC_DOUBLE_R_TLV enough for the aic3204 codec? I have not looked at the datasheet (if it is available), but I'll try to find it.
http://focus.ti.com/docs/prod/folders/print/tlv320aic3204.html has the datasheet.
As I mentioned; the bitfields are signed integers, 6 bits wide in the case of the example given, there are some that are 7 bits.
In the examples that I gave (driver gain settings), they have a range of -6dB to 29dB... which corresponds to bitfield values of 0x3a (for -6dB) and 0x1d (for 29dB).
I did try using the regular TLV macro, and found that the volume "wrapped" around in a very undesirable fashion. SOC_DOUBLE_R_SX_TLV was the suggestion made at the time[1]. The way it is now, it works okay ... if you leave the mute untouched.
On Tue, Jun 15, 2010 at 03:11:10PM +1000, Stuart Longland wrote:
When a read is performed, this structure points to this function as the means for reading the register (defined in sound/soc-core.c):
/**
- snd_soc_get_volsw_2r_sx - double with tlv and variable data size
- mixer get callback
- @kcontrol: mixer control
- @uinfo: control element information
- Returns 0 for success.
*/ int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); unsigned int mask = (1<<mc->shift)-1; int min = mc->min; int val = snd_soc_read(codec, mc->reg) & mask; int valr = snd_soc_read(codec, mc->rreg) & mask;
ucontrol->value.integer.value[0] = ((val & 0xff)-min); ucontrol->value.integer.value[1] = ((valr & 0xff)-min); return 0;} EXPORT_SYMBOL_GPL(snd_soc_get_volsw_2r_sx);
I've figured it out now... and there'll be a patch to fix it.
The mute <--> gain interaction was pure coincidence as it turns out. There's no interaction between the two, just that when I toggle the mute, the gain gets re-read, and hence it *appeared* to interact.
Notice the value calculated for the channel is:
output = ( regval & 0xff ) - minimum
In my case, my minimum is -6. Suppose I set my driver gain to -1dB, in hexadecimal, 0x3f. Since no sign extension is done, (regval & 0xff) = 63. Adding 6 to this (double negation) you get 69... which is greater than the maximum gain of 29. Hence the scale shoots up to maximum.
The fix, is to change the above so that they mask the result; thus wrapping around the zero point like so:
output = ( ( regval & 0xff ) - minimum ) & mask
This fixes the problem that I observe, and shouldn't break the other user of the control. A patch to fix this issue is coming.
When SX_TLV widgets are read, if the gain is set to a value below 0dB, the mixer control is erroniously read as being at maximum volume.
The value read out of the CODEC register is never sign-extended, and when the minimum value is subtracted (read; added, since the minimum is negative) the result is a number greater than the maximum allowed value for the control, and hence it saturates.
Solution: Mask the result so that it "wraps around", emulating sign-extension.
Signed-off-by: Stuart Longland redhatter@gentoo.org --- sound/soc/soc-core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index a82a797..0470288 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2400,8 +2400,8 @@ int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol, int val = snd_soc_read(codec, mc->reg) & mask; int valr = snd_soc_read(codec, mc->rreg) & mask;
- ucontrol->value.integer.value[0] = ((val & 0xff)-min); - ucontrol->value.integer.value[1] = ((valr & 0xff)-min); + ucontrol->value.integer.value[0] = ((val & 0xff)-min) & mask; + ucontrol->value.integer.value[1] = ((valr & 0xff)-min) & mask; return 0; } EXPORT_SYMBOL_GPL(snd_soc_get_volsw_2r_sx);
On Fri, 2010-06-18 at 12:56 +1000, Stuart Longland wrote:
When SX_TLV widgets are read, if the gain is set to a value below 0dB, the mixer control is erroniously read as being at maximum volume.
The value read out of the CODEC register is never sign-extended, and when the minimum value is subtracted (read; added, since the minimum is negative) the result is a number greater than the maximum allowed value for the control, and hence it saturates.
Solution: Mask the result so that it "wraps around", emulating sign-extension.
Signed-off-by: Stuart Longland redhatter@gentoo.org
sound/soc/soc-core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index a82a797..0470288 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2400,8 +2400,8 @@ int snd_soc_get_volsw_2r_sx(struct snd_kcontrol *kcontrol, int val = snd_soc_read(codec, mc->reg) & mask; int valr = snd_soc_read(codec, mc->rreg) & mask;
- ucontrol->value.integer.value[0] = ((val & 0xff)-min);
- ucontrol->value.integer.value[1] = ((valr & 0xff)-min);
- ucontrol->value.integer.value[0] = ((val & 0xff)-min) & mask;
- ucontrol->value.integer.value[1] = ((valr & 0xff)-min) & mask; return 0;
} EXPORT_SYMBOL_GPL(snd_soc_get_volsw_2r_sx);
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
On Fri, Jun 18, 2010 at 12:56:10PM +1000, Stuart Longland wrote:
When SX_TLV widgets are read, if the gain is set to a value below 0dB, the mixer control is erroniously read as being at maximum volume.
Applied, thanks.
BTW, you might want to have a look at the CCs - you probably don't need to CCing the ARM list for ASoC patches that aren't ARM specific, for example.
On 15 Jun 2010, at 01:27, Stuart Longland wrote:
I've started documenting some of this stuff on the ALSA Wiki at http://www.alsa-project.org/main/index.php/DAPM. I'm hoping this might be a useful resource for others. Was about to do it on an intranet wiki here, but figured it'd be of benefit to others too.
If you're doing this it would be a bit nicer if you could contribute the documentation to the kernel. The APIs do change over time and the in tree documentation has a much better chance of being in sync with what people are actually working with than online documentation (which may be for a later kernel version with features they can't use), though it's not been so great recently.
SND_SOC_DAPM_SUPPLY can help you here - you can have a conditional supply.
Is there an example of the supply doing something like this? I'm thinking a mux might be the way to represent it, but it's then a matter of getting DAPM to switch the MUX over to the other source when the current source gets shut down.
WM8994 does this for the AIF clocks. You can't use a mux since you're not routing an audio path.
Provide a display_register() callback which filters out the undefined registers or otherwise improve the standard functionality.
I shall look into this... but a thought, is there any merrit in the two-file (register select" and "register data") interface to the CODEC registers? The current interface looks great if you want to quickly get a register dump, but the solution I have is easily used in scripts.
It's fairly easy to use the existing solution in scripts with a little bit of filtering (even grep can do a good job) and I have to say I'm concerned about the idea of production use of this stuff. It's supposed to give the impression that you're not meant to be using it in production for anything except pulling diagnostics out.
If the registers are mostly clustered at the starts of pages then an array of pointers to per-page arrays might do the trick.
As in this?
static u8 aic3204_reg_page0[128];
Yes.
I wasn't sure how compatible that would be with the rest of ALSA, but I might give that a try shortly.
The rest of ALSA shouldn't be able to tell what you're doing, all the operations which interact with the register cache are provided by your driver. If it can see what's going on we've got a problem anyway.
Please post to the mailing list if you want review - the standard workflow is to provide comments by replying to the patch.
No problems, I'll post the code to the list in a moment. Last time I tried this it got held for moderation because the email was too big. Hence my posting it to a website and linking to it. I'll post the files up in my next email. When things are a little more mature, I'll post the patch for the driver.
This is one of the things that the requirement to CC the maintainers and other relevant people helps with - it means that they get a copy as soon as their mail server takes the message rather than having to wait for the list.
On Fri, Jun 11, 2010 at 11:18:04AM +0100, Mark Brown wrote:
On Fri, Jun 11, 2010 at 03:55:12PM +1000, Stuart Longland wrote:
The driver is accessible online at http://www.longlandclan.yi.org/~stuartl/asoc/ along with some comments about how to add it to your kernel sources. I'd appreciate any feedback or advice on how the driver can be improved.
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
On Fri, Jun 11, 2010 at 11:18:04AM +0100, Mark Brown wrote:
On Fri, Jun 11, 2010 at 03:55:12PM +1000, Stuart Longland wrote:
The driver is accessible online at http://www.longlandclan.yi.org/~stuartl/asoc/ along with some comments about how to add it to your kernel sources. I'd appreciate any feedback or advice on how the driver can be improved.
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.h
Attached :- include/sound/tlv320aic3204.h
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?
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. :-)
On Wed, Jun 16, 2010 at 08:46:23AM +1000, Stuart Longland wrote:
On Tue, Jun 15, 2010 at 03:20:50PM +0100, Mark Brown wrote:
scripts/checkpatch.pl is your friend.
I take it this checks for coding style? I'll have a look.
Coding style and other things - anything that's amenable to automatic checking.
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.
The PLL can probably be handled as a supply to other components.
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.
If you need to explicitly control them either the hw_params or using a supply for the DAC/ADC might work.
#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.
I'd be tempted to skip the ifdefs until SPI is actually implemented since the driver is useless otherwise.
participants (4)
-
Liam Girdwood -
Mark Brown -
Peter Ujfalusi -
Stuart Longland