Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
In-Reply-To: <0093b56c-95a0-7344-1480-2473f790db90@quicinc.com>
References: <1644850708-11099-1-git-send-email-quic_srivasam@quicinc.com>
 <1644850708-11099-8-git-send-email-quic_srivasam@quicinc.com>
 <CAE-0n507RB89eoTPGUemdWh4cbcXtWkxKLt_0nCy8xGeJhb1sA@mail.gmail.com> <0093b56c-95a0-7344-1480-2473f790db90@quicinc.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Thu, 17 Feb 2022 11:50:30 -0800
Message-ID: <CAE-0n52+r5nN6HC6KQt-Yioh3r+9bgY_V-KA1yQ071-zY7qfEQ@mail.gmail.com>
Subject: Re: [RESEND v13 07/10] ASoC: qcom: Add support for codec dma driver
To:     Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>,
        agross@kernel.org, alsa-devel@alsa-project.org,
        bgoswami@codeaurora.org, bjorn.andersson@linaro.org,
        broonie@kernel.org, devicetree@vger.kernel.org,
        judyhsiao@chromium.org, lgirdwood@gmail.com,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        perex@perex.cz, quic_plai@quicinc.com, robh+dt@kernel.org,
        rohitkr@codeaurora.org, srinivas.kandagatla@linaro.org,
        tiwai@suse.com
Cc:     Venkata Prasad Potturu <quic_potturu@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Srinivasa Rao Mandadapu (2022-02-15 22:53:11)
>
> On 2/15/2022 6:57 AM, Stephen Boyd wrote:
> Thanks for your time and valuable review comments Stephen!!!
> > Quoting Srinivasa Rao Mandadapu (2022-02-14 06:58:25)
> >> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
> >> index 5d77240..12b8d40 100644
> >> --- a/sound/soc/qcom/lpass-platform.c
> >> +++ b/sound/soc/qcom/lpass-platform.c
[...]
> >
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       buf = &substream->dma_buffer;
> >> +       buf->dev.dev = pcm->card->dev;
> >> +       buf->private_data = NULL;
> >> +
> >> +       /* Assign Codec DMA buffer pointers */
> >> +       buf->dev.type = SNDRV_DMA_TYPE_CONTINUOUS;
> >> +
> >> +       switch (dai_id) {
> >> +       case LPASS_CDC_DMA_RX0 ... LPASS_CDC_DMA_RX9:
> >> +               buf->bytes = lpass_platform_rxtx_hardware.buffer_bytes_max;
> >> +               buf->addr = drvdata->rxtx_cdc_dma_lpm_buf;
> >> +               break;
> >> +       case LPASS_CDC_DMA_TX0 ... LPASS_CDC_DMA_TX8:
> >> +               buf->bytes = lpass_platform_rxtx_hardware.buffer_bytes_max;
> >> +               buf->addr = drvdata->rxtx_cdc_dma_lpm_buf + LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE;
> >> +               break;
> >> +       case LPASS_CDC_DMA_VA_TX0 ... LPASS_CDC_DMA_VA_TX8:
> >> +               buf->bytes = lpass_platform_va_hardware.buffer_bytes_max;
> >> +               buf->addr = drvdata->va_cdc_dma_lpm_buf;
> >> +               break;
> >> +       default:
> >> +               break;
> >> +       }
> >> +
> >> +       buf->area = (unsigned char * __force)ioremap(buf->addr, buf->bytes);
> > Why aren't we using the DMA mapping framework?
> Here, Need to use hardware memory, that is LPASS LPM region for codec DMA.

It does not look like iomem, so the usage of ioremap() is wrong. I
understand that it is some place inside the audio subsystem used to DMA.
ioremap() memory should be accessed through the io accessors,
readl/writel, ioread/iowrite.

