Re: [PATCH 01/19] ASoC: amd: ps: create platform devices based on acp config

+#define AMD_SDW_CLK_STOP_MODE 1

there are multiple modes for clock stop in SoundWire, and multiple ways for the link manager to deal with clock stop, you want a comment to describe what this define refers to.

+#define AMD_SDW_POWER_OFF_MODE 2

+struct acp_sdw_pdata {

• u16 instance;
• struct mutex *sdw_lock;

need a comment on what this lock protects.

+}; +#endif diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h index b7535c7d093f..ed979e6d0c1d 100644 --- a/sound/soc/amd/ps/acp63.h +++ b/sound/soc/amd/ps/acp63.h @@ -10,7 +10,7 @@ #define ACP_DEVICE_ID 0x15E2 #define ACP63_REG_START 0x1240000 #define ACP63_REG_END 0x1250200 -#define ACP63_DEVS 3 +#define ACP63_DEVS 5

#define ACP_SOFT_RESET_SOFTRESET_AUDDONE_MASK 0x00010001 #define ACP_PGFSM_CNTL_POWER_ON_MASK 1 @@ -55,8 +55,14 @@

#define ACP63_DMIC_ADDR 2 #define ACP63_PDM_MODE_DEVS 3 -#define ACP63_PDM_DEV_MASK 1 #define ACP_DMIC_DEV 2 +#define ACP63_SDW0_MODE_DEVS 2 +#define ACP63_SDW0_SDW1_MODE_DEVS 3 +#define ACP63_SDW0_PDM_MODE_DEVS 4 +#define ACP63_SDW0_SDW1_PDM_MODE_DEVS 5 +#define ACP63_DMIC_ADDR 2 +#define ACP63_SDW_ADDR 5 +#define AMD_SDW_MAX_CONTROLLERS 2

enum acp_config { ACP_CONFIG_0 = 0, @@ -77,6 +83,12 @@ enum acp_config { ACP_CONFIG_15, };

+enum acp_pdev_mask {

• ACP63_PDM_DEV_MASK = 1,
• ACP63_SDW_DEV_MASK,
• ACP63_SDW_PDM_DEV_MASK,

+};

struct pdm_stream_instance { u16 num_pages; u16 channels; @@ -107,7 +119,15 @@ struct acp63_dev_data { struct resource *res; struct platform_device *pdev[ACP63_DEVS]; struct mutex acp_lock; /* protect shared registers */

• struct fwnode_handle *sdw_fw_node; u16 pdev_mask; u16 pdev_count; u16 pdm_dev_index;
• u8 sdw_master_count;

for new contributions, it's recommended to use manager and peripheral.

• u16 sdw0_dev_index;
• u16 sdw1_dev_index;

probably need a comment on what the 0 and 1 refer to, it's not clear if there's any sort of dependency/link with the 'sdw_master_count' above.

If this is related to the two controllers mentioned in the cover letter, then an explanation of the sdw_master_count would be needed as well (single variable for two controllers?)

• u16 sdw_dma_dev_index;
• bool is_dmic_dev;
• bool is_sdw_dev;
• bool acp_sdw_power_off;

}; diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c index e86f23d97584..85154cf0b2a2 100644 --- a/sound/soc/amd/ps/pci-ps.c +++ b/sound/soc/amd/ps/pci-ps.c @@ -14,6 +14,7 @@ #include <linux/interrupt.h> #include <sound/pcm_params.h> #include <linux/pm_runtime.h> +#include <linux/soundwire/sdw_amd.h>

#include "acp63.h"

@@ -134,12 +135,68 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id) return IRQ_NONE; }

-static void get_acp63_device_config(u32 config, struct pci_dev *pci,

• 		    struct acp63_dev_data *acp_data)
  

+static int sdw_amd_scan_controller(struct device *dev) +{

• struct acp63_dev_data *acp_data;
• struct fwnode_handle *link;
• char name[32];
• u8 count = 0;
• u32 acp_sdw_power_mode = 0;
• int index;
• int ret;
• acp_data = dev_get_drvdata(dev);
• acp_data->acp_sdw_power_off = true;
• /* Found controller, find links supported */
• ret = fwnode_property_read_u8_array((acp_data->sdw_fw_node),

• 			    "mipi-sdw-master-count", &count, 1);
  

• if (ret) {

• dev_err(dev,
  

• 	"Failed to read mipi-sdw-master-count: %d\n", ret);
  

one line?

• return -EINVAL;
  

• }
• /* Check count is within bounds */
• if (count > AMD_SDW_MAX_CONTROLLERS) {

• dev_err(dev, "Controller count %d exceeds max %d\n",
  

• 	count, AMD_SDW_MAX_CONTROLLERS);
  

No. controllers and masters are different concepts, see the DisCo specification for SoundWire. A Controller can have multiple Masters.

• return -EINVAL;
  

• }
• if (!count) {

• dev_warn(dev, "No SoundWire controllers detected\n");
  

• return -EINVAL;
  

• }

is this really a warning, looks like a dev_dbg or info to me.

• dev_dbg(dev, "ACPI reports %d Soundwire Controller devices\n", count);

the term device is incorrect here, the DisCo spec does not expose ACPI devices for each master.

"ACPI reports %d Managers"

• acp_data->sdw_master_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, "Master node %s not found\n", name);
  

• 	return -EIO;
  

• }
  

• fwnode_property_read_u32(link, "amd-sdw-power-mode",
  

• 			 &acp_sdw_power_mode);
  

• if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
  

• 	acp_data->acp_sdw_power_off = false;
  

does power-off mean 'clock-stop'?

• }
• return 0;

+}

• if (is_dmic_dev && is_sdw_dev) {
  

• 	switch (acp_data->sdw_master_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;
  

so the cover letter is indeed wrong and confuses two controllers for two managers.

• 	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_master_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;

}