Improvements for mchp-pdmc
Andrei Simion
maintainercover Improvements for mchp-pdmc
This patch set is intended to enhance the functionality and maintainability of the mchp-pdmc driver:
- Enhances performance by refining maxburst logic.
- Introduces a name for better identification and management.
- Ensures controls remain intact when streams start/finish, returning -EBUSY if the controller is busy.
Codrin Ciubotariu (3):
ASoC: atmel: mchp-pdmc: Improve maxburst calculation for better
performance
ASoC: atmel: mchp-pdmc: Add snd_soc_dai_driver name
ASoC: atmel: mchp-pdmc: Retain Non-Runtime Controls
sound/soc/atmel/mchp-pdmc.c | 118 +++++++++++++++++++----------------- 1 file changed, 63 insertions(+), 55 deletions(-)
base-commit: 6708132e80a2ced620bde9b9c36e426183544a23
Signature · 1 line
2.34.1
Andrei Simion
maintainerPATCH 1/3 ASoC: atmel: mchp-pdmc: Improve maxburst calculation for better performance
From: Codrin Ciubotariu <[email protected]>
Improve the DMA descriptor calculation by dividing the period size by the product of sample size and DMA chunk size, rather than just DMA chunk size. Ensure that all DMA descriptors start from a well-aligned address to improve the reliability and efficiency of DMA operations and avoid potential issues related to misaligned descriptors.
[[email protected]: Adjust the commit title. Reword the commit message. Add MACROS for each DMA size chunk supported by mchp-pdmc. Add DMA_BURST_ALIGNED preprocesor function to check the alignment of the DMA burst.]
--- sound/soc/atmel/mchp-pdmc.c | 39 ++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)
sound/soc/atmel/mchp-pdmc.c +28 -11
@@ -90,6 +90,15 @@#define MCHP_PDMC_DS_NO 2#define MCHP_PDMC_EDGE_NO 2 +/* +* ---- DMA chunk size allowed ---- +*/ +#define MCHP_PDMC_DMA_8_WORD_CHUNK 8 +#define MCHP_PDMC_DMA_4_WORD_CHUNK 4 +#define MCHP_PDMC_DMA_2_WORD_CHUNK 2 +#define MCHP_PDMC_DMA_1_WORD_CHUNK 1 +#define DMA_BURST_ALIGNED (_p, _s, _w) !(_p % (_s * _w)) +struct mic_map {int ds_pos;int clk_edge;
@@ -511,15 +520,18 @@ static u32 mchp_pdmc_mr_set_osr(int audio_filter_en, unsigned int osr)return 0 ; } -static inline int mchp_pdmc_period_to_maxburst (int period_size) +static inline int mchp_pdmc_period_to_maxburst (int period_size,int sample_size) { -if (!(period_size %8 )) -return 8 ; -if (!(period_size %4 )) -return 4 ; -if (!(period_size %2 )) -return 2 ; -return 1 ; +int p_size = period_size; +int s_size = sample_size; + +if (DMA_BURST_ALIGNED (p_size, s_size,MCHP_PDMC_DMA_8_WORD_CHUNK )) +return MCHP_PDMC_DMA_8_WORD_CHUNK ; +if (DMA_BURST_ALIGNED (p_size, s_size,MCHP_PDMC_DMA_4_WORD_CHUNK )) +return MCHP_PDMC_DMA_4_WORD_CHUNK ; +if (DMA_BURST_ALIGNED (p_size, s_size,MCHP_PDMC_DMA_2_WORD_CHUNK )) +return MCHP_PDMC_DMA_2_WORD_CHUNK ; +return MCHP_PDMC_DMA_1_WORD_CHUNK ; }static struct snd_pcm_chmap_elem mchp_pdmc_std_chmaps[] = {
@@ -547,14 +559,18 @@ static int mchp_pdmc_hw_params(struct snd_pcm_substream *substream,unsigned int channels =params_channels (params);unsigned int osr =0 , osr_start;unsigned int fs =params_rate (params); +int sample_bytes =params_physical_width (params) /8 ; +int period_bytes =params_period_size (params) * +params_channels (params) * sample_bytes; +int maxburst;u32 mr_val =0 ;u32 cfgr_val =0 ;int i;int ret; -dev_dbg (comp->dev,"%s() rate=%u format=%#x width=%u channels=%u\n" , +dev_dbg (comp->dev,"%s() rate=%u format=%#x width=%u channels=%u period_bytes=%d\n" , __func__,params_rate (params),params_format (params), -params_width (params),params_channels (params)); +params_width (params),params_channels (params), period_bytes);if (channels > dd->mic_no) { dev_err(comp->dev, "more channels %u than microphones %d\n",
@@ -608,7 +624,8 @@ static int mchp_pdmc_hw_params(struct snd_pcm_substream *substream, mr_val |=FIELD_PREP (MCHP_PDMC_MR_SINCORDER_MASK , dd->sinc_order); - dd->addr.maxburst =mchp_pdmc_period_to_maxburst (snd_pcm_lib_period_bytes (substream)); + maxburst =mchp_pdmc_period_to_maxburst (period_bytes, sample_bytes); + dd->addr.maxburst = maxburst; mr_val |=FIELD_PREP (MCHP_PDMC_MR_CHUNK_MASK , dd->addr.maxburst);dev_dbg (comp->dev,"maxburst set to %d\n" , dd->addr.maxburst);
Signature · 1 line
2.34.1
Andrei Simion
maintainerPATCH 2/3 ASoC: atmel: mchp-pdmc: Add snd_soc_dai_driver name
In reply to Andrei SimionFrom: Codrin Ciubotariu <[email protected]>
Set snd_soc_dai_driver name to improve controller's display of the DAI name.
--- sound/soc/atmel/mchp-pdmc.c | 1 + 1 file changed, 1 insertion(+)
sound/soc/atmel/mchp-pdmc.c +1 -0
@@ -777,6 +777,7 @@ static const struct snd_soc_dai_ops mchp_pdmc_dai_ops = { };static struct snd_soc_dai_driver mchp_pdmc_dai = { + .name ="mchp-pdmc" , .capture = { .stream_name ="Capture" , .channels_min =1 ,
Signature · 1 line
2.34.1
Andrei Simion
maintainerPATCH 3/3 ASoC: atmel: mchp-pdmc: Retain Non-Runtime Controls
In reply to Andrei SimionFrom: Codrin Ciubotariu <[email protected]>
Avoid removing these controls, as doing so can cause issues if the stream is initiated from another control. Ensure these controls remain intact when the stream is started or finished. Instead of removing them, return an -EBUSY error code to indicate that the controller is busy, especially when the audio filter and the SINC filter are in use.
[[email protected]: Reword the commit title and the commit message. Adapt the code from kernel v6.1 -> v6.6 -> latest kernel version.]
--- sound/soc/atmel/mchp-pdmc.c | 78 ++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 44 deletions(-)
sound/soc/atmel/mchp-pdmc.c +34 -44
@@ -14,6 +14,7 @@#include <linux/of.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> +#include <linux/spinlock.h> #include <sound/core.h> #include <sound/dmaengine_pcm.h>
@@ -113,6 +114,7 @@ struct mchp_pdmc_chmap {struct mchp_pdmc {struct mic_map channel_mic_map[MCHP_PDMC_MAX_CHANNELS ]; +spinlock_t busy_lock;/* lock protecting busy */ struct device *dev;struct snd_dmaengine_dai_dma_data addr;struct regmap *regmap;
@@ -124,6 +126,7 @@ struct mchp_pdmc {int mic_no;int sinc_order;bool audio_filter_en; + u8 busy:1; };static const char *const mchp_pdmc_sinc_filter_order_text[] = {
@@ -167,10 +170,19 @@ static int mchp_pdmc_sinc_order_put(struct snd_kcontrol *kcontrol,return -EINVAL ; val =snd_soc_enum_item_to_val (e, item[0 ]) << e->shift_l; -if (val == dd->sinc_order) + +spin_lock (&dd->busy_lock); +if (dd->busy) { +spin_unlock ((&dd->busy_lock)); +return -EBUSY ; + } +if (val == dd->sinc_order) { +spin_unlock ((&dd->busy_lock));return 0 ; + } dd->sinc_order = val; +spin_unlock ((&dd->busy_lock));return 1 ; }
@@ -193,10 +205,18 @@ static int mchp_pdmc_af_put(struct snd_kcontrol *kcontrol,struct mchp_pdmc *dd =snd_soc_component_get_drvdata (component);bool af = uvalue->value.integer.value[0 ] ? true : false; -if (dd->audio_filter_en == af) +spin_lock (&dd->busy_lock); +if (dd->busy) { +spin_unlock ((&dd->busy_lock)); +return -EBUSY ; + } +if (dd->audio_filter_en == af) { +spin_unlock ((&dd->busy_lock));return 0 ; + } dd->audio_filter_en = af; +spin_unlock ((&dd->busy_lock));return 1 ; }
@@ -379,52 +399,10 @@ static const struct snd_kcontrol_new mchp_pdmc_snd_controls[] = { }, }; -static int mchp_pdmc_close (struct snd_soc_component *component, -struct snd_pcm_substream *substream) -{ -return snd_soc_add_component_controls (component, mchp_pdmc_snd_controls, -ARRAY_SIZE (mchp_pdmc_snd_controls)); -} - -static int mchp_pdmc_open (struct snd_soc_component *component, -struct snd_pcm_substream *substream) -{ -int i; - -/* remove controls that can't be changed at runtime */ -for (i =0 ; i <ARRAY_SIZE (mchp_pdmc_snd_controls); i++) { -const struct snd_kcontrol_new *control = &mchp_pdmc_snd_controls[i]; -struct snd_ctl_elem_id id; -int err; - -if (component->name_prefix) -snprintf (id.name,sizeof (id.name),"%s %s" , component->name_prefix, - control->name); -else -strscpy (id.name, control->name,sizeof (id.name)); - - id.numid =0 ; - id.iface = control->iface; - id.device = control->device; - id.subdevice = control->subdevice; - id.index = control->index; - err =snd_ctl_remove_id (component->card->snd_card, &id); -if (err <0 ) -dev_err (component->dev,"%d: Failed to remove %s\n" , err, - control->name); - } - -return 0 ; -} -static const struct snd_soc_component_driver mchp_pdmc_dai_component = { .name ="mchp-pdmc" , .controls = mchp_pdmc_snd_controls, .num_controls =ARRAY_SIZE (mchp_pdmc_snd_controls), - .open = &mchp_pdmc_open, - .close = &mchp_pdmc_close, - .legacy_dai_naming =1 , - .trigger_start =SND_SOC_TRIGGER_ORDER_LDC , };static const unsigned int mchp_pdmc_1mic[] = {1 };
@@ -587,6 +565,13 @@ static int mchp_pdmc_hw_params(struct snd_pcm_substream *substream, cfgr_val |=MCHP_PDMC_CFGR_BSSEL (i); } +/* +* from these point forward, we consider the controller busy, so the +* audio filter and SINC order can't be changed +*/ +spin_lock (&dd->busy_lock); + dd->busy =1 ; +spin_unlock ((&dd->busy_lock));for (osr_start = dd->audio_filter_en ?64 :8 ; osr_start <=256 && best_diff_rate; osr_start *=2 ) {long round_rate;
@@ -1099,6 +1084,7 @@ static int mchp_pdmc_probe(struct platform_device *pdev)*/ dd->audio_filter_en = true; dd->sinc_order =3 ; +spin_lock_init (&dd->busy_lock); dd->addr.addr = (dma_addr_t )res->start +MCHP_PDMC_RHR ;platform_set_drvdata (pdev, dd);
@@ -1143,6 +1129,10 @@ static void mchp_pdmc_remove(struct platform_device *pdev) {struct mchp_pdmc *dd =platform_get_drvdata (pdev); +spin_lock (&dd->busy_lock); + dd->busy =0 ; +spin_unlock ((&dd->busy_lock)); +if (!pm_runtime_status_suspended (dd->dev))mchp_pdmc_runtime_suspend (dd->dev);
Signature · 1 line
2.34.1
claudiu beznea
maintainerRe: [PATCH 3/3] ASoC: atmel: mchp-pdmc: Retain Non-Runtime Controls
Hi, Andrei,
Quoted 38 lines · Andrei Simion
From: Codrin Ciubotariu <[email protected]>
Avoid removing these controls, as doing so can cause issues if the stream is initiated from another control. Ensure these controls remain intact when the stream is started or finished. Instead of removing them, return an
-EBUSYerror code to indicate that the controller is busy, especially when the audio filter and the SINC filter are in use.[[email protected]: Reword the commit title and the commit message. Adapt the code from kernel v6.1 -> v6.6 -> latest kernel version.]
Signed-off-by Codrin Ciubotariu <[email protected]>Signed-off-by Andrei Simion <[email protected]>--- sound/soc/atmel/mchp-pdmc.c | 78 ++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 44 deletions(-)sound/soc/atmel/mchp-pdmc.c +2
-0@@ -14,6 +14,7 @@#include <linux/of.h> #include <linux/pm_runtime.h> #include <linux/regmap.h> +#include <linux/spinlock.h> #include <sound/core.h> #include <sound/dmaengine_pcm.h> @@ -113,6 +114,7 @@ struct mchp_pdmc_chmap {struct mchp_pdmc {struct mic_map channel_mic_map[MCHP_PDMC_MAX_CHANNELS ]; +spinlock_t busy_lock;/* lock protecting busy */ struct device *dev;struct snd_dmaengine_dai_dma_data addr;struct regmap *regmap;
sound/soc/atmel/mchp-pdmc.c struct mchp_pdmc { · line 129
Quoted 4 lines
sound/soc/atmel/mchp-pdmc.c +1
-0@@ -124,6 +126,7 @@ struct mchp_pdmc {int mic_no;int sinc_order;bool audio_filter_en; + u8 busy:1;
Can the spinlock and busy flag be replaced by an atomic variable?
sound/soc/atmel/mchp-pdmc.c static int mchp_pdmc_sinc_order_put(struct snd_kcontrol *kcontrol, · line 176
Quoted 12 lines
sound/soc/atmel/mchp-pdmc.c +4
-1};static const char *const mchp_pdmc_sinc_filter_order_text[] = {@@ -167,10 +170,19 @@ static int mchp_pdmc_sinc_order_put(struct snd_kcontrol *kcontrol,return -EINVAL ; val =snd_soc_enum_item_to_val (e, item[0 ]) << e->shift_l; -if (val == dd->sinc_order) + +spin_lock (&dd->busy_lock); +if (dd->busy) { +spin_unlock ((&dd->busy_lock));
You can remove () around (&dd->busy_lock). Valid for the rest of occurrences.
Quoted 118 lines
sound/soc/atmel/mchp-pdmc.c +27
-43+return -EBUSY ; + } +if (val == dd->sinc_order) { +spin_unlock ((&dd->busy_lock));return 0 ; + } dd->sinc_order = val; +spin_unlock ((&dd->busy_lock));return 1 ; }@@ -193,10 +205,18 @@ static int mchp_pdmc_af_put(struct snd_kcontrol *kcontrol,struct mchp_pdmc *dd =snd_soc_component_get_drvdata (component);bool af = uvalue->value.integer.value[0 ] ? true : false; -if (dd->audio_filter_en == af) +spin_lock (&dd->busy_lock); +if (dd->busy) { +spin_unlock ((&dd->busy_lock)); +return -EBUSY ; + } +if (dd->audio_filter_en == af) { +spin_unlock ((&dd->busy_lock));return 0 ; + } dd->audio_filter_en = af; +spin_unlock ((&dd->busy_lock));return 1 ; }@@ -379,52 +399,10 @@ static const struct snd_kcontrol_new mchp_pdmc_snd_controls[] = { }, }; -static int mchp_pdmc_close (struct snd_soc_component *component, -struct snd_pcm_substream *substream) -{ -return snd_soc_add_component_controls (component, mchp_pdmc_snd_controls, -ARRAY_SIZE (mchp_pdmc_snd_controls)); -} - -static int mchp_pdmc_open (struct snd_soc_component *component, -struct snd_pcm_substream *substream) -{ -int i; - -/* remove controls that can't be changed at runtime */ -for (i =0 ; i <ARRAY_SIZE (mchp_pdmc_snd_controls); i++) { -const struct snd_kcontrol_new *control = &mchp_pdmc_snd_controls[i]; -struct snd_ctl_elem_id id; -int err; - -if (component->name_prefix) -snprintf (id.name,sizeof (id.name),"%s %s" , component->name_prefix, - control->name); -else -strscpy (id.name, control->name,sizeof (id.name)); - - id.numid =0 ; - id.iface = control->iface; - id.device = control->device; - id.subdevice = control->subdevice; - id.index = control->index; - err =snd_ctl_remove_id (component->card->snd_card, &id); -if (err <0 ) -dev_err (component->dev,"%d: Failed to remove %s\n" , err, - control->name); - } - -return 0 ; -} -static const struct snd_soc_component_driver mchp_pdmc_dai_component = { .name ="mchp-pdmc" , .controls = mchp_pdmc_snd_controls, .num_controls =ARRAY_SIZE (mchp_pdmc_snd_controls), - .open = &mchp_pdmc_open, - .close = &mchp_pdmc_close, - .legacy_dai_naming =1 , - .trigger_start =SND_SOC_TRIGGER_ORDER_LDC , };static const unsigned int mchp_pdmc_1mic[] = {1 };@@ -587,6 +565,13 @@ static int mchp_pdmc_hw_params(struct snd_pcm_substream *substream, cfgr_val |=MCHP_PDMC_CFGR_BSSEL (i); } +/* +* from these point forward, we consider the controller busy, so the +* audio filter and SINC order can't be changed +*/ +spin_lock (&dd->busy_lock); + dd->busy =1 ; +spin_unlock ((&dd->busy_lock));for (osr_start = dd->audio_filter_en ?64 :8 ; osr_start <=256 && best_diff_rate; osr_start *=2 ) {long round_rate;@@ -1099,6 +1084,7 @@ static int mchp_pdmc_probe(struct platform_device *pdev)*/ dd->audio_filter_en = true; dd->sinc_order =3 ; +spin_lock_init (&dd->busy_lock); dd->addr.addr = (dma_addr_t )res->start +MCHP_PDMC_RHR ;platform_set_drvdata (pdev, dd);@@ -1143,6 +1129,10 @@ static void mchp_pdmc_remove(struct platform_device *pdev) {struct mchp_pdmc *dd =platform_get_drvdata (pdev); +spin_lock (&dd->busy_lock); + dd->busy =0 ; +spin_unlock ((&dd->busy_lock)); +if (!pm_runtime_status_suspended (dd->dev))mchp_pdmc_runtime_suspend (dd->dev);
Andrei Simion
maintainerRe: [PATCH 3/3] ASoC: atmel: mchp-pdmc: Retain Non-Runtime Controls
Quoted 5 lines
Quoted 5 lines
struct
mchp_pdmc{sound/soc/atmel/mchp-pdmc.c +1
-0struct mic_map channel_mic_map[MCHP_PDMC_MAX_CHANNELS ]; +spinlock_t busy_lock;/* lock protecting busy */ struct device *dev;struct snd_dmaengine_dai_dma_data addr;
Quoted 8 lines
sound/soc/atmel/mchp-pdmc.c struct mchp_pdmc { · line 129
Quoted 6 lines
sound/soc/atmel/mchp-pdmc.c +1
-0struct regmap *regmap;@@ -124,6 +126,7 @@ struct mchp_pdmc {int mic_no;int sinc_order;bool audio_filter_en; + u8 busy:1;Can the spinlock and busy flag be replaced by an atomic variable?
I will use atomic_t variable with atomic_set and atomic_read. I will do a test and send V2.
Quoted 6 lines
Quoted 6 lines
sound/soc/atmel/mchp-pdmc.c +0
-0};static const char *const mchp_pdmc_sinc_filter_order_text[] = {@@ -167,10 +170,19 @@ static int mchp_pdmc_sinc_order_put(struct snd_kcontrol *kcontrol,return -EINVAL ;
Quoted 8 lines
sound/soc/atmel/mchp-pdmc.c static int mchp_pdmc_sinc_order_put(struct snd_kcontrol *kcontrol, · line 176
Quoted 6 lines
sound/soc/atmel/mchp-pdmc.c +4
-1val =snd_soc_enum_item_to_val (e, item[0 ]) << e->shift_l; -if (val == dd->sinc_order) + +spin_lock (&dd->busy_lock); +if (dd->busy) { +spin_unlock ((&dd->busy_lock));You can remove () around (
&dd->busy_lock). Valid for the rest of occurrences.
OK. Got it!
Quoted 12 lines
Quoted 12 lines
sound/soc/atmel/mchp-pdmc.c +6
-0+return -EBUSY ; + } +if (val == dd->sinc_order) { +spin_unlock ((&dd->busy_lock));return 0 ; + } dd->sinc_order = val; +spin_unlock ((&dd->busy_lock));return 1 ; }
Mark Brown
maintainerQuoted 8 lines · Andrei Simion
This patch set is intended to enhance the functionality and maintainability of the mchp-pdmc driver:
- Enhances performance by refining maxburst logic.
- Introduces a name for better identification and management.
- Ensures controls remain intact when streams start/finish, returning-EBUSYif the controller is busy.[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: atmel: mchp-pdmc: Improve maxburst calculation for better performance
commit: 8f0280c84607afe122788e508a171ba163d71be6
[2/3] ASoC: atmel: mchp-pdmc: Add snd_soc_dai_driver name
commit: e6b95bdc1e333e14e4fdf71fd4e8962429d9b6cd[3/3] ASoC: atmel: mchp-pdmc: Retain Non-Runtime Controls
(no commit info)
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks,
Mark