[alsa-devel] Correct use of ak4114.c?
Hello,
while working on support for AK4114 in Audiotrak MI/ODI/O card, I used corresponding code from revo.c (and similarly from juli.c), calling function snd_ak4114_create. However, this function does not initialize array ak4114->kctls[idx], leading to consequent kernel oops after the periodically called snd_ak4114_check_rate_and_errors, specifically in function snd_ctl_notify called by snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[0]->id)
Temporarily I resolved the problem by commenting out the snd_ctl_notify calls. However, it seems author of ak4114.c intended to create the device with snd_ak4114_build instead which defines all the controls. I did not hit the problem until fixing the 4-wire communication in revo.c - that explaing why nobody had this problem with revo cards. I do not understand how the code can work in juli.c - perhaps I have overlooked something there.
My question: Is the correct way to add null pointer checks before snd_ctl_notify calls only, or to use snd_ak4114_build insted of snd_ak4114_create? If using snd_ak4114_build, where do I get the parameters ply_substream and cap_substream?
int snd_ak4114_build(struct ak4114 *ak4114, struct snd_pcm_substream *playback_substream, struct snd_pcm_substream *capture_substream);
Thanks for any suggestions.
Pavel.
At Fri, 30 Mar 2007 12:24:39 +0200 (CEST), dustin@seznam.cz wrote:
Hello,
while working on support for AK4114 in Audiotrak MI/ODI/O card, I used corresponding code from revo.c (and similarly from juli.c), calling function snd_ak4114_create. However, this function does not initialize array ak4114->kctls[idx], leading to consequent kernel oops after the periodically called snd_ak4114_check_rate_and_errors, specifically in function snd_ctl_notify called by snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[0]->id)
Temporarily I resolved the problem by commenting out the snd_ctl_notify calls. However, it seems author of ak4114.c intended to create the device with snd_ak4114_build instead which defines all the controls. I did not hit the problem until fixing the 4-wire communication in revo.c - that explaing why nobody had this problem with revo cards. I do not understand how the code can work in juli.c
- perhaps I have overlooked something there.
I don't know how well the juli code works over all as I've never had this hardware and had no chance for tests. But my guess is that the driver is more or less borken now.
My question: Is the correct way to add null pointer checks before snd_ctl_notify calls only, or to use snd_ak4114_build insted of snd_ak4114_create? If using snd_ak4114_build, where do I get the parameters ply_substream and cap_substream?
I fixed this on HG tree now, so at least the driver should work even without calling snd_ak4114_build(). The patch is below.
For working properly with the parameter change notifications, you'd need to call snd_ak4114_build() with PCM substreams that you made. That is, first create ak4114 instance via snd_ak4114_create(), create PCMs, then attach PCMs with snd_ak4114_build() later.
Thanks,
Takashi
# HG changeset patch # User tiwai # Date 1175261919 -7200 # Node ID 1df47bdd721aed75cedc4c6d9456c7cb72b92c8e # Parent c88499e08ee15f55c0adf5807331d4b15c8cd2d8 ak4114 - Fix possible Oops with callbacks
ak4114 code may trigger Oops when the parameters are changed without call of snd_ak4114_build(). Now it checks the existence of kctl element, and the workq is triggered after building the necessary kcontrols.
Also, did some code clean up.
diff -r c88499e08ee1 -r 1df47bdd721a i2c/other/ak4114.c --- a/i2c/other/ak4114.c Thu Mar 29 17:02:45 2007 +0200 +++ b/i2c/other/ak4114.c Fri Mar 30 15:38:39 2007 +0200 @@ -36,6 +36,7 @@ MODULE_LICENSE("GPL"); #define AK4114_ADDR 0x00 /* fixed address */
static void ak4114_stats(struct work_struct *work); +static void ak4114_init_regs(struct ak4114 *chip);
static void reg_write(struct ak4114 *ak4114, unsigned char reg, unsigned char val) { @@ -105,7 +106,7 @@ int snd_ak4114_create(struct snd_card *c for (reg = 0; reg < 5; reg++) chip->txcsb[reg] = txcsb[reg];
- snd_ak4114_reinit(chip); + ak4114_init_regs(chip);
chip->rcs0 = reg_read(chip, AK4114_REG_RCS0) & ~(AK4114_QINT | AK4114_CINT); chip->rcs1 = reg_read(chip, AK4114_REG_RCS1); @@ -131,13 +132,10 @@ void snd_ak4114_reg_write(struct ak4114 (chip->txcsb[reg-AK4114_REG_TXCSB0] & ~mask) | val); }
-void snd_ak4114_reinit(struct ak4114 *chip) +static void ak4114_init_regs(struct ak4114 *chip) { unsigned char old = chip->regmap[AK4114_REG_PWRDN], reg;
- chip->init = 1; - mb(); - flush_scheduled_work(); /* bring the chip to reset state and powerdown state */ reg_write(chip, AK4114_REG_PWRDN, old & ~(AK4114_RST|AK4114_PWN)); udelay(200); @@ -150,9 +148,18 @@ void snd_ak4114_reinit(struct ak4114 *ch reg_write(chip, reg + AK4114_REG_TXCSB0, chip->txcsb[reg]); /* release powerdown, everything is initialized now */ reg_write(chip, AK4114_REG_PWRDN, old | AK4114_RST | AK4114_PWN); +} + +void snd_ak4114_reinit(struct ak4114 *chip) +{ + chip->init = 1; + mb(); + flush_scheduled_work(); + ak4114_init_regs(chip); /* bring up statistics / event queing */ chip->init = 0; - schedule_delayed_work(&chip->work, HZ / 10); + if (chip->kctls[0]) + schedule_delayed_work(&chip->work, HZ / 10); }
static unsigned int external_rate(unsigned char rcs1) @@ -472,7 +479,53 @@ int snd_ak4114_build(struct ak4114 *ak41 return err; ak4114->kctls[idx] = kctl; } - return 0; + /* trigger workq */ + schedule_delayed_work(&ak4114->work, HZ / 10); + return 0; +} + +/* notify kcontrols if any parameters are changed */ +static void ak4114_notify(struct ak4114 *ak4114, + unsigned char rcs0, unsigned char rcs1, + unsigned char c0, unsigned char c1) +{ + if (!ak4114->kctls[0]) + return; + + if (rcs0 & AK4114_PAR) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[0]->id); + if (rcs0 & AK4114_V) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[1]->id); + if (rcs1 & AK4114_CCRC) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[2]->id); + if (rcs1 & AK4114_QCRC) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[3]->id); + + /* rate change */ + if (c1 & 0xf0) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[4]->id); + + if ((c0 & AK4114_PEM) | (c0 & AK4114_CINT)) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[9]->id); + if (c0 & AK4114_QINT) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[10]->id); + + if (c0 & AK4114_AUDION) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[11]->id); + if (c0 & AK4114_AUTO) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[12]->id); + if (c0 & AK4114_DTSCD) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[13]->id); }
int snd_ak4114_external_rate(struct ak4114 *ak4114) @@ -511,31 +564,7 @@ int snd_ak4114_check_rate_and_errors(str ak4114->rcs1 = rcs1; spin_unlock_irqrestore(&ak4114->lock, _flags);
- if (rcs0 & AK4114_PAR) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[0]->id); - if (rcs0 & AK4114_V) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[1]->id); - if (rcs1 & AK4114_CCRC) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[2]->id); - if (rcs1 & AK4114_QCRC) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[3]->id); - - /* rate change */ - if (c1 & 0xf0) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[4]->id); - - if ((c0 & AK4114_PEM) | (c0 & AK4114_CINT)) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[9]->id); - if (c0 & AK4114_QINT) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[10]->id); - - if (c0 & AK4114_AUDION) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[11]->id); - if (c0 & AK4114_AUTO) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[12]->id); - if (c0 & AK4114_DTSCD) - snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[13]->id); - + ak4114_notify(ak4114, rcs0, rcs1, c0, c1); if (ak4114->change_callback && (c0 | c1) != 0) ak4114->change_callback(ak4114, c0, c1);
@@ -559,8 +588,8 @@ static void ak4114_stats(struct work_str struct ak4114 *chip = container_of(work, struct ak4114, work.work);
if (chip->init) - return; - snd_ak4114_check_rate_and_errors(chip, 0); + snd_ak4114_check_rate_and_errors(chip, 0); + schedule_delayed_work(&chip->work, HZ / 10); }
diff -r c88499e08ee1 -r 1df47bdd721a pci/ice1712/juli.c --- a/pci/ice1712/juli.c Thu Mar 29 17:02:45 2007 +0200 +++ b/pci/ice1712/juli.c Fri Mar 30 15:38:39 2007 +0200 @@ -160,13 +160,6 @@ static int __devinit juli_init(struct sn int err; struct snd_akm4xxx *ak;
-#if 0 - for (err = 0; err < 0x20; err++) - juli_ak4114_read(ice, err); - juli_ak4114_write(ice, 0, 0x0f); - juli_ak4114_read(ice, 0); - juli_ak4114_read(ice, 1); -#endif err = snd_ak4114_create(ice->card, juli_ak4114_read, juli_ak4114_write,
Hello,
------------ Původní zpráva ------------ Od: Takashi Iwai tiwai@suse.de Předmět: Re: [alsa-devel] Correct use of ak4114.c? Datum: 30.3.2007 15:44:02
My question: Is the correct way to add null pointer checks before snd_ctl_notify calls only, or to use snd_ak4114_build insted of snd_ak4114_create? If using snd_ak4114_build, where do I get the parameters ply_substream and cap_substream?
I fixed this on HG tree now, so at least the driver should work even without calling snd_ak4114_build(). The patch is below.
For working properly with the parameter change notifications, you'd need to call snd_ak4114_build() with PCM substreams that you made. That is, first create ak4114 instance via snd_ak4114_create(), create PCMs, then attach PCMs with snd_ak4114_build() later.
Thanks a lot for incredibly fast response. Could snd_ak4114_build() be called for testing purposes somewhere towards the end of snd_vt1724_probe (ice1724.c), when pcms are already created?
BTW, what is difference between professional and spdif pcms? Is it only about digital format (like professional/consumer in iecset)? Sorry for stupid questions, but I do not know which pcm to use.
Would I get to playback/capture substreams e.g. using ice->pcm_pro->streams[0/1]->substream at the end of snd_vt1724_probe? I understand I could probably find this information in alsa documentation.
Pavel.
At Fri, 30 Mar 2007 16:20:36 +0200 (CEST), dustin@seznam.cz wrote:
Hello,
------------ Původní zpráva ------------ Od: Takashi Iwai tiwai@suse.de Předmět: Re: [alsa-devel] Correct use of ak4114.c? Datum: 30.3.2007 15:44:02
My question: Is the correct way to add null pointer checks before snd_ctl_notify calls only, or to use snd_ak4114_build insted of snd_ak4114_create? If using snd_ak4114_build, where do I get the parameters ply_substream and cap_substream?
I fixed this on HG tree now, so at least the driver should work even without calling snd_ak4114_build(). The patch is below.
For working properly with the parameter change notifications, you'd need to call snd_ak4114_build() with PCM substreams that you made. That is, first create ak4114 instance via snd_ak4114_create(), create PCMs, then attach PCMs with snd_ak4114_build() later.
Thanks a lot for incredibly fast response. Could snd_ak4114_build() be called for testing purposes somewhere towards the end of snd_vt1724_probe (ice1724.c), when pcms are already created?
See below.
BTW, what is difference between professional and spdif pcms? Is it only about digital format (like professional/consumer in iecset)? Sorry for stupid questions, but I do not know which pcm to use.
VT1724 has seperate DMAs for the analog and the SPDIF streams while ICE1712 has only one for both (mixed up).
Confusingly the analog PCM is named "professional" there because it was called so in ice1712 driver, and vt1724 driver is derived from ice1712 driver. ICE1712 has two analog connections modes, consumer mode (usually via ac97) and professional mode (via i2s).
Would I get to playback/capture substreams e.g. using ice->pcm_pro->streams[0/1]->substream at the end of snd_vt1724_probe? I understand I could probably find this information in alsa documentation.
The build_controls callback is called after creation of PCMs. So, you can call it there. The proposed fix for Juli is below.
Can any Juli users test the latest HG version with this patch?
thanks,
Takashi
diff -r 1df47bdd721a pci/ice1712/juli.c --- a/pci/ice1712/juli.c Fri Mar 30 15:38:39 2007 +0200 +++ b/pci/ice1712/juli.c Fri Mar 30 16:31:35 2007 +0200 @@ -138,7 +138,16 @@ static struct snd_akm4xxx akm_juli_dac _
static int __devinit juli_add_controls(struct snd_ice1712 *ice) { - return snd_ice1712_akm4xxx_build_controls(ice); + int err; + err = snd_ice1712_akm4xxx_build_controls(ice); + if (err < 0) + return err; + err = snd_ak4114_build(ice->spec.juli.ak4114, + ice->pcm_pro->streams[SNDRV_PCM_STREAM_PLAYBACK].substream, + ice->pcm_pro->streams[SNDRV_PCM_STREAM_CAPTURE].substream); + if (err < 0) + return err; + return 0; }
/*
Hi Takashi
BTW, what is difference between professional and spdif pcms? Is it only about digital format (like professional/consumer in iecset)? Sorry for stupid questions, but I do not know which pcm to use.
VT1724 has seperate DMAs for the analog and the SPDIF streams while ICE1712 has only one for both (mixed up).
Confusingly the analog PCM is named "professional" there because it was called so in ice1712 driver, and vt1724 driver is derived from ice1712 driver. ICE1712 has two analog connections modes, consumer mode (usually via ac97) and professional mode (via i2s).
Uff, that is a lot of background knowledge. Would it be possible to put this explanation into ice1724.c code? It would definitely help newcomers. Thanks a lot.
Would I get to playback/capture substreams e.g. using ice->pcm_pro->streams[0/1]->substream at the end of snd_vt1724_probe? I understand I could probably find this information in alsa documentation.
The build_controls callback is called after creation of PCMs. So, you can call it there. The proposed fix for Juli is below.
Can any Juli users test the latest HG version with this patch?
I tested the code with prodigy192 and my current ak4114 code. I used ice->pcm stream instead of ice->pcm_pro since I guess that is the one for SPDIF data.
Probing the module produced error -16 (EBUSY). Analysis showed duplicity of control item SNDRV_CTL_NAME_IEC958("",PLAYBACK,DEFAULT) defined in ice1724.c (snd_vt1724_spdif_default) as well as ak4114.c . Upon removing the control from ak4114.c (and adjusting AK4114_CONTROLS) the module loaded correctly and all the new PCM controls were available in amixer.
I think the problem is that the ak4114 support assumes both SPDIF OUT and IN functions are used, whereas ice1724 support assumes SPDIF-OUT is handled by ice1724, leaving only SPDIF-IN to other chips. At least with Prodigy192 + MI/ODI/O that is the case in reality too (ak4114 spdif-out capability is not used, signal goes from ice1724 directly to output buffers on the add-on card).
What solution would you suggest? I guess a solution would be to check for duplicity before adding new controls, or perhaps continue after hitting error EBUSY in snd_ctl_add.
Best regards,
Pavel.
Perhaps a solution could involve splitting ak4114 support into spdif-in and spdif-out. When creating ak4114 we could specify which part should be active. Default would be both parts in order to keep existing funcionality. The change would only concern controls definition and corresponding snd_ctl_notify calls anyway.
Thanks.
Pavel.
------------ Původní zpráva ------------ Od: dustin@seznam.cz Předmět: Re: [alsa-devel] Correct use of ak4114.c? Datum: 01.4.2007 23:09:13
Hi Takashi
BTW, what is difference between professional and spdif pcms? Is it only about digital format (like professional/consumer in iecset)? Sorry for stupid questions, but I do not know which pcm to use.
VT1724 has seperate DMAs for the analog and the SPDIF streams while ICE1712 has only one for both (mixed up).
Confusingly the analog PCM is named "professional" there because it was called so in ice1712 driver, and vt1724 driver is derived from ice1712 driver. ICE1712 has two analog connections modes, consumer mode (usually via ac97) and professional mode (via i2s).
Uff, that is a lot of background knowledge. Would it be possible to put this explanation into ice1724.c code? It would definitely help newcomers. Thanks a lot.
Would I get to playback/capture substreams e.g. using ice->pcm_pro->streams[0/1]->substream at the end of snd_vt1724_probe? I understand I could probably find this information in alsa documentation.
The build_controls callback is called after creation of PCMs. So, you can call it there. The proposed fix for Juli is below.
Can any Juli users test the latest HG version with this patch?
I tested the code with prodigy192 and my current ak4114 code. I used ice->pcm stream instead of ice->pcm_pro since I guess that is the one for SPDIF data.
Probing the module produced error -16 (EBUSY). Analysis showed duplicity of control item SNDRV_CTL_NAME_IEC958("",PLAYBACK,DEFAULT) defined in ice1724.c (snd_vt1724_spdif_default) as well as ak4114.c . Upon removing the control from ak4114.c (and adjusting AK4114_CONTROLS) the module loaded correctly and all the new PCM controls were available in amixer.
I think the problem is that the ak4114 support assumes both SPDIF OUT and IN functions are used, whereas ice1724 support assumes SPDIF-OUT is handled by ice1724, leaving only SPDIF-IN to other chips. At least with Prodigy192 + MI/ODI/O that is the case in reality too (ak4114 spdif-out capability is not used, signal goes from ice1724 directly to output buffers on the add-on card).
What solution would you suggest? I guess a solution would be to check for duplicity before adding new controls, or perhaps continue after hitting error EBUSY in snd_ctl_add.
Best regards,
Pavel. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Sun, 01 Apr 2007 23:09:01 +0200 (CEST), dustin@seznam.cz wrote:
Hi Takashi
BTW, what is difference between professional and spdif pcms? Is it only about digital format (like professional/consumer in iecset)? Sorry for stupid questions, but I do not know which pcm to use.
VT1724 has seperate DMAs for the analog and the SPDIF streams while ICE1712 has only one for both (mixed up).
Confusingly the analog PCM is named "professional" there because it was called so in ice1712 driver, and vt1724 driver is derived from ice1712 driver. ICE1712 has two analog connections modes, consumer mode (usually via ac97) and professional mode (via i2s).
Uff, that is a lot of background knowledge. Would it be possible to put this explanation into ice1724.c code? It would definitely help newcomers. Thanks a lot.
A patch is welcome ;)
Would I get to playback/capture substreams e.g. using ice->pcm_pro->streams[0/1]->substream at the end of snd_vt1724_probe? I understand I could probably find this information in alsa documentation.
The build_controls callback is called after creation of PCMs. So, you can call it there. The proposed fix for Juli is below.
Can any Juli users test the latest HG version with this patch?
I tested the code with prodigy192 and my current ak4114 code. I used ice->pcm stream instead of ice->pcm_pro since I guess that is the one for SPDIF data.
Probing the module produced error -16 (EBUSY). Analysis showed duplicity of control item SNDRV_CTL_NAME_IEC958("",PLAYBACK,DEFAULT) defined in ice1724.c (snd_vt1724_spdif_default) as well as ak4114.c . Upon removing the control from ak4114.c (and adjusting AK4114_CONTROLS) the module loaded correctly and all the new PCM controls were available in amixer.
I think the problem is that the ak4114 support assumes both SPDIF OUT and IN functions are used, whereas ice1724 support assumes SPDIF-OUT is handled by ice1724, leaving only SPDIF-IN to other chips. At least with Prodigy192 + MI/ODI/O that is the case in reality too (ak4114 spdif-out capability is not used, signal goes from ice1724 directly to output buffers on the add-on card).
What solution would you suggest? I guess a solution would be to check for duplicity before adding new controls, or perhaps continue after hitting error EBUSY in snd_ctl_add.
Then the problem is that it passed the playback substream to snd_ak4114_build(). I guess passing NULL there instead should work.
Takashi
Uff, that is a lot of background knowledge. Would it be possible to put this explanation into ice1724.c code? It would definitely help newcomers. Thanks a lot.
A patch is welcome ;)
Sure, I will make one.
Then the problem is that it passed the playback substream to snd_ak4114_build(). I guess passing NULL there instead should work.
Sounds like a perfectly simple solution. I am just wondering if the following code in ak4114.c is correct:
if (!strstr(kctl->id.name, "Playback")) { if (ply_substream == NULL) { snd_ctl_free_one(kctl); ak4114->kctls[idx] = NULL; continue; } kctl->id.device = ply_substream->pcm->device; kctl->id.subdevice = ply_substream->number; } else { kctl->id.device = cap_substream->pcm->device; kctl->id.subdevice = cap_substream->number; }
If control name does NOT contain "Playback", playback stream is used. Should not be the check reversed? You are right, that code can handle the solution you propose.
Pavel.
At Mon, 02 Apr 2007 11:52:45 +0200 (CEST), dustin@seznam.cz wrote:
Uff, that is a lot of background knowledge. Would it be possible to put this explanation into ice1724.c code? It would definitely help newcomers. Thanks a lot.
A patch is welcome ;)
Sure, I will make one.
Then the problem is that it passed the playback substream to snd_ak4114_build(). I guess passing NULL there instead should work.
Sounds like a perfectly simple solution. I am just wondering if the following code in ak4114.c is correct:
if (!strstr(kctl->id.name, "Playback")) { if (ply_substream == NULL) { snd_ctl_free_one(kctl); ak4114->kctls[idx] = NULL; continue; } kctl->id.device = ply_substream->pcm->device; kctl->id.subdevice = ply_substream->number; } else { kctl->id.device = cap_substream->pcm->device; kctl->id.subdevice = cap_substream->number; }
If control name does NOT contain "Playback", playback stream is used. Should not be the check reversed?
Yeah, obviously. Thanks for pointing out.
Takashi
Hello Takashi,
There is a minor bug in ak4114.c, patch is below (including the reversed check, diffed against sources from hg a few hours ago). Tests show the code works fine (PCM controls get updated correctly), only sample rate is shown incorrect in "IEC958 External Rate". I will do more tests tomorrow to elaborate on the sample rate.
Thanks,
Pavel.
diff -r 05ecca0fba92 i2c/other/ak4114.c --- a/i2c/other/ak4114.c Tue Apr 03 13:20:49 2007 +0200 +++ b/i2c/other/ak4114.c Tue Apr 03 21:36:18 2007 +0200 @@ -435,7 +435,7 @@ static struct snd_kcontrol_new snd_ak411 .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, .info = snd_ak4114_in_bit_info, .get = snd_ak4114_in_bit_get, - .private_value = (6<<8) | AK4114_REG_RCS1, + .private_value = (6<<8) | AK4114_REG_RCS0, }, { .iface = SNDRV_CTL_ELEM_IFACE_PCM, @@ -443,7 +443,7 @@ static struct snd_kcontrol_new snd_ak411 .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, .info = snd_ak4114_in_bit_info, .get = snd_ak4114_in_bit_get, - .private_value = (3<<8) | AK4114_REG_RCS1, + .private_value = (3<<8) | AK4114_REG_RCS0, } };
@@ -462,7 +462,7 @@ int snd_ak4114_build(struct ak4114 *ak41 kctl = snd_ctl_new1(&snd_ak4114_iec958_controls[idx], ak4114); if (kctl == NULL) return -ENOMEM; - if (!strstr(kctl->id.name, "Playback")) { + if (strstr(kctl->id.name, "Playback")) { if (ply_substream == NULL) { snd_ctl_free_one(kctl); ak4114->kctls[idx] = NULL;
At Tue, 03 Apr 2007 23:13:53 +0200 (CEST), dustin@seznam.cz wrote:
Hello Takashi,
There is a minor bug in ak4114.c, patch is below (including the reversed check, diffed against sources from hg a few hours ago). Tests show the code works fine (PCM controls get updated correctly), only sample rate is shown incorrect in "IEC958 External Rate". I will do more tests tomorrow to elaborate on the sample rate.
Great. Then I'll commit the fixes after your test result.
Thanks!
Takashi
Great. Then I'll commit the fixes after your test result.
I am attaching a tested patch for ak4114.c and ak4114.h
Fixes: ------- * correct register for "IEC958 Non-PCM Bitstream", "IEC958 DTS Bitstream": AK4114_REG_RCS0 * correct check for control name: if (strstr(kctl->id.name, "Playback")) * correct check: if (! chip->init) in snd_ak4114_external_rate
New features: ----------------- * added PCM control "IEC958 PPL Lock Status"
All capture functions work, as well as all PCM + IEC958 controls (their values correspond to datasheet specifications). Since AK4114 is initialized without a crystal connected, the "IEC958 External Rate" is read correctly from IEC958 header in professional mode. In consumer mode the chip is able to detect 44100 and 48000, all other rates are returned as 44100. All the rates are correctly set in "IEC958 External Rate". I did the tests using the actual SPDIF output of the same card, which is capable of 32 - 192kHz consumer/professional mode. Other test data were provided by digital output of my CD player.
To make testing easier I patched amixer.c to display 5 bytes of SND_CTL_ELEM_TYPE_IEC958 which was so far unsupported (question mark instead). I find it usefull, it is up to you to decide if it finds way into official sources.
diff -r 2a2bde6bf7c2 amixer/amixer.c --- a/amixer/amixer.c Mon Jan 15 14:47:35 2007 +0100 +++ b/amixer/amixer.c Wed Apr 04 22:29:06 2007 +0200 @@ -534,6 +534,7 @@ static int show_control(const char *spac snd_ctl_elem_id_t *id; snd_ctl_elem_info_t *info; snd_ctl_elem_value_t *control; + snd_aes_iec958_t iec958; snd_ctl_elem_id_alloca(&id); snd_ctl_elem_info_alloca(&info); snd_ctl_elem_value_alloca(&control); @@ -604,6 +605,12 @@ static int show_control(const char *spac break; case SND_CTL_ELEM_TYPE_BYTES: printf("0x%02x", snd_ctl_elem_value_get_byte(control, idx)); + break; + case SND_CTL_ELEM_TYPE_IEC958: + snd_ctl_elem_value_get_iec958 (control, &iec958); + printf("4=0x%01x, 3=0x%01x, 2=0x%01x, 1=0x%01x, 0=0x%01x", + iec958.status[4], iec958.status[3], iec958.status[2], + iec958.status[1], iec958.status[0]); break; default: printf("?");
I am attaching a tested patch for ak4114.c and ak4114.h
Hmm, the mailing list does not like attachments. Here is the patch:
diff -r 05ecca0fba92 i2c/other/ak4114.c --- a/i2c/other/ak4114.c Tue Apr 03 13:20:49 2007 +0200 +++ b/i2c/other/ak4114.c Wed Apr 04 22:01:04 2007 +0200 @@ -435,7 +435,7 @@ static struct snd_kcontrol_new snd_ak411 .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, .info = snd_ak4114_in_bit_info, .get = snd_ak4114_in_bit_get, - .private_value = (6<<8) | AK4114_REG_RCS1, + .private_value = (6<<8) | AK4114_REG_RCS0, }, { .iface = SNDRV_CTL_ELEM_IFACE_PCM, @@ -443,7 +443,15 @@ static struct snd_kcontrol_new snd_ak411 .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, .info = snd_ak4114_in_bit_info, .get = snd_ak4114_in_bit_get, - .private_value = (3<<8) | AK4114_REG_RCS1, + .private_value = (3<<8) | AK4114_REG_RCS0, +}, +{ + .iface = SNDRV_CTL_ELEM_IFACE_PCM, + .name = "IEC958 PPL Lock Status", + .access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE, + .info = snd_ak4114_in_bit_info, + .get = snd_ak4114_in_bit_get, + .private_value = (1<<31) | (4<<8) | AK4114_REG_RCS0, } };
@@ -462,7 +470,7 @@ int snd_ak4114_build(struct ak4114 *ak41 kctl = snd_ctl_new1(&snd_ak4114_iec958_controls[idx], ak4114); if (kctl == NULL) return -ENOMEM; - if (!strstr(kctl->id.name, "Playback")) { + if (strstr(kctl->id.name, "Playback")) { if (ply_substream == NULL) { snd_ctl_free_one(kctl); ak4114->kctls[idx] = NULL; @@ -526,6 +534,9 @@ static void ak4114_notify(struct ak4114 if (c0 & AK4114_DTSCD) snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, &ak4114->kctls[13]->id); + if (c0 & AK4114_UNLCK) + snd_ctl_notify(ak4114->card, SNDRV_CTL_EVENT_MASK_VALUE, + &ak4114->kctls[14]->id); }
int snd_ak4114_external_rate(struct ak4114 *ak4114) @@ -587,7 +598,7 @@ static void ak4114_stats(struct work_str { struct ak4114 *chip = container_of(work, struct ak4114, work.work);
- if (chip->init) + if (! chip->init) snd_ak4114_check_rate_and_errors(chip, 0);
schedule_delayed_work(&chip->work, HZ / 10); diff -r 05ecca0fba92 include/ak4114.h --- a/include/ak4114.h Tue Apr 03 13:20:49 2007 +0200 +++ b/include/ak4114.h Wed Apr 04 21:57:44 2007 +0200 @@ -158,7 +158,7 @@ #define AK4114_CHECK_NO_STAT (1<<0) /* no statistics */ #define AK4114_CHECK_NO_RATE (1<<1) /* no rate check */
-#define AK4114_CONTROLS 14 +#define AK4114_CONTROLS 15
typedef void (ak4114_write_t)(void *private_data, unsigned char addr, unsigned char data); typedef unsigned char (ak4114_read_t)(void *private_data, unsigned char addr);
At Wed, 04 Apr 2007 22:33:05 +0200 (CEST), dustin@seznam.cz wrote:
Great. Then I'll commit the fixes after your test result.
I am attaching a tested patch for ak4114.c and ak4114.h
Fixes:
- correct register for "IEC958 Non-PCM Bitstream", "IEC958 DTS Bitstream": AK4114_REG_RCS0
- correct check for control name: if (strstr(kctl->id.name, "Playback"))
- correct check: if (! chip->init) in snd_ak4114_external_rate
New features:
- added PCM control "IEC958 PPL Lock Status"
Looks fine. Could you give a sign-off to commit to the tree?
thanks,
Takashi
------------ Původní zpráva ------------ Od: Takashi Iwai tiwai@suse.de Předmět: Re: [alsa-devel] Correct use of ak4114.c? Datum: 04.4.2007 22:41:53
At Wed, 04 Apr 2007 22:33:05 +0200 (CEST), dustin@seznam.cz wrote:
Great. Then I'll commit the fixes after your test result.
I am attaching a tested patch for ak4114.c and ak4114.h
Fixes:
- correct register for "IEC958 Non-PCM Bitstream", "IEC958 DTS Bitstream":
AK4114_REG_RCS0
- correct check for control name: if (strstr(kctl->id.name, "Playback"))
- correct check: if (! chip->init) in snd_ak4114_external_rate
New features:
- added PCM control "IEC958 PPL Lock Status"
Looks fine. Could you give a sign-off to commit to the tree?
thanks,
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
------------ Původní zpráva ------------ Od: Takashi Iwai tiwai@suse.de Předmět: Re: [alsa-devel] Correct use of ak4114.c? Datum: 04.4.2007 22:41:49
Looks fine. Could you give a sign-off to commit to the tree?
My first time :) :
Signed-off-by: Pavel Hofman dustin@seznam.cz
At Thu, 05 Apr 2007 09:30:36 +0200 (CEST), dustin@seznam.cz wrote:
------------ Původní zpráva ------------ Od: Takashi Iwai tiwai@suse.de Předmět: Re: [alsa-devel] Correct use of ak4114.c? Datum: 04.4.2007 22:41:49
Looks fine. Could you give a sign-off to commit to the tree?
My first time :) :
Signed-off-by: Pavel Hofman dustin@seznam.cz
Thanks, applied to HG tree now.
Takashi
Signed-off-by: Pavel Hofman dustin@seznam.cz
Thanks, applied to HG tree now.
Thanks. That's weird, I cannot see the change. My hgrc:default = http://hg.alsa-project.org/alsa-kernel and update does not provide any changes. Last log on ak4114.c still
changeset: 4970:1df47bdd721a user: tiwai date: Fri Mar 30 15:38:39 2007 +0200 summary: ak4114 - Fix possible Oops with callbacks
I may be doing something wrong, I have no hg experience, only svn/cvs.
Pavel.
Do "hg pull" first to update your repository, then do "hg update".
On Thu, 2007-04-05 at 20:08 +0200, dustin@seznam.cz wrote:
Signed-off-by: Pavel Hofman dustin@seznam.cz
Thanks, applied to HG tree now.
Thanks. That's weird, I cannot see the change. My hgrc:default = http://hg.alsa-project.org/alsa-kernel and update does not provide any changes. Last log on ak4114.c still
changeset: 4970:1df47bdd721a user: tiwai date: Fri Mar 30 15:38:39 2007 +0200 summary: ak4114 - Fix possible Oops with callbacks
I may be doing something wrong, I have no hg experience, only svn/cvs.
Pavel. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
------------ Původní zpráva ------------ Od: Tobin Davis tdavis@dsl-only.net Předmět: Re: [alsa-devel] Correct use of ak4114.c? Datum: 05.4.2007 20:17:36
Do "hg pull" first to update your repository, then do "hg update".
Thanks a lot, works fine now.
Pavel.
VT1724 has seperate DMAs for the analog and the SPDIF streams while ICE1712 has only one for both (mixed up).
Confusingly the analog PCM is named "professional" there because it was called so in ice1712 driver, and vt1724 driver is derived from ice1712 driver. ICE1712 has two analog connections modes, consumer mode (usually via ac97) and professional mode (via i2s).
Uff, that is a lot of background knowledge. Would it be possible to put this explanation into ice1724.c code? It would definitely help newcomers. Thanks a lot.
A patch is welcome ;)
I know, not exactly creative :) :
diff -r 42321871a7dc pci/ice1712/ice1724.c --- a/pci/ice1712/ice1724.c Thu Apr 05 17:08:57 2007 +0200 +++ b/pci/ice1712/ice1724.c Fri Apr 06 22:59:33 2007 +0200 @@ -2345,6 +2345,14 @@ static int __devinit snd_vt1724_probe(st } c = &no_matched; __found: + /* + * VT1724 has separate DMAs for the analog and the SPDIF streams while + * ICE1712 has only one for both (mixed up). + * + * Confusingly the analog PCM is named "professional" here because it + * was called so in ice1712 driver, and vt1724 driver is derived from + * ice1712 driver. + */
if ((err = snd_vt1724_pcm_profi(ice, pcm_dev++)) < 0) { snd_card_free(card);
participants (3)
-
dustin＠seznam.cz -
Takashi Iwai -
Tobin Davis