by Aidan MacDonald

[permalink] [raw]

Subject: Re: [PATCH 10/11] ASoC: jz4740-i2s: Support S20_LE and S24_LE sample formats

Paul Cercueil <[email protected]> writes:

> Le mer., juil. 6 2022 at 22:13:29 +0100, Aidan MacDonald
> <[email protected]> a écrit :
>> The audio controller on JZ47xx SoCs supports 20- and 24-bit
>> samples coming from memory. Allow those formats to be used
>> with the I2S driver.
>> Signed-off-by: Aidan MacDonald <[email protected]>
>> ---
>> sound/soc/jz4740/jz4740-i2s.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>> diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
>> index 80b355d715ce..ee99c5e781ec 100644
>> --- a/sound/soc/jz4740/jz4740-i2s.c
>> +++ b/sound/soc/jz4740/jz4740-i2s.c
>> @@ -222,9 +222,15 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream
>> *substream,
>> case SNDRV_PCM_FORMAT_S8:
>> sample_size = 0;
>> break;
>> - case SNDRV_PCM_FORMAT_S16:
>> + case SNDRV_PCM_FORMAT_S16_LE:
>> sample_size = 1;
>> break;
>> + case SNDRV_PCM_FORMAT_S20_LE:
>> + sample_size = 3;
>> + break;
>> + case SNDRV_PCM_FORMAT_S24_LE:
>> + sample_size = 4;
>> + break;
>
> Did you test these? It is unclear to me, looking at the JZ4740 PM, if the
> 18-bit, 20-bit and 24-bit samples are in 4 bytes or 3 bytes.
>
> Cheers,
> -Paul
>

I was only able to test 24-bit -- in practice 20-bit seems pretty rare
and I'm finding it difficult to convert anything into that format with
standard tools like ffmpeg, sox, etc. so I can't really test it.

From what I understand, this is configuring the number of significant
bits in the FIFO and has nothing to do with the in-memory format. So
my commit message is a bit inaccurate.

DMA can only do 4-byte transfers so that's why only 4-byte formats are
usable. I suppose I could handle the 3-byte variants here, but there's
probably no point to doing that if DMA can't transfer them.

