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_SSM2602
> + select SND_SOC_SSM2602_I2C
> + help
> + Say 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?
Attachment:
signature.asc
Description: PGP signature
