Paul Cercueil paul@crapouillou.net writes:
Hi Aidan,
Le mer., juil. 6 2022 at 22:13:22 +0100, Aidan MacDonald aidanmacdonald.0x0@gmail.com 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 aidanmacdonald.0x0@gmail.com
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);
ctrl &= ~mask;
regmap_update_bits(i2s->regmap, JZ_REG_AIC_CTRL, mask, 0);
- 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);@@ -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);@@ -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);@@ -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