Messages in this thread | | | Subject | Re: [PATCH v1 3/3] ASoC: amd: acp: Add legacy audio driver support for Rembrandt platform | From | "Reddy, V sujith kumar" <> | Date | Mon, 1 Aug 2022 10:41:12 +0530 |
| |
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@amd.com> >> --- >> 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@amd.com> >> +// V sujith kumar Reddy <Vsujithkumar.Reddy@amd.com> >> +/* >> + * 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; >> +} > > [...] >
| |