Re: [PATCH 05/14] ASoC: amd: add ACP3x PDM platform driver - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Alex Deucher <alexdeucher@gmail.com>,
	alsa-devel@alsa-project.org, broonie@kernel.org,
	vijendar.mukunda@amd.com, tiwai@suse.de
Cc: Alex Deucher <alexander.deucher@amd.com>
Subject: Re: [PATCH 05/14] ASoC: amd: add ACP3x PDM platform driver
Date: Tue, 5 May 2020 17:03:18 -0500	[thread overview]
Message-ID: <3fa76374-4125-ed8a-436e-7127476baedd@linux.intel.com> (raw)
In-Reply-To: <20200505205327.642282-6-alexander.deucher@amd.com>

> +static struct snd_soc_dai_ops acp_pdm_dai_ops = {
> +	.hw_params = NULL,
> +	.trigger   = NULL,
> +};

doesn't seem very useful? can you remove this pdm_dai_ops?

> +
> +static struct snd_soc_dai_driver acp_pdm_dai_driver = {
> +	.capture = {
> +		.rates = SNDRV_PCM_RATE_48000,
> +		.formats = SNDRV_PCM_FMTBIT_S24_LE |
> +			   SNDRV_PCM_FMTBIT_S32_LE,
> +		.channels_min = 2,
> +		.channels_max = 2,
> +		.rate_min = 48000,
> +		.rate_max = 48000,
> +	},
> +	.ops = &acp_pdm_dai_ops,
> +};
> +
> +static const struct snd_soc_component_driver acp_pdm_component = {
> +	.name		= DRV_NAME,
> +};
> +
> +static int acp_pdm_audio_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct pdm_dev_data *adata;
> +	unsigned int irqflags;
> +	int status;
> +
> +	if (!pdev->dev.platform_data) {
> +		dev_err(&pdev->dev, "platform_data not retrieved\n");
> +		return -ENODEV;
> +	}

is this test needed?

> +	irqflags = *((unsigned int *)(pdev->dev.platform_data));
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
> +		return -ENODEV;
> +	}
> +
> +	adata = devm_kzalloc(&pdev->dev, sizeof(*adata), GFP_KERNEL);
> +	if (!adata)
> +		return -ENOMEM;
> +
> +	adata->acp_base = devm_ioremap(&pdev->dev, res->start,
> +				       resource_size(res));
> +	if (!adata->acp_base)
> +		return -ENOMEM;
> +
> +	adata->capture_stream = NULL;
> +
> +	dev_set_drvdata(&pdev->dev, adata);
> +	status = devm_snd_soc_register_component(&pdev->dev,
> +						 &acp_pdm_component,
> +						 &acp_pdm_dai_driver, 1);
> +	if (status) {
> +		dev_err(&pdev->dev, "Fail to register acp pdm dai\n");
> +
> +		return -ENODEV;

return status;

> +	}
> +	return 0;
> +}
> +
> +static int acp_pdm_audio_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}

not needed?

> +
> +static struct platform_driver acp_pdm_dma_driver = {
> +	.probe = acp_pdm_audio_probe,
> +	.remove = acp_pdm_audio_remove,
> +	.driver = {
> +		.name = "acp_rn_pdm_dma",
> +	},
> +};
> +
> +module_platform_driver(acp_pdm_dma_driver);