From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Rohit kumar <rohitkr@codeaurora.org>,
agross@kernel.org, bjorn.andersson@linaro.org,
lgirdwood@gmail.com, broonie@kernel.org, robh+dt@kernel.org,
plai@codeaurora.org, bgoswami@codeaurora.org, perex@perex.cz,
tiwai@suse.com, linux-arm-msm@vger.kernel.org,
alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/8] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers
Date: Thu, 9 Jul 2020 10:26:54 +0100 [thread overview]
Message-ID: <a59dc25f-cac2-5540-f1f4-78fd749c65c9@linaro.org> (raw)
In-Reply-To: <1594184896-10629-4-git-send-email-rohitkr@codeaurora.org>
On 08/07/2020 06:08, Rohit kumar wrote:
> I2SCTL and DMACTL registers has different bits alignment for newer
> LPASS variants of SC7180 soc. Use REG_FIELD_ID() to define the
> reg_fields in platform specific file and removed shifts and mask
> macros for such registers from header file.
>
> Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
Thanks Rohit for doing this, this looks much better now!
I have few minor comments..
> ---
> sound/soc/qcom/lpass-apq8016.c | 24 ++++++
> sound/soc/qcom/lpass-cpu.c | 163 +++++++++++++++++++++++----------------
> sound/soc/qcom/lpass-ipq806x.c | 24 ++++++
> sound/soc/qcom/lpass-lpaif-reg.h | 157 +++++++++++++++++++------------------
> sound/soc/qcom/lpass-platform.c | 151 +++++++++++++++++++++++++++---------
> sound/soc/qcom/lpass.h | 53 +++++++++++++
> 6 files changed, 398 insertions(+), 174 deletions(-)
>
index f0c7e93..f358d12 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -29,6 +29,32 @@
> #define LPASS_CPU_I2S_SD0_1_2_MASK GENMASK(2, 0)
> #define LPASS_CPU_I2S_SD0_1_2_3_MASK GENMASK(3, 0)
>
> static int lpass_cpu_daiops_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> unsigned int freq, int dir)
> {
> @@ -79,12 +105,13 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> {
> struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> + struct lpaif_i2sctl *i2sctl = drvdata->i2sctl;
> + unsigned int id = dai->driver->id;
> snd_pcm_format_t format = params_format(params);
> unsigned int channels = params_channels(params);
> unsigned int rate = params_rate(params);
> unsigned int mode;
> - unsigned int regval;
> - int bitwidth, ret;
> + int bitwidth, ret, regval;
Why is this change?
>
> bitwidth = snd_pcm_format_width(format);
> if (bitwidth < 0) {
> @@ -92,28 +119,45 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
> return bitwidth;
> }
>
> - regval = LPAIF_I2SCTL_LOOPBACK_DISABLE |
> - LPAIF_I2SCTL_WSSRC_INTERNAL;
> + ret = regmap_fields_write(i2sctl->loopback, id,
> + LPAIF_I2SCTL_LOOPBACK_DISABLE);
> + if (ret) {
> + dev_err(dai->dev, "error updating loopback field: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regmap_fields_write(i2sctl->wssrc, id,
> + LPAIF_I2SCTL_WSSRC_INTERNAL);
> + if (ret) {
> + dev_err(dai->dev, "error updating wssrc field: %d\n", ret);
> + return ret;
> + }
>
> switch (bitwidth) {
> case 16:
> - regval |= LPAIF_I2SCTL_BITWIDTH_16;
> + regval = LPAIF_I2SCTL_BITWIDTH_16;
> break;
> case 24:
> - regval |= LPAIF_I2SCTL_BITWIDTH_24;
> + regval = LPAIF_I2SCTL_BITWIDTH_24;
> break;
> case 32:
> - regval |= LPAIF_I2SCTL_BITWIDTH_32;
> + regval = LPAIF_I2SCTL_BITWIDTH_32;
> break;
> default:
> dev_err(dai->dev, "invalid bitwidth given: %d\n", bitwidth);
> return -EINVAL;
> }
>
> + ret = regmap_fields_write(i2sctl->bitwidth, id, regval);
> + if (ret) {
> + dev_err(dai->dev, "error updating bitwidth field: %d\n", ret);
> + return ret;
> + }
> +
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> - mode = drvdata->mi2s_playback_sd_mode[dai->driver->id];
> + mode = drvdata->mi2s_playback_sd_mode[id];
> else
> - mode = drvdata->mi2s_capture_sd_mode[dai->driver->id];
> + mode = drvdata->mi2s_capture_sd_mode[id];
>
> if (!mode) {
> dev_err(dai->dev, "no line is assigned\n");
> @@ -175,30 +219,34 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
> }
>
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> - regval |= LPAIF_I2SCTL_SPKMODE(mode);
> + ret = regmap_fields_write(i2sctl->spkmode, id,
> + LPAIF_I2SCTL_SPKMODE(mode));
>
> if (channels >= 2)
> - regval |= LPAIF_I2SCTL_SPKMONO_STEREO;
> + ret = regmap_fields_write(i2sctl->spkmono, id,
> + LPAIF_I2SCTL_SPKMONO_STEREO);
> else
> - regval |= LPAIF_I2SCTL_SPKMONO_MONO;
> + ret = regmap_fields_write(i2sctl->spkmono, id,
> + LPAIF_I2SCTL_SPKMONO_MONO);
> } else {
> - regval |= LPAIF_I2SCTL_MICMODE(mode);
> + ret = regmap_fields_write(i2sctl->micmode, id,
> + LPAIF_I2SCTL_MICMODE(mode));
>
> if (channels >= 2)
> - regval |= LPAIF_I2SCTL_MICMONO_STEREO;
> + ret = regmap_fields_write(i2sctl->micmono, id,
> + LPAIF_I2SCTL_MICMONO_STEREO);
> else
> - regval |= LPAIF_I2SCTL_MICMONO_MONO;
> + ret = regmap_fields_write(i2sctl->micmono, id,
> + LPAIF_I2SCTL_MICMONO_MONO);
> }
>
> - ret = regmap_write(drvdata->lpaif_map,
> - LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
> - regval);
> if (ret) {
> - dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret);
> + dev_err(dai->dev, "error writing to i2sctl channels mode: %d\n",
> + ret);
> return ret;
> }
>
> - ret = clk_set_rate(drvdata->mi2s_bit_clk[dai->driver->id],
> + ret = clk_set_rate(drvdata->mi2s_bit_clk[id],
> rate * bitwidth * 2);
> if (ret) {
> dev_err(dai->dev, "error setting mi2s bitclk to %u: %d\n",
> @@ -209,41 +257,24 @@ static int lpass_cpu_daiops_hw_params(struct snd_pcm_substream *substream,
> return 0;
> }
>
> -static int lpass_cpu_daiops_hw_free(struct snd_pcm_substream *substream,
> - struct snd_soc_dai *dai)
> -{
> - struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> - int ret;
> -
> - ret = regmap_write(drvdata->lpaif_map,
> - LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
> - 0);
> - if (ret)
> - dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret);
> -
> - return ret;
> -}
Any particular reason why this function remove
> -
> static int lpass_cpu_daiops_prepare(struct snd_pcm_substream *substream,
> struct snd_soc_dai *dai)
> {
> struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
> + struct lpaif_i2sctl *i2sctl = drvdata->i2sctl;
> + unsigned int id = dai->driver->id;
> int ret;
> - unsigned int val, mask;
>
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> - val = LPAIF_I2SCTL_SPKEN_ENABLE;
> - mask = LPAIF_I2SCTL_SPKEN_MASK;
> - } else {
> - val = LPAIF_I2SCTL_MICEN_ENABLE;
> - mask = LPAIF_I2SCTL_MICEN_MASK;
> + ret = regmap_fields_write(i2sctl->spken, id,
> + LPAIF_I2SCTL_SPKEN_ENABLE);
> + } else {
> + ret = regmap_fields_write(i2sctl->micen, id,
> + LPAIF_I2SCTL_MICEN_ENABLE);
> }
>
> - ret = regmap_update_bits(drvdata->lpaif_map,
> - LPAIF_I2SCTL_REG(drvdata->variant, dai->driver->id),
> - mask, val);
> if (ret)
> - dev_err(dai->dev, "error writing to i2sctl reg: %d\n", ret);
> + dev_err(dai->dev, "error writing to i2sctl enable: %d\n", ret);
>
> return ret;
> }
...
> @@ -304,7 +326,6 @@ const struct snd_soc_dai_ops asoc_qcom_lpass_cpu_dai_ops = {
> .startup = lpass_cpu_daiops_startup,
> .shutdown = lpass_cpu_daiops_shutdown,
> .hw_params = lpass_cpu_daiops_hw_params,
> - .hw_free = lpass_cpu_daiops_hw_free,
> .prepare = lpass_cpu_daiops_prepare,
> .trigger = lpass_cpu_daiops_trigger,
> };
> @@ -599,6 +620,18 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
> }
> }
>
> + /* Allocation for i2sctl regmap fields */
> + drvdata->i2sctl = devm_kzalloc(&pdev->dev, sizeof(struct lpaif_i2sctl),
> + GFP_KERNEL);
> +
> + /* Initialize bitfields for dai I2SCTL register */
> + ret = lpass_cpu_init_i2sctl_bitfields(dev, drvdata->i2sctl,
> + drvdata->lpaif_map);
> + if (ret) {
> + dev_err(dev, "error init i2sctl field: %d\n", ret);
> + return ret;
> + }
> +
> ret = devm_snd_soc_register_component(dev,
> &lpass_cpu_comp_driver,
> variant->dai_driver,
> diff --git a/sound/soc/qcom/lpass-lpaif-reg.h b/sound/soc/qcom/lpass-lpaif-reg.h
> index 72a3e2f..5258e60 100644
> --- a/sound/soc/qcom/lpass-lpaif-reg.h
> +++ b/sound/soc/qcom/lpass-lpaif-reg.h
> @@ -12,15 +12,12 @@
...
> #endif /* __LPASS_LPAIF_REG_H__ */
> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
> index 34f7fd1..445ca193 100644
> --- a/sound/soc/qcom/lpass-platform.c
> +++ b/sound/soc/qcom/lpass-platform.c
> @@ -50,6 +50,53 @@ static const struct snd_pcm_hardware lpass_platform_pcm_hardware = {
> .fifo_size = 0,
> };
>
...
> static int lpass_platform_pcmops_open(struct snd_soc_component *component,
> struct snd_pcm_substream *substream)
> {
> @@ -59,9 +106,9 @@ static int lpass_platform_pcmops_open(struct snd_soc_component *component,
> struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
> struct lpass_variant *v = drvdata->variant;
> int ret, dma_ch, dir = substream->stream;
> - struct lpass_pcm_data *data;
> + struct lpass_pcm_data *data = NULL;
>
> - data = devm_kzalloc(soc_runtime->dev, sizeof(*data), GFP_KERNEL);
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
Does this change belong in this patch?
> if (!data)
> return -ENOMEM;
>
> @@ -111,13 +158,13 @@ static int lpass_platform_pcmops_close(struct snd_soc_component *component,
> struct snd_pcm_runtime *runtime = substream->runtime;
> struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
> struct lpass_variant *v = drvdata->variant;
> - struct lpass_pcm_data *data;
> + struct lpass_pcm_data *data = runtime->private_data;
>
> - data = runtime->private_data;
> drvdata->substream[data->dma_ch] = NULL;
> if (v->free_dma_channel)
> v->free_dma_channel(drvdata, data->dma_ch);
>
> + kfree(data);
> return 0;
> }
>
>
> return devm_snd_soc_register_component(&pdev->dev,
> &lpass_component_driver, NULL, 0);
> diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
> index 450020e..4294ec2 100644
> --- a/sound/soc/qcom/lpass.h
> +++ b/sound/soc/qcom/lpass.h
> @@ -17,6 +17,28 @@
> #define LPASS_MAX_MI2S_PORTS (8)
> #define LPASS_MAX_DMA_CHANNELS (8)
>
...
> /* Both the CPU DAI and platform drivers will access this data */
> struct lpass_data {
>
> @@ -55,6 +77,10 @@ struct lpass_data {
> struct clk_bulk_data *clks;
> int num_clks;
>
> + /* Regmap fields of I2SCTL & DMACTL registers bitfields */
> + struct lpaif_i2sctl *i2sctl;
> + struct lpaif_dmactl *rd_dmactl;
> + struct lpaif_dmactl *wr_dmactl;
> };
>
> /* Vairant data per each SOC */
> @@ -72,6 +98,33 @@ struct lpass_variant {
> u32 wrdma_reg_stride;
> u32 wrdma_channels;
>
Above two along with rddma members can be removed, these become
redundant after adding regmap field!
> + /* I2SCTL Register fields */
> + struct reg_field loopback;
> + struct reg_field spken;
> + struct reg_field spkmode;
> + struct reg_field spkmono;
> + struct reg_field micen;
> + struct reg_field micmode;
> + struct reg_field micmono;
> + struct reg_field wssrc;
> + struct reg_field bitwidth;
> +
> + /* RD_DMA Register fields */
> + struct reg_field rdma_bursten;
> + struct reg_field rdma_wpscnt;
> + struct reg_field rdma_intf;
> + struct reg_field rdma_fifowm;
> + struct reg_field rdma_enable;
> + struct reg_field rdma_dyncclk;
> +
> + /* RD_DMA Register fields */
> + struct reg_field wrdma_bursten;
> + struct reg_field wrdma_wpscnt;
> + struct reg_field wrdma_intf;
> + struct reg_field wrdma_fifowm;
> + struct reg_field wrdma_enable;
> + struct reg_field wrdma_dyncclk;
> +
> /**
> * on SOCs like APQ8016 the channel control bits start
> * at different offset to ipq806x
>
next prev parent reply other threads:[~2020-07-09 9:29 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-08 5:08 [PATCH v3 0/8] ASoC: qcom: Add support for SC7180 lpass variant Rohit kumar
2020-07-08 5:08 ` [PATCH v3 1/8] ASoC: qcom: Add common array to initialize soc based core clocks Rohit kumar
2020-07-09 9:26 ` Srinivas Kandagatla
2020-07-08 5:08 ` [PATCH v3 2/8] ASoC: qcom: lpass-cpu: Move ahbix clk to platform specific function Rohit kumar
2020-07-09 9:26 ` Srinivas Kandagatla
2020-07-08 5:08 ` [PATCH v3 3/8] ASoC: qcom: lpass: Use regmap_field for i2sctl and dmactl registers Rohit kumar
2020-07-09 9:26 ` Srinivas Kandagatla [this message]
2020-07-09 9:57 ` Rohit Kumar
2020-07-09 10:06 ` Srinivas Kandagatla
2020-07-09 10:14 ` Rohit Kumar
2020-07-08 5:08 ` [PATCH v3 4/8] include: dt-bindings: sound: Add sc7180-lpass bindings header Rohit kumar
2020-07-08 14:45 ` Mark Brown
2020-07-09 10:06 ` Rohit Kumar
2020-07-08 5:08 ` [PATCH v3 5/8] ASoC: qcom: lpass-platform: Replace card->dev with component->dev Rohit kumar
2020-07-08 16:50 ` Mark Brown
2020-07-09 3:46 ` Rohit Kumar
2020-07-09 9:26 ` Srinivas Kandagatla
2020-07-08 5:08 ` [PATCH v3 6/8] dt-bindings: sound: lpass-cpu: Add sc7180 lpass cpu node Rohit kumar
2020-07-09 9:27 ` Srinivas Kandagatla
2020-07-09 10:01 ` Rohit Kumar
2020-07-09 10:08 ` Srinivas Kandagatla
2020-07-09 10:12 ` Rohit Kumar
2020-07-09 10:14 ` Srinivas Kandagatla
2020-07-09 10:34 ` Mark Brown
2020-07-08 5:08 ` [PATCH v3 7/8] ASoC: qcom: lpass-sc7180: Add platform driver for lpass audio Rohit kumar
2020-07-09 9:27 ` Srinivas Kandagatla
2020-07-08 5:08 ` [PATCH v3 8/8] dt-bindings: sound: lpass-cpu: Move to yaml format Rohit kumar
2020-07-13 22:53 ` Rob Herring
2020-07-22 10:44 ` Rohit Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a59dc25f-cac2-5540-f1f4-78fd749c65c9@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=agross@kernel.org \
--cc=alsa-devel@alsa-project.org \
--cc=bgoswami@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=perex@perex.cz \
--cc=plai@codeaurora.org \
--cc=robh+dt@kernel.org \
--cc=rohitkr@codeaurora.org \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).