On Fri, Apr 14, 2023 at 04:01:55PM +0200, Paweł Anikiel wrote:
.../boot/dts/socfpga_arria10_chameleonv3.dts | 28 ++
Updates to the DT should be in a separate patch.
sound/soc/chameleonv3/chv3-audio.c | 111 ++++++ sound/soc/chameleonv3/chv3-i2s.c | 347 ++++++++++++++++++ sound/soc/chameleonv3/chv3-it68051.c | 41 +++
The machine driver and board drivers (if needed) should also be separate patches - one patch per driver.
+config SND_SOC_CHV3
tristate "SoC Audio support for Chameleon v3"select SND_SOC_SSM2602select SND_SOC_SSM2602_I2ChelpSay Y if you want to add audio support for the Chameleon v3.
It woudl be better to have a separate selectable symbol for each drier.
+static int chv3_ssm2603_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
- struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
- struct snd_soc_dai *dai = asoc_rtd_to_codec(rtd, 0);
- return snd_soc_dai_set_sysclk(dai, 0, 22579200, SND_SOC_CLOCK_IN);
+}
This could be done once at init, though in general I can't tell why this isn't audio-graph-card.
- Because of the two pointer design, the ring buffer can never be full. With
- capture this isn't a problem, because the hardware being the producer
- will wait for the consumer index to move out of the way. With playback,
- however, this is problematic, because ALSA wants to fill up the buffer
- completely when waiting for hardware. In the .ack callback, the driver
- would have to wait for the consumer index to move out of the way by
- busy-waiting, which would keep stalling the kernel for quite a long time.
- The workaround to this problem is to "lie" to ALSA that the hw_pointer
- is one period behind what it actually is (see chv3_dma_pointer). This
- way, ALSA will not try to fill up the entire buffer, and all callbacks
- are wait-free.
Would it not be better to just lag by one (or some small number of) sample instead?
+static irqreturn_t chv3_i2s_isr(int irq, void *data) +{
- struct chv3_i2s_dev *i2s = data;
- u32 reg;
- reg = readl(i2s->iobase_irq + I2S_IRQ_CLR);
- if (!reg)
return IRQ_NONE;- if (reg & I2S_IRQ_RX_BIT)
snd_pcm_period_elapsed(i2s->rx_substream);- if (reg & I2S_IRQ_TX_BIT) {
if (i2s->tx_ready)snd_pcm_period_elapsed(i2s->tx_substream);i2s->tx_ready = 1;- }
- writel(reg, i2s->iobase_irq + I2S_IRQ_CLR);
- return IRQ_HANDLED;
+}
Really we should only ack things that were handled here and report appropriately, that's defensive against bugs causing interrupts to scream and shared interrupts.
- dev_info(&pdev->dev, "probed\n");
This is just noise, remove it.
+++ b/sound/soc/chameleonv3/chv3-it68051.c @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <linux/module.h> +#include <sound/soc.h>
+static struct snd_soc_dai_driver it68051_dai = {
- .name = "it68051-hifi",
- .capture = {
.stream_name = "Capture",.channels_min = 8,.channels_max = 8,.rates = SNDRV_PCM_RATE_CONTINUOUS,.formats = SNDRV_PCM_FMTBIT_S32_LE,- },
+};
+static const struct snd_soc_component_driver soc_component_dev_it68051 = { +};
This looks awfully like it's a generic CODEC driver for a device with no control available, why is it not being added as a CODEC?