On Mon, Jun 12, 2023 at 04:53:18PM +0800, YingKun Meng wrote:
Loongson I2S controller is found on 7axxx/2kxxx chips from loongson,These don't really make sense for a new driver.
it is a PCI device with two private DMA controllers, one for playback,
the other for capture.
The driver supports the use of DTS or ACPI to describe device resources.
Reported-by: kernel test robot <lkp@xxxxxxxxx>
Closes: https://lore.kernel.org/oe-kbuild-all/202306060223.9hdivLrx-lkp@xxxxxxxxx/
Closes: https://lore.kernel.org/oe-kbuild-all/202306060320.Sphw0ihy-lkp@xxxxxxxxx/
@@ -0,0 +1,213 @@Please make the entire comment a C++ one so things look more
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Loongson-2K I2S master mode driver
+ *
+ * Copyright (C) 2022 Loongson Technology Corporation Limited
+ */
intentional. You might also want to update the copyright year if there
was any substantial work on the driver recently.
+ /* Enable master mode */Ideally you'd have a set_dai_fmt() operation and support both clock
+ regmap_update_bits(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_MASTER,
+ I2S_CTRL_MASTER);
+ if (i2s->rev_id == 1) {
+ ret = regmap_read_poll_timeout_atomic(i2s->regmap,
+ LS_I2S_CTRL, val,
+ val & I2S_CTRL_CLK_READY,
+ 10, 2000);
+ if (ret < 0)
+ dev_warn(dai->dev, "wait BCLK ready timeout\n");
+ }
provider and consumer mode.
+ if (i2s->rev_id == 0) {This looks like a switch statement.
+ } else if (i2s->rev_id == 1) {
+ } else
+ dev_err(i2s->dev, "I2S revision invalid\n");
+
You are right, PCI bus has a fixed clock.
+static int loongson_i2s_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,Should this be integrated with the clock API rather than just using the
+ unsigned int freq, int dir)
+{
+ struct loongson_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+
+ i2s->sysclk = freq;
+
+ return 0;
+}
ASoC specific stuff - where does this sysclk come from? I do note that
the PCI driver appears to have a fixed clock...
+void loongson_i2s_init(struct loongson_i2s *i2s)What's this for? I'd expect initialising the hardware to be handled
+{
+ if (i2s->rev_id == 1) {
+ regmap_write(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_RESET);
+ udelay(200);
+ }
+}
internally within the driver but this is semi-exported?
diff --git a/sound/soc/loongson/loongson_i2s_pci.c b/sound/soc/loongson/loongson_i2s_pci.cPlease split the PCI driver into a separate patch to keep the individual
new file mode 100644
index 000000000000..f09713b560e9
--- /dev/null
+++ b/sound/soc/loongson/loongson_i2s_pci.c
@@ -0,0 +1,500 @@
reviews smaller.
+static int loongson_pcm_trigger(struct snd_soc_component *component,Missing break; here.
+ struct snd_pcm_substream *substream, int cmd)
+{
+ switch (cmd) {
+ default:
+ ret = -EINVAL;
+ }
+ /* initialize dma descriptor array */We didn't validate that the number of periods fits in the array.
+ mem_addr = runtime->dma_addr;
+ order_addr = prtd->dma_desc_arr_phy;
+ for (i = 0; i < num_periods; i++) {
+ desc = &prtd->dma_desc_arr[i];
+ if (num_periods > 0) {This is always true, right?
+ desc = &prtd->dma_desc_arr[num_periods - 1];
+ desc->order = lower_32_bits(prtd->dma_desc_arr_phy | BIT(0));
+ desc->order_hi = upper_32_bits(prtd->dma_desc_arr_phy);
+ }
+ ret = request_irq(dma_data->irq, loongson_pcm_dma_irq,Does it really make sense to request and free the interrupt when the
+ IRQF_TRIGGER_HIGH, LS_I2S_DRVNAME, substream);
stream is running? It's generally better to just request the interrupt
once during probe(), that way we know we've got all the resources we
need. If we do need to allocate/free all the time a comment explaining
why would be good so people don't go trying to fix this.
+ ret = pci_request_region(pdev, BAR_NUM, LS_I2S_DRVNAME);pcim_iomap_regions()? At the minute you'll free the region before devm
+ if (ret) {
+ dev_err(&pdev->dev, "request regions failed %d\n", ret);
+ return ret;
+ }
frees the regmap which is probably harmless but not ideal.