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

[Pierre-Louis Bossart]

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