>> default:
>> return -EINVAL;
>> }
>> @@ -362,7 +368,9 @@ static const struct snd_soc_dai_ops jz4740_i2s_dai_ops =
>> {
>> };
>> #define JZ4740_I2S_FMTS (SNDRV_PCM_FMTBIT_S8 | \
>> - SNDRV_PCM_FMTBIT_S16_LE)
>> + SNDRV_PCM_FMTBIT_S16_LE | \
>> + SNDRV_PCM_FMTBIT_S20_LE | \
>> + SNDRV_PCM_FMTBIT_S24_LE)
>> static struct snd_soc_dai_driver jz4740_i2s_dai = {
>> .probe = jz4740_i2s_dai_probe,
>> --
>> 2.35.1
>>

2022-07-07 14:49:58

by Aidan MacDonald

[permalink] [raw]

Subject: Re: [PATCH 03/11] ASoC: jz4740-i2s: Convert to regmap API

Paul Cercueil <[email protected]> writes:

> Hi Aidan,
>
> Le mer., juil. 6 2022 at 22:13:22 +0100, Aidan MacDonald
> <[email protected]> a écrit :
>> Using regmap for accessing the AIC registers makes the driver a
>> little easier to read, and later refactors can take advantage of
>> regmap APIs to further simplify the driver.
>> Signed-off-by: Aidan MacDonald <[email protected]>
>> ---
>> sound/soc/jz4740/Kconfig | 1 +
>> sound/soc/jz4740/jz4740-i2s.c | 99 +++++++++++++++--------------------
>> 2 files changed, 42 insertions(+), 58 deletions(-)
>> diff --git a/sound/soc/jz4740/Kconfig b/sound/soc/jz4740/Kconfig
>> index e72f826062e9..dd3b4507fbe6 100644
>> --- a/sound/soc/jz4740/Kconfig
>> +++ b/sound/soc/jz4740/Kconfig
>> @@ -3,6 +3,7 @@ config SND_JZ4740_SOC_I2S
>> tristate "SoC Audio (I2S protocol) for Ingenic JZ4740 SoC"
>> depends on MIPS || COMPILE_TEST
>> depends on HAS_IOMEM
>> + select REGMAP_MMIO
>> select SND_SOC_GENERIC_DMAENGINE_PCM
>> help
>> Say Y if you want to use I2S protocol and I2S codec on Ingenic JZ4740
>> diff --git a/sound/soc/jz4740/jz4740-i2s.c b/sound/soc/jz4740/jz4740-i2s.c
>> index ecd8df70d39c..66a901f56392 100644
>> --- a/sound/soc/jz4740/jz4740-i2s.c
>> +++ b/sound/soc/jz4740/jz4740-i2s.c
>> @@ -9,6 +9,7 @@
>> #include <linux/module.h>
>> #include <linux/mod_devicetable.h>
>> #include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> #include <linux/slab.h>
>> #include <linux/clk.h>
>> @@ -94,7 +95,7 @@ struct i2s_soc_info {
>> struct jz4740_i2s {
>> struct resource *mem;
>> - void __iomem *base;
>> + struct regmap *regmap;
>> struct clk *clk_aic;
>> struct clk *clk_i2s;
>> @@ -105,39 +106,24 @@ struct jz4740_i2s {
>> const struct i2s_soc_info *soc_info;
>> };
>> -static inline uint32_t jz4740_i2s_read(const struct jz4740_i2s *i2s,
>> - unsigned int reg)
>> -{
>> - return readl(i2s->base + reg);
>> -}
>> -
>> -static inline void jz4740_i2s_write(const struct jz4740_i2s *i2s,
>> - unsigned int reg, uint32_t value)
>> -{
>> - writel(value, i2s->base + reg);
>> -}
>> -
>> static int jz4740_i2s_startup(struct snd_pcm_substream *substream,
>> struct snd_soc_dai *dai)
>> {
>> struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>> - uint32_t conf, ctrl;
>> int ret;
>> if (snd_soc_dai_active(dai))
>> return 0;
>> - ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>> - ctrl |= JZ_AIC_CTRL_FLUSH;
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>> + regmap_write_bits(i2s->regmap, JZ_REG_AIC_CTRL,
>> + JZ_AIC_CTRL_FLUSH, JZ_AIC_CTRL_FLUSH);
>
> I don't think you need regmap_write_bits() here, since there is no cache to
> bypass. You could use regmap_update_bits(), or even better, regmap_set_bits().
>

write_bits isn't _exactly_ just a cache bypass operation -- it means
"write the register even if the value is the same as what was read."
An update_bits doesn't necessarily perform a register write, even if
there is no cache and the register is volatile.

The distinction shouldn't matter here, since the flush bit is supposed
to be self-clearing. So I might as well use regmap_set_bits().

Also: I just noticed this will need to be a regmap field. It seems that
all SoCs newer than jz4740 have separate transmit/receive flush bits.
At least the JZ4760, JZ4780, and X1000 manuals say as much. Not sure
about the JZ4770 since I don't have any documentation for that SoC.

>> ret = clk_prepare_enable(i2s->clk_i2s);
>> if (ret)
>> return ret;
>> - conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
>> - conf |= JZ_AIC_CONF_ENABLE;
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
>> + regmap_update_bits(i2s->regmap, JZ_REG_AIC_CONF,
>> + JZ_AIC_CONF_ENABLE, JZ_AIC_CONF_ENABLE);
>
> Use regmap_set_bits() when you want to set all the bits of the mask.
>
>> return 0;
>> }
>> @@ -146,14 +132,12 @@ static void jz4740_i2s_shutdown(struct
>> snd_pcm_substream *substream,
>> struct snd_soc_dai *dai)
>> {
>> struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>> - uint32_t conf;
>> if (snd_soc_dai_active(dai))
>> return;
>> - conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
>> - conf &= ~JZ_AIC_CONF_ENABLE;
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
>> + regmap_update_bits(i2s->regmap, JZ_REG_AIC_CONF,
>> + JZ_AIC_CONF_ENABLE, 0);
>
> Use regmap_clear_bits() when you want to clear all bits of the mask.
>
> Otherwise, looks fairly good!
>
> Cheers,
> -Paul
>

Thanks, I didn't know about set/clear bits but that'll make it even
simpler.

>> clk_disable_unprepare(i2s->clk_i2s);
>> }
>> @@ -162,8 +146,6 @@ static int jz4740_i2s_trigger(struct snd_pcm_substream
>> *substream, int cmd,
>> struct snd_soc_dai *dai)
>> {
>> struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>> -
>> - uint32_t ctrl;
>> uint32_t mask;
>> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> @@ -171,38 +153,30 @@ static int jz4740_i2s_trigger(struct snd_pcm_substream
>> *substream, int cmd,
>> else
>> mask = JZ_AIC_CTRL_ENABLE_CAPTURE | JZ_AIC_CTRL_ENABLE_RX_DMA;
>> - ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>> -
>> switch (cmd) {
>> case SNDRV_PCM_TRIGGER_START:
>> case SNDRV_PCM_TRIGGER_RESUME:
>> case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> - ctrl |= mask;
>> + regmap_update_bits(i2s->regmap, JZ_REG_AIC_CTRL, mask, mask);
>> break;
>> case SNDRV_PCM_TRIGGER_STOP:
>> case SNDRV_PCM_TRIGGER_SUSPEND:
>> case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> - ctrl &= ~mask;
>> + regmap_update_bits(i2s->regmap, JZ_REG_AIC_CTRL, mask, 0);
>> break;
>> default:
>> return -EINVAL;
>> }
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>> -
>> return 0;
>> }
>> static int jz4740_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>> {
>> struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
>> -
>> - uint32_t format = 0;
>> - uint32_t conf;
>> -
>> - conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
>> -
>> - conf &= ~(JZ_AIC_CONF_BIT_CLK_MASTER | JZ_AIC_CONF_SYNC_CLK_MASTER);
>> + const unsigned int conf_mask = JZ_AIC_CONF_BIT_CLK_MASTER |
>> + JZ_AIC_CONF_SYNC_CLK_MASTER;
>> + unsigned int conf = 0, format = 0;
>> switch (fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) {
>> case SND_SOC_DAIFMT_BP_FP:
>> @@ -238,8 +212,8 @@ static int jz4740_i2s_set_fmt(struct snd_soc_dai *dai,
>> unsigned int fmt)
>> return -EINVAL;
>> }
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_I2S_FMT, format);
>> + regmap_update_bits(i2s->regmap, JZ_REG_AIC_CONF, conf_mask, conf);
>> + regmap_write(i2s->regmap, JZ_REG_AIC_I2S_FMT, format);
>> return 0;
>> }
>> @@ -252,9 +226,9 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream
>> *substream,
>> uint32_t ctrl, div_reg;
>> int div;
>> - ctrl = jz4740_i2s_read(i2s, JZ_REG_AIC_CTRL);
>> + regmap_read(i2s->regmap, JZ_REG_AIC_CTRL, &ctrl);
>> + regmap_read(i2s->regmap, JZ_REG_AIC_CLK_DIV, &div_reg);
>> - div_reg = jz4740_i2s_read(i2s, JZ_REG_AIC_CLK_DIV);
>> div = clk_get_rate(i2s->clk_i2s) / (64 * params_rate(params));
>> switch (params_format(params)) {
>> @@ -291,8 +265,8 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream
>> *substream,
>> }
>> }
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CTRL, ctrl);
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CLK_DIV, div_reg);
>> + regmap_write(i2s->regmap, JZ_REG_AIC_CTRL, ctrl);
>> + regmap_write(i2s->regmap, JZ_REG_AIC_CLK_DIV, div_reg);
>> return 0;
>> }
>> @@ -329,12 +303,10 @@ static int jz4740_i2s_set_sysclk(struct snd_soc_dai
>> *dai, int clk_id,
>> static int jz4740_i2s_suspend(struct snd_soc_component *component)
>> {
>> struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
>> - uint32_t conf;
>> if (snd_soc_component_active(component)) {
>> - conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
>> - conf &= ~JZ_AIC_CONF_ENABLE;
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
>> + regmap_update_bits(i2s->regmap, JZ_REG_AIC_CONF,
>> + JZ_AIC_CONF_ENABLE, 0);
>> clk_disable_unprepare(i2s->clk_i2s);
>> }
>> @@ -347,7 +319,6 @@ static int jz4740_i2s_suspend(struct snd_soc_component
>> *component)
>> static int jz4740_i2s_resume(struct snd_soc_component *component)
>> {
>> struct jz4740_i2s *i2s = snd_soc_component_get_drvdata(component);
>> - uint32_t conf;
>> int ret;
>> ret = clk_prepare_enable(i2s->clk_aic);
>> @@ -361,9 +332,8 @@ static int jz4740_i2s_resume(struct snd_soc_component
>> *component)
>> return ret;
>> }
>> - conf = jz4740_i2s_read(i2s, JZ_REG_AIC_CONF);
>> - conf |= JZ_AIC_CONF_ENABLE;
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
>> + regmap_update_bits(i2s->regmap, JZ_REG_AIC_CONF,
>> + JZ_AIC_CONF_ENABLE, JZ_AIC_CONF_ENABLE);
>> }
>> return 0;
>> @@ -396,8 +366,8 @@ static int jz4740_i2s_dai_probe(struct snd_soc_dai *dai)
>> JZ_AIC_CONF_INTERNAL_CODEC;
>> }
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
>> - jz4740_i2s_write(i2s, JZ_REG_AIC_CONF, conf);
>> + regmap_write(i2s->regmap, JZ_REG_AIC_CONF, JZ_AIC_CONF_RESET);
>> + regmap_write(i2s->regmap, JZ_REG_AIC_CONF, conf);
>> return 0;
>> }
>> @@ -495,11 +465,19 @@ static const struct of_device_id jz4740_of_matches[] =
>> {
>> };
>> MODULE_DEVICE_TABLE(of, jz4740_of_matches);
>> +static const struct regmap_config jz4740_i2s_regmap_config = {
>> + .reg_bits = 32,
>> + .reg_stride = 4,
>> + .val_bits = 32,
>> + .max_register = JZ_REG_AIC_FIFO,
>> +};
>> +
>> static int jz4740_i2s_dev_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> struct jz4740_i2s *i2s;
>> struct resource *mem;
>> + void __iomem *regs;
>> int ret;
>> i2s = devm_kzalloc(dev, sizeof(*i2s), GFP_KERNEL);
>> @@ -508,9 +486,9 @@ static int jz4740_i2s_dev_probe(struct platform_device
>> *pdev)
>> i2s->soc_info = device_get_match_data(dev);
>> - i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
>> - if (IS_ERR(i2s->base))
>> - return PTR_ERR(i2s->base);
>> + regs = devm_platform_get_and_ioremap_resource(pdev, 0, &mem);
>> + if (IS_ERR(regs))
>> + return PTR_ERR(regs);
>> i2s->playback_dma_data.maxburst = 16;
>> i2s->playback_dma_data.addr = mem->start + JZ_REG_AIC_FIFO;
>> @@ -526,6 +504,11 @@ static int jz4740_i2s_dev_probe(struct platform_device
>> *pdev)
>> if (IS_ERR(i2s->clk_i2s))
>> return PTR_ERR(i2s->clk_i2s);
>> + i2s->regmap = devm_regmap_init_mmio(&pdev->dev, regs,
>> + &jz4740_i2s_regmap_config);
>> + if (IS_ERR(i2s->regmap))
>> + return PTR_ERR(i2s->regmap);
>> +
>> platform_set_drvdata(pdev, i2s);
>> ret = devm_snd_soc_register_component(dev, &jz4740_i2s_component,
>> --
>> 2.35.1
>>