[RESEND v13 07/10] ASoC: qcom: Add support for codec dma driver
Srinivasa Rao Mandadapu (Temp)
quic_srivasam at quicinc.com
Sat Feb 19 19:53:39 CET 2022
On 2/18/2022 1:20 AM, Stephen Boyd wrote:
Thanks for your time and valuable comments Stephen!!!
> 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.
Okay. will change it to memremap() and re post it.
>
>>>> @@ -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?
Shall we use it as it is for now. If it's really matters, we shall
update as a fresh patches on top of these patches as a fix,
after this series got accepted. Otherwise because of only this review
comment, whole series getting blocked.
>
>>>> + }
>>>> +
>>>> + 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.
Okay. will replace flag with zero and resend it.
More information about the Alsa-devel
mailing list