| Message ID | 1393924251-6754-1-git-send-email-broonie@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 03/04/2014 10:10 AM, Mark Brown wrote: > From: Mark Brown <broonie@linaro.org> > > We now no longer have any users of hw_read() in the kernel so remove the > code in order to prevent any new users being added. Users should be using > regmap. > > Signed-off-by: Mark Brown <broonie@linaro.org> > --- > include/sound/soc.h | 1 - > sound/soc/soc-io.c | 13 ------------- > 2 files changed, 14 deletions(-) > > diff --git a/include/sound/soc.h b/include/sound/soc.h > index 56c4c71..dc36331 100644 > --- a/include/sound/soc.h > +++ b/include/sound/soc.h > @@ -706,7 +706,6 @@ struct snd_soc_codec { > /* codec IO */ > void *control_data; /* codec control (i2c/3wire) data */ > hw_write_t hw_write; > - unsigned int (*hw_read)(struct snd_soc_codec *, unsigned int); This part is OK. > unsigned int (*read)(struct snd_soc_codec *, unsigned int); > int (*write)(struct snd_soc_codec *, unsigned int, unsigned int); > void *reg_cache; > diff --git a/sound/soc/soc-io.c b/sound/soc/soc-io.c > index c3b6a0a..3c1de9c 100644 > --- a/sound/soc/soc-io.c > +++ b/sound/soc/soc-io.c > @@ -26,18 +26,6 @@ static int hw_write(struct snd_soc_codec *codec, unsigned int reg, > return regmap_write(codec->control_data, reg, value); > } > > -static unsigned int hw_read(struct snd_soc_codec *codec, unsigned int reg) > -{ > - int ret; > - unsigned int val; > - > - ret = regmap_read(codec->control_data, reg, &val); > - if (ret == 0) > - return val; > - else > - return -1; > -} > - > /** > * snd_soc_codec_set_cache_io: Set up standard I/O functions. > * > @@ -64,7 +52,6 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, > int ret; > > codec->write = hw_write; > - codec->read = hw_read; But this part will break snd_soc_read() and with it basically every driver that has any controls or DAPM widgets. I do have a set of patches that moves IO to the component level that will clean this all up quite a bit. I'm just waiting for Xiubo Li snd_soc_codec_set_cache_io() cleanup to be merged first, since it will make things a bit simpler. > > switch (control) { > case SND_SOC_REGMAP: >
On Tue, Mar 04, 2014 at 10:20:16AM +0100, Lars-Peter Clausen wrote: > > codec->write = hw_write; > >- codec->read = hw_read; > But this part will break snd_soc_read() and with it basically every > driver that has any controls or DAPM widgets. Bah, so it does. > I do have a set of patches that moves IO to the component level that > will clean this all up quite a bit. I'm just waiting for Xiubo Li > snd_soc_codec_set_cache_io() cleanup to be merged first, since it > will make things a bit simpler. That seems to be going really slowly... in any case the real fix here is to get all the users converted over to regmap. We could also just do something similar to what we've done with update_bits() (where we did it for locking) which is going to be much quicker.
On 03/04/2014 10:27 AM, Mark Brown wrote: > On Tue, Mar 04, 2014 at 10:20:16AM +0100, Lars-Peter Clausen wrote: > >>> codec->write = hw_write; >>> - codec->read = hw_read; > >> But this part will break snd_soc_read() and with it basically every >> driver that has any controls or DAPM widgets. > > Bah, so it does. > >> I do have a set of patches that moves IO to the component level that >> will clean this all up quite a bit. I'm just waiting for Xiubo Li >> snd_soc_codec_set_cache_io() cleanup to be merged first, since it >> will make things a bit simpler. > > That seems to be going really slowly... Yea, but we are not in a hurry. My plan was to get all the small independent driver cleanups necessary for componetization merged for 3.15 and then for 3.16 do the componetnization in the ASoC core. > in any case the real fix here > is to get all the users converted over to regmap. We could also just > do something similar to what we've done with update_bits() (where we did > it for locking) which is going to be much quicker. > So what the patchset does is add a regmap pointer to the component struct. For now it also adds snd_soc_component_{read,write,update_bits, update_bits_async} wrapper functions which have the same signature as the matching regmap function. Internally it does check if the regmap field of the component is set, if yes, just forwards the call to the matching regmap function. If not it falls back to the read, write callbacks setup by CODEC or platform drivers. Once every driver has been converted to regmap we can remove the fallback and make the function static inline. - Lars
On Tue, Mar 04, 2014 at 10:45:47AM +0100, Lars-Peter Clausen wrote: > Yea, but we are not in a hurry. My plan was to get all the small YM "yes". :) > independent driver cleanups necessary for componetization merged for > 3.15 and then for 3.16 do the componetnization in the ASoC core. I'm keen to avoid getting lots of people sitting on unmerged patches, it's a risk source due to interaction issues. > So what the patchset does is add a regmap pointer to the component > struct. For now it also adds > snd_soc_component_{read,write,update_bits, > update_bits_async} wrapper functions which have the same signature > as the matching regmap function. Internally it does check if the > regmap field of the component is set, if yes, just forwards the call > to the matching regmap function. If not it falls back to the read, > write callbacks setup by CODEC or platform drivers. Once every > driver has been converted to regmap we can remove the fallback and > make the function static inline. Yes, that's what I'd expect and roughly what I was suggesting doing anyway inside the existing implementation (ie, not the new function but making the edit inside the existing functions). I'm not sure I like the _component_ naming since it's getting a bit verbose, but on the other hand it probably should only be the core using this stuff (the drivers ought to be able to find their own regmaps, or at least doing the same lookup) so probably it doesn't matter terribly much.
On 03/04/2014 11:11 AM, Mark Brown wrote: > On Tue, Mar 04, 2014 at 10:45:47AM +0100, Lars-Peter Clausen wrote: > >> Yea, but we are not in a hurry. My plan was to get all the small > > YM "yes". :) > >> independent driver cleanups necessary for componetization merged for >> 3.15 and then for 3.16 do the componetnization in the ASoC core. > > I'm keen to avoid getting lots of people sitting on unmerged patches, > it's a risk source due to interaction issues. > >> So what the patchset does is add a regmap pointer to the component >> struct. For now it also adds >> snd_soc_component_{read,write,update_bits, >> update_bits_async} wrapper functions which have the same signature >> as the matching regmap function. Internally it does check if the >> regmap field of the component is set, if yes, just forwards the call >> to the matching regmap function. If not it falls back to the read, >> write callbacks setup by CODEC or platform drivers. Once every >> driver has been converted to regmap we can remove the fallback and >> make the function static inline. > > Yes, that's what I'd expect and roughly what I was suggesting doing > anyway inside the existing implementation (ie, not the new function > but making the edit inside the existing functions). > > I'm not sure I like the _component_ naming since it's getting a bit > verbose, but on the other hand it probably should only be the core > using this stuff (the drivers ought to be able to find their own > regmaps, or at least doing the same lookup) so probably it doesn't > matter terribly much. > Yes, it's only used in the core. There is still snd_soc_write(codec) and friends which wraps them like snd_soc_component_write(&codec->component). But on the long run I'd like to see all snd_soc_writes be removed from driver code. - Lars
On Tue, Mar 04, 2014 at 12:25:59PM +0100, Lars-Peter Clausen wrote: > Yes, it's only used in the core. There is still snd_soc_write(codec) > and friends which wraps them like > snd_soc_component_write(&codec->component). But on the long run I'd > like to see all snd_soc_writes be removed from driver code. I'm not entirely sure there - the repeated indirections we have aren't always the most pretty bit of the code we have, helpers wrapping the lookups can be useful.
diff --git a/include/sound/soc.h b/include/sound/soc.h index 56c4c71..dc36331 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -706,7 +706,6 @@ struct snd_soc_codec { /* codec IO */ void *control_data; /* codec control (i2c/3wire) data */ hw_write_t hw_write; - unsigned int (*hw_read)(struct snd_soc_codec *, unsigned int); unsigned int (*read)(struct snd_soc_codec *, unsigned int); int (*write)(struct snd_soc_codec *, unsigned int, unsigned int); void *reg_cache; diff --git a/sound/soc/soc-io.c b/sound/soc/soc-io.c index c3b6a0a..3c1de9c 100644 --- a/sound/soc/soc-io.c +++ b/sound/soc/soc-io.c @@ -26,18 +26,6 @@ static int hw_write(struct snd_soc_codec *codec, unsigned int reg, return regmap_write(codec->control_data, reg, value); } -static unsigned int hw_read(struct snd_soc_codec *codec, unsigned int reg) -{ - int ret; - unsigned int val; - - ret = regmap_read(codec->control_data, reg, &val); - if (ret == 0) - return val; - else - return -1; -} - /** * snd_soc_codec_set_cache_io: Set up standard I/O functions. * @@ -64,7 +52,6 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, int ret; codec->write = hw_write; - codec->read = hw_read; switch (control) { case SND_SOC_REGMAP: