| Message ID | 20200518171704.24999-5-Vijendar.Mukunda@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Renoir ACP driver | expand |
On Tue, May 19, 2020 at 01:16:54AM +0800, Vijendar Mukunda wrote: > + adata->res = devm_kzalloc(&pci->dev, > + sizeof(struct resource) * 2, > + GFP_KERNEL); > + if (!adata->res) { > + ret = -ENOMEM; > + goto de_init; > + } > + > + adata->res[0].name = "acp_pdm_iomem"; > + adata->res[0].flags = IORESOURCE_MEM; > + adata->res[0].start = addr; > + adata->res[0].end = addr + (ACP_REG_END - ACP_REG_START); > + adata->res[1].name = "acp_pdm_irq"; > + adata->res[1].flags = IORESOURCE_IRQ; > + adata->res[1].start = pci->irq; > + adata->res[1].end = pci->irq; Creating the subdevice is good and sensible but this is basically open coding what the MFD framework does. The subdevices should probably be created with that instead.
[AMD Official Use Only - Internal Distribution Only] > -----Original Message----- > From: Mark Brown <broonie@kernel.org> > Sent: Tuesday, May 19, 2020 4:24 PM > To: Mukunda, Vijendar <Vijendar.Mukunda@amd.com> > Cc: alsa-devel@alsa-project.org; tiwai@suse.de; Deucher, Alexander > <Alexander.Deucher@amd.com> > Subject: Re: [PATCH v3 04/14] ASoC: amd: create acp3x pdm platform device > > On Tue, May 19, 2020 at 01:16:54AM +0800, Vijendar Mukunda wrote: > > > + adata->res = devm_kzalloc(&pci->dev, > > + sizeof(struct resource) * 2, > > + GFP_KERNEL); > > + if (!adata->res) { > > + ret = -ENOMEM; > > + goto de_init; > > + } > > + > > + adata->res[0].name = "acp_pdm_iomem"; > > + adata->res[0].flags = IORESOURCE_MEM; > > + adata->res[0].start = addr; > > + adata->res[0].end = addr + (ACP_REG_END - ACP_REG_START); > > + adata->res[1].name = "acp_pdm_irq"; > > + adata->res[1].flags = IORESOURCE_IRQ; > > + adata->res[1].start = pci->irq; > > + adata->res[1].end = pci->irq; > > Creating the subdevice is good and sensible but this is basically open > coding what the MFD framework does. The subdevices should probably be > created with that instead. Previously during raven I2S driver development to support multiple I2S controller instances, we have seen challenges with MFD framework. That's why we opted current design. There is lengthy discussions happened over the mail thread during Raven I2S driver upstream review. Finally current design is accepted. This driver further going to be expanded to support I2S Endpoint also. We believe current design is good enough to handle it.
[AMD Official Use Only - Internal Distribution Only] > -----Original Message----- > From: Mukunda, Vijendar <Vijendar.Mukunda@amd.com> > Sent: Tuesday, May 19, 2020 7:03 AM > To: Mark Brown <broonie@kernel.org> > Cc: alsa-devel@alsa-project.org; tiwai@suse.de; Deucher, Alexander > <Alexander.Deucher@amd.com> > Subject: RE: [PATCH v3 04/14] ASoC: amd: create acp3x pdm platform device > > [AMD Official Use Only - Internal Distribution Only] > > > > > -----Original Message----- > > From: Mark Brown <broonie@kernel.org> > > Sent: Tuesday, May 19, 2020 4:24 PM > > To: Mukunda, Vijendar <Vijendar.Mukunda@amd.com> > > Cc: alsa-devel@alsa-project.org; tiwai@suse.de; Deucher, Alexander > > <Alexander.Deucher@amd.com> > > Subject: Re: [PATCH v3 04/14] ASoC: amd: create acp3x pdm platform > > device > > > > On Tue, May 19, 2020 at 01:16:54AM +0800, Vijendar Mukunda wrote: > > > > > + adata->res = devm_kzalloc(&pci->dev, > > > + sizeof(struct resource) * 2, > > > + GFP_KERNEL); > > > + if (!adata->res) { > > > + ret = -ENOMEM; > > > + goto de_init; > > > + } > > > + > > > + adata->res[0].name = "acp_pdm_iomem"; > > > + adata->res[0].flags = IORESOURCE_MEM; > > > + adata->res[0].start = addr; > > > + adata->res[0].end = addr + (ACP_REG_END - ACP_REG_START); > > > + adata->res[1].name = "acp_pdm_irq"; > > > + adata->res[1].flags = IORESOURCE_IRQ; > > > + adata->res[1].start = pci->irq; > > > + adata->res[1].end = pci->irq; > > > > Creating the subdevice is good and sensible but this is basically open > > coding what the MFD framework does. The subdevices should probably be > > created with that instead. > > Previously during raven I2S driver development to support multiple I2S > controller instances, we have seen challenges with MFD framework. > That's why we opted current design. > There is lengthy discussions happened over the mail thread during Raven I2S > driver upstream review. Finally current design is accepted. > > This driver further going to be expanded to support I2S Endpoint also. > We believe current design is good enough to handle it. For reference: https://lore.kernel.org/alsa-devel/20191014070318.GC4545@dell/ Basically MFD should only be used for drivers in drivers/mfd. Everything else should use platform devices. Alex
On Tue, May 19, 2020 at 02:40:17PM +0000, Deucher, Alexander wrote: > For reference: > https://lore.kernel.org/alsa-devel/20191014070318.GC4545@dell/ > Basically MFD should only be used for drivers in drivers/mfd. Everything else should use platform devices. Right, but that does suggest the put the core in drivers/mfd option.
diff --git a/sound/soc/amd/renoir/rn-pci-acp3x.c b/sound/soc/amd/renoir/rn-pci-acp3x.c index 429813f6ba1c..362409ef0d85 100644 --- a/sound/soc/amd/renoir/rn-pci-acp3x.c +++ b/sound/soc/amd/renoir/rn-pci-acp3x.c @@ -8,6 +8,8 @@ #include <linux/module.h> #include <linux/io.h> #include <linux/delay.h> +#include <linux/platform_device.h> +#include <linux/interrupt.h> #include "rn_acp3x.h" @@ -17,6 +19,8 @@ MODULE_PARM_DESC(acp_power_gating, "Enable acp power gating"); struct acp_dev_data { void __iomem *acp_base; + struct resource *res; + struct platform_device *pdev; }; static int rn_acp_power_on(void __iomem *acp_base) @@ -151,6 +155,8 @@ static int snd_rn_acp_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) { struct acp_dev_data *adata; + struct platform_device_info pdevinfo; + unsigned int irqflags; int ret; u32 addr; @@ -172,20 +178,70 @@ static int snd_rn_acp_probe(struct pci_dev *pci, goto release_regions; } + /* check for msi interrupt support */ + ret = pci_enable_msi(pci); + if (ret) + /* msi is not enabled */ + irqflags = IRQF_SHARED; + else + /* msi is enabled */ + irqflags = 0; + addr = pci_resource_start(pci, 0); adata->acp_base = devm_ioremap(&pci->dev, addr, pci_resource_len(pci, 0)); if (!adata->acp_base) { ret = -ENOMEM; - goto release_regions; + goto disable_msi; } pci_set_master(pci); pci_set_drvdata(pci, adata); ret = rn_acp_init(adata->acp_base); if (ret) - goto release_regions; + goto disable_msi; + + adata->res = devm_kzalloc(&pci->dev, + sizeof(struct resource) * 2, + GFP_KERNEL); + if (!adata->res) { + ret = -ENOMEM; + goto de_init; + } + + adata->res[0].name = "acp_pdm_iomem"; + adata->res[0].flags = IORESOURCE_MEM; + adata->res[0].start = addr; + adata->res[0].end = addr + (ACP_REG_END - ACP_REG_START); + adata->res[1].name = "acp_pdm_irq"; + adata->res[1].flags = IORESOURCE_IRQ; + adata->res[1].start = pci->irq; + adata->res[1].end = pci->irq; + + memset(&pdevinfo, 0, sizeof(pdevinfo)); + pdevinfo.name = "acp_rn_pdm_dma"; + pdevinfo.id = 0; + pdevinfo.parent = &pci->dev; + pdevinfo.num_res = 2; + pdevinfo.res = adata->res; + pdevinfo.data = &irqflags; + pdevinfo.size_data = sizeof(irqflags); + + adata->pdev = platform_device_register_full(&pdevinfo); + if (IS_ERR(adata->pdev)) { + dev_err(&pci->dev, "cannot register %s device\n", + pdevinfo.name); + ret = PTR_ERR(adata->pdev); + goto unregister_devs; + } return 0; +unregister_devs: + platform_device_unregister(adata->pdev); +de_init: + if (rn_acp_deinit(adata->acp_base)) + dev_err(&pci->dev, "ACP de-init failed\n"); +disable_msi: + pci_disable_msi(pci); release_regions: pci_release_regions(pci); disable_pci: @@ -200,6 +256,7 @@ static void snd_rn_acp_remove(struct pci_dev *pci) int ret; adata = pci_get_drvdata(pci); + platform_device_unregister(adata->pdev); ret = rn_acp_deinit(adata->acp_base); if (ret) dev_err(&pci->dev, "ACP de-init failed\n"); diff --git a/sound/soc/amd/renoir/rn_acp3x.h b/sound/soc/amd/renoir/rn_acp3x.h index ec2a85085163..5e4fd99397d5 100644 --- a/sound/soc/amd/renoir/rn_acp3x.h +++ b/sound/soc/amd/renoir/rn_acp3x.h @@ -8,6 +8,9 @@ #include "rn_chip_offset_byte.h" #define ACP_PHY_BASE_ADDRESS 0x1240000 +#define ACP_REG_START 0x1240000 +#define ACP_REG_END 0x1250200 + #define ACP_DEVICE_ID 0x15E2 #define ACP_POWER_ON 0x00 #define ACP_POWER_ON_IN_PROGRESS 0x01
ACP 3x IP has PDM decoder as one of IP blocks. Create a platform device for it, so that the PDM platform driver can be bound to this device. Pass PCI resources like MMIO, irq to this platform device. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> --- sound/soc/amd/renoir/rn-pci-acp3x.c | 61 ++++++++++++++++++++++++++++- sound/soc/amd/renoir/rn_acp3x.h | 3 ++ 2 files changed, 62 insertions(+), 2 deletions(-)