On 5/17/23 03:38, Mukunda,Vijendar wrote:
> On 16/05/23 20:02, Pierre-Louis Bossart wrote:
>>
>> On 5/16/23 05:35, Vijendar Mukunda wrote:
>>> Create platform devices for Soundwire Manager instances and
>>> PDM controller based on ACP pin config selection
>>> and ACPI fw handle for pink sardine platform.
>>>
>>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
>>> ---
>>> sound/soc/amd/ps/acp63.h | 43 ++++++-
>>> sound/soc/amd/ps/pci-ps.c | 250 ++++++++++++++++++++++++++++++++++++--
>>> 2 files changed, 280 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
>>> index 2f94448102d0..f27f71116598 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_MANAGERS 2
>>>
>>> /* time in ms for acp timeout */
>>> #define ACP_TIMEOUT 500
>>> @@ -80,6 +86,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,
>>> +};
>> a comment or kernel-doc wouldn't hurt to explain the difference between
>> ACP63_PDM_DEV_MASK and ACP63_SDW_PDM_DEV_MASK, the meaning of the 'SDW"
>> prefix is far from obvious.
> Above enum's are listed to know the platform device masks.
> For example - if ACP63_PDM_DEV_MASK is set, then ACP PCI driver
> will create platform device for PDM controller.
>
> If ACP63_SDW_DEV_MASK is set, ACP PCI driver will create platform device
> nodes for soundwire manager instances based on instance count retrieved
> by scanning the SoundWire Controller.
>
> If ACP63_SDW_PDM_DEV_MASK is set, ACP PCI driver will create platform device
> nodes for PDM controller and SoundWire manager instances.
>
> We will add comment for the same.
Ah ok, I completely missed that you could have PDM, SoundWire or
PDM+SoundWire configurations. I was reading this with SoundWire blinders
and thought you wanted to have PDM over SoundWire or something.
>>> - dev_dbg(&pci->dev, "No PDM devices found\n");
>>> + dev_dbg(&pci->dev, "No PDM or Soundwire manager devices found\n");
>> what does this mean? I find this debug adds more confusion.
> Currently, we are trying to create platform devices for PDM controller and SoundWire
> Manager instances based on ACP pin config and ACPI _ADDR fields scan under ACP PCI device
> scope.
> Earlier We have added support for ACP PDM controller.
> ACP PIN config supports different audio configurations other than PDM and SoundWire
> based audio endpoints.
>
> If there is no pdev_mask set, it refers to default switch case.
> This dev_dbg statement to notify that no PDM and Soundwire manager devices found
> from ACPI scan.
>
> This patch adds support for platform device creation logic for Soundwire manager instances &
> PDM controller combinations based on ACP PIN Config and ACPI _ADDR field scan.
>
> Possible combination of platform device nodes:
>
> 1) ACP PDM Controller, dmic-codec, machine driver platform device node
> 2) ACP PDM Controller , dmic-codec, SW0 manager instance, platform device for SoundWire DMA driver
> 3) SW0, SW1 SoundWire manager instances, platform device for SoundWire DMA driver
> 3) ACP PDM Controller, dmic-codec, SDW0, SDW1 manager instances, platform device for SoundWire DMA driver
right, you really want this in the commit message so that reviewers
understand the various configurations upfront. Trying to
reverse-engineer the code induces migraines ;-)
