> +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");
