Message-ID: <818629a3-c934-d0d7-b872-9cf82ab49b98@loongson.cn>
Date:   Wed, 14 Jun 2023 17:05:54 +0800
From:   Yingkun Meng <mengyingkun@...ngson.cn>
To:     Mark Brown <broonie@...nel.org>
Cc:     lgirdwood@...il.com, linux-kernel@...r.kernel.org,
        alsa-devel@...a-project.org, loongarch@...ts.linux.dev,
        loongson-kernel@...ts.loongnix.cn,
        kernel test robot <lkp@...el.com>
Subject: Re: [ PATCH v2 1/3] ASoC: Add support for Loongson I2S controller


On 2023/6/13 02:52, Mark Brown wrote:
> 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@...el.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202306060223.9hdivLrx-lkp@intel.com/
>> Closes: https://lore.kernel.org/oe-kbuild-all/202306060320.Sphw0ihy-lkp@intel.com/
> These don't really make sense for a new driver.
>

OK, removed in new version.

>> @@ -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.

OK.

>> +		/* 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.

Got it, add .set_fmt callback for DAI ops in new version.

>> +	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.


OK, replaced with a switch statement in new version.

>
>> +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...
>
You are right, PCI bus has a fixed clock.

On loongson 7A2000/2K2000 chips, the I2S controller has PCI configuration
address space, IO address space, and memory address space, it works as
a PCI device, but it's not a standard PCI device.

Its clock signal comes from the APB bus or AIX bus, not from PCI bus.
Currently, there is no good support from clock subsystem, the frequency of
the I2S controller's driver clock is a fixed value.

>> +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?


On 2K1000LA chip, the i2s controller is a platform device, it's supported
by another driver. My original purpose was to export an API to initializing
hardware.
It's OK to move this to specific I2S driver.

>> 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.


OK, the driver split into PCI driver and DMA driver in new version.

>> +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.


Fixed. Return directly.

>
>> +	/* 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.
>

Fixed. Check the number of periods before initializing the descriptor 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?


Yes, remove the condition check.

>> +       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.

No, we can request the interrupt just once.

As one DMA controller controls the transmission of audio data in one direction
(playback or capture), it is associated with a substream.
The operation requesting interrupt is placed in .pcm_construct callback of
snd_soc_component_driver in new version.

>> +	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.


OK, replaced in new version.

Powered by blists - more mailing lists