Re: [PATCH 1/9] ASoC: Add Chameleon v3 audio

[Mark Brown]

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