> +#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;
> }
