[v2] ASoC: amd: put off registering mach platform_dev to avoid -517 err

Commit Message

Hui Wang May 22, 2020, 8:17 a.m. UTC

If the mach driver's probe is called ahead of codec driver's probe,
the kernel will print -517 error message although the audio still
works finally:
acp_pdm_mach acp_pdm_mach.0: snd_soc_register_card(acp) failed: -517

we could workaround this issue by moving the registration of mach
platform_dev to plat driver's probe.

Signed-off-by: Hui Wang <[e-mail]>
---
 sound/soc/amd/renoir/acp3x-pdm-dma.c | 13 +++++++++++++
 sound/soc/amd/renoir/rn-pci-acp3x.c  |  3 ---
 sound/soc/amd/renoir/rn_acp3x.h      |  3 ++-
 3 files changed, 15 insertions(+), 4 deletions(-)

Comments

Vijendar Mukunda May 22, 2020, 9:16 a.m. UTC | #1

> -----Original Message-----
> From: Hui Wang <[e-mail]>
> Sent: Friday, May 22, 2020 1:48 PM
> To: [e-mail]; [e-mail]; Mukunda, Vijendar
> <[e-mail]>
> Subject: [PATCH v2] ASoC: amd: put off registering mach platform_dev to avoid
> -517 err
> 
> If the mach driver's probe is called ahead of codec driver's probe,
> the kernel will print -517 error message although the audio still
> works finally:
> acp_pdm_mach acp_pdm_mach.0: snd_soc_register_card(acp) failed: -517
> 
> we could workaround this issue by moving the registration of mach
> platform_dev to plat driver's probe.

ACP PCI driver creates the platform devices for DMA driver,
Machine driver & DMIC generic driver.

During machine driver probe, Sound card registration  fails if
it is unable to find Codec component and platform component 
throwing an error as EPROBE_DEFER.(-517)

It will try few more iterations to bind the DAI links.
I guess this is an expected behavior only.

Sound card registration is successful after several attempts.
This is the reason audio is working without any issues.
Below is the sample log.
acp_pdm_mach acp_pdm_mach.0: dmic-hifi <-> acp_rn_pdm_dma.0 mapping ok

Not really sure do we still need to fix the sound card registration fail issue?
We have seen similar issue in Raven I2S driver also.

Ideally ACP PCI device should be parent device for platform devices needed by
DMA driver,  generic dmic driver and machine driver.

In one way, we are forcing machine driver gets probed only after 
DMA driver and codec driver to avoid sound card registration failure

Correct me , If my understanding is wrong.

