[alsa-devel] [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.
These changes are needed to keep up with the changes in the MFD core and V4L2 parts of the wl1273 FM radio driver.
Use function pointers instead of exported functions for I2C IO. Also move all preprocessor constants from the wl1273.h to include/linux/mfd/wl1273-core.h.
Signed-off-by: Matti J. Aaltonen matti.j.aaltonen@nokia.com --- sound/soc/codecs/Kconfig | 2 +- sound/soc/codecs/wl1273.c | 29 +++++++----------- sound/soc/codecs/wl1273.h | 71 --------------------------------------------- 3 files changed, 13 insertions(+), 89 deletions(-)
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 3b5690d..937517a 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -43,7 +43,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_TWL6040 if TWL4030_CORE select SND_SOC_UDA134X select SND_SOC_UDA1380 if I2C - select SND_SOC_WL1273 if WL1273_CORE + select SND_SOC_WL1273 if RADIO_WL1273 select SND_SOC_WM2000 if I2C select SND_SOC_WM8350 if MFD_WM8350 select SND_SOC_WM8400 if MFD_WM8400 diff --git a/sound/soc/codecs/wl1273.c b/sound/soc/codecs/wl1273.c index 0c47c78..4c22ae9 100644 --- a/sound/soc/codecs/wl1273.c +++ b/sound/soc/codecs/wl1273.c @@ -43,7 +43,7 @@ struct wl1273_priv { static int snd_wl1273_fm_set_i2s_mode(struct wl1273_core *core, int rate, int width) { - struct device *dev = &core->i2c_dev->dev; + struct device *dev = &core->client->dev; int r = 0; u16 mode;
@@ -124,13 +124,13 @@ static int snd_wl1273_fm_set_i2s_mode(struct wl1273_core *core, dev_dbg(dev, "mode: 0x%04x\n", mode);
if (core->i2s_mode != mode) { - r = wl1273_fm_write_cmd(core, WL1273_I2S_MODE_CONFIG_SET, mode); + r = core->write(core, WL1273_I2S_MODE_CONFIG_SET, mode); if (r) goto out;
core->i2s_mode = mode; - r = wl1273_fm_write_cmd(core, WL1273_AUDIO_ENABLE, - WL1273_AUDIO_ENABLE_I2S); + r = core->write(core, WL1273_AUDIO_ENABLE, + WL1273_AUDIO_ENABLE_I2S); if (r) goto out; } @@ -143,8 +143,7 @@ out: static int snd_wl1273_fm_set_channel_number(struct wl1273_core *core, int channel_number) { - struct i2c_client *client = core->i2c_dev; - struct device *dev = &client->dev; + struct device *dev = &core->client->dev; int r = 0;
dev_dbg(dev, "%s\n", __func__); @@ -155,17 +154,13 @@ static int snd_wl1273_fm_set_channel_number(struct wl1273_core *core, goto out;
if (channel_number == 1 && core->mode == WL1273_MODE_RX) - r = wl1273_fm_write_cmd(core, WL1273_MOST_MODE_SET, - WL1273_RX_MONO); + r = core->write(core, WL1273_MOST_MODE_SET, WL1273_RX_MONO); else if (channel_number == 1 && core->mode == WL1273_MODE_TX) - r = wl1273_fm_write_cmd(core, WL1273_MONO_SET, - WL1273_TX_MONO); + r = core->write(core, WL1273_MONO_SET, WL1273_TX_MONO); else if (channel_number == 2 && core->mode == WL1273_MODE_RX) - r = wl1273_fm_write_cmd(core, WL1273_MOST_MODE_SET, - WL1273_RX_STEREO); + r = core->write(core, WL1273_MOST_MODE_SET, WL1273_RX_STEREO); else if (channel_number == 2 && core->mode == WL1273_MODE_TX) - r = wl1273_fm_write_cmd(core, WL1273_MONO_SET, - WL1273_TX_STEREO); + r = core->write(core, WL1273_MONO_SET, WL1273_TX_STEREO); else r = -EINVAL; out: @@ -238,7 +233,7 @@ static int snd_wl1273_fm_audio_put(struct snd_kcontrol *kcontrol, if (wl1273->core->audio_mode == val) return 0;
- r = wl1273_fm_set_audio(wl1273->core, val); + r = wl1273->core->set_audio(wl1273->core, val); if (r < 0) return r;
@@ -273,8 +268,8 @@ static int snd_wl1273_fm_volume_put(struct snd_kcontrol *kcontrol,
dev_dbg(codec->dev, "%s: enter.\n", __func__);
- r = wl1273_fm_set_volume(wl1273->core, - ucontrol->value.integer.value[0]); + r = wl1273->core->set_volume(wl1273->core, + ucontrol->value.integer.value[0]); if (r) return r;
diff --git a/sound/soc/codecs/wl1273.h b/sound/soc/codecs/wl1273.h index 14ed027..43ec7e6 100644 --- a/sound/soc/codecs/wl1273.h +++ b/sound/soc/codecs/wl1273.h @@ -25,77 +25,6 @@ #ifndef __WL1273_CODEC_H__ #define __WL1273_CODEC_H__
-/* I2S protocol, left channel first, data width 16 bits */ -#define WL1273_PCM_DEF_MODE 0x00 - -/* Rx */ -#define WL1273_AUDIO_ENABLE_I2S (1 << 0) -#define WL1273_AUDIO_ENABLE_ANALOG (1 << 1) - -/* Tx */ -#define WL1273_AUDIO_IO_SET_ANALOG 0 -#define WL1273_AUDIO_IO_SET_I2S 1 - -#define WL1273_POWER_SET_OFF 0 -#define WL1273_POWER_SET_FM (1 << 0) -#define WL1273_POWER_SET_RDS (1 << 1) -#define WL1273_POWER_SET_RETENTION (1 << 4) - -#define WL1273_PUPD_SET_OFF 0x00 -#define WL1273_PUPD_SET_ON 0x01 -#define WL1273_PUPD_SET_RETENTION 0x10 - -/* I2S mode */ -#define WL1273_IS2_WIDTH_32 0x0 -#define WL1273_IS2_WIDTH_40 0x1 -#define WL1273_IS2_WIDTH_22_23 0x2 -#define WL1273_IS2_WIDTH_23_22 0x3 -#define WL1273_IS2_WIDTH_48 0x4 -#define WL1273_IS2_WIDTH_50 0x5 -#define WL1273_IS2_WIDTH_60 0x6 -#define WL1273_IS2_WIDTH_64 0x7 -#define WL1273_IS2_WIDTH_80 0x8 -#define WL1273_IS2_WIDTH_96 0x9 -#define WL1273_IS2_WIDTH_128 0xa -#define WL1273_IS2_WIDTH 0xf - -#define WL1273_IS2_FORMAT_STD (0x0 << 4) -#define WL1273_IS2_FORMAT_LEFT (0x1 << 4) -#define WL1273_IS2_FORMAT_RIGHT (0x2 << 4) -#define WL1273_IS2_FORMAT_USER (0x3 << 4) - -#define WL1273_IS2_MASTER (0x0 << 6) -#define WL1273_IS2_SLAVEW (0x1 << 6) - -#define WL1273_IS2_TRI_AFTER_SENDING (0x0 << 7) -#define WL1273_IS2_TRI_ALWAYS_ACTIVE (0x1 << 7) - -#define WL1273_IS2_SDOWS_RR (0x0 << 8) -#define WL1273_IS2_SDOWS_RF (0x1 << 8) -#define WL1273_IS2_SDOWS_FR (0x2 << 8) -#define WL1273_IS2_SDOWS_FF (0x3 << 8) - -#define WL1273_IS2_TRI_OPT (0x0 << 10) -#define WL1273_IS2_TRI_ALWAYS (0x1 << 10) - -#define WL1273_IS2_RATE_48K (0x0 << 12) -#define WL1273_IS2_RATE_44_1K (0x1 << 12) -#define WL1273_IS2_RATE_32K (0x2 << 12) -#define WL1273_IS2_RATE_22_05K (0x4 << 12) -#define WL1273_IS2_RATE_16K (0x5 << 12) -#define WL1273_IS2_RATE_12K (0x8 << 12) -#define WL1273_IS2_RATE_11_025 (0x9 << 12) -#define WL1273_IS2_RATE_8K (0xa << 12) -#define WL1273_IS2_RATE (0xf << 12) - -#define WL1273_I2S_DEF_MODE (WL1273_IS2_WIDTH_32 | \ - WL1273_IS2_FORMAT_STD | \ - WL1273_IS2_MASTER | \ - WL1273_IS2_TRI_AFTER_SENDING | \ - WL1273_IS2_SDOWS_RR | \ - WL1273_IS2_TRI_OPT | \ - WL1273_IS2_RATE_48K) - int wl1273_get_format(struct snd_soc_codec *codec, unsigned int *fmt);
#endif /* End of __WL1273_CODEC_H__ */
On Thu, Jan 13, 2011 at 03:22:45PM +0200, Matti J. Aaltonen wrote:
These changes are needed to keep up with the changes in the MFD core and V4L2 parts of the wl1273 FM radio driver.
This needs to be submitted as part of the patch that changes the API of the core otherwise you'll break builds.
On Thu, 2011-01-13 at 15:22 +0200, Matti J. Aaltonen wrote:
These changes are needed to keep up with the changes in the MFD core and V4L2 parts of the wl1273 FM radio driver.
Use function pointers instead of exported functions for I2C IO. Also move all preprocessor constants from the wl1273.h to include/linux/mfd/wl1273-core.h.
Signed-off-by: Matti J. Aaltonen matti.j.aaltonen@nokia.com
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
Although it looks like we have a build dependency on the MFD change here ? If so, it may be easier submitting this together with your MFD changes.
Hi.
On Thu, 2011-01-13 at 13:35 +0000, ext Liam Girdwood wrote:
On Thu, 2011-01-13 at 15:22 +0200, Matti J. Aaltonen wrote:
These changes are needed to keep up with the changes in the MFD core and V4L2 parts of the wl1273 FM radio driver.
Use function pointers instead of exported functions for I2C IO. Also move all preprocessor constants from the wl1273.h to include/linux/mfd/wl1273-core.h.
Signed-off-by: Matti J. Aaltonen matti.j.aaltonen@nokia.com
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
Although it looks like we have a build dependency on the MFD change here ? If so, it may be easier submitting this together with your MFD changes.
I've already submitted the MFD & V4L2 changes, they have been accepted and should be in the 2.6.38 kernel. One possibility is to wait until v. 38 is out... if necessary.
Thanks, Matti
On Thu, Jan 13, 2011 at 04:17:53PM +0200, Matti J. Aaltonen wrote:
On Thu, 2011-01-13 at 13:35 +0000, ext Liam Girdwood wrote:
Although it looks like we have a build dependency on the MFD change here ? If so, it may be easier submitting this together with your MFD changes.
I've already submitted the MFD & V4L2 changes, they have been accepted and should be in the 2.6.38 kernel. One possibility is to wait until v. 38 is out... if necessary.
Gah! Don't do this. Any changes you're submitting should leave the kernnel buildable. If you're submitting a change in the MFD driver to change the API incompatibly then you need to ensure that *all* callers are updated to match.
We really shouldn't be getting patches fixing up stuff like this in the merge window :(
On Thu, 2011-01-13 at 15:01 +0000, ext Mark Brown wrote:
On Thu, Jan 13, 2011 at 04:17:53PM +0200, Matti J. Aaltonen wrote:
On Thu, 2011-01-13 at 13:35 +0000, ext Liam Girdwood wrote:
Although it looks like we have a build dependency on the MFD change here ? If so, it may be easier submitting this together with your MFD changes.
I've already submitted the MFD & V4L2 changes, they have been accepted and should be in the 2.6.38 kernel. One possibility is to wait until v. 38 is out... if necessary.
Gah! Don't do this. Any changes you're submitting should leave the kernnel buildable. If you're submitting a change in the MFD driver to change the API incompatibly then you need to ensure that *all* callers are updated to match. We really shouldn't be getting patches fixing up stuff like this in the merge window :(
Sorry if I sent the patch at a wrong time...
I'm not changing the MFD driver, the first version is hopefully going into the v. 2.6.38 kernel.
At first I started to upstream all three parts of the driver at the same time, about a year ago. At first I sent all parts to the media list, there I was - quite reasonably - asked to send the codec to the alsa list. After some tuning and fine tuning the codec got accepted. But getting the rest of the driver in took much longer and in the process the MFD and V4L2 parts became incompatible with the codec.
So we'll just wait until 2.6.38 is out and everything remains compilable... When 38 gets released the codec cannot be used with rest of the driver until this patch is applied, but that can be done when a suitable window opens, right?
Cheers, Matti
On Thu, Jan 13, 2011 at 06:18:21PM +0200, Matti J. Aaltonen wrote:
I'm not changing the MFD driver, the first version is hopefully going into the v. 2.6.38 kernel.
Oh, fail.
At first I started to upstream all three parts of the driver at the same time, about a year ago. At first I sent all parts to the media list, there I was - quite reasonably - asked to send the codec to the alsa list. After some tuning and fine tuning the codec got accepted. But getting the rest of the driver in took much longer and in the process the MFD and V4L2 parts became incompatible with the codec.
This is something that should really have been brought up when making changes. It's really bad to just go and make other bits of the kernel fail to build.
While looking at this I also notice that it's surprisingly difficult to actually build any of this stuff - the MFD core can't be enabled directly, it's only available if you enable the V4L driver, and the core V4L build appears to be rather large adding a noticable amount of time to the build needed to get coverage of the CODECs. It'd be good if you could fix this to remove the dependency, I'd really expect the MFD to be able to build by itself.
So we'll just wait until 2.6.38 is out and everything remains compilable... When 38 gets released the codec cannot be used with rest of the driver until this patch is applied, but that can be done when a suitable window opens, right?
*Ideally* we'd have this API change included as part of the MFD driver merge or included in 2.6.37 since otherwise we cause build issues. ASoC has already been sent to Linus for this merge window. As it is I guess we have to apply this and send a fix later.
On Thu, 2011-01-13 at 17:12 +0000, ext Mark Brown wrote: wrote:
I'm not changing the MFD driver, the first version is hopefully going
into the v. 2.6.38 kernel.
Oh, fail.
?
At first I started to upstream all three parts of the driver at the same time, about a year ago. At first I sent all parts to the media list, there I was - quite reasonably - asked to send the codec to the alsa list. After some tuning and fine tuning the codec got accepted. But getting the rest of the driver in took much longer and in the process the MFD and V4L2 parts became incompatible with the codec.
This is something that should really have been brought up when making changes. It's really bad to just go and make other bits of the kernel fail to build.
I'm not exactly sure what you mean here... It's easy to say now - when looking back that - the whole driver should have been handled more as a whole. An ACK from you and an ACK from Samuel to the media guys etc...
But I'm not trying to break the kernel or the build process or irritate you. I'm just trying to get this driver in because that's a part of my job. And on the other hand I've followed every bit of advice I've gotten along the way.
While looking at this I also notice that it's surprisingly difficult to actually build any of this stuff - the MFD core can't be enabled directly, it's only available if you enable the V4L driver, and the core V4L build appears to be rather large adding a noticable amount of time to the build needed to get coverage of the CODECs. It'd be good if you could fix this to remove the dependency, I'd really expect the MFD to be able to build by itself.
My original plan was to have an MFD part with all the core functionality and then the child drivers as two different clients to the core. With that design it was possible to build the MFD with either, both or none of the children... But the media people wanted an empty MFD part and all basic functionality in the V4L2 driver. And the MFD driver was acked by Samuel the MFD maintainer.
The codec can already be built alone, I can't see what benefit would building the empty MFD driver offer. With the current structure nothing can actually be done without the V4L2 driver. But if there's something I don't get right now, I'll be happy make changes.
So we'll just wait until 2.6.38 is out and everything remains compilable... When 38 gets released the codec cannot be used with rest of the driver until this patch is applied, but that can be done when a suitable window opens, right?
*Ideally* we'd have this API change included as part of the MFD driver merge or included in 2.6.37 since otherwise we cause build issues. ASoC has already been sent to Linus for this merge window. As it is I guess we have to apply this and send a fix later.
OK, sounds good...
Cheers, Matti
On Fri, Jan 14, 2011 at 09:43:04AM +0200, Matti J. Aaltonen wrote:
On Thu, 2011-01-13 at 17:12 +0000, ext Mark Brown
This is something that should really have been brought up when making changes. It's really bad to just go and make other bits of the kernel fail to build.
I'm not exactly sure what you mean here... It's easy to say now - when looking back that - the whole driver should have been handled more as a whole. An ACK from you and an ACK from Samuel to the media guys etc...
If you're making an incompatible change in an API used by code that is already part of the kernel then you need to make sure that that code is updated as part of introducing the new API.
While looking at this I also notice that it's surprisingly difficult to actually build any of this stuff - the MFD core can't be enabled directly, it's only available if you enable the V4L driver, and the core V4L build appears to be rather large adding a noticable amount of time to the build needed to get coverage of the CODECs. It'd be good if you could fix this to remove the dependency, I'd really expect the MFD to be able to build by itself.
The codec can already be built alone, I can't see what benefit would building the empty MFD driver offer. With the current structure nothing can actually be done without the V4L2 driver. But if there's something I don't get right now, I'll be happy make changes.
As things stand the only way the CODEC driver can be built is if V4L is enabled, which like I say isn't a trivial build. This isn't ideal when trying to get build coverage of the CODEC drivers for work on the core, it adds noticable additional delay.
On Fri, 2011-01-14 at 12:22 +0000, ext Mark Brown wrote:
On Fri, Jan 14, 2011 at 09:43:04AM +0200, Matti J. Aaltonen wrote:
On Thu, 2011-01-13 at 17:12 +0000, ext Mark Brown
This is something that should really have been brought up when making changes. It's really bad to just go and make other bits of the kernel fail to build.
I'm not exactly sure what you mean here... It's easy to say now - when looking back that - the whole driver should have been handled more as a whole. An ACK from you and an ACK from Samuel to the media guys etc...
If you're making an incompatible change in an API used by code that is already part of the kernel then you need to make sure that that code is updated as part of introducing the new API.
OK, I agree, that's something I overlooked...
While looking at this I also notice that it's surprisingly difficult to actually build any of this stuff - the MFD core can't be enabled directly, it's only available if you enable the V4L driver, and the core V4L build appears to be rather large adding a noticable amount of time to the build needed to get coverage of the CODECs. It'd be good if you could fix this to remove the dependency, I'd really expect the MFD to be able to build by itself.
The codec can already be built alone, I can't see what benefit would building the empty MFD driver offer. With the current structure nothing can actually be done without the V4L2 driver. But if there's something I don't get right now, I'll be happy make changes.
As things stand the only way the CODEC driver can be built is if V4L is enabled, which like I say isn't a trivial build. This isn't ideal when trying to get build coverage of the CODEC drivers for work on the core, it adds noticable additional delay.
The codec can be compiled alone as the comment in sound/soc/codecs/Kconfig suggest:
help Normally ASoC codec drivers are only built if a machine driver which uses them is also built since they are only usable with a machine driver. Selecting this option will allow these drivers to be built without an explicit machine driver for test and development purposes.
And as I said, with my original design the core (MFD) could have been compiled (and used) with either child driver: the codec and the V4L2 part. A fact is that Mauro didn't accept that structure, he wanted to have all functionality (except for the audio) in the V4L2 driver.
And also the question is: what should be done now or next. If you mean that the dependence between V4L2 part and the core should be removed, that's easy to do but what's we gain with that? I would like to return to the original structure, but that doesn't seem to be possible?
On Mon, Jan 17, 2011 at 10:52:26AM +0200, Matti J. Aaltonen wrote:
On Fri, 2011-01-14 at 12:22 +0000, ext Mark Brown wrote:
As things stand the only way the CODEC driver can be built is if V4L is enabled, which like I say isn't a trivial build. This isn't ideal when trying to get build coverage of the CODEC drivers for work on the core, it adds noticable additional delay.
The codec can be compiled alone as the comment in sound/soc/codecs/Kconfig suggest:
help Normally ASoC codec drivers are only built if a machine driver which uses them is also built since they are only usable with a machine driver. Selecting this option will allow these drivers to be built without an explicit machine driver for test and development purposes.
I'm not entirely clear how that follows from the above? The issue here is primarily in terms of test building with SND_SOC_ALL_CODECS.
And as I said, with my original design the core (MFD) could have been compiled (and used) with either child driver: the codec and the V4L2 part. A fact is that Mauro didn't accept that structure, he wanted to have all functionality (except for the audio) in the V4L2 driver.
I don't particularly care if the resulting driver is useful but it should at least be possible to build the two subsystems independantly. If it's not even possible to do that then why is there a MFD driver in the first place?
And also the question is: what should be done now or next. If you mean that the dependence between V4L2 part and the core should be removed, that's easy to do but what's we gain with that? I would like to return to the original structure, but that doesn't seem to be possible?
It means we'd be able to get build coverage of each subsystem without having to enable the other, having to pick up only the core rather than an entire new subsystem.
On Mon, 2011-01-17 at 13:56 +0000, ext Mark Brown wrote:
On Mon, Jan 17, 2011 at 10:52:26AM +0200, Matti J. Aaltonen wrote:
On Fri, 2011-01-14 at 12:22 +0000, ext Mark Brown wrote:
help Normally ASoC codec drivers are only built if a machine driver which uses them is also built since they are only usable with a machine driver. Selecting this option will allow these drivers to be built without an explicit machine driver for test and development purposes.
I'm not entirely clear how that follows from the above? The issue here is primarily in terms of test building with SND_SOC_ALL_CODECS.
Would you be happy with the following change?
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 937517a..c2b39fa 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -43,7 +43,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_TWL6040 if TWL4030_CORE select SND_SOC_UDA134X select SND_SOC_UDA1380 if I2C - select SND_SOC_WL1273 if RADIO_WL1273 + select SND_SOC_WL1273 select SND_SOC_WM2000 if I2C select SND_SOC_WM8350 if MFD_WM8350 select SND_SOC_WM8400 if MFD_WM8400
That way the compilation of the codec would be independent of the rest of the driver...
And as I said, with my original design the core (MFD) could have been compiled (and used) with either child driver: the codec and the V4L2 part. A fact is that Mauro didn't accept that structure, he wanted to have all functionality (except for the audio) in the V4L2 driver.
I don't particularly care if the resulting driver is useful but it should at least be possible to build the two subsystems independantly. If it's not even possible to do that then why is there a MFD driver in the first place?
After I had to break the original design of functional core with add-on interfaces, I have also started question things... But actually I didn't remember the above Kconfic dependence to the V4L2 driver (I was mainly thinking about building a codec that can in some sense be run)...
And also the question is: what should be done now or next. If you mean that the dependence between V4L2 part and the core should be removed, that's easy to do but what's we gain with that? I would like to return to the original structure, but that doesn't seem to be possible?
It means we'd be able to get build coverage of each subsystem without having to enable the other, having to pick up only the core rather than an entire new subsystem.
With above change you only need to have the core header file in place and then the codec can be compiled.
On Mon, Jan 17, 2011 at 04:58:14PM +0200, Matti J. Aaltonen wrote:
On Mon, 2011-01-17 at 13:56 +0000, ext Mark Brown wrote:
select SND_SOC_WL1273 if RADIO_WL1273
select SND_SOC_WL1273 select SND_SOC_WM2000 if I2C select SND_SOC_WM8350 if MFD_WM8350 select SND_SOC_WM8400 if MFD_WM8400
That way the compilation of the codec would be independent of the rest of the driver...
Will the resulting code actually link if the MFD driver hasn't been built? Looks like it given that everything looks like hard coded vtable dereferences and there's no API from the MFD driver to its users. I'm really unsure what the MFD driver is supposed to do now, it'll only ever instantiate a single subdevice and offers no services to users - it seems better to just remove it, it looks like it's just creating needless complication.
I have to say it looks like the locking at best a bit odd, some of the APIs require that the CODEC driver lock a lock from the core by hand while others have no locking in the interface.
participants (3)
-
Liam Girdwood -
Mark Brown -
Matti J. Aaltonen