Re: [ PATCH v2 1/3] ASoC: Add support for Loongson I2S controller

[Mark Brown]

On Mon, Jun 12, 2023 at 04:53:18PM +0800, YingKun Meng wrote:

> Loongson I2S controller is found on 7axxx/2kxxx chips from loongson,
> 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/

These don't really make sense for a new driver.

> @@ -0,0 +1,213 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Loongson-2K I2S master mode driver
> + *
> + * Copyright (C) 2022 Loongson Technology Corporation Limited
> + */

Please make the entire comment a C++ one so things look more
intentional.  You might also want to update the copyright year if there
was any substantial work on the driver recently.

> +		/* Enable master mode */
> +		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");
> +		}

Ideally you'd have a set_dai_fmt() operation and support both clock
provider and consumer mode.  

> +	if (i2s->rev_id == 0) {

> +	} else if (i2s->rev_id == 1) {

> +	} else
> +		dev_err(i2s->dev, "I2S revision invalid\n");
> +

This looks like a switch statement.

> +static int loongson_i2s_set_dai_sysclk(struct snd_soc_dai *dai, int clk_id,
> +				       unsigned int freq, int dir)
> +{
> +	struct loongson_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> +
> +	i2s->sysclk = freq;
> +
> +	return 0;
> +}

Should this be integrated with the clock API rather than just using the
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)
> +{
> +	if (i2s->rev_id == 1) {
> +		regmap_write(i2s->regmap, LS_I2S_CTRL, I2S_CTRL_RESET);
> +		udelay(200);
> +	}
> +}

What's this for?  I'd expect initialising the hardware to be handled
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.c
> new file mode 100644
> index 000000000000..f09713b560e9
> --- /dev/null
> +++ b/sound/soc/loongson/loongson_i2s_pci.c
> @@ -0,0 +1,500 @@

Please split the PCI driver into a separate patch to keep the individual
reviews smaller.

> +static int loongson_pcm_trigger(struct snd_soc_component *component,
> +				struct snd_pcm_substream *substream, int cmd)
> +{

> +	switch (cmd) {

> +	default:
> +		ret = -EINVAL;
> +	}

Missing break; here.

> +	/* initialize dma descriptor 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];

We didn't validate that the number of periods fits in the array.

> +	if (num_periods > 0) {
> +		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);
> +	}

This is always true, right?

> +       ret = request_irq(dma_data->irq, loongson_pcm_dma_irq,
> +                         IRQF_TRIGGER_HIGH, LS_I2S_DRVNAME, substream);

Does it really make sense to request and free the interrupt when the
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);
> +	if (ret) {
> +		dev_err(&pdev->dev, "request regions failed %d\n", ret);
> +		return ret;
> +	}

pcim_iomap_regions()?  At the minute you'll free the region before devm
frees the regmap which is probably harmless but not ideal.

Attachment: signature.asc
Description: PGP signature