On 7/31/2022 9:11 PM, Christophe JAILLET wrote:
[CAUTION: External Email]
Hi,
this patch has already reached -next, but a few nit below.
Le 07/07/2022 à 18:11, V sujith kumar Reddy a écrit :
Add i2s and dmic support for Rembrandt platform,
Add machine support for nau8825, max98360 and rt5682s,rt1019 codec
in legacy driver for rembrandt platform.
Here codec is in a slave mode.
Signed-off-by: V sujith kumar Reddy <Vsujithkumar.Reddy@xxxxxxx>
---
sound/soc/amd/acp/Kconfig | 11 +
sound/soc/amd/acp/Makefile | 2 +
sound/soc/amd/acp/acp-i2s.c | 137 ++++++++-
sound/soc/amd/acp/acp-legacy-mach.c | 32 +++
sound/soc/amd/acp/acp-mach-common.c | 86 +++++-
sound/soc/amd/acp/acp-mach.h | 6 +
sound/soc/amd/acp/acp-pci.c | 6 +
sound/soc/amd/acp/acp-platform.c | 16 +-
sound/soc/amd/acp/acp-rembrandt.c | 401 +++++++++++++++++++++++++++
sound/soc/amd/acp/amd.h | 62 ++++-
sound/soc/amd/acp/chip_offset_byte.h | 28 ++
11 files changed, 781 insertions(+), 6 deletions(-)
create mode 100644 sound/soc/amd/acp/acp-rembrandt.c
[...]
diff --git a/sound/soc/amd/acp/acp-rembrandt.c
b/sound/soc/amd/acp/acp-rembrandt.c
new file mode 100644
index 000000000000..2b57c0ca4e99
--- /dev/null
+++ b/sound/soc/amd/acp/acp-rembrandt.c
@@ -0,0 +1,401 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+//
+// This file is provided under a dual BSD/GPLv2 license. When using or
+// redistributing this file, you may do so under either license.
These lines are useless. There is already a SPDX-License-Identifier just
above.
+//
+// Copyright(c) 2022 Advanced Micro Devices, Inc.
+//
+// Authors: Ajit Kumar Pandey <AjitKumar.Pandey@xxxxxxx>
+// V sujith kumar Reddy <Vsujithkumar.Reddy@xxxxxxx>
+/*
+ * Hardware interface for Renoir ACP block
+ */
+
[...]
+static int rembrandt_audio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct acp_chip_info *chip;
+ struct acp_dev_data *adata;
+ struct resource *res;
+
+ chip = dev_get_platdata(&pdev->dev);
+ if (!chip || !chip->base) {
+ dev_err(&pdev->dev, "ACP chip data is NULL\n");
+ return -ENODEV;
+ }
+
+ if (chip->acp_rev != ACP6X_DEV) {
+ dev_err(&pdev->dev, "Un-supported ACP Revision %d\n",
chip->acp_rev);
+ return -ENODEV;
+ }
+
+ rmb_acp_init(chip->base);
Should rmb_acp_deinit() be called if an error occurs below?
Or a devm_add_action_or_reset() + .remove() simplification?
(this is called in the .remove() function)
Yes,we can check the error status ,we will do it up in a cleanup patch.
+
+ adata = devm_kzalloc(dev, sizeof(struct acp_dev_data),
GFP_KERNEL);
+ if (!adata)
+ return -ENOMEM;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
"acp_mem");
+ if (!res) {
+ dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
+ return -ENODEV;
+ }
+
+ adata->acp_base = devm_ioremap(&pdev->dev, res->start,
resource_size(res));
+ if (!adata->acp_base)
+ return -ENOMEM;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
"acp_dai_irq");
+ if (!res) {
+ dev_err(&pdev->dev, "IORESOURCE_IRQ FAILED\n");
+ return -ENODEV;
+ }
+
+ adata->i2s_irq = res->start;
+ adata->dev = dev;
+ adata->dai_driver = acp_rmb_dai;
+ adata->num_dai = ARRAY_SIZE(acp_rmb_dai);
+ adata->rsrc = &rsrc;
+
+ adata->machines = snd_soc_acpi_amd_rmb_acp_machines;
+ acp_machine_select(adata);
+
+ dev_set_drvdata(dev, adata);
+ acp6x_enable_interrupts(adata);
+ acp_platform_register(dev);
+
+ return 0;
+}
+
+static int rembrandt_audio_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct acp_dev_data *adata = dev_get_drvdata(dev);
+ struct acp_chip_info *chip;
+
+ chip = dev_get_platdata(&pdev->dev);
+ if (!chip || !chip->base) {
+ dev_err(&pdev->dev, "ACP chip data is NULL\n");
+ return -ENODEV;
+ }
These tests and dev_err and return look useless.
The same is already tested at the biginning of the probe and if it
fails, the probe will fail, right?
yes ,agreed we will do it in a cleanup patch as it is merged.
+
+ rmb_acp_deinit(chip->base);
+
+ acp6x_disable_interrupts(adata);
+ acp_platform_unregister(dev);
+ return 0;
+}
[...]
