[PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Matti J. Aaltonen]

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__ */
-- 
1.6.1.3

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Mark Brown]

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.

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Liam Girdwood]

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.
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Matti J. Aaltonen]

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

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Mark Brown]

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 :(

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Matti J. Aaltonen]

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

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Mark Brown]

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.

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Matti J. Aaltonen]

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

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Mark Brown]

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.

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Matti J. Aaltonen]

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?

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Mark Brown]

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.

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Matti J. Aaltonen]

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.

Re: [PATCH] ASoC: WL1273 FM radio: Access I2C IO functions through pointers.

[Mark Brown]

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.