Re: [PATCH V2 1/9] ASoC: amd: ps: create platform devices based on acp config

[Mukunda,Vijendar]

On 22/05/23 21:46, Pierre-Louis Bossart wrote:
>> +static int sdw_amd_scan_controller(struct device *dev)
>> +{
>> +	struct acp63_dev_data *acp_data;
>> +	struct fwnode_handle *link;
>> +	char name[32];
>> +	u32 sdw_manager_bitmap;
>> +	u8 count = 0;
>> +	u32 acp_sdw_power_mode = 0;
>> +	int index;
>> +	int ret;
>> +
>> +	acp_data = dev_get_drvdata(dev);
>> +	acp_data->acp_reset = true;
>> +	/* Found controller, find links supported */
>> +	ret = fwnode_property_read_u32_array((acp_data->sdw_fw_node), "mipi-sdw-manager-list",
>> +					     &sdw_manager_bitmap, 1);
>> +
>> +	if (ret) {
>> +		dev_err(dev, "Failed to read mipi-sdw-manager-list: %d\n", ret);
>> +		return -EINVAL;
>> +	}
>> +	count = hweight32(sdw_manager_bitmap);
>> +	/* Check count is within bounds */
>> +	if (count > AMD_SDW_MAX_MANAGERS) {
>> +		dev_err(dev, "Manager count %d exceeds max %d\n", count, AMD_SDW_MAX_MANAGERS);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!count) {
>> +		dev_dbg(dev, "No SoundWire Managers detected\n");
>> +		return -EINVAL;
>> +	}
>> +	dev_dbg(dev, "ACPI reports %d SoundWire Manager devices\n", count);
>> +	acp_data->sdw_manager_count = count;
>> +	for (index = 0; index < count; index++) {
>> +		snprintf(name, sizeof(name), "mipi-sdw-link-%d-subproperties", index);
>> +		link = fwnode_get_named_child_node(acp_data->sdw_fw_node, name);
>> +		if (!link) {
>> +			dev_err(dev, "Manager node %s not found\n", name);
>> +			return -EIO;
>> +		}
>> +
>> +		fwnode_property_read_u32(link, "amd-sdw-power-mode",
>> +					 &acp_sdw_power_mode);
> What happens if this property is not found or doesn't exist?
>
> acp_sdw_power_mode is zero, so....
>
If acp_sdw_power_mode is zero then ACP PCI driver won't apply
ACP de-init sequence while entering D3 state.

It's a miss. loop should exit when property is not found.
will modify the code.
>> +		/*
>> +		 * when SoundWire configuration is selected from acp pin config,
>> +		 * based on manager instances count, acp init/de-init sequence should be
>> +		 * executed as part of PM ops only when Bus reset is applied for the active
>> +		 * SoundWire manager instances.
>> +		 */
>> +		if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
>> +			acp_data->acp_reset = false;
> ... here this branch is taken.
>
> Is this intentional?
This loop should be exited if acp_sdw_power_mode is not set to power off mode.
will modify code.
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63_dev_data *acp_data)
>>  {
>>  	struct acpi_device *dmic_dev;
>> +	struct acpi_device *sdw_dev;
>>  	const union acpi_object *obj;
>>  	bool is_dmic_dev = false;
> useless init
We are checking is_dmic_dev & is_sdw_dev flags in same code.
Either we need to explicitly update value as false when no ACP PDM
/SoundWire manager instances not found.
>
>> +	bool is_sdw_dev = false;
> and useless init as well...
>
>> +	int ret;
>>  
>>  	dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
>>  	if (dmic_dev) {
>> +		/* is_dmic_dev flag will be set when ACP PDM controller device exists */
>>  		if (!acpi_dev_get_property(dmic_dev, "acp-audio-device-type",
>>  					   ACPI_TYPE_INTEGER, &obj) &&
>>  					   obj->integer.value == ACP_DMIC_DEV)
>>  			is_dmic_dev = true;
>>  	}
>>  
>> +	sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
>> +	if (sdw_dev) {
>> +		acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
>> +		ret = sdw_amd_scan_controller(&pci->dev);
>> +		/* is_sdw_dev flag will be set when SoundWire Manager device exists */
>> +		if (!ret)
>> +			is_sdw_dev = true;
> sdw_amd_scan_controller() can return -EINVAL, how is this handled?
> Shouldn't you stop execution and return here in the < 0 case?
As per our design, ACP PCI driver probe should be successful, even
there are no ACP PDM or Soundwire Manager instance configuration
related platform devices.

The ACP PCI driver is multi-use and that even if SoundWire manager
instances or PDM controller is not found, it will still be used to set the
hardware to proper low power states. i.e ACP should enter D3 state
after successful execution of probe sequence.
>
>> +	}
>> +
>> +	dev_dbg(&pci->dev, "Audio Mode %d\n", config);
>>  	switch (config) {
>> -	case ACP_CONFIG_0:
>> -	case ACP_CONFIG_1:
>> +	case ACP_CONFIG_4:
>> +	case ACP_CONFIG_5:
>> +	case ACP_CONFIG_10:
>> +	case ACP_CONFIG_11:
>> +		if (is_dmic_dev) {
>> +			acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
>> +			acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
>> +		}
>> +		break;
>>  	case ACP_CONFIG_2:
>>  	case ACP_CONFIG_3:
>> -	case ACP_CONFIG_9:
>> -	case ACP_CONFIG_15:
>> -		dev_dbg(&pci->dev, "Audio Mode %d\n", config);
>> +		if (is_sdw_dev) {
>> +			switch (acp_data->sdw_manager_count) {
>> +			case 1:
>> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
>> +				break;
>> +			case 2:
>> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
>> +		}
>>  		break;
>> -	default:
>> -		if (is_dmic_dev) {
>> +	case ACP_CONFIG_6:
>> +	case ACP_CONFIG_7:
>> +	case ACP_CONFIG_12:
>> +	case ACP_CONFIG_8:
>> +	case ACP_CONFIG_13:
>> +	case ACP_CONFIG_14:
>> +		if (is_dmic_dev && is_sdw_dev) {
>> +			switch (acp_data->sdw_manager_count) {
>> +			case 1:
>> +				acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_PDM_MODE_DEVS;
>> +				break;
>> +			case 2:
>> +				acp_data->pdev_mask = ACP63_SDW_PDM_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_SDW1_PDM_MODE_DEVS;
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
>> +		} else if (is_dmic_dev) {
>>  			acp_data->pdev_mask = ACP63_PDM_DEV_MASK;
>>  			acp_data->pdev_count = ACP63_PDM_MODE_DEVS;
>> +		} else if (is_sdw_dev) {
>> +			switch (acp_data->sdw_manager_count) {
>> +			case 1:
>> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_MODE_DEVS;
>> +				break;
>> +			case 2:
>> +				acp_data->pdev_mask = ACP63_SDW_DEV_MASK;
>> +				acp_data->pdev_count = ACP63_SDW0_SDW1_MODE_DEVS;
>> +				break;
>> +			default:
>> +				return -EINVAL;
>> +			}
>>  		}
>>  		break;
>> +	default:
>> +		break;
>>  	}
>> +	return 0;
>>  }
>>  		dev_err(&pci->dev, "ACP platform devices creation failed\n");