| Message ID | 20211018114844.1746351-1-john@metanate.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: rockchip: use generic DMA engine configuration | expand |
On Mon, Oct 18, 2021 at 12:48:44PM +0100, John Keeping wrote: > This effectively reverts commit 75b31192fe6a ("ASoC: rockchip: add > config for rockchip dmaengine pcm register"). > There doesn't seem to be any rationale given for why these specific > values are helpful. The generic DMA engine provides sensible defaults > here and works well with Rockchip I2S. > In fact the period size here is really quite restrictive when dealing > with 8 channels of 32-bit data as the effective period size is just 256 > frames. Copying in Jianqun who wrote that patch. If you're reverting a patch it's generally good to make sure the original author is aware, particularly if you're unsure as to why the patch does what it does. > Signed-off-by: John Keeping <john@metanate.com> > --- > sound/soc/rockchip/Makefile | 3 +-- > sound/soc/rockchip/rockchip_i2s.c | 3 +-- > sound/soc/rockchip/rockchip_pcm.c | 44 ------------------------------- > sound/soc/rockchip/rockchip_pcm.h | 11 -------- > 4 files changed, 2 insertions(+), 59 deletions(-) > delete mode 100644 sound/soc/rockchip/rockchip_pcm.c > delete mode 100644 sound/soc/rockchip/rockchip_pcm.h > > diff --git a/sound/soc/rockchip/Makefile b/sound/soc/rockchip/Makefile > index b10f5e7b136d..6a3e61178152 100644 > --- a/sound/soc/rockchip/Makefile > +++ b/sound/soc/rockchip/Makefile > @@ -2,11 +2,10 @@ > # ROCKCHIP Platform Support > snd-soc-rockchip-i2s-objs := rockchip_i2s.o > snd-soc-rockchip-i2s-tdm-objs := rockchip_i2s_tdm.o > -snd-soc-rockchip-pcm-objs := rockchip_pcm.o > snd-soc-rockchip-pdm-objs := rockchip_pdm.o > snd-soc-rockchip-spdif-objs := rockchip_spdif.o > > -obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-rockchip-i2s.o snd-soc-rockchip-pcm.o > +obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-rockchip-i2s.o > obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S_TDM) += snd-soc-rockchip-i2s-tdm.o > obj-$(CONFIG_SND_SOC_ROCKCHIP_PDM) += snd-soc-rockchip-pdm.o > obj-$(CONFIG_SND_SOC_ROCKCHIP_SPDIF) += snd-soc-rockchip-spdif.o > diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c > index 7e89f5b0c237..a6d7656c206e 100644 > --- a/sound/soc/rockchip/rockchip_i2s.c > +++ b/sound/soc/rockchip/rockchip_i2s.c > @@ -20,7 +20,6 @@ > #include <sound/dmaengine_pcm.h> > > #include "rockchip_i2s.h" > -#include "rockchip_pcm.h" > > #define DRV_NAME "rockchip-i2s" > > @@ -756,7 +755,7 @@ static int rockchip_i2s_probe(struct platform_device *pdev) > goto err_suspend; > } > > - ret = rockchip_pcm_platform_register(&pdev->dev); > + ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0); > if (ret) { > dev_err(&pdev->dev, "Could not register PCM\n"); > goto err_suspend; > diff --git a/sound/soc/rockchip/rockchip_pcm.c b/sound/soc/rockchip/rockchip_pcm.c > deleted file mode 100644 > index 02254e42135e..000000000000 > --- a/sound/soc/rockchip/rockchip_pcm.c > +++ /dev/null > @@ -1,44 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0-only > -/* > - * Copyright (c) 2018 Rockchip Electronics Co. Ltd. > - */ > - > -#include <linux/device.h> > -#include <linux/init.h> > -#include <linux/module.h> > - > -#include <sound/core.h> > -#include <sound/pcm.h> > -#include <sound/soc.h> > -#include <sound/dmaengine_pcm.h> > - > -#include "rockchip_pcm.h" > - > -static const struct snd_pcm_hardware snd_rockchip_hardware = { > - .info = SNDRV_PCM_INFO_MMAP | > - SNDRV_PCM_INFO_MMAP_VALID | > - SNDRV_PCM_INFO_PAUSE | > - SNDRV_PCM_INFO_RESUME | > - SNDRV_PCM_INFO_INTERLEAVED, > - .period_bytes_min = 32, > - .period_bytes_max = 8192, > - .periods_min = 1, > - .periods_max = 52, > - .buffer_bytes_max = 64 * 1024, > - .fifo_size = 32, > -}; > - > -static const struct snd_dmaengine_pcm_config rk_dmaengine_pcm_config = { > - .pcm_hardware = &snd_rockchip_hardware, > - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > - .prealloc_buffer_size = 32 * 1024, > -}; > - > -int rockchip_pcm_platform_register(struct device *dev) > -{ > - return devm_snd_dmaengine_pcm_register(dev, &rk_dmaengine_pcm_config, > - SND_DMAENGINE_PCM_FLAG_COMPAT); > -} > -EXPORT_SYMBOL_GPL(rockchip_pcm_platform_register); > - > -MODULE_LICENSE("GPL v2"); > diff --git a/sound/soc/rockchip/rockchip_pcm.h b/sound/soc/rockchip/rockchip_pcm.h > deleted file mode 100644 > index 7f00e2ce3603..000000000000 > --- a/sound/soc/rockchip/rockchip_pcm.h > +++ /dev/null > @@ -1,11 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0-only */ > -/* > - * Copyright (c) 2018 Rockchip Electronics Co. Ltd. > - */ > - > -#ifndef _ROCKCHIP_PCM_H > -#define _ROCKCHIP_PCM_H > - > -int rockchip_pcm_platform_register(struct device *dev); > - > -#endif > -- > 2.33.1 >
On Tue, 19 Oct 2021 10:11:27 +0800 sugar zhang <sugar.zhang@rock-chips.com> wrote: > Actually, I have submit patch[1] to do the same thing a few weeks ago, > and explain the original purpose. > > [1] > https://x-lore.kernel.org/linux-rockchip/1632792957-80428-1-git-send-email-sugar.zhang@rock-chips.com/ I missed that one! I've added my R-b now. Mark, please ignore this patch and consider Sugar's. Thanks, John > On 2021/10/18 20:46, Mark Brown wrote: > > On Mon, Oct 18, 2021 at 12:48:44PM +0100, John Keeping wrote: > >> This effectively reverts commit 75b31192fe6a ("ASoC: rockchip: add > >> config for rockchip dmaengine pcm register"). > >> There doesn't seem to be any rationale given for why these specific > >> values are helpful. The generic DMA engine provides sensible defaults > >> here and works well with Rockchip I2S. > >> In fact the period size here is really quite restrictive when dealing > >> with 8 channels of 32-bit data as the effective period size is just 256 > >> frames. > > Copying in Jianqun who wrote that patch. If you're reverting a patch > > it's generally good to make sure the original author is aware, > > particularly if you're unsure as to why the patch does what it does. > > > >> Signed-off-by: John Keeping <john@metanate.com> > >> --- > >> sound/soc/rockchip/Makefile | 3 +-- > >> sound/soc/rockchip/rockchip_i2s.c | 3 +-- > >> sound/soc/rockchip/rockchip_pcm.c | 44 ------------------------------- > >> sound/soc/rockchip/rockchip_pcm.h | 11 -------- > >> 4 files changed, 2 insertions(+), 59 deletions(-) > >> delete mode 100644 sound/soc/rockchip/rockchip_pcm.c > >> delete mode 100644 sound/soc/rockchip/rockchip_pcm.h > >> > >> diff --git a/sound/soc/rockchip/Makefile b/sound/soc/rockchip/Makefile > >> index b10f5e7b136d..6a3e61178152 100644 > >> --- a/sound/soc/rockchip/Makefile > >> +++ b/sound/soc/rockchip/Makefile > >> @@ -2,11 +2,10 @@ > >> # ROCKCHIP Platform Support > >> snd-soc-rockchip-i2s-objs := rockchip_i2s.o > >> snd-soc-rockchip-i2s-tdm-objs := rockchip_i2s_tdm.o > >> -snd-soc-rockchip-pcm-objs := rockchip_pcm.o > >> snd-soc-rockchip-pdm-objs := rockchip_pdm.o > >> snd-soc-rockchip-spdif-objs := rockchip_spdif.o > >> > >> -obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-rockchip-i2s.o snd-soc-rockchip-pcm.o > >> +obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-rockchip-i2s.o > >> obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S_TDM) += snd-soc-rockchip-i2s-tdm.o > >> obj-$(CONFIG_SND_SOC_ROCKCHIP_PDM) += snd-soc-rockchip-pdm.o > >> obj-$(CONFIG_SND_SOC_ROCKCHIP_SPDIF) += snd-soc-rockchip-spdif.o > >> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c > >> index 7e89f5b0c237..a6d7656c206e 100644 > >> --- a/sound/soc/rockchip/rockchip_i2s.c > >> +++ b/sound/soc/rockchip/rockchip_i2s.c > >> @@ -20,7 +20,6 @@ > >> #include <sound/dmaengine_pcm.h> > >> > >> #include "rockchip_i2s.h" > >> -#include "rockchip_pcm.h" > >> > >> #define DRV_NAME "rockchip-i2s" > >> > >> @@ -756,7 +755,7 @@ static int rockchip_i2s_probe(struct platform_device *pdev) > >> goto err_suspend; > >> } > >> > >> - ret = rockchip_pcm_platform_register(&pdev->dev); > >> + ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0); > >> if (ret) { > >> dev_err(&pdev->dev, "Could not register PCM\n"); > >> goto err_suspend; > >> diff --git a/sound/soc/rockchip/rockchip_pcm.c b/sound/soc/rockchip/rockchip_pcm.c > >> deleted file mode 100644 > >> index 02254e42135e..000000000000 > >> --- a/sound/soc/rockchip/rockchip_pcm.c > >> +++ /dev/null > >> @@ -1,44 +0,0 @@ > >> -// SPDX-License-Identifier: GPL-2.0-only > >> -/* > >> - * Copyright (c) 2018 Rockchip Electronics Co. Ltd. > >> - */ > >> - > >> -#include <linux/device.h> > >> -#include <linux/init.h> > >> -#include <linux/module.h> > >> - > >> -#include <sound/core.h> > >> -#include <sound/pcm.h> > >> -#include <sound/soc.h> > >> -#include <sound/dmaengine_pcm.h> > >> - > >> -#include "rockchip_pcm.h" > >> - > >> -static const struct snd_pcm_hardware snd_rockchip_hardware = { > >> - .info = SNDRV_PCM_INFO_MMAP | > >> - SNDRV_PCM_INFO_MMAP_VALID | > >> - SNDRV_PCM_INFO_PAUSE | > >> - SNDRV_PCM_INFO_RESUME | > >> - SNDRV_PCM_INFO_INTERLEAVED, > >> - .period_bytes_min = 32, > >> - .period_bytes_max = 8192, > >> - .periods_min = 1, > >> - .periods_max = 52, > >> - .buffer_bytes_max = 64 * 1024, > >> - .fifo_size = 32, > >> -}; > >> - > >> -static const struct snd_dmaengine_pcm_config rk_dmaengine_pcm_config = { > >> - .pcm_hardware = &snd_rockchip_hardware, > >> - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > >> - .prealloc_buffer_size = 32 * 1024, > >> -}; > >> - > >> -int rockchip_pcm_platform_register(struct device *dev) > >> -{ > >> - return devm_snd_dmaengine_pcm_register(dev, &rk_dmaengine_pcm_config, > >> - SND_DMAENGINE_PCM_FLAG_COMPAT); > >> -} > >> -EXPORT_SYMBOL_GPL(rockchip_pcm_platform_register); > >> - > >> -MODULE_LICENSE("GPL v2"); > >> diff --git a/sound/soc/rockchip/rockchip_pcm.h b/sound/soc/rockchip/rockchip_pcm.h > >> deleted file mode 100644 > >> index 7f00e2ce3603..000000000000 > >> --- a/sound/soc/rockchip/rockchip_pcm.h > >> +++ /dev/null > >> @@ -1,11 +0,0 @@ > >> -/* SPDX-License-Identifier: GPL-2.0-only */ > >> -/* > >> - * Copyright (c) 2018 Rockchip Electronics Co. Ltd. > >> - */ > >> - > >> -#ifndef _ROCKCHIP_PCM_H > >> -#define _ROCKCHIP_PCM_H > >> - > >> -int rockchip_pcm_platform_register(struct device *dev); > >> - > >> -#endif > >> -- > >> 2.33.1 > >> > >> > >> _______________________________________________ > >> Linux-rockchip mailing list > >> Linux-rockchip@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-rockchip >
On Tue, Oct 19, 2021 at 10:11:27AM +0800, sugar zhang wrote: > Hi Mark, John, > > Actually, I have submit patch[1] to do the same thing a few weeks ago, and > explain the original purpose. > > [1] https://x-lore.kernel.org/linux-rockchip/1632792957-80428-1-git-send-email-sugar.zhang@rock-chips.com/ Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone. Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed.
Hi Mark, On 2021/10/19 20:18, Mark Brown wrote: > On Tue, Oct 19, 2021 at 10:11:27AM +0800, sugar zhang wrote: >> Hi Mark, John, >> >> Actually, I have submit patch[1] to do the same thing a few weeks ago, and >> explain the original purpose. >> >> [1] https://x-lore.kernel.org/linux-rockchip/1632792957-80428-1-git-send-email-sugar.zhang@rock-chips.com/ > Please submit patches using subject lines reflecting the style for the > subsystem, this makes it easier for people to identify relevant patches. > Look at what existing commits in the area you're changing are doing and > make sure your subject lines visually resemble what they're doing. > There's no need to resubmit to fix this alone. ok. > Please don't top post, reply in line with needed context. This allows > readers to readily follow the flow of conversation and understand what > you are talking about and also helps ensure that everything in the > discussion is being addressed. got it, thanks.
diff --git a/sound/soc/rockchip/Makefile b/sound/soc/rockchip/Makefile index b10f5e7b136d..6a3e61178152 100644 --- a/sound/soc/rockchip/Makefile +++ b/sound/soc/rockchip/Makefile @@ -2,11 +2,10 @@ # ROCKCHIP Platform Support snd-soc-rockchip-i2s-objs := rockchip_i2s.o snd-soc-rockchip-i2s-tdm-objs := rockchip_i2s_tdm.o -snd-soc-rockchip-pcm-objs := rockchip_pcm.o snd-soc-rockchip-pdm-objs := rockchip_pdm.o snd-soc-rockchip-spdif-objs := rockchip_spdif.o -obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-rockchip-i2s.o snd-soc-rockchip-pcm.o +obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-rockchip-i2s.o obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S_TDM) += snd-soc-rockchip-i2s-tdm.o obj-$(CONFIG_SND_SOC_ROCKCHIP_PDM) += snd-soc-rockchip-pdm.o obj-$(CONFIG_SND_SOC_ROCKCHIP_SPDIF) += snd-soc-rockchip-spdif.o diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c index 7e89f5b0c237..a6d7656c206e 100644 --- a/sound/soc/rockchip/rockchip_i2s.c +++ b/sound/soc/rockchip/rockchip_i2s.c @@ -20,7 +20,6 @@ #include <sound/dmaengine_pcm.h> #include "rockchip_i2s.h" -#include "rockchip_pcm.h" #define DRV_NAME "rockchip-i2s" @@ -756,7 +755,7 @@ static int rockchip_i2s_probe(struct platform_device *pdev) goto err_suspend; } - ret = rockchip_pcm_platform_register(&pdev->dev); + ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0); if (ret) { dev_err(&pdev->dev, "Could not register PCM\n"); goto err_suspend; diff --git a/sound/soc/rockchip/rockchip_pcm.c b/sound/soc/rockchip/rockchip_pcm.c deleted file mode 100644 index 02254e42135e..000000000000 --- a/sound/soc/rockchip/rockchip_pcm.c +++ /dev/null @@ -1,44 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * Copyright (c) 2018 Rockchip Electronics Co. Ltd. - */ - -#include <linux/device.h> -#include <linux/init.h> -#include <linux/module.h> - -#include <sound/core.h> -#include <sound/pcm.h> -#include <sound/soc.h> -#include <sound/dmaengine_pcm.h> - -#include "rockchip_pcm.h" - -static const struct snd_pcm_hardware snd_rockchip_hardware = { - .info = SNDRV_PCM_INFO_MMAP | - SNDRV_PCM_INFO_MMAP_VALID | - SNDRV_PCM_INFO_PAUSE | - SNDRV_PCM_INFO_RESUME | - SNDRV_PCM_INFO_INTERLEAVED, - .period_bytes_min = 32, - .period_bytes_max = 8192, - .periods_min = 1, - .periods_max = 52, - .buffer_bytes_max = 64 * 1024, - .fifo_size = 32, -}; - -static const struct snd_dmaengine_pcm_config rk_dmaengine_pcm_config = { - .pcm_hardware = &snd_rockchip_hardware, - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, - .prealloc_buffer_size = 32 * 1024, -}; - -int rockchip_pcm_platform_register(struct device *dev) -{ - return devm_snd_dmaengine_pcm_register(dev, &rk_dmaengine_pcm_config, - SND_DMAENGINE_PCM_FLAG_COMPAT); -} -EXPORT_SYMBOL_GPL(rockchip_pcm_platform_register); - -MODULE_LICENSE("GPL v2"); diff --git a/sound/soc/rockchip/rockchip_pcm.h b/sound/soc/rockchip/rockchip_pcm.h deleted file mode 100644 index 7f00e2ce3603..000000000000 --- a/sound/soc/rockchip/rockchip_pcm.h +++ /dev/null @@ -1,11 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* - * Copyright (c) 2018 Rockchip Electronics Co. Ltd. - */ - -#ifndef _ROCKCHIP_PCM_H -#define _ROCKCHIP_PCM_H - -int rockchip_pcm_platform_register(struct device *dev); - -#endif
This effectively reverts commit 75b31192fe6a ("ASoC: rockchip: add config for rockchip dmaengine pcm register"). There doesn't seem to be any rationale given for why these specific values are helpful. The generic DMA engine provides sensible defaults here and works well with Rockchip I2S. In fact the period size here is really quite restrictive when dealing with 8 channels of 32-bit data as the effective period size is just 256 frames. Signed-off-by: John Keeping <john@metanate.com> --- sound/soc/rockchip/Makefile | 3 +-- sound/soc/rockchip/rockchip_i2s.c | 3 +-- sound/soc/rockchip/rockchip_pcm.c | 44 ------------------------------- sound/soc/rockchip/rockchip_pcm.h | 11 -------- 4 files changed, 2 insertions(+), 59 deletions(-) delete mode 100644 sound/soc/rockchip/rockchip_pcm.c delete mode 100644 sound/soc/rockchip/rockchip_pcm.h