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

+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....

• /*
  

•  * 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?

• }
• 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

• 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?

• }
• 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");