Re: [PATCH 2/3] ASoC: loongson: Add Loongson Generic ASoC Sound Card Support

Re: [PATCH 2/3] ASoC: loongson: Add Loongson Generic ASoC Sound Card Support

[YingKun Meng]

Hi Brown,

Thanks for your kindly work.

On 2023/6/5 20:47, Mark Brown wrote:
> On Mon, Jun 05, 2023 at 08:09:33PM +0800, YingKun Meng wrote:
>
>> The driver supports the use of ACPI table to describe device resources.
>> On loongson 7axxx platforms, the audio device is an ACPI device.
> I'm not seeing any ACPI IDs added here, this is all DT other than this:

I use the special ACPI ID "PRP0001", it provides a means to use the existing

DT-compatible device identification in ACPI.

As described in the document "ACPI Based Device Enumeration".

>> +	if (has_acpi_companion(&pdev->dev)) {
>> +		device_property_read_string(&pdev->dev, "codec-name",
>> +						&codec_name);
>> +		for (i = 0; i < card->num_links; i++)
>> +			loongson_dai_links[i].codecs->name = codec_name;
>> +
>> +		device_property_read_string(&pdev->dev, "codec-dai-name",
>> +						&codec_dai_name);
>> +		for (i = 0; i < card->num_links; i++)
>> +			loongson_dai_links[i].codecs->dai_name = codec_dai_name;
>> +	} else {
> which doesn't look *great* from an ACPI point of view TBH.  I thought
> there were now some (semi?) standard ACPI bindings these days to support
> referencing other devices better?
Got it! I will rewrite this part.

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

[Yingkun Meng]

On 2023/6/5 21:17, Mark Brown wrote:
> On Mon, Jun 05, 2023 at 08:09:32PM +0800, YingKun Meng wrote:
>
>> +			regmap_read_poll_timeout_atomic(i2s->regmap,
>> +						LS_I2S_CTRL, val,
>> +						!(val & I2S_CTRL_MCLK_READY),
>> +						10, 2000);
> The driver is waiting for status bits to change in the regmap but...


Break condition reversed. Fixed in new version.

>
>> +		pr_err("buf len not multiply of period len\n");
> Use dev_ functions to log things please.


OK.

>> +static const struct regmap_config loongson_i2s_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 32,
>> +	.max_register = 0x110,
>> +	.cache_type = REGCACHE_FLAT,
>> +};
> ...there are no volatile registers in the regmap so we will never read
> from the hardware.  I don't understand how this can work?
>

The I2S controller has two private DMA controllers to transfer the audio
data.
Its register set is divided into two parts: I2S control registers and
DMA control registers.

1) The I2S control registers are used to config I2S parameters, accessed
by regmap API. So we don't need to read back.

2) The DMA control registers are used to maintain the status of audio
data transmission.
These registers isn't maintained by regmap. They are accessed using
readx()/writex() APIs.

>> +	i2s->reg_base = pci_iomap(pdev, BAR_NUM, 0);
>> +	if (!i2s->reg_base) {
>> +		dev_err(&pdev->dev, "pci_iomap_error\n");
>> +		ret = -EIO;
>> +		goto err_release;
>> +	}
> pcim_iomap()?


OK.

>> +	dev_set_name(&pdev->dev, "%s", loongson_i2s_dai.name);
> Don't log static information like this, it just adds noise and makes the
> boot slower.


Removed in new version. Its original purpose is to set a fixed value for
platform component name, and match this value in machine driver.

>> +	pci_disable_device(pdev);
> pcim_enable_device() too.


OK.

Re: [PATCH 3/3] ASoC: dt-bindings: Add support for Loongson audio card

[Yingkun Meng]

Hi Krzysztof,

Thanks for your kindly work.


On 2023/6/5 22:45, Krzysztof Kozlowski wrote:
> On 05/06/2023 14:09, YingKun Meng wrote:
>> From: Yingkun Meng <mengyingkun@loongson.cn>
>>
>> The audio card uses loongson I2S controller present in 7axxx/2kxxx chips
>> to transfer audio data.
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
>
> You missed at least DT list (maybe more), so this won't be tested.
> Please resend and include all necessary entries.
>

Sorry for my mistake. Fixed in new version.

>> Signed-off-by: Yingkun Meng <mengyingkun@loongson.cn>
>> ---
>>   .../sound/loongson,ls-audio-card.yaml         | 64 +++++++++++++++++++
>>   1 file changed, 64 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml b/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
>> new file mode 100644
>> index 000000000000..f1d6ee346bb3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/loongson,ls-audio-card.yaml
>> @@ -0,0 +1,64 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/sound/loongson-audio-card.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Loongson generic ASoC audio sound card.
> What is a "generic audio card"? Does it even match hardware? Bindings
> are supposed to describe hardware, which is usually very specific.
>
> Also: Drop full stop. It's a title.
>
On loongson platform, the I2S controllers connect different codecs to
form different audio devices that can be driven by the same machine driver.
The "generic audio card" refers to these audio devices.

Currently, it can match some specific hardware, such as
2k2000 + es8323: the 2k2000 is a loogson SoC with a I2S controller
7a2000 + es8288: the 7a2000 is a bridge chip with a I2S controller


>> +
>> +maintainers:
>> +  - Yingkun Meng <mengyingkun@loongson.cn>
>> +
>> +description:
>> +  Generic ASoC audio device for loongson platform.
>> +
>> +properties:
>> +  compatible:
>> +    const: loongson,ls-audio-card
>> +
>> +  model:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: User specified audio sound card name
>> +
>> +  mclk-fs:
>> +    $ref: simple-card.yaml#/definitions/mclk-fs
>> +
>> +  cpu:
>> +    description: Holds subnode which indicates cpu dai.
>> +    type: object
>> +    additionalProperties: false
>> +    properties:
>> +      sound-dai:
>> +        maxItems: 1
> In the cpu: required with sound-dai


OK.

>> +
>> +  codec:
>> +    description: Holds subnode which indicates codec dai.
>> +    type: object
>> +    additionalProperties: false
>> +    properties:
>> +      sound-dai:
>> +        maxItems: 1
> In the codec: required with sound-dai
>
> No multiple dai links? Are you sure this card is so limited?


Yes.  The audio device has only one channel.

>
> Best regards,
> Krzysztof


Thanks,

Yingkun Meng