Re: [alsa-devel] snd-soc-cs4270: Convert to a new-style i2c driver (work in progress)
Hi Timur,
On Sun, 31 Aug 2008 11:28:08 -0500, Timur Tabi wrote:
On Sun, Aug 31, 2008 at 9:18 AM, Jean Delvare khali@linux-fr.org wrote:
Hi Tibur,
It's "Timur"
Oops, apparently I mixed your first name and last name. Sorry about that.
I am in the process of converting your cs4270 codec driver from the legacy i2c model to the new (standard) one.
It has already been converted.
http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=commit;h=0da...
Ah, excellent. One less thing for me to do :) Thanks!
Two comments about your patch:
* Your driver now lacks a remove method. Unless I miss something, if the snd-soc-cs4270 driver is unloaded, you will leave dangling resources behind (codec->reg_cache in particular.)
* I2C_DRIVERID_CS4270 must be removed from i2c-id.h.
I don't know why this commit hasn't been pushed upstream, though.
As I understand it, without that patch the mpc8610_hpcd doesn't work, as the I2C address of the sound codec will be made busy by the platform code and thus the snd-soc-cs4270 driver won't be able to attach to it. If I am correct then I suggest that you ask Takashi to push the patch to Linus now to fix that.
This is work in progress. The patch below converts the cs4270 driver itself. However we also need to convert its users. As far as I can see there's only one user at this point: mpc8610_hpcd.
That's correct. I've never seen any interest in the CS4270 outside of Freescale.
The problem is that this driver doesn't look like the other codec drivers I have already converted. So, we need to add code to instantiate the cs4270 i2c device, but I don't know where this should happen. Given that the mpc8610_hpcd is apparently based on Open Firmware, I guess that the i2c device should be instantiated directly by the platform code. I see that the device is declared in mpc8610_hpcd.dts, so maybe it's already done and my patch should work already? What do you think?
I think you need to use the right git repository for your development. :-)
Very good point indeed. I'll make sure to check Takashi's sound-2.6 tree before attempting to convert any other SoC codec driver. Do you know off the top of your head if other drivers have already been converted?
Thanks,
On Sun, Aug 31, 2008 at 07:09:28PM +0200, Jean Delvare wrote:
On Sun, 31 Aug 2008 11:28:08 -0500, Timur Tabi wrote:
I don't know why this commit hasn't been pushed upstream, though.
As I understand it, without that patch the mpc8610_hpcd doesn't work, as the I2C address of the sound codec will be made busy by the platform code and thus the snd-soc-cs4270 driver won't be able to attach to it. If I am correct then I suggest that you ask Takashi to push the patch to Linus now to fix that.
Yeah, AFAICR it wasn't made clear when this was posted that it was a bug fix.
That's correct. I've never seen any interest in the CS4270 outside of Freescale.
That's not entirely suprising - the vast majority of embedded devices never go anywhere near mainline so it tends to only be reference boards and devices with some sort of public community that have activity that's visible upstream.
Very good point indeed. I'll make sure to check Takashi's sound-2.6 tree before attempting to convert any other SoC codec driver. Do you know off the top of your head if other drivers have already been converted?
None - the only other one is Takashi's current tree is WM8903.
Jean Delvare wrote:
- Your driver now lacks a remove method. Unless I miss something, if
the snd-soc-cs4270 driver is unloaded, you will leave dangling resources behind (codec->reg_cache in particular.)
Indeed, but then, I don't think I ever supported loading and unload this driver as a module. The Kconfig does say it's a tristate, though. I'll take a look at it.
Most of my real development is going to the ASoC V2 version of this driver, and that version should be a lot better. I'll take another look at both driver to make sure I didn't screw this up.
- I2C_DRIVERID_CS4270 must be removed from i2c-id.h.
It can't be removed until this patch goes upstream, but thanks for the reminder.
As I understand it, without that patch the mpc8610_hpcd doesn't work, as the I2C address of the sound codec will be made busy by the platform code and thus the snd-soc-cs4270 driver won't be able to attach to it.
That's correct. I though I made that clear in the changelog.
Do you know off the top of your head if other drivers have already been converted?
I doubt it. ASoC V1 doesn't generally support PowerPC, although there is code to make it work. That's why there's not a lot of support across the board for PowerPC-isms.
On Sun, Aug 31, 2008 at 12:09 PM, Jean Delvare khali@linux-fr.org wrote:
- Your driver now lacks a remove method. Unless I miss something, if
the snd-soc-cs4270 driver is unloaded, you will leave dangling resources behind (codec->reg_cache in particular.)
I'm working on a fix for this, but there's something you need to know. I don't think ASoC V1 modules can be unloaded. The drivers have hard-coded cross-dependencies. The codec driver doesn't have this problem, but in general it's not really possible to select ASoC drivers as modules in Kconfig anyway. The "tristate" I mentioned is misleading.
On Wed, Sep 3, 2008 at 2:47 PM, Timur Tabi timur@freescale.com wrote:
I'm working on a fix for this, but there's something you need to know. I don't think ASoC V1 modules can be unloaded.
Correction: ASoC drivers can't be compiled as modules, not even the codec drivers. I had to hack up the Kconfigs to get this to work, but there is no way to compile the drivers as modules. If you try, you get this:
CC [M] sound/soc/codecs/cs4270.o LD [M] sound/soc/codecs/snd-soc-cs4270.o ... sound/built-in.o: In function `mpc8610_hpcd_probe': /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:455: undefined reference to `cs4270_dai' /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:455: undefined reference to `cs4270_dai' /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:469: undefined reference to `soc_codec_device_cs4270' /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:469: undefined reference to `soc_codec_device_cs4270' make: *** [.tmp_vmlinux1] Error 1
So I don't see how an I2C 'remove' function would ever be called anyway. If you can show me how, I'll add it.
Now, the ASoC V2 driver can be loaded and unloaded, and I'm pretty sure I have bugs there.
On Wed, 3 Sep 2008 15:30:33 -0500, Timur Tabi wrote:
On Wed, Sep 3, 2008 at 2:47 PM, Timur Tabi timur@freescale.com wrote:
I'm working on a fix for this, but there's something you need to know. I don't think ASoC V1 modules can be unloaded.
Correction: ASoC drivers can't be compiled as modules, not even the codec drivers. I had to hack up the Kconfigs to get this to work, but there is no way to compile the drivers as modules. If you try, you get this:
CC [M] sound/soc/codecs/cs4270.o LD [M] sound/soc/codecs/snd-soc-cs4270.o ... sound/built-in.o: In function `mpc8610_hpcd_probe': /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:455: undefined reference to `cs4270_dai' /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:455: undefined reference to `cs4270_dai' /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:469: undefined reference to `soc_codec_device_cs4270' /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:469: undefined reference to `soc_codec_device_cs4270' make: *** [.tmp_vmlinux1] Error 1
So I don't see how an I2C 'remove' function would ever be called anyway. If you can show me how, I'll add it.
By writing to the unbind file that the i2c driver in question exposes in /sys? I didn't try it but it should cause the driver to unbind from the device in question and thus ->remove would be called.
Note that I do not care much myself. I mentioned the lack of a remove function merely because the driver could be build as a module and because the original code did have an equivalent function. If you are confident that the code isn't needed or you decide that you don't care, up to you. I am really not familiar with the ASoC drivers so I don't think my view really matters.
Now, the ASoC V2 driver can be loaded and unloaded, and I'm pretty sure I have bugs there.
Jean Delvare wrote:
By writing to the unbind file that the i2c driver in question exposes in /sys? I didn't try it but it should cause the driver to unbind from the device in question and thus ->remove would be called.
Can you give me specifics on how to use the unbind file? I tried a few things, but none of them worked, and google isn't helpful either.
On Wed, Sep 3, 2008 at 4:21 PM, Timur Tabi timur@freescale.com wrote:
Can you give me specifics on how to use the unbind file? I tried a few things, but none of them worked, and google isn't helpful either.
Never mind, I figured it out. Sure enough, it works. It probably breaks ASoC, though. I'm going to add support for it anyway, though.
On Wed, Sep 03, 2008 at 03:30:33PM -0500, Timur Tabi wrote:
On Wed, Sep 3, 2008 at 2:47 PM, Timur Tabi timur@freescale.com wrote:
I'm working on a fix for this, but there's something you need to know. I don't think ASoC V1 modules can be unloaded.
Correction: ASoC drivers can't be compiled as modules, not even the codec drivers. I had to hack up the Kconfigs to get this to work, but
They can be - I do this on a regular basis during development. If it weren't possible I'd probably get annoyed enough to make it so :)
there is no way to compile the drivers as modules. If you try, you get this:
CC [M] sound/soc/codecs/cs4270.o LD [M] sound/soc/codecs/snd-soc-cs4270.o ... sound/built-in.o: In function `mpc8610_hpcd_probe': /temp/alsa.1994/sound/soc/fsl/mpc8610_hpcd.c:455: undefined reference to `cs4270_dai'
If you build the codec driver as a module then you also need to build the machine driver as a module. Your Kconfig ought to be enforcing that which I guess is the hack you had to put in?
Mark Brown wrote:
If you build the codec driver as a module then you also need to build the machine driver as a module. Your Kconfig ought to be enforcing that which I guess is the hack you had to put in?
I guess that's my problem, I was just loading the codec driver as a module.
participants (4)
-
Jean Delvare -
Mark Brown -
Mark Brown -
Timur Tabi