> >> @@ -827,6 +1207,31 @@ static int lpass_platform_pcmops_resume(struct snd_soc_component *component)
> >>          return regcache_sync(map);
> >>   }
> >>
> >> +static int lpass_platform_copy(struct snd_soc_component *component,
> >> +                              struct snd_pcm_substream *substream, int channel,
> >> +                              unsigned long pos, void __user *buf, unsigned long bytes)
> >> +{
> >> +       struct snd_pcm_runtime *rt = substream->runtime;
> >> +       unsigned int dai_id = component->id;
> >> +       int ret = 0;
> >> +
> >> +       void __iomem *dma_buf = rt->dma_area + pos +
> >> +                               channel * (rt->dma_bytes / rt->channels);
> >> +
> >> +       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> >> +               if (is_cdc_dma_port(dai_id))
> >> +                       ret = copy_from_user_toio(dma_buf, buf, bytes);
> >> +               else
> >> +                       ret = copy_from_user((void __force *)dma_buf, buf, bytes);
> >> +       } else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> >> +               if (is_cdc_dma_port(dai_id))
> >> +                       ret = copy_to_user_fromio(buf, dma_buf, bytes);
> >> +               else
> >> +                       ret = copy_to_user(buf, (void __force *)dma_buf, bytes);
> > Having __force in here highlights the lack of DMA API usage. I guess
> > there's a sound dma wrapper library in sound/core/memalloc.c? Why can't
> > that be used?
> Didn't see any memcopy wrapper functions in memalloc.c. Could You please
> elaborate or share some example.

Can you add some memcpy wrappers to memalloc.c? Or implement the copy
wrapper you need?

> >
> >> +       }
> >> +
> >> +       return ret;
> >> +}
> >>
> >>   static const struct snd_soc_component_driver lpass_component_driver = {
> >>          .name           = DRV_NAME,
> >> @@ -837,9 +1242,11 @@ static const struct snd_soc_component_driver lpass_component_driver = {
> >>          .prepare        = lpass_platform_pcmops_prepare,
> >>          .trigger        = lpass_platform_pcmops_trigger,
> >>          .pointer        = lpass_platform_pcmops_pointer,
> >> +       .mmap           = lpass_platform_pcmops_mmap,
> >>          .pcm_construct  = lpass_platform_pcm_new,
> >>          .suspend                = lpass_platform_pcmops_suspend,
> >>          .resume                 = lpass_platform_pcmops_resume,
> >> +       .copy_user              = lpass_platform_copy,
> >>
> >>   };
> >>
> >> @@ -877,6 +1284,60 @@ int asoc_qcom_lpass_platform_register(struct platform_device *pdev)
> >>                  return ret;
> >>          }
> >>
> >> +       if (drvdata->codec_dma_enable) {
> >> +               ret = regmap_write(drvdata->rxtx_lpaif_map,
> >> +                       LPAIF_RXTX_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST), 0x0);
> >> +               if (ret) {
> >> +                       dev_err(&pdev->dev, "error writing to rxtx irqen reg: %d\n", ret);
> >> +                       return ret;
> >> +               }
> >> +               ret = regmap_write(drvdata->va_lpaif_map,
> >> +                       LPAIF_VA_IRQEN_REG(v, LPAIF_IRQ_PORT_HOST), 0x0);
> >> +               if (ret) {
> >> +                       dev_err(&pdev->dev, "error writing to rxtx irqen reg: %d\n", ret);
> >> +                       return ret;
> >> +               }
> >> +               drvdata->rxtxif_irq = platform_get_irq_byname(pdev, "lpass-irq-rxtxif");
> >> +               if (drvdata->rxtxif_irq < 0)
> >> +                       return -ENODEV;
> >> +
> >> +               ret = devm_request_irq(&pdev->dev, drvdata->rxtxif_irq,
> >> +                               lpass_platform_rxtxif_irq, IRQF_TRIGGER_RISING,
> > Drop flags and get it from firmware please.
> Same is followed in existing for other i2s and HDMI interrupts. Could
> You please give some example if it's really matters?

It matters in the case that the hardware team decides to change the pin
to falling. DT already has the flags encoded, so having a zero here
avoids conflicting with what DT has set and also alleviates us from
having to set different flags on different devices. Everyone wins. Look
around for drivers that pass 0 in place of IRQF_TRIGGER_RISING, there
are many examples.