> 
> Signed-off-by: Hui Wang <[e-mail]>
> ---
>  sound/soc/amd/renoir/acp3x-pdm-dma.c | 13 +++++++++++++
>  sound/soc/amd/renoir/rn-pci-acp3x.c  |  3 ---
>  sound/soc/amd/renoir/rn_acp3x.h      |  3 ++-
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/amd/renoir/acp3x-pdm-dma.c
> b/sound/soc/amd/renoir/acp3x-pdm-dma.c
> index 623dfd3ea705..46055c244998 100644
> --- a/sound/soc/amd/renoir/acp3x-pdm-dma.c
> +++ b/sound/soc/amd/renoir/acp3x-pdm-dma.c
> @@ -402,6 +402,7 @@ static int acp_pdm_audio_probe(struct
> platform_device *pdev)
>  	struct pdm_dev_data *adata;
>  	unsigned int irqflags;
>  	int status;
> +	struct platform_device_info pdevinfo = {0};
> 
>  	if (!pdev->dev.platform_data) {
>  		dev_err(&pdev->dev, "platform_data not retrieved\n");
> @@ -448,6 +449,16 @@ static int acp_pdm_audio_probe(struct
> platform_device *pdev)
>  		dev_err(&pdev->dev, "ACP PDM IRQ request failed\n");
>  		return -ENODEV;
>  	}
> +
> +	pdevinfo.name = "acp_pdm_mach";
> +	pdevinfo.id = 0;
> +	pdevinfo.parent = &pdev->dev;
> +	adata->m_pdev = platform_device_register_full(&pdevinfo);
> +	if (IS_ERR(adata->m_pdev)) {
> +		dev_err(&pdev->dev, "cannot register %s device\n",
> +			pdevinfo.name);
> +		return PTR_ERR(adata->m_pdev);
> +	}
>  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> ACP_SUSPEND_DELAY_MS);
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
> @@ -457,7 +468,9 @@ static int acp_pdm_audio_probe(struct
> platform_device *pdev)
> 
>  static int acp_pdm_audio_remove(struct platform_device *pdev)
>  {
> +	struct pdm_dev_data *adata = dev_get_drvdata(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	platform_device_unregister(adata->m_pdev);
>  	return 0;
>  }
> 
> diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-
> pci-acp3x.c
> index 859ed67b93cf..f5f450cbd249 100644
> --- a/sound/soc/amd/renoir/rn-pci-acp3x.c
> +++ b/sound/soc/amd/renoir/rn-pci-acp3x.c
> @@ -230,9 +230,6 @@ static int snd_rn_acp_probe(struct pci_dev *pci,
>  	pdevinfo[1].name = "dmic-codec";
>  	pdevinfo[1].id = 0;
>  	pdevinfo[1].parent = &pci->dev;
> -	pdevinfo[2].name = "acp_pdm_mach";
> -	pdevinfo[2].id = 0;
> -	pdevinfo[2].parent = &pci->dev;
>  	for (index = 0; index < ACP_DEVS; index++) {
>  		adata->pdev[index] =
> 
> 	platform_device_register_full(&pdevinfo[index]);
> diff --git a/sound/soc/amd/renoir/rn_acp3x.h
> b/sound/soc/amd/renoir/rn_acp3x.h
> index 75228e306e0b..232eda4db055 100644
> --- a/sound/soc/amd/renoir/rn_acp3x.h
> +++ b/sound/soc/amd/renoir/rn_acp3x.h
> @@ -7,7 +7,7 @@
> 
>  #include "rn_chip_offset_byte.h"
> 
> -#define ACP_DEVS		3
> +#define ACP_DEVS		2
>  #define ACP_PHY_BASE_ADDRESS 0x1240000
>  #define	ACP_REG_START	0x1240000
>  #define	ACP_REG_END	0x1250200
> @@ -59,6 +59,7 @@ struct pdm_dev_data {
>  	u32 pdm_irq;
>  	void __iomem *acp_base;
>  	struct snd_pcm_substream *capture_stream;
> +	struct platform_device *m_pdev;
>  };
> 
>  struct pdm_stream_instance {
> --
> 2.17.1

Mark Brown May 22, 2020, 11:08 a.m. UTC | #2

On Fri, May 22, 2020 at 04:17:38PM +0800, Hui Wang wrote:
> If the mach driver's probe is called ahead of codec driver's probe,
> the kernel will print -517 error message although the audio still
> works finally:
> acp_pdm_mach acp_pdm_mach.0: snd_soc_register_card(acp) failed: -517

This is just a bodge which will be at best unrelaible, it might work
around your problem right now on your specific system but it may well
introduce issues on someone else's.

Vijendar Mukunda May 22, 2020, 11:13 a.m. UTC | #3

[AMD Official Use Only - Internal Distribution Only]



> -----Original Message-----
> From: Mark Brown <[e-mail]>
> Sent: Friday, May 22, 2020 4:38 PM
> To: Hui Wang <[e-mail]>
> Cc: [e-mail]; Mukunda, Vijendar
> <[e-mail]>
> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach platform_dev to
> avoid -517 err
> 
> On Fri, May 22, 2020 at 04:17:38PM +0800, Hui Wang wrote:
> > If the mach driver's probe is called ahead of codec driver's probe,
> > the kernel will print -517 error message although the audio still
> > works finally:
> > acp_pdm_mach acp_pdm_mach.0: snd_soc_register_card(acp) failed: -517
> 
> This is just a bodge which will be at best unrelaible, it might work
> around your problem right now on your specific system but it may well
> introduce issues on someone else's.

Does sound card registration failure at boot time due to modules loading order can be considered as bug ?

Mark Brown May 22, 2020, 11:22 a.m. UTC | #4

On Fri, May 22, 2020 at 11:13:43AM +0000, Mukunda, Vijendar wrote:

> Does sound card registration failure at boot time due to modules loading order can be considered as bug ?

No, this is totally normal.  If it failed to bind ever then that'd be a
problem but this is just deferring.

Vijendar Mukunda May 22, 2020, 11:28 a.m. UTC | #5

> -----Original Message-----
> From: Mark Brown <[e-mail]>
> Sent: Friday, May 22, 2020 4:52 PM
> To: Mukunda, Vijendar <[e-mail]>
> Cc: Hui Wang <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach platform_dev to
> avoid -517 err
> 
> On Fri, May 22, 2020 at 11:13:43AM +0000, Mukunda, Vijendar wrote:
> 
> > Does sound card registration failure at boot time due to modules loading
> order can be considered as bug ?
> 
> No, this is totally normal.  If it failed to bind ever then that'd be a
> problem but this is just deferring.

In that case, we don't need this work around I guess.
At maximum during boot time, we may see few sound card registration failure logs,
Which is normal.

With this change, our concept of ACP parent device which will create platform devices
for DMA driver, Generic DMIC driver and Machine driver will be changed.
We implemented same design for Raven I2S driver as well which got productized.

Hui Wang May 22, 2020, 12:59 p.m. UTC | #6

On 2020/5/22 下午7:28, Mukunda, Vijendar wrote:
>
>> -----Original Message-----
>> From: Mark Brown <[e-mail]>
>> Sent: Friday, May 22, 2020 4:52 PM
>> To: Mukunda, Vijendar <[e-mail]>
>> Cc: Hui Wang <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach platform_dev to
>> avoid -517 err
>>
>> On Fri, May 22, 2020 at 11:13:43AM +0000, Mukunda, Vijendar wrote:
>>
>>> Does sound card registration failure at boot time due to modules loading
>> order can be considered as bug ?
>>
>> No, this is totally normal.  If it failed to bind ever then that'd be a
>> problem but this is just deferring.
> In that case, we don't need this work around I guess.
> At maximum during boot time, we may see few sound card registration failure logs,
> Which is normal.
>
> With this change, our concept of ACP parent device which will create platform devices
> for DMA driver, Generic DMIC driver and Machine driver will be changed.
> We implemented same design for Raven I2S driver as well which got productized.
>
OK, got it.  It doesn't affect the sound function anyway.

Hui Wang May 22, 2020, 1:57 p.m. UTC | #7

On 2020/5/22 下午8:59, Hui Wang wrote:
>
> On 2020/5/22 下午7:28, Mukunda, Vijendar wrote:
>>
>>> -----Original Message-----
>>> From: Mark Brown <[e-mail]>
>>> Sent: Friday, May 22, 2020 4:52 PM
>>> To: Mukunda, Vijendar <[e-mail]>
>>> Cc: Hui Wang <[e-mail]>; [e-mail]
>>> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach 
>>> platform_dev to
>>> avoid -517 err
>>>
>>> On Fri, May 22, 2020 at 11:13:43AM +0000, Mukunda, Vijendar wrote:
>>>
>>>> Does sound card registration failure at boot time due to modules 
>>>> loading
>>> order can be considered as bug ?
>>>
>>> No, this is totally normal.  If it failed to bind ever then that'd be a
>>> problem but this is just deferring.
>> In that case, we don't need this work around I guess.
>> At maximum during boot time, we may see few sound card registration 
>> failure logs,
>> Which is normal.
>>
>> With this change, our concept of ACP parent device which will create 
>> platform devices
>> for DMA driver, Generic DMIC driver and Machine driver will be changed.
>> We implemented same design for Raven I2S driver as well which got 
>> productized.
>>
> OK, got it.  It doesn't affect the sound function anyway.

BTW, so far, I have not seen the -517 error from ubuntu kernel dmesg on 
PC or laptop. Maybe this is the 1st driver to print this error in the 
dmesg on PC or laptop, so I guess it probably confuses the users or our QA.

Vijendar Mukunda May 22, 2020, 2:30 p.m. UTC | #8

[AMD Official Use Only - Internal Distribution Only]



> -----Original Message-----
> From: Hui Wang <[e-mail]>
> Sent: Friday, May 22, 2020 7:27 PM
> To: Mukunda, Vijendar <[e-mail]>; Mark Brown
> <[e-mail]>
> Cc: [e-mail]
> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach platform_dev to
> avoid -517 err
> 
> 
> On 2020/5/22 下午8:59, Hui Wang wrote:
> >
> > On 2020/5/22 下午7:28, Mukunda, Vijendar wrote:
> >>
> >>> -----Original Message-----
> >>> From: Mark Brown <[e-mail]>
> >>> Sent: Friday, May 22, 2020 4:52 PM
> >>> To: Mukunda, Vijendar <[e-mail]>
> >>> Cc: Hui Wang <[e-mail]>; [e-mail]
> >>> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach
> >>> platform_dev to
> >>> avoid -517 err
> >>>
> >>> On Fri, May 22, 2020 at 11:13:43AM +0000, Mukunda, Vijendar wrote:
> >>>
> >>>> Does sound card registration failure at boot time due to modules
> >>>> loading
> >>> order can be considered as bug ?
> >>>
> >>> No, this is totally normal.  If it failed to bind ever then that'd be a
> >>> problem but this is just deferring.
> >> In that case, we don't need this work around I guess.
> >> At maximum during boot time, we may see few sound card registration
> >> failure logs,
> >> Which is normal.
> >>
> >> With this change, our concept of ACP parent device which will create
> >> platform devices
> >> for DMA driver, Generic DMIC driver and Machine driver will be changed.
> >> We implemented same design for Raven I2S driver as well which got
> >> productized.
> >>
> > OK, got it.  It doesn't affect the sound function anyway.
> 
> BTW, so far, I have not seen the -517 error from ubuntu kernel dmesg on
> PC or laptop. Maybe this is the 1st driver to print this error in the
> dmesg on PC or laptop, so I guess it probably confuses the users or our QA.
> 

We have similar design for I2S endpoint which got productized on other OS
variant.
Most of the PC and Laptop comes up with Legacy HD Audio Driver solution.
This is ASoC based driver.
As this failure message only appears during boot time,  Its okay to go ahead.
End user should really worry if sound card is not created even after system
boots up.

Vijendar Mukunda May 22, 2020, 3:14 p.m. UTC | #9

> -----Original Message-----
> From: Mukunda, Vijendar
> Sent: Friday, May 22, 2020 8:01 PM
> To: 'Hui Wang' <[e-mail]>; Mark Brown
> <[e-mail]>
> Cc: [e-mail]
> Subject: RE: [PATCH v2] ASoC: amd: put off registering mach platform_dev to
> avoid -517 err
> 
> [AMD Official Use Only - Internal Distribution Only]
> 
> 
> 
> > -----Original Message-----
> > From: Hui Wang <[e-mail]>
> > Sent: Friday, May 22, 2020 7:27 PM
> > To: Mukunda, Vijendar <[e-mail]>; Mark Brown
> > <[e-mail]>
> > Cc: [e-mail]
> > Subject: Re: [PATCH v2] ASoC: amd: put off registering mach platform_dev to
> > avoid -517 err
> >
> >
> > On 2020/5/22 下午8:59, Hui Wang wrote:
> > >
> > > On 2020/5/22 下午7:28, Mukunda, Vijendar wrote:
> > >>
> > >>> -----Original Message-----
> > >>> From: Mark Brown <[e-mail]>
> > >>> Sent: Friday, May 22, 2020 4:52 PM
> > >>> To: Mukunda, Vijendar <[e-mail]>
> > >>> Cc: Hui Wang <[e-mail]>; [e-mail]
> > >>> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach
> > >>> platform_dev to
> > >>> avoid -517 err
> > >>>
> > >>> On Fri, May 22, 2020 at 11:13:43AM +0000, Mukunda, Vijendar wrote:
> > >>>
> > >>>> Does sound card registration failure at boot time due to modules
> > >>>> loading
> > >>> order can be considered as bug ?
> > >>>
> > >>> No, this is totally normal.  If it failed to bind ever then that'd be a
> > >>> problem but this is just deferring.
> > >> In that case, we don't need this work around I guess.
> > >> At maximum during boot time, we may see few sound card registration
> > >> failure logs,
> > >> Which is normal.
> > >>
> > >> With this change, our concept of ACP parent device which will create
> > >> platform devices
> > >> for DMA driver, Generic DMIC driver and Machine driver will be changed.
> > >> We implemented same design for Raven I2S driver as well which got
> > >> productized.
> > >>
> > > OK, got it.  It doesn't affect the sound function anyway.
> >
> > BTW, so far, I have not seen the -517 error from ubuntu kernel dmesg on
> > PC or laptop. Maybe this is the 1st driver to print this error in the
> > dmesg on PC or laptop, so I guess it probably confuses the users or our QA.
> >
> 
> We have similar design for I2S endpoint which got productized on other OS
> variant.
> Most of the PC and Laptop comes up with Legacy HD Audio Driver solution.
> This is ASoC based driver.
> As this failure message only appears during boot time,  Its okay to go ahead.
> End user should really worry if sound card is not created even after system
> boots up.

I have seen sample implementation of deferred probe in one of the machine
driver code using late_initcall() API.
Not sure how this api really works which will resolve the modules loading sequence
issue.

Mark Brown May 22, 2020, 3:32 p.m. UTC | #10

On Fri, May 22, 2020 at 03:14:21PM +0000, Mukunda, Vijendar wrote:

> I have seen sample implementation of deferred probe in one of the machine
> driver code using late_initcall() API.
> Not sure how this api really works which will resolve the modules loading sequence
> issue.

What deferred probe does is keep a list of devices that failed to bind
with a deferred probe error code then every time a device does manage to
bind it retries all those failed devices in case the new device provides
whatever was missing from one of the others.  It's a bit brute force and
ignorance but it does sort things out in the end if all the drivers are
actually there and just loaded in the wrong order.

kernel test robot May 22, 2020, 7:10 p.m. UTC | #11

Hi Hui,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on asoc/for-next]
[also build test WARNING on next-20200522]
[cannot apply to v5.7-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hui-Wang/ASoC-amd-put-off-registering-mach-platform_dev-to-avoid-517-err/20200522-162223
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-193-gb8fad4bc-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[e-mail]>


sparse warnings: (new ones prefixed by >>)

>> sound/soc/amd/renoir/acp3x-pdm-dma.c:405:49: sparse: sparse: Using plain integer as NULL pointer

vim +405 sound/soc/amd/renoir/acp3x-pdm-dma.c

   398	
   399	static int acp_pdm_audio_probe(struct platform_device *pdev)
   400	{
   401		struct resource *res;
   402		struct pdm_dev_data *adata;
   403		unsigned int irqflags;
   404		int status;
 > 405		struct platform_device_info pdevinfo = {0};
   406	
   407		if (!pdev->dev.platform_data) {
   408			dev_err(&pdev->dev, "platform_data not retrieved\n");
   409			return -ENODEV;
   410		}
   411		irqflags = *((unsigned int *)(pdev->dev.platform_data));
   412	
   413		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   414		if (!res) {
   415			dev_err(&pdev->dev, "IORESOURCE_MEM FAILED\n");
   416			return -ENODEV;
   417		}
   418	
   419		adata = devm_kzalloc(&pdev->dev, sizeof(*adata), GFP_KERNEL);
   420		if (!adata)
   421			return -ENOMEM;
   422	
   423		adata->acp_base = devm_ioremap(&pdev->dev, res->start,
   424					       resource_size(res));
   425		if (!adata->acp_base)
   426			return -ENOMEM;
   427	
   428		res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
   429		if (!res) {
   430			dev_err(&pdev->dev, "IORESOURCE_IRQ FAILED\n");
   431			return -ENODEV;
   432		}
   433	
   434		adata->pdm_irq = res->start;
   435		adata->capture_stream = NULL;
   436	
   437		dev_set_drvdata(&pdev->dev, adata);
   438		status = devm_snd_soc_register_component(&pdev->dev,
   439							 &acp_pdm_component,
   440							 &acp_pdm_dai_driver, 1);
   441		if (status) {
   442			dev_err(&pdev->dev, "Fail to register acp pdm dai\n");
   443	
   444			return -ENODEV;
   445		}
   446		status = devm_request_irq(&pdev->dev, adata->pdm_irq, pdm_irq_handler,
   447					  irqflags, "ACP_PDM_IRQ", adata);
   448		if (status) {
   449			dev_err(&pdev->dev, "ACP PDM IRQ request failed\n");
   450			return -ENODEV;
   451		}
   452	
   453		pdevinfo.name = "acp_pdm_mach";
   454		pdevinfo.id = 0;
   455		pdevinfo.parent = &pdev->dev;
   456		adata->m_pdev = platform_device_register_full(&pdevinfo);
   457		if (IS_ERR(adata->m_pdev)) {
   458			dev_err(&pdev->dev, "cannot register %s device\n",
   459				pdevinfo.name);
   460			return PTR_ERR(adata->m_pdev);
   461		}
   462		pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS);
   463		pm_runtime_use_autosuspend(&pdev->dev);
   464		pm_runtime_enable(&pdev->dev);
   465		pm_runtime_allow(&pdev->dev);
   466		return 0;
   467	}
   468	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Vijendar Mukunda May 23, 2020, 12:11 a.m. UTC | #12

> -----Original Message-----
> From: Mark Brown <[e-mail]>
> Sent: Friday, May 22, 2020 9:02 PM
> To: Mukunda, Vijendar <[e-mail]>
> Cc: Hui Wang <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach platform_dev to
> avoid -517 err
> 
> On Fri, May 22, 2020 at 03:14:21PM +0000, Mukunda, Vijendar wrote:
> 
> > I have seen sample implementation of deferred probe in one of the machine
> > driver code using late_initcall() API.
> > Not sure how this api really works which will resolve the modules loading
> sequence
> > issue.
> 
> What deferred probe does is keep a list of devices that failed to bind
> with a deferred probe error code then every time a device does manage to
> bind it retries all those failed devices in case the new device provides
> whatever was missing from one of the others.  It's a bit brute force and
> ignorance but it does sort things out in the end if all the drivers are
> actually there and just loaded in the wrong order.

We see one more problem with this patch.
We are going to add support for I2S endpoint on another
platform based on Renoir.

Platform device creation logic going to be expanded in ACP PCI
driver probe call.
Because ACP Is Parent device , Ideally platform devices creation logic
should be handled in ACP PCI driver.

Based on Audio configuration, During ACP PCI driver probe call, 
platform devices will be created.

In case of I2S endpoint support,  machine driver gets probed via acpi dev id match.
If we go ahead with this patch, We have to expand the work around logic
by adding extra check in PDM DMA driver  which doesn't seems to be good.

Currently I observed only two times sound card registration failure in dmesg 
during boot time.
For the sake of avoiding  few card registration failure logs observed in dmesg, 
I don't think at this stage, we really need to go ahead with this patch.

Hui Wang May 25, 2020, 3:32 a.m. UTC | #13

On 2020/5/23 上午8:11, Mukunda, Vijendar wrote:
>
>> -----Original Message-----
>> From: Mark Brown <[e-mail]>
>> Sent: Friday, May 22, 2020 9:02 PM
>> To: Mukunda, Vijendar <[e-mail]>
>> Cc: Hui Wang <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v2] ASoC: amd: put off registering mach platform_dev to
>> avoid -517 err
>>
>> On Fri, May 22, 2020 at 03:14:21PM +0000, Mukunda, Vijendar wrote:
>>
>>> I have seen sample implementation of deferred probe in one of the machine
>>> driver code using late_initcall() API.
>>> Not sure how this api really works which will resolve the modules loading
>> sequence
>>> issue.
>> What deferred probe does is keep a list of devices that failed to bind
>> with a deferred probe error code then every time a device does manage to
>> bind it retries all those failed devices in case the new device provides
>> whatever was missing from one of the others.  It's a bit brute force and
>> ignorance but it does sort things out in the end if all the drivers are
>> actually there and just loaded in the wrong order.
> We see one more problem with this patch.
> We are going to add support for I2S endpoint on another
> platform based on Renoir.
>
> Platform device creation logic going to be expanded in ACP PCI
> driver probe call.
> Because ACP Is Parent device , Ideally platform devices creation logic
> should be handled in ACP PCI driver.
>
> Based on Audio configuration, During ACP PCI driver probe call,
> platform devices will be created.
>
> In case of I2S endpoint support,  machine driver gets probed via acpi dev id match.
> If we go ahead with this patch, We have to expand the work around logic
> by adding extra check in PDM DMA driver  which doesn't seems to be good.
>
> Currently I observed only two times sound card registration failure in dmesg
> during boot time.
> For the sake of avoiding  few card registration failure logs observed in dmesg,
> I don't think at this stage, we really need to go ahead with this patch.

OK, got it, thanks.


>
>
>
>
>
>
>

Patch

diff --git a/sound/soc/amd/renoir/acp3x-pdm-dma.c b/sound/soc/amd/renoir/acp3x-pdm-dma.c
index 623dfd3ea705..46055c244998 100644
--- a/sound/soc/amd/renoir/acp3x-pdm-dma.c
+++ b/sound/soc/amd/renoir/acp3x-pdm-dma.c
@@ -402,6 +402,7 @@  static int acp_pdm_audio_probe(struct platform_device *pdev)
 	struct pdm_dev_data *adata;
 	unsigned int irqflags;
 	int status;
+	struct platform_device_info pdevinfo = {0};
 
 	if (!pdev->dev.platform_data) {
 		dev_err(&pdev->dev, "platform_data not retrieved\n");
@@ -448,6 +449,16 @@  static int acp_pdm_audio_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "ACP PDM IRQ request failed\n");
 		return -ENODEV;
 	}
+
+	pdevinfo.name = "acp_pdm_mach";
+	pdevinfo.id = 0;
+	pdevinfo.parent = &pdev->dev;
+	adata->m_pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(adata->m_pdev)) {
+		dev_err(&pdev->dev, "cannot register %s device\n",
+			pdevinfo.name);
+		return PTR_ERR(adata->m_pdev);
+	}
 	pm_runtime_set_autosuspend_delay(&pdev->dev, ACP_SUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
@@ -457,7 +468,9 @@  static int acp_pdm_audio_probe(struct platform_device *pdev)
 
 static int acp_pdm_audio_remove(struct platform_device *pdev)
 {
+	struct pdm_dev_data *adata = dev_get_drvdata(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	platform_device_unregister(adata->m_pdev);
 	return 0;
 }
 
diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-pci-acp3x.c
index 859ed67b93cf..f5f450cbd249 100644
--- a/sound/soc/amd/renoir/rn-pci-acp3x.c
+++ b/sound/soc/amd/renoir/rn-pci-acp3x.c
@@ -230,9 +230,6 @@  static int snd_rn_acp_probe(struct pci_dev *pci,
 	pdevinfo[1].name = "dmic-codec";
 	pdevinfo[1].id = 0;
 	pdevinfo[1].parent = &pci->dev;
-	pdevinfo[2].name = "acp_pdm_mach";
-	pdevinfo[2].id = 0;
-	pdevinfo[2].parent = &pci->dev;
 	for (index = 0; index < ACP_DEVS; index++) {
 		adata->pdev[index] =
 				platform_device_register_full(&pdevinfo[index]);
diff --git a/sound/soc/amd/renoir/rn_acp3x.h b/sound/soc/amd/renoir/rn_acp3x.h
index 75228e306e0b..232eda4db055 100644
--- a/sound/soc/amd/renoir/rn_acp3x.h
+++ b/sound/soc/amd/renoir/rn_acp3x.h
@@ -7,7 +7,7 @@ 
 
 #include "rn_chip_offset_byte.h"
 
-#define ACP_DEVS		3
+#define ACP_DEVS		2
 #define ACP_PHY_BASE_ADDRESS 0x1240000
 #define	ACP_REG_START	0x1240000
 #define	ACP_REG_END	0x1250200
@@ -59,6 +59,7 @@  struct pdm_dev_data {
 	u32 pdm_irq;
 	void __iomem *acp_base;
 	struct snd_pcm_substream *capture_stream;
+	struct platform_device *m_pdev;
 };
 
 struct pdm_stream_instance {