[PATCH] ASoC: io: Remove hw_read() operation

[PATCH] ASoC: io: Remove hw_read() operation

[Mark Brown]

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);
 	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:
-- 
1.9.0

Re: [PATCH] ASoC: io: Remove hw_read() operation

[Lars-Peter Clausen]

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

Re: [PATCH] ASoC: io: Remove hw_read() operation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 773 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: io: Remove hw_read() operation

[Lars-Peter Clausen]

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

Re: [PATCH] ASoC: io: Remove hw_read() operation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1512 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: io: Remove hw_read() operation

[Lars-Peter Clausen]

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

Re: [PATCH] ASoC: io: Remove hw_read() operation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 472 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]