Re: [PATCH 1/2] ASoC: amd: acp: fix for acp platform device creation failure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/05/24 19:33, Vijendar Mukunda wrote:
> ACP pin configuration varies based on acp version.
> ACP PCI driver should read the ACP PIN config value and based on config
> value, it has to create a platform device in below two conditions.
> 1) If ACP PDM configuration is selected from BIOS and ACP PDM controller
> exists.
> 2) If ACP I2S configuration is selected from BIOS.
>
> Other than above scenarios, ACP PCI driver should skip the platform
> device creation logic, i.e. ACP PCI driver probe sequence should never
> fail if other acp pin configuration is selected. It should skip platform
> device creation logic.
>
> check_acp_pdm() function was implemented for ACP6.x platforms to check
> ACP PDM configuration. Previously, this code was safe guarded by
> FLAG_AMD_LEGACY_ONLY_DMIC flag check.
>
> This implementation breaks audio use cases for Huawei Matebooks which are
> based on ACP3.x varaint uses I2S configuration.
> In current scenario, check_acp_pdm() function returns -ENODEV value
> which results in ACP PCI driver probe failure without creating a platform
> device even in case of valid ACP pin configuration.
>
> Implement check_acp_config() as a common function which invokes platform
> specific acp pin configuration check functions for ACP3.x, ACP6.0 & ACP6.3
> & ACP7.0 variants and checks for ACP PDM controller.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218780
> Fixes: 4af565de9f8c ("ASoC: amd: acp: fix for acp pdm configuration check")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
> ---
>  sound/soc/amd/acp/acp-legacy-common.c | 96 ++++++++++++++++++++++-----
>  sound/soc/amd/acp/acp-pci.c           |  9 ++-
>  sound/soc/amd/acp/amd.h               | 10 ++-
>  sound/soc/amd/acp/chip_offset_byte.h  |  1 +
>  4 files changed, 95 insertions(+), 21 deletions(-)
>
> diff --git a/sound/soc/amd/acp/acp-legacy-common.c b/sound/soc/amd/acp/acp-legacy-common.c
> index b5aff3f230be..3be7c6d55a6f 100644
> --- a/sound/soc/amd/acp/acp-legacy-common.c
> +++ b/sound/soc/amd/acp/acp-legacy-common.c
> @@ -358,11 +358,25 @@ int smn_read(struct pci_dev *dev, u32 smn_addr)
>  }
>  EXPORT_SYMBOL_NS_GPL(smn_read, SND_SOC_ACP_COMMON);
>  
> -int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip)
> +static void check_acp3x_config(struct acp_chip_info *chip)
>  {
> -	struct acpi_device *pdm_dev;
> -	const union acpi_object *obj;
> -	u32 pdm_addr, val;
> +	u32 val;
> +
> +	val = readl(chip->base + ACP3X_PIN_CONFIG);
> +	switch (val) {
> +	case ACP_CONFIG_4:
> +		chip->is_i2s_config = true;
> +		chip->is_pdm_config = true;
> +		break;
> +	default:
> +		chip->is_pdm_config = true;
> +		break;
> +	}
> +}
> +
> +static void check_acp6x_config(struct acp_chip_info *chip)
> +{
> +	u32 val;
>  
>  	val = readl(chip->base + ACP_PIN_CONFIG);
>  	switch (val) {
> @@ -371,42 +385,94 @@ int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip)
>  	case ACP_CONFIG_6:
>  	case ACP_CONFIG_7:
>  	case ACP_CONFIG_8:
> -	case ACP_CONFIG_10:
>  	case ACP_CONFIG_11:
> +	case ACP_CONFIG_14:
> +		chip->is_pdm_config = true;
> +		break;
> +	case ACP_CONFIG_9:
> +		chip->is_i2s_config = true;
> +		break;
> +	case ACP_CONFIG_10:
>  	case ACP_CONFIG_12:
>  	case ACP_CONFIG_13:
> +		chip->is_i2s_config = true;
> +		chip->is_pdm_config = true;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void check_acp70_config(struct acp_chip_info *chip)
> +{
> +	u32 val;
> +
> +	val = readl(chip->base + ACP_PIN_CONFIG);
> +	switch (val) {
> +	case ACP_CONFIG_4:
> +	case ACP_CONFIG_5:
> +	case ACP_CONFIG_6:
> +	case ACP_CONFIG_7:
> +	case ACP_CONFIG_8:
> +	case ACP_CONFIG_11:
>  	case ACP_CONFIG_14:
> +	case ACP_CONFIG_17:
> +	case ACP_CONFIG_18:
> +		chip->is_pdm_config = true;
> +		break;
> +	case ACP_CONFIG_9:
> +		chip->is_i2s_config = true;
> +		break;
> +	case ACP_CONFIG_10:
> +	case ACP_CONFIG_12:
> +	case ACP_CONFIG_13:
> +	case ACP_CONFIG_19:
> +	case ACP_CONFIG_20:
> +		chip->is_i2s_config = true;
> +		chip->is_pdm_config = true;
>  		break;
>  	default:
> -		return -EINVAL;
> +		break;
>  	}
> +}
> +
> +void check_acp_config(struct pci_dev *pci, struct acp_chip_info *chip)
> +{
> +	struct acpi_device *pdm_dev;
> +	const union acpi_object *obj;
> +	u32 pdm_addr;
>  
>  	switch (chip->acp_rev) {
>  	case ACP3X_DEV:
>  		pdm_addr = ACP_RENOIR_PDM_ADDR;
> +		check_acp3x_config(chip);
>  		break;
>  	case ACP6X_DEV:
>  		pdm_addr = ACP_REMBRANDT_PDM_ADDR;
> +		check_acp6x_config(chip);
>  		break;
>  	case ACP63_DEV:
>  		pdm_addr = ACP63_PDM_ADDR;
> +		check_acp6x_config(chip);
>  		break;
>  	case ACP70_DEV:
>  		pdm_addr = ACP70_PDM_ADDR;
> +		check_acp70_config(chip);
>  		break;
>  	default:
> -		return -EINVAL;
> +		break;
>  	}
>  
> -	pdm_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), pdm_addr, 0);
> -	if (pdm_dev) {
> -		if (!acpi_dev_get_property(pdm_dev, "acp-audio-device-type",
> -					   ACPI_TYPE_INTEGER, &obj) &&
> -					   obj->integer.value == pdm_addr)
> -			return 0;
> +	if (chip->is_pdm_config) {
> +		pdm_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), pdm_addr, 0);
> +		if (pdm_dev) {
> +			if (!acpi_dev_get_property(pdm_dev, "acp-audio-device-type",
> +						   ACPI_TYPE_INTEGER, &obj) &&
> +						   obj->integer.value == pdm_addr)
> +				chip->is_pdm_dev = true;
> +		}
>  	}
> -	return -ENODEV;
>  }
> -EXPORT_SYMBOL_NS_GPL(check_acp_pdm, SND_SOC_ACP_COMMON);
> +EXPORT_SYMBOL_NS_GPL(check_acp_config, SND_SOC_ACP_COMMON);
>  
>  MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/sound/soc/amd/acp/acp-pci.c b/sound/soc/amd/acp/acp-pci.c
> index 5f35b90eab8d..ad320b29e87d 100644
> --- a/sound/soc/amd/acp/acp-pci.c
> +++ b/sound/soc/amd/acp/acp-pci.c
> @@ -100,7 +100,6 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
>  		ret = -EINVAL;
>  		goto release_regions;
>  	}
> -
Empty line drop is not relevant to this patch.
Will fix and re-spin the patch series.
>  	dmic_dev = platform_device_register_data(dev, "dmic-codec", PLATFORM_DEVID_NONE, NULL, 0);
>  	if (IS_ERR(dmic_dev)) {
>  		dev_err(dev, "failed to create DMIC device\n");
> @@ -119,6 +118,10 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
>  	if (ret)
>  		goto unregister_dmic_dev;
>  
> +	check_acp_config(pci, chip);
> +	if (!chip->is_pdm_dev && !chip->is_i2s_config)
> +		goto skip_pdev_creation;
> +
>  	res = devm_kcalloc(&pci->dev, num_res, sizeof(struct resource), GFP_KERNEL);
>  	if (!res) {
>  		ret = -ENOMEM;
> @@ -136,10 +139,6 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
>  		}
>  	}
>  
> -	ret = check_acp_pdm(pci, chip);
> -	if (ret < 0)
> -		goto skip_pdev_creation;
> -
>  	chip->flag = flag;
>  	memset(&pdevinfo, 0, sizeof(pdevinfo));
>  
> diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h
> index 5017e868f39b..d75b4eb34de8 100644
> --- a/sound/soc/amd/acp/amd.h
> +++ b/sound/soc/amd/acp/amd.h
> @@ -138,6 +138,9 @@ struct acp_chip_info {
>  	void __iomem *base;	/* ACP memory PCI base */
>  	struct platform_device *chip_pdev;
>  	unsigned int flag;	/* Distinguish b/w Legacy or Only PDM */
> +	bool is_pdm_dev;	/* flag set to true when ACP PDM controller exists */
> +	bool is_pdm_config;	/* flag set to true when PDM configuration is selected from BIOS */
> +	bool is_i2s_config;	/* flag set to true when I2S configuration is selected from BIOS */
>  };
>  
>  struct acp_stream {
> @@ -212,6 +215,11 @@ enum acp_config {
>  	ACP_CONFIG_13,
>  	ACP_CONFIG_14,
>  	ACP_CONFIG_15,
> +	ACP_CONFIG_16,
> +	ACP_CONFIG_17,
> +	ACP_CONFIG_18,
> +	ACP_CONFIG_19,
> +	ACP_CONFIG_20,
>  };
>  
>  extern const struct snd_soc_dai_ops asoc_acp_cpu_dai_ops;
> @@ -240,7 +248,7 @@ void restore_acp_pdm_params(struct snd_pcm_substream *substream,
>  int restore_acp_i2s_params(struct snd_pcm_substream *substream,
>  			   struct acp_dev_data *adata, struct acp_stream *stream);
>  
> -int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip);
> +void check_acp_config(struct pci_dev *pci, struct acp_chip_info *chip);
>  
>  static inline u64 acp_get_byte_count(struct acp_dev_data *adata, int dai_id, int direction)
>  {
> diff --git a/sound/soc/amd/acp/chip_offset_byte.h b/sound/soc/amd/acp/chip_offset_byte.h
> index cfd6c4d07594..18da734c0e9e 100644
> --- a/sound/soc/amd/acp/chip_offset_byte.h
> +++ b/sound/soc/amd/acp/chip_offset_byte.h
> @@ -20,6 +20,7 @@
>  #define ACP_SOFT_RESET                          0x1000
>  #define ACP_CONTROL                             0x1004
>  #define ACP_PIN_CONFIG				0x1440
> +#define ACP3X_PIN_CONFIG			0x1400
>  
>  #define ACP_EXTERNAL_INTR_REG_ADDR(adata, offset, ctrl) \
>  	(adata->acp_base + adata->rsrc->irq_reg_offset + offset + (ctrl * 0x04))




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux