sound/soc/amd/acp/acp-legacy-common.c | 3 ++- sound/soc/amd/acp/acp-pdm.c | 3 ++- sound/soc/amd/acp/amd.h | 6 +++++- 3 files changed, 9 insertions(+), 3 deletions(-)
Adjust pdm dimc gain value using module param.
In case of regressions for any users that the new pdm_gain value is
too high and for additional debugging, introduce a module parameter
that would let them configure it.
This parameter should be removed in the future:
* If it's determined that the parameter is not needed, just hardcode
the correct value as before
* If users do end up using it to debug and report different values
we should introduce a config knob that can have policy set by ucm.
Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com>
---
sound/soc/amd/acp/acp-legacy-common.c | 3 ++-
sound/soc/amd/acp/acp-pdm.c | 3 ++-
sound/soc/amd/acp/amd.h | 6 +++++-
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/sound/soc/amd/acp/acp-legacy-common.c b/sound/soc/amd/acp/acp-legacy-common.c
index 57982d057c3a..dd804fb95790 100644
--- a/sound/soc/amd/acp/acp-legacy-common.c
+++ b/sound/soc/amd/acp/acp-legacy-common.c
@@ -173,7 +173,8 @@ static void set_acp_pdm_clk(struct snd_pcm_substream *substream,
/* Enable default ACP PDM clk */
writel(PDM_CLK_FREQ_MASK, chip->base + ACP_WOV_CLK_CTRL);
pdm_ctrl = readl(chip->base + ACP_WOV_MISC_CTRL);
- pdm_ctrl |= PDM_MISC_CTRL_MASK;
+ pdm_ctrl &= ~ACP_WOV_GAIN_CONTROL;
+ pdm_ctrl |= FIELD_PREP(ACP_WOV_GAIN_CONTROL, clamp(pdm_gain, 0, 3));
writel(pdm_ctrl, chip->base + ACP_WOV_MISC_CTRL);
set_acp_pdm_ring_buffer(substream, dai);
}
diff --git a/sound/soc/amd/acp/acp-pdm.c b/sound/soc/amd/acp/acp-pdm.c
index 1bfc34c2aa53..ffb622a7a69a 100644
--- a/sound/soc/amd/acp/acp-pdm.c
+++ b/sound/soc/amd/acp/acp-pdm.c
@@ -38,7 +38,8 @@ static int acp_dmic_prepare(struct snd_pcm_substream *substream,
/* Enable default DMIC clk */
writel(PDM_CLK_FREQ_MASK, chip->base + ACP_WOV_CLK_CTRL);
dmic_ctrl = readl(chip->base + ACP_WOV_MISC_CTRL);
- dmic_ctrl |= PDM_MISC_CTRL_MASK;
+ dmic_ctrl &= ~ACP_WOV_GAIN_CONTROL;
+ dmic_ctrl |= FIELD_PREP(ACP_WOV_GAIN_CONTROL, clamp(pdm_gain, 0, 3));
writel(dmic_ctrl, chip->base + ACP_WOV_MISC_CTRL);
period_bytes = frames_to_bytes(substream->runtime,
diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h
index cb8d97122f95..f2567e06ccd3 100644
--- a/sound/soc/amd/acp/amd.h
+++ b/sound/soc/amd/acp/amd.h
@@ -130,7 +130,7 @@
#define PDM_DMA_INTR_MASK 0x10000
#define PDM_DEC_64 0x2
#define PDM_CLK_FREQ_MASK 0x07
-#define PDM_MISC_CTRL_MASK 0x10
+#define ACP_WOV_GAIN_CONTROL GENMASK(4, 3)
#define PDM_ENABLE 0x01
#define PDM_DISABLE 0x00
#define DMA_EN_MASK 0x02
@@ -138,6 +138,10 @@
#define PDM_TIMEOUT 1000
#define ACP_REGION2_OFFSET 0x02000000
+static int pdm_gain = 3;
+module_param(pdm_gain, int, 0644);
+MODULE_PARM_DESC(pdm_gain, "Gain control (0-3)");
+
struct acp_chip_info {
char *name; /* Platform name */
struct resource *res;
--
2.43.0On Mon, Jul 28, 2025 at 03:12:27PM +0530, Venkata Prasad Potturu wrote: > Adjust pdm dimc gain value using module param. > In case of regressions for any users that the new pdm_gain value is > too high and for additional debugging, introduce a module parameter > that would let them configure it. > > This parameter should be removed in the future: > * If it's determined that the parameter is not needed, just hardcode > the correct value as before > * If users do end up using it to debug and report different values > we should introduce a config knob that can have policy set by ucm. Note, you can not break a user/kernel api like this once you introduce it, so be VERY careful (yet another reason for it to NOT be a module parameter...)
On Mon, Jul 28, 2025 at 03:12:27PM +0530, Venkata Prasad Potturu wrote: > Adjust pdm dimc gain value using module param. > In case of regressions for any users that the new pdm_gain value is > too high and for additional debugging, introduce a module parameter > that would let them configure it. > > This parameter should be removed in the future: > * If it's determined that the parameter is not needed, just hardcode > the correct value as before > * If users do end up using it to debug and report different values > we should introduce a config knob that can have policy set by ucm. > > Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com> > --- > sound/soc/amd/acp/acp-legacy-common.c | 3 ++- > sound/soc/amd/acp/acp-pdm.c | 3 ++- > sound/soc/amd/acp/amd.h | 6 +++++- > 3 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/amd/acp/acp-legacy-common.c b/sound/soc/amd/acp/acp-legacy-common.c > index 57982d057c3a..dd804fb95790 100644 > --- a/sound/soc/amd/acp/acp-legacy-common.c > +++ b/sound/soc/amd/acp/acp-legacy-common.c > @@ -173,7 +173,8 @@ static void set_acp_pdm_clk(struct snd_pcm_substream *substream, > /* Enable default ACP PDM clk */ > writel(PDM_CLK_FREQ_MASK, chip->base + ACP_WOV_CLK_CTRL); > pdm_ctrl = readl(chip->base + ACP_WOV_MISC_CTRL); > - pdm_ctrl |= PDM_MISC_CTRL_MASK; > + pdm_ctrl &= ~ACP_WOV_GAIN_CONTROL; > + pdm_ctrl |= FIELD_PREP(ACP_WOV_GAIN_CONTROL, clamp(pdm_gain, 0, 3)); > writel(pdm_ctrl, chip->base + ACP_WOV_MISC_CTRL); > set_acp_pdm_ring_buffer(substream, dai); > } > diff --git a/sound/soc/amd/acp/acp-pdm.c b/sound/soc/amd/acp/acp-pdm.c > index 1bfc34c2aa53..ffb622a7a69a 100644 > --- a/sound/soc/amd/acp/acp-pdm.c > +++ b/sound/soc/amd/acp/acp-pdm.c > @@ -38,7 +38,8 @@ static int acp_dmic_prepare(struct snd_pcm_substream *substream, > /* Enable default DMIC clk */ > writel(PDM_CLK_FREQ_MASK, chip->base + ACP_WOV_CLK_CTRL); > dmic_ctrl = readl(chip->base + ACP_WOV_MISC_CTRL); > - dmic_ctrl |= PDM_MISC_CTRL_MASK; > + dmic_ctrl &= ~ACP_WOV_GAIN_CONTROL; > + dmic_ctrl |= FIELD_PREP(ACP_WOV_GAIN_CONTROL, clamp(pdm_gain, 0, 3)); > writel(dmic_ctrl, chip->base + ACP_WOV_MISC_CTRL); > > period_bytes = frames_to_bytes(substream->runtime, > diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h > index cb8d97122f95..f2567e06ccd3 100644 > --- a/sound/soc/amd/acp/amd.h > +++ b/sound/soc/amd/acp/amd.h > @@ -130,7 +130,7 @@ > #define PDM_DMA_INTR_MASK 0x10000 > #define PDM_DEC_64 0x2 > #define PDM_CLK_FREQ_MASK 0x07 > -#define PDM_MISC_CTRL_MASK 0x10 > +#define ACP_WOV_GAIN_CONTROL GENMASK(4, 3) > #define PDM_ENABLE 0x01 > #define PDM_DISABLE 0x00 > #define DMA_EN_MASK 0x02 > @@ -138,6 +138,10 @@ > #define PDM_TIMEOUT 1000 > #define ACP_REGION2_OFFSET 0x02000000 > > +static int pdm_gain = 3; > +module_param(pdm_gain, int, 0644); > +MODULE_PARM_DESC(pdm_gain, "Gain control (0-3)"); This is not the 1990's, please do not add new module parameters, it will not work for modern systems and is a horrible thing to attempt to support over time :( Please do this properly, with a per-device setting. thanks, greg k-h
On 7/28/2025 4:22 PM, Greg KH wrote: > On Mon, Jul 28, 2025 at 03:12:27PM +0530, Venkata Prasad Potturu wrote: >> Adjust pdm dimc gain value using module param. >> In case of regressions for any users that the new pdm_gain value is >> too high and for additional debugging, introduce a module parameter >> that would let them configure it. >> >> This parameter should be removed in the future: >> * If it's determined that the parameter is not needed, just hardcode >> the correct value as before >> * If users do end up using it to debug and report different values >> we should introduce a config knob that can have policy set by ucm. >> >> Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com> >> --- >> sound/soc/amd/acp/acp-legacy-common.c | 3 ++- >> sound/soc/amd/acp/acp-pdm.c | 3 ++- >> sound/soc/amd/acp/amd.h | 6 +++++- >> 3 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/sound/soc/amd/acp/acp-legacy-common.c b/sound/soc/amd/acp/acp-legacy-common.c >> index 57982d057c3a..dd804fb95790 100644 >> --- a/sound/soc/amd/acp/acp-legacy-common.c >> +++ b/sound/soc/amd/acp/acp-legacy-common.c >> @@ -173,7 +173,8 @@ static void set_acp_pdm_clk(struct snd_pcm_substream *substream, >> /* Enable default ACP PDM clk */ >> writel(PDM_CLK_FREQ_MASK, chip->base + ACP_WOV_CLK_CTRL); >> pdm_ctrl = readl(chip->base + ACP_WOV_MISC_CTRL); >> - pdm_ctrl |= PDM_MISC_CTRL_MASK; >> + pdm_ctrl &= ~ACP_WOV_GAIN_CONTROL; >> + pdm_ctrl |= FIELD_PREP(ACP_WOV_GAIN_CONTROL, clamp(pdm_gain, 0, 3)); >> writel(pdm_ctrl, chip->base + ACP_WOV_MISC_CTRL); >> set_acp_pdm_ring_buffer(substream, dai); >> } >> diff --git a/sound/soc/amd/acp/acp-pdm.c b/sound/soc/amd/acp/acp-pdm.c >> index 1bfc34c2aa53..ffb622a7a69a 100644 >> --- a/sound/soc/amd/acp/acp-pdm.c >> +++ b/sound/soc/amd/acp/acp-pdm.c >> @@ -38,7 +38,8 @@ static int acp_dmic_prepare(struct snd_pcm_substream *substream, >> /* Enable default DMIC clk */ >> writel(PDM_CLK_FREQ_MASK, chip->base + ACP_WOV_CLK_CTRL); >> dmic_ctrl = readl(chip->base + ACP_WOV_MISC_CTRL); >> - dmic_ctrl |= PDM_MISC_CTRL_MASK; >> + dmic_ctrl &= ~ACP_WOV_GAIN_CONTROL; >> + dmic_ctrl |= FIELD_PREP(ACP_WOV_GAIN_CONTROL, clamp(pdm_gain, 0, 3)); >> writel(dmic_ctrl, chip->base + ACP_WOV_MISC_CTRL); >> >> period_bytes = frames_to_bytes(substream->runtime, >> diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h >> index cb8d97122f95..f2567e06ccd3 100644 >> --- a/sound/soc/amd/acp/amd.h >> +++ b/sound/soc/amd/acp/amd.h >> @@ -130,7 +130,7 @@ >> #define PDM_DMA_INTR_MASK 0x10000 >> #define PDM_DEC_64 0x2 >> #define PDM_CLK_FREQ_MASK 0x07 >> -#define PDM_MISC_CTRL_MASK 0x10 >> +#define ACP_WOV_GAIN_CONTROL GENMASK(4, 3) >> #define PDM_ENABLE 0x01 >> #define PDM_DISABLE 0x00 >> #define DMA_EN_MASK 0x02 >> @@ -138,6 +138,10 @@ >> #define PDM_TIMEOUT 1000 >> #define ACP_REGION2_OFFSET 0x02000000 >> >> +static int pdm_gain = 3; >> +module_param(pdm_gain, int, 0644); >> +MODULE_PARM_DESC(pdm_gain, "Gain control (0-3)"); > > This is not the 1990's, please do not add new module parameters, it will > not work for modern systems and is a horrible thing to attempt to > support over time :( > > Please do this properly, with a per-device setting. > > thanks, > > greg k-h As the main purpose for this parameter is for being able to tune whether the property is correct, how about adding a debugfs file instead? AFAICT it should just be a single register write, so debugfs to read current value and write the debugging value seems pretty straightforward.
On Tue, Jul 29, 2025 at 11:34:59AM +0530, Mario Limonciello wrote: > On 7/28/2025 4:22 PM, Greg KH wrote: > > On Mon, Jul 28, 2025 at 03:12:27PM +0530, Venkata Prasad Potturu wrote: > > > * If users do end up using it to debug and report different values > > > we should introduce a config knob that can have policy set by ucm. > > Please do this properly, with a per-device setting. > As the main purpose for this parameter is for being able to tune whether the > property is correct, how about adding a debugfs file instead? > AFAICT it should just be a single register write, so debugfs to read current > value and write the debugging value seems pretty straightforward. Or you could just go direct to making it user tunable like a normal gain control?
© 2016 - 2025 Red Hat, Inc.