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.
