Re: [PATCH v5 2/2] ASoC: airoha: Add support for AN7581 PCM driver

[Mark Brown]

> The Airoha AN7581 SoC provides a PCM interface for VoIP applications,
> supporting playback and capture at sample rates of 8 kHz and 16 kHz.
> The driver configures the PCM bit clock to 2.048 MHz and supports
> 8-bit and 16-bit samples, with up to 32 channels.

> +config SND_SOC_AN7581_PCM
> +	tristate "PCM support for Airoha AN7581 chip"
> +	depends on ARCH_AIROHA || COMPILE_TEST

This needs to select CONFIG_REGMAP_MMIO.

> +#include <linux/mfd/syscon.h>
> +#include <linux/pinctrl/consumer.h>

These aren't used.

> +#define     AN7581_PCM_BIT_CLK_1024KHZ		FIELD_PREP_CONST(AN7581_PCM_BIT_CLK, 0x2)
> +#define     AN7581_PCM_BIT_CLK_2048HZ		FIELD_PREP_CONST(AN7581_PCM_BIT_CLK, 0x3)

Is this really 2048Hz, not kHz?  

> +static const struct snd_pcm_hardware an7581_pcm_hardware = {
> +	.info			= SNDRV_PCM_INFO_MMAP |
> +				  SNDRV_PCM_INFO_MMAP_VALID |
> +				  SNDRV_PCM_INFO_INTERLEAVED,

The pointer callback works in chunks of descriptors so you want
SNDRV_PCM_INFO_BATCH as well.

> +	.period_bytes_min = AN7581_PCM_FRAMES_PER_DESC,

This is using a frame count to specify bytes.

> +static int an7581_pcm_setup_time_slot(struct an7581_pcm_priv *priv,
> +				      int direction, int bit_width)

We don't have anything constraining things so that the slots fit in the
BCLK.

> +static int an7581_pcm_fe_startup(struct snd_pcm_substream *substream,
> +				 struct snd_soc_dai *dai)
> +{

> +	/* On open mask interrupt and disable DMA */
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		regmap_clear_bits(priv->map, AN7581_PCM_IMR,
> +				  AN7581_PCM_IMR_TDESC_UPDATE_INT);
> +		regmap_clear_bits(priv->map, AN7581_PCM_TRDCR,
> +				  AN7581_PCM_TXDMA_ENABLE);
> +	} else {
> +		regmap_clear_bits(priv->map, AN7581_PCM_IMR,
> +				  AN7581_PCM_IMR_RDESC_UPDATE_INT);
> +		regmap_clear_bits(priv->map, AN7581_PCM_TRDCR,
> +				  AN7581_PCM_RXDMA_ENABLE);
> +	}
> +
> +	usleep_range(10000, 20000);

That's a fairly long delay for a digital part?

> +static int an7581_pcm_fe_prepare(struct snd_pcm_substream *substream,
> +				 struct snd_soc_dai *dai)
> +{

Note that we redo prepare as part of over/underrun recovery - we should
reset the hardware position as well as the driver one.

> +static int an7581_pcm_fe_hw_params(struct snd_pcm_substream *substream,
> +				   struct snd_pcm_hw_params *params,
> +				   struct snd_soc_dai *dai)
> +{

> +
> +	if (priv->users <= 1) {
> +		/* Setup Descriptor size and number */
> +		regmap_update_bits(priv->map, AN7581_PCM_TRDRSR,
> +				   AN7581_DESC_OFFSET | AN7581_DESC_SIZE,
> +				   FIELD_PREP(AN7581_DESC_OFFSET, sizeof(*desc) / sizeof(u32)) |
> +				   FIELD_PREP(AN7581_DESC_SIZE, num_desc));
> +
> +		regmap_update_bits(priv->map, AN7581_PCM_CHBFOSR,
> +				   AN7581_PCM_DESC_CHBF_OFFSET,
> +				   FIELD_PREP(AN7581_PCM_DESC_CHBF_OFFSET,
> +					      AN7581_PCM_FRAMES_PER_DESC * bit_width / 8));

This is a little confusing, the driver says it does interleaved format
but the offset programming looks odd for that?

> +
> +static struct snd_soc_dai_link an7581_pcm_dai_links[] = {
> +	/* FE */
> +	{
> +		.name = "an7581-playback",
> +		.stream_name = "an7581-playback",
> +		.trigger = {SND_SOC_DPCM_TRIGGER_POST,

> +			    SND_SOC_DPCM_TRIGGER_POST},
> +		.playback_only = 1,
> +		SND_SOC_DAILINK_REG(playback),
> +	},
> +	{
> +		.name = "an7581-capture",
> +		.stream_name = "an7581-capture",
> +		.trigger = {SND_SOC_DPCM_TRIGGER_POST,
> +			    SND_SOC_DPCM_TRIGGER_POST},
> +		.capture_only = 1,
> +		SND_SOC_DAILINK_REG(capture),
> +	},
> +};

This isn't actually a DPCM driver?

> +static struct snd_soc_card an7581_pcm_card = {
> +	.name = "an7581-pcm",
> +	.owner = THIS_MODULE,
> +	.dai_link = an7581_pcm_dai_links,
> +	.num_links = ARRAY_SIZE(an7581_pcm_dai_links),
> +};

This should be const.

> +static irqreturn_t an7581_pcm_irq_handler(int irq, void *data)
> +{
> +	struct snd_pcm_substream *substream;
> +	struct an7581_pcm_priv *priv = data;
> +	unsigned long flags;
> +	u32 status, isr = 0;
> +
> +	regmap_read(priv->map, AN7581_PCM_ISR, &isr);

> +	if (isr & AN7581_PCM_IMR_RDESC_UPDATE_INT) {

> +	if (isr & AN7581_PCM_IMR_TDESC_UPDATE_INT) {

> +	return IRQ_HANDLED;
> +}

This unconditionally returns IRQ_HANDLED regardless of if we actually
found anything, this breaks any potential sharing as well as the debug
and robustness features of genirq.

-- 
Mark Brown <broonie@xxxxxxxxxx>