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

+/**

• • acp_pdev_mask corresponds to platform device mask based on audio endpoint combinations.
• • acp_pdev_mask will be calculated based on ACPI Scan under ACP PCI device and
• • ACP PIN Configuration.
• • Based acp_pdev_mask, platform devices will be created.
• • Below are possible platform device combinations.
• • 1. ACP PDM Controller, dmic-codec, machine driver platform device node
• • 2. ACP PDM Controller , dmic-codec, SW0 SoundWire manager instance, platform device for
• • SoundWire DMA driver
• • 3. SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver
• • 4. ACP PDM Controller, dmic-codec, SDW0, SDW1 manager instances, platform device for
• • SoundWire DMA driver
• • ACP63_PDM_DEV_MASK corresponds to platform device mask for ACP PDM controller.
• • ACP63_SDW_DEV_MASK corresponds to platform device mask for SDW manager instances.
• • ACP63_SDW_PDM_DEV_MASK corresponds to platform device mask for ACP PDM + SDW manager combination
• */

+enum acp_pdev_mask {

• ACP63_PDM_DEV_MASK = 1,
• ACP63_SDW_DEV_MASK,
• ACP63_SDW_PDM_DEV_MASK,

+};

This does not look like a mask, the definitions prevent bit-wise operations from happening.

Either use BIT(0), BIT(1), BIT(2) or GENMASK(1, 0), or demote this to a regular enum (e.g. pdev_config or something)

struct pdm_stream_instance { u16 num_pages; u16 channels; @@ -95,14 +144,38 @@ struct pdm_dev_data { struct snd_pcm_substream *capture_stream; };

+/**

• • struct acp63_dev_data - acp pci driver context
• • @acp63_base: acp mmio base
• • @res: resource
• • @pdev: array of child platform device node structures
• • @acp_lock: used to protect acp common registers
• • @sdw_fw_node: SoundWire controller fw node handle
• • @pdev_mask: platform device mask
• • @pdev_count: platform devices count
• • @pdm_dev_index: pdm platform device index
• • @sdw_manager_count: SoundWire manager instance count
• • @sdw0_dev_index: SoundWire Manager-0 platform device index
• • @sdw1_dev_index: SoundWire Manager-1 platform device index
• • @sdw_dma_dev_index: SoundWire DMA controller platform device index
• • @acp_reset: flag set to true when bus reset is applied across all
• • the active SoundWire manager instances
• */

struct acp63_dev_data { void __iomem *acp63_base; 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_manager_count;
• u16 sdw0_dev_index;
• u16 sdw1_dev_index;
• u16 sdw_dma_dev_index;
• bool acp_reset;

};

int snd_amd_acp_find_config(struct pci_dev *pci); diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c index 54752d6040d6..816c22e7f1ab 100644 --- a/sound/soc/amd/ps/pci-ps.c +++ b/sound/soc/amd/ps/pci-ps.c @@ -6,6 +6,7 @@ */

#include <linux/pci.h> +#include <linux/bitops.h> #include <linux/module.h> #include <linux/io.h> #include <linux/delay.h> @@ -15,6 +16,7 @@ #include <sound/pcm_params.h> #include <linux/pm_runtime.h> #include <linux/iopoll.h> +#include <linux/soundwire/sdw_amd.h>

#include "acp63.h"

@@ -119,37 +121,162 @@ 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];
• 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);
  

IIRC this is only defined in the DisCo 2.0 spec, previous editions had a 'mipi-master-count'. A comment would not hurt to point to the minimal DisCo spec version.

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

• }
  

• ret = fwnode_property_read_u32(link, "amd-sdw-power-mode", &acp_sdw_power_mode);
  

• if (ret)
  

• 	return ret;
  

• /*
  

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

• 	return 0;
  

• }
  

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

• bool is_sdw_dev = false;

• 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",

usually properties start with the 'mipi-' or 'vendor-' prefix. Is there a missing 'amd-' here or is 'acp-' unique enough?