[alsa-devel] [SAMPLE-PATCH 0/x] ASoC: replace snd_soc_read/write
Hi Mark, Lars-Peter
These are part of "replace snd_soc_read/write to snd_soc_component_read/write" patch-set. Full-patch-set will be over 100 patches.
[0/x] - [x-1/x] : ASoC: use snd_soc_component_read/write on xxxx [x/x] : ASoC: remove snd_soc_component_read/write
[0/x] - [x-1/x] are almost same patches. so, I pickuped few of them for reviewing to avoid patch flood on ML.
If these review were OK, I will post full-patch-set, or send git-pull-request.
Main purpose of these patches are replace current snd_soc_read/write to snd_soc_component_read/write to remove codec related function.
Best regards --- Kuninori Morimoto
struct snd_soc_codec related function/feature will be merged into struct snd_soc_component. This patch replace current snd_soc_read/write to snd_soc_component_read/write.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/atmel/atmel-pdmic.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/atmel/atmel-pdmic.c b/sound/soc/atmel/atmel-pdmic.c index c917df7..f04ee98 100644 --- a/sound/soc/atmel/atmel-pdmic.c +++ b/sound/soc/atmel/atmel-pdmic.c @@ -289,14 +289,16 @@ static int pdmic_get_mic_volsw(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol); + struct snd_soc_component *component = &codec->component; unsigned int dgain_val, scale_val; + unsigned int val; int i;
- dgain_val = (snd_soc_read(codec, PDMIC_DSPR1) & PDMIC_DSPR1_DGAIN_MASK) - >> PDMIC_DSPR1_DGAIN_SHIFT; + snd_soc_component_read(component, PDMIC_DSPR1, &val); + dgain_val = (val & PDMIC_DSPR1_DGAIN_MASK) >> PDMIC_DSPR1_DGAIN_SHIFT;
- scale_val = (snd_soc_read(codec, PDMIC_DSPR0) & PDMIC_DSPR0_SCALE_MASK) - >> PDMIC_DSPR0_SCALE_SHIFT; + snd_soc_component_read(component, PDMIC_DSPR0, &val); + scale_val = (val & PDMIC_DSPR0_SCALE_MASK) >> PDMIC_DSPR0_SCALE_SHIFT;
for (i = 0; i < ARRAY_SIZE(mic_gain_table); i++) { if ((mic_gain_table[i].dgain == dgain_val) &&
struct snd_soc_codec related function/feature will be merged into struct snd_soc_component. This patch replace current snd_soc_read/write to snd_soc_component_read/write.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/codecs/88pm860x-codec.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/sound/soc/codecs/88pm860x-codec.c b/sound/soc/codecs/88pm860x-codec.c index b013a4c..25d3c6d 100644 --- a/sound/soc/codecs/88pm860x-codec.c +++ b/sound/soc/codecs/88pm860x-codec.c @@ -273,14 +273,21 @@ static int snd_soc_get_volsw_2r_st(struct snd_kcontrol *kcontrol, struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol); + struct snd_soc_component *component = &codec->component; + unsigned int tmp; unsigned int reg = mc->reg; unsigned int reg2 = mc->rreg; int val[2], val2[2], i;
- val[0] = snd_soc_read(codec, reg) & 0x3f; - val[1] = (snd_soc_read(codec, PM860X_SIDETONE_SHIFT) >> 4) & 0xf; - val2[0] = snd_soc_read(codec, reg2) & 0x3f; - val2[1] = (snd_soc_read(codec, PM860X_SIDETONE_SHIFT)) & 0xf; + snd_soc_component_read(component, reg, &tmp); + val[0] = tmp & 0x3f; + + snd_soc_component_read(component, reg2, &tmp); + val2[0] = tmp & 0x3f; + + snd_soc_component_read(component, PM860X_SIDETONE_SHIFT, &tmp); + val[1] = (tmp >> 4) & 0xf; + val2[1] = tmp & 0xf;
for (i = 0; i < ARRAY_SIZE(st_table); i++) { if ((st_table[i].m == val[0]) && (st_table[i].n == val[1])) @@ -288,6 +295,7 @@ static int snd_soc_get_volsw_2r_st(struct snd_kcontrol *kcontrol, if ((st_table[i].m == val2[0]) && (st_table[i].n == val2[1])) ucontrol->value.integer.value[1] = i; } + return 0; }
@@ -330,14 +338,19 @@ static int snd_soc_get_volsw_2r_out(struct snd_kcontrol *kcontrol, struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol); + struct snd_soc_component *component = &codec->component; unsigned int reg = mc->reg; unsigned int reg2 = mc->rreg; unsigned int shift = mc->shift; int max = mc->max, val, val2; unsigned int mask = (1 << fls(max)) - 1;
- val = snd_soc_read(codec, reg) >> shift; - val2 = snd_soc_read(codec, reg2) >> shift; + snd_soc_component_read(component, reg, &val); + val >>= shift; + + snd_soc_component_read(component, reg2, &val2); + val2 >>= shift; + ucontrol->value.integer.value[0] = (max - val) & mask; ucontrol->value.integer.value[1] = (max - val2) & mask;
@@ -400,8 +413,9 @@ static int pm860x_dac_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm); + struct snd_soc_component *component = &codec->component; unsigned int dac = 0; - int data; + unsigned int data;
if (!strcmp(w->name, "Left DAC")) dac = DAC_LEFT; @@ -429,11 +443,11 @@ static int pm860x_dac_event(struct snd_soc_dapm_widget *w, snd_soc_update_bits(codec, PM860X_EAR_CTRL_2, RSYNC_CHANGE, RSYNC_CHANGE); /* update dac */ - data = snd_soc_read(codec, PM860X_DAC_EN_2); + snd_soc_component_read(component, PM860X_DAC_EN_2, &data); data &= ~dac; if (!(data & (DAC_LEFT | DAC_RIGHT))) data &= ~MODULATOR; - snd_soc_write(codec, PM860X_DAC_EN_2, data); + snd_soc_component_write(component, PM860X_DAC_EN_2, data); } break; }
struct snd_soc_codec related function/feature will be merged into struct snd_soc_component. This patch remove snd_soc_read/write
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 5 ----- sound/soc/soc-io.c | 20 -------------------- 2 files changed, 25 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 4f1c784..9f0094d 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1425,11 +1425,6 @@ static inline struct snd_soc_codec *snd_soc_dapm_kcontrol_codec( return snd_soc_dapm_to_codec(snd_soc_dapm_kcontrol_dapm(kcontrol)); }
-/* codec IO */ -unsigned int snd_soc_read(struct snd_soc_codec *codec, unsigned int reg); -int snd_soc_write(struct snd_soc_codec *codec, unsigned int reg, - unsigned int val); - /** * snd_soc_cache_sync() - Sync the register cache with the hardware * @codec: CODEC to sync diff --git a/sound/soc/soc-io.c b/sound/soc/soc-io.c index 9b39390..dbd3caa 100644 --- a/sound/soc/soc-io.c +++ b/sound/soc/soc-io.c @@ -193,26 +193,6 @@ int snd_soc_component_test_bits(struct snd_soc_component *component, } EXPORT_SYMBOL_GPL(snd_soc_component_test_bits);
-unsigned int snd_soc_read(struct snd_soc_codec *codec, unsigned int reg) -{ - unsigned int val; - int ret; - - ret = snd_soc_component_read(&codec->component, reg, &val); - if (ret < 0) - return -1; - - return val; -} -EXPORT_SYMBOL_GPL(snd_soc_read); - -int snd_soc_write(struct snd_soc_codec *codec, unsigned int reg, - unsigned int val) -{ - return snd_soc_component_write(&codec->component, reg, val); -} -EXPORT_SYMBOL_GPL(snd_soc_write); - /** * snd_soc_update_bits - update codec register bits * @codec: audio codec
On Wed, 19 Oct 2016 07:50:08 +0200, Kuninori Morimoto wrote:
Hi Mark, Lars-Peter
These are part of "replace snd_soc_read/write to snd_soc_component_read/write" patch-set. Full-patch-set will be over 100 patches.
[0/x] - [x-1/x] : ASoC: use snd_soc_component_read/write on xxxx [x/x] : ASoC: remove snd_soc_component_read/write
[0/x] - [x-1/x] are almost same patches. so, I pickuped few of them for reviewing to avoid patch flood on ML.
If these review were OK, I will post full-patch-set, or send git-pull-request.
Main purpose of these patches are replace current snd_soc_read/write to snd_soc_component_read/write to remove codec related function.
I really don't see any big merits by these changes. What's wrong with keeping as is? The driver is accessing the codec register, after all.
For keeping consistency, replacing a few exceptions would be OK. But replacing hundreds of callers needs a proper justification to do it so.
thanks,
Takashi
Hi Takashi-san
Thank you for your feedback
These are part of "replace snd_soc_read/write to snd_soc_component_read/write" patch-set. Full-patch-set will be over 100 patches.
[0/x] - [x-1/x] : ASoC: use snd_soc_component_read/write on xxxx [x/x] : ASoC: remove snd_soc_component_read/write
[0/x] - [x-1/x] are almost same patches. so, I pickuped few of them for reviewing to avoid patch flood on ML.
If these review were OK, I will post full-patch-set, or send git-pull-request.
Main purpose of these patches are replace current snd_soc_read/write to snd_soc_component_read/write to remove codec related function.
I really don't see any big merits by these changes. What's wrong with keeping as is? The driver is accessing the codec register, after all.
For keeping consistency, replacing a few exceptions would be OK. But replacing hundreds of callers needs a proper justification to do it so.
Now, ALSA SoC is planning to remove "struct snd_soc_codec" and "struct snd_soc_platform", and merge these into "struct snd_soc_component". New ALSA SoC will be based on "component", and additional new feature (= bridge ?).
As 1st phase, I would like to cleanup these. my 1st step is removing codec related read/write (= use component read/write), and 2nd step is remove codec probe/remove (= use component probe/remove). and 3rd, 4th... I don't know how many steps are exist.
In 2nd phase, ALSA SoC will support more modern design HW as normal style. (Current ALSA SoC needs complex feature for it, like DPCM)
Lars-Peter is considering about 2nd phase, but it will be after 1st phase.
On 10/19/2016 09:43 AM, Kuninori Morimoto wrote:
Hi Takashi-san
Thank you for your feedback
These are part of "replace snd_soc_read/write to snd_soc_component_read/write" patch-set. Full-patch-set will be over 100 patches.
[0/x] - [x-1/x] : ASoC: use snd_soc_component_read/write on xxxx [x/x] : ASoC: remove snd_soc_component_read/write
[0/x] - [x-1/x] are almost same patches. so, I pickuped few of them for reviewing to avoid patch flood on ML.
If these review were OK, I will post full-patch-set, or send git-pull-request.
Main purpose of these patches are replace current snd_soc_read/write to snd_soc_component_read/write to remove codec related function.
I really don't see any big merits by these changes. What's wrong with keeping as is? The driver is accessing the codec register, after all.
For keeping consistency, replacing a few exceptions would be OK. But replacing hundreds of callers needs a proper justification to do it so.
Now, ALSA SoC is planning to remove "struct snd_soc_codec" and "struct snd_soc_platform", and merge these into "struct snd_soc_component". New ALSA SoC will be based on "component", and additional new feature (= bridge ?).
As 1st phase, I would like to cleanup these. my 1st step is removing codec related read/write (= use component read/write), and 2nd step is remove codec probe/remove (= use component probe/remove). and 3rd, 4th... I don't know how many steps are exist.
I agree with Takashi, in my opinion this is not the right change at this point in time. snd_soc_{write,read}() are not problematic they are simple wrapper functions around snd_soc_component_{write,read}() that make it more convenient to use them in a CODEC driver. Same for the CODEC specific probe()/remove() callbacks.
And while we are phasing out the concept of differentiating between different types of components, since the hardware landscape has changed so that there is no longer a really meaningful differentiating factor, snd_soc_codec and snd_soc_codec drivers will still be around for a while.
The problematic bits are in the ASoC core where we have code that needs to know whether a component is a CODEC or not. These are the issue we need to resolve before we can think about converting more driver snd_soc_codec to snd_soc_component.
Doing these partial conversions only leaves us with a lot of drivers that are more convoluted than they need to be. In my opinion a driver should be converted in one go, making it a complete component driver. But not leave it partially component and partially CODEC. We've done this for the drivers where it is possible, e.g. see [1].
But now it is time to fix the hard problems in the core first before we start converting other drivers.
- Lars
[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/sound/...
Hi Lars
The problematic bits are in the ASoC core where we have code that needs to know whether a component is a CODEC or not. These are the issue we need to resolve before we can think about converting more driver snd_soc_codec to snd_soc_component.
Yeah, I agree about this, but
I agree with Takashi, in my opinion this is not the right change at this point in time. snd_soc_{write,read}() are not problematic they are simple wrapper functions around snd_soc_component_{write,read}() that make it more convenient to use them in a CODEC driver. Same for the CODEC specific probe()/remove() callbacks.
And while we are phasing out the concept of differentiating between different types of components, since the hardware landscape has changed so that there is no longer a really meaningful differentiating factor, snd_soc_codec and snd_soc_codec drivers will still be around for a while.
(snip)
Doing these partial conversions only leaves us with a lot of drivers that are more convoluted than they need to be. In my opinion a driver should be converted in one go, making it a complete component driver. But not leave it partially component and partially CODEC. We've done this for the drivers where it is possible, e.g. see [1].
Your "converted in one go" includes many conversion in 1 patch, right ? But it will be difficult to review, and is big risk.
I talked this topic with Mark in Berlin, and if my understand was correct, he agreed about step-by-step approach, because current driver is very complex. many conversion in 1 patch x 100 driver is very big risk IMO.
Best regards --- Kuninori Morimoto
On 10/19/2016 10:19 AM, Kuninori Morimoto wrote:
Hi Lars
The problematic bits are in the ASoC core where we have code that needs to know whether a component is a CODEC or not. These are the issue we need to resolve before we can think about converting more driver snd_soc_codec to snd_soc_component.
Yeah, I agree about this, but
I agree with Takashi, in my opinion this is not the right change at this point in time. snd_soc_{write,read}() are not problematic they are simple wrapper functions around snd_soc_component_{write,read}() that make it more convenient to use them in a CODEC driver. Same for the CODEC specific probe()/remove() callbacks.
And while we are phasing out the concept of differentiating between different types of components, since the hardware landscape has changed so that there is no longer a really meaningful differentiating factor, snd_soc_codec and snd_soc_codec drivers will still be around for a while.
(snip)
Doing these partial conversions only leaves us with a lot of drivers that are more convoluted than they need to be. In my opinion a driver should be converted in one go, making it a complete component driver. But not leave it partially component and partially CODEC. We've done this for the drivers where it is possible, e.g. see [1].
Your "converted in one go" includes many conversion in 1 patch, right ? But it will be difficult to review, and is big risk.
I talked this topic with Mark in Berlin, and if my understand was correct, he agreed about step-by-step approach, because current driver is very complex. many conversion in 1 patch x 100 driver is very big risk IMO.
It does not have to be in one patch, but it should be in one series. A patch series should result in a overall improvement of the code. Just changing snd_soc_read() to snd_soc_component_read() does not improve the code, it makes it slightly worse since as you can see from the diffstat of your patches it requires more boilerplate code.
So such a change needs to be paired with other changes so that the overall result of the patch series is a improvement.
You said in your earlier mail "and 3rd, 4th... I don't know how many steps are exist.". We need to figure those steps out before we start converting a driver, otherwise we'll be left with lots of frankenstein drivers that are part snd_soc_component, part snd_soc_codec.
participants (3)
-
Kuninori Morimoto -
Lars-Peter Clausen -
Takashi Iwai