| Message ID | 1574085861-22818-2-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,1/6] ASoC: amd:Create multiple I2S platform device endpoint | expand |
> static int snd_acp3x_probe(struct pci_dev *pci, > const struct pci_device_id *pci_id) > { > - int ret; > - u32 addr, val; > struct acp3x_dev_data *adata; > - struct platform_device_info pdevinfo; > + struct platform_device_info pdevinfo[ACP3x_DEVS]; > unsigned int irqflags; > + int ret, val, i; > + u32 addr; > > if (pci_enable_device(pci)) { if you are using devm_ across the board then you could also use pcim_enable_device() here? > dev_err(&pci->dev, "pci_enable_device failed\n"); > @@ -40,10 +40,10 @@ static int snd_acp3x_probe(struct pci_dev *pci, > } > > adata = devm_kzalloc(&pci->dev, sizeof(struct acp3x_dev_data), > - GFP_KERNEL); > + GFP_KERNEL); > if (!adata) { > ret = -ENOMEM; > - goto release_regions; > + goto adata_free; > } > > /* check for msi interrupt support */ > @@ -56,7 +56,8 @@ static int snd_acp3x_probe(struct pci_dev *pci, > irqflags = 0; > > addr = pci_resource_start(pci, 0); > - adata->acp3x_base = ioremap(addr, pci_resource_len(pci, 0)); > + adata->acp3x_base = devm_ioremap(&pci->dev, addr, > + pci_resource_len(pci, 0)); > if (!adata->acp3x_base) { > ret = -ENOMEM; > goto release_regions; > @@ -68,11 +69,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, > switch (val) { > case I2S_MODE: > adata->res = devm_kzalloc(&pci->dev, > - sizeof(struct resource) * 2, > + sizeof(struct resource) * 4, > GFP_KERNEL); > if (!adata->res) { > ret = -ENOMEM; > - goto unmap_mmio; > + goto release_regions; > } > > adata->res[0].name = "acp3x_i2s_iomem"; > @@ -80,28 +81,52 @@ static int snd_acp3x_probe(struct pci_dev *pci, > adata->res[0].start = addr; > adata->res[0].end = addr + (ACP3x_REG_END - ACP3x_REG_START); > > - adata->res[1].name = "acp3x_i2s_irq"; > - adata->res[1].flags = IORESOURCE_IRQ; > - adata->res[1].start = pci->irq; > - adata->res[1].end = pci->irq; > + adata->res[1].name = "acp3x_i2s_sp"; > + adata->res[1].flags = IORESOURCE_MEM; > + adata->res[1].start = addr + ACP3x_I2STDM_REG_START; > + adata->res[1].end = addr + ACP3x_I2STDM_REG_END; > + > + adata->res[2].name = "acp3x_i2s_bt"; > + adata->res[2].flags = IORESOURCE_MEM; > + adata->res[2].start = addr + ACP3x_BT_TDM_REG_START; > + adata->res[2].end = addr + ACP3x_BT_TDM_REG_END; > + > + adata->res[3].name = "acp3x_i2s_irq"; > + adata->res[3].flags = IORESOURCE_IRQ; > + adata->res[3].start = pci->irq; > + adata->res[3].end = adata->res[3].start; > > adata->acp3x_audio_mode = ACP3x_I2S_MODE; > > memset(&pdevinfo, 0, sizeof(pdevinfo)); > - pdevinfo.name = "acp3x_rv_i2s"; > - 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 unmap_mmio; > + pdevinfo[0].name = "acp3x_rv_i2s_dma"; > + pdevinfo[0].id = 0; > + pdevinfo[0].parent = &pci->dev; > + pdevinfo[0].num_res = 4; > + pdevinfo[0].res = &adata->res[0]; > + pdevinfo[0].data = &irqflags; > + pdevinfo[0].size_data = sizeof(irqflags); > + > + pdevinfo[1].name = "acp3x_i2s_playcap"; > + pdevinfo[1].id = 0; > + pdevinfo[1].parent = &pci->dev; > + pdevinfo[1].num_res = 1; > + pdevinfo[1].res = &adata->res[1]; > + > + pdevinfo[2].name = "acp3x_i2s_playcap"; > + pdevinfo[2].id = 1; > + pdevinfo[2].parent = &pci->dev; > + pdevinfo[2].num_res = 1; > + pdevinfo[2].res = &adata->res[2]; > + for (i = 0; i < ACP3x_DEVS ; i++) { > + adata->pdev[i] = > + platform_device_register_full(&pdevinfo[i]); > + if (IS_ERR(adata->pdev[i])) { > + dev_err(&pci->dev, "cannot register %s device\n", > + pdevinfo[i].name); > + ret = PTR_ERR(adata->pdev[i]); > + goto unmap_mmio; > + } > } > break; > default: > @@ -112,10 +137,22 @@ static int snd_acp3x_probe(struct pci_dev *pci, > return 0; > > unmap_mmio: > + if (val == I2S_MODE) > + for (i = 0 ; i < ACP3x_DEVS ; i++) > + platform_device_unregister(adata->pdev[i]); > + devm_kfree(&pci->dev, adata->res); > + devm_kfree(&pci->dev, adata); > pci_disable_msi(pci); > - iounmap(adata->acp3x_base); > + pci_release_regions(pci); > + pci_disable_device(pci); > release_regions: > + devm_kfree(&pci->dev, adata); > + pci_disable_msi(pci); > pci_release_regions(pci); > + pci_disable_device(pci); > +adata_free: > + pci_release_regions(pci); > + pci_disable_device(pci); > disable_pci: > pci_disable_device(pci); this error flow is probably wrong, you should not repeat the same sequences when going from one label to the other. it should be something like (not verified, example only) devm_kfree(&pci->dev, adata->res); release_regions: devm_kfree(&pci->dev, adata); pci_disable_msi(pci); adata_free: pci_release_regions(pci); disable_pci: pci_disable_device(pci); Stopping the review here.
On Mon, Nov 18, 2019 at 07:34:16PM +0530, Ravulapati Vishnu vardhan rao wrote: > static int snd_acp3x_probe(struct pci_dev *pci, > const struct pci_device_id *pci_id) > { > - int ret; > - u32 addr, val; > struct acp3x_dev_data *adata; > - struct platform_device_info pdevinfo; > + struct platform_device_info pdevinfo[ACP3x_DEVS]; > unsigned int irqflags; > + int ret, val, i; > + u32 addr; > > if (pci_enable_device(pci)) { > dev_err(&pci->dev, "pci_enable_device failed\n"); > @@ -40,10 +40,10 @@ static int snd_acp3x_probe(struct pci_dev *pci, > } > > adata = devm_kzalloc(&pci->dev, sizeof(struct acp3x_dev_data), > - GFP_KERNEL); > + GFP_KERNEL); > if (!adata) { > ret = -ENOMEM; > - goto release_regions; > + goto adata_free; > } > > /* check for msi interrupt support */ > @@ -56,7 +56,8 @@ static int snd_acp3x_probe(struct pci_dev *pci, > irqflags = 0; > > addr = pci_resource_start(pci, 0); > - adata->acp3x_base = ioremap(addr, pci_resource_len(pci, 0)); > + adata->acp3x_base = devm_ioremap(&pci->dev, addr, > + pci_resource_len(pci, 0)); > if (!adata->acp3x_base) { > ret = -ENOMEM; > goto release_regions; > @@ -68,11 +69,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, > switch (val) { > case I2S_MODE: > adata->res = devm_kzalloc(&pci->dev, > - sizeof(struct resource) * 2, > + sizeof(struct resource) * 4, > GFP_KERNEL); > if (!adata->res) { > ret = -ENOMEM; > - goto unmap_mmio; > + goto release_regions; > } > > adata->res[0].name = "acp3x_i2s_iomem"; > @@ -80,28 +81,52 @@ static int snd_acp3x_probe(struct pci_dev *pci, > adata->res[0].start = addr; > adata->res[0].end = addr + (ACP3x_REG_END - ACP3x_REG_START); > > - adata->res[1].name = "acp3x_i2s_irq"; > - adata->res[1].flags = IORESOURCE_IRQ; > - adata->res[1].start = pci->irq; > - adata->res[1].end = pci->irq; > + adata->res[1].name = "acp3x_i2s_sp"; > + adata->res[1].flags = IORESOURCE_MEM; > + adata->res[1].start = addr + ACP3x_I2STDM_REG_START; > + adata->res[1].end = addr + ACP3x_I2STDM_REG_END; > + > + adata->res[2].name = "acp3x_i2s_bt"; > + adata->res[2].flags = IORESOURCE_MEM; > + adata->res[2].start = addr + ACP3x_BT_TDM_REG_START; > + adata->res[2].end = addr + ACP3x_BT_TDM_REG_END; > + > + adata->res[3].name = "acp3x_i2s_irq"; > + adata->res[3].flags = IORESOURCE_IRQ; > + adata->res[3].start = pci->irq; > + adata->res[3].end = adata->res[3].start; > > adata->acp3x_audio_mode = ACP3x_I2S_MODE; > > memset(&pdevinfo, 0, sizeof(pdevinfo)); > - pdevinfo.name = "acp3x_rv_i2s"; > - 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 unmap_mmio; > + pdevinfo[0].name = "acp3x_rv_i2s_dma"; > + pdevinfo[0].id = 0; > + pdevinfo[0].parent = &pci->dev; > + pdevinfo[0].num_res = 4; > + pdevinfo[0].res = &adata->res[0]; > + pdevinfo[0].data = &irqflags; > + pdevinfo[0].size_data = sizeof(irqflags); > + > + pdevinfo[1].name = "acp3x_i2s_playcap"; > + pdevinfo[1].id = 0; > + pdevinfo[1].parent = &pci->dev; > + pdevinfo[1].num_res = 1; > + pdevinfo[1].res = &adata->res[1]; > + > + pdevinfo[2].name = "acp3x_i2s_playcap"; > + pdevinfo[2].id = 1; > + pdevinfo[2].parent = &pci->dev; > + pdevinfo[2].num_res = 1; > + pdevinfo[2].res = &adata->res[2]; > + for (i = 0; i < ACP3x_DEVS ; i++) { > + adata->pdev[i] = > + platform_device_register_full(&pdevinfo[i]); > + if (IS_ERR(adata->pdev[i])) { > + dev_err(&pci->dev, "cannot register %s device\n", > + pdevinfo[i].name); > + ret = PTR_ERR(adata->pdev[i]); > + goto unmap_mmio; > + } > } > break; > default: > @@ -112,10 +137,22 @@ static int snd_acp3x_probe(struct pci_dev *pci, > return 0; > > unmap_mmio: > + if (val == I2S_MODE) > + for (i = 0 ; i < ACP3x_DEVS ; i++) > + platform_device_unregister(adata->pdev[i]); I really wish you had done this the way, I said. That's the easiest way to audit. Otherwise you have to check that adata->pdev[] is initialized and that platform_device_unregister() accepts both error pointers and NULLs. It does. But it's easier to audit if you only free things which have been allocated and never free things which were not allocated. > + devm_kfree(&pci->dev, adata->res); Don't free devm_ allocated data. It gets freed for you automatically. > + devm_kfree(&pci->dev, adata); > pci_disable_msi(pci); > - iounmap(adata->acp3x_base); > + pci_release_regions(pci); > + pci_disable_device(pci); > release_regions: > + devm_kfree(&pci->dev, adata); > + pci_disable_msi(pci); This disables the msi twice so it's a double free situation. It would have been better to just cut and paste the code that I wrote for you. regards, dan carpenter
diff --git a/sound/soc/amd/raven/acp3x.h b/sound/soc/amd/raven/acp3x.h index 4f2cadd..2f15fe1 100644 --- a/sound/soc/amd/raven/acp3x.h +++ b/sound/soc/amd/raven/acp3x.h @@ -7,10 +7,15 @@ #include "chip_offset_byte.h" +#define ACP3x_DEVS 3 #define ACP3x_PHY_BASE_ADDRESS 0x1240000 #define ACP3x_I2S_MODE 0 #define ACP3x_REG_START 0x1240000 #define ACP3x_REG_END 0x1250200 +#define ACP3x_I2STDM_REG_START 0x1242400 +#define ACP3x_I2STDM_REG_END 0x1242410 +#define ACP3x_BT_TDM_REG_START 0x1242800 +#define ACP3x_BT_TDM_REG_END 0x1242810 #define I2S_MODE 0x04 #define BT_TX_THRESHOLD 26 #define BT_RX_THRESHOLD 25 diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c index facec24..174adbc 100644 --- a/sound/soc/amd/raven/pci-acp3x.c +++ b/sound/soc/amd/raven/pci-acp3x.c @@ -16,17 +16,17 @@ struct acp3x_dev_data { void __iomem *acp3x_base; bool acp3x_audio_mode; struct resource *res; - struct platform_device *pdev; + struct platform_device *pdev[ACP3x_DEVS]; }; static int snd_acp3x_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) { - int ret; - u32 addr, val; struct acp3x_dev_data *adata; - struct platform_device_info pdevinfo; + struct platform_device_info pdevinfo[ACP3x_DEVS]; unsigned int irqflags; + int ret, val, i; + u32 addr; if (pci_enable_device(pci)) { dev_err(&pci->dev, "pci_enable_device failed\n"); @@ -40,10 +40,10 @@ static int snd_acp3x_probe(struct pci_dev *pci, } adata = devm_kzalloc(&pci->dev, sizeof(struct acp3x_dev_data), - GFP_KERNEL); + GFP_KERNEL); if (!adata) { ret = -ENOMEM; - goto release_regions; + goto adata_free; } /* check for msi interrupt support */ @@ -56,7 +56,8 @@ static int snd_acp3x_probe(struct pci_dev *pci, irqflags = 0; addr = pci_resource_start(pci, 0); - adata->acp3x_base = ioremap(addr, pci_resource_len(pci, 0)); + adata->acp3x_base = devm_ioremap(&pci->dev, addr, + pci_resource_len(pci, 0)); if (!adata->acp3x_base) { ret = -ENOMEM; goto release_regions; @@ -68,11 +69,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, switch (val) { case I2S_MODE: adata->res = devm_kzalloc(&pci->dev, - sizeof(struct resource) * 2, + sizeof(struct resource) * 4, GFP_KERNEL); if (!adata->res) { ret = -ENOMEM; - goto unmap_mmio; + goto release_regions; } adata->res[0].name = "acp3x_i2s_iomem"; @@ -80,28 +81,52 @@ static int snd_acp3x_probe(struct pci_dev *pci, adata->res[0].start = addr; adata->res[0].end = addr + (ACP3x_REG_END - ACP3x_REG_START); - adata->res[1].name = "acp3x_i2s_irq"; - adata->res[1].flags = IORESOURCE_IRQ; - adata->res[1].start = pci->irq; - adata->res[1].end = pci->irq; + adata->res[1].name = "acp3x_i2s_sp"; + adata->res[1].flags = IORESOURCE_MEM; + adata->res[1].start = addr + ACP3x_I2STDM_REG_START; + adata->res[1].end = addr + ACP3x_I2STDM_REG_END; + + adata->res[2].name = "acp3x_i2s_bt"; + adata->res[2].flags = IORESOURCE_MEM; + adata->res[2].start = addr + ACP3x_BT_TDM_REG_START; + adata->res[2].end = addr + ACP3x_BT_TDM_REG_END; + + adata->res[3].name = "acp3x_i2s_irq"; + adata->res[3].flags = IORESOURCE_IRQ; + adata->res[3].start = pci->irq; + adata->res[3].end = adata->res[3].start; adata->acp3x_audio_mode = ACP3x_I2S_MODE; memset(&pdevinfo, 0, sizeof(pdevinfo)); - pdevinfo.name = "acp3x_rv_i2s"; - 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 unmap_mmio; + pdevinfo[0].name = "acp3x_rv_i2s_dma"; + pdevinfo[0].id = 0; + pdevinfo[0].parent = &pci->dev; + pdevinfo[0].num_res = 4; + pdevinfo[0].res = &adata->res[0]; + pdevinfo[0].data = &irqflags; + pdevinfo[0].size_data = sizeof(irqflags); + + pdevinfo[1].name = "acp3x_i2s_playcap"; + pdevinfo[1].id = 0; + pdevinfo[1].parent = &pci->dev; + pdevinfo[1].num_res = 1; + pdevinfo[1].res = &adata->res[1]; + + pdevinfo[2].name = "acp3x_i2s_playcap"; + pdevinfo[2].id = 1; + pdevinfo[2].parent = &pci->dev; + pdevinfo[2].num_res = 1; + pdevinfo[2].res = &adata->res[2]; + for (i = 0; i < ACP3x_DEVS ; i++) { + adata->pdev[i] = + platform_device_register_full(&pdevinfo[i]); + if (IS_ERR(adata->pdev[i])) { + dev_err(&pci->dev, "cannot register %s device\n", + pdevinfo[i].name); + ret = PTR_ERR(adata->pdev[i]); + goto unmap_mmio; + } } break; default: @@ -112,10 +137,22 @@ static int snd_acp3x_probe(struct pci_dev *pci, return 0; unmap_mmio: + if (val == I2S_MODE) + for (i = 0 ; i < ACP3x_DEVS ; i++) + platform_device_unregister(adata->pdev[i]); + devm_kfree(&pci->dev, adata->res); + devm_kfree(&pci->dev, adata); pci_disable_msi(pci); - iounmap(adata->acp3x_base); + pci_release_regions(pci); + pci_disable_device(pci); release_regions: + devm_kfree(&pci->dev, adata); + pci_disable_msi(pci); pci_release_regions(pci); + pci_disable_device(pci); +adata_free: + pci_release_regions(pci); + pci_disable_device(pci); disable_pci: pci_disable_device(pci); @@ -125,10 +162,12 @@ static int snd_acp3x_probe(struct pci_dev *pci, static void snd_acp3x_remove(struct pci_dev *pci) { struct acp3x_dev_data *adata = pci_get_drvdata(pci); + int i; - platform_device_unregister(adata->pdev); - iounmap(adata->acp3x_base); - + if (adata->acp3x_audio_mode == ACP3x_I2S_MODE) { + for (i = 0 ; i < ACP3x_DEVS ; i++) + platform_device_unregister(adata->pdev[i]); + } pci_disable_msi(pci); pci_release_regions(pci); pci_disable_device(pci); @@ -151,6 +190,7 @@ static struct pci_driver acp3x_driver = { module_pci_driver(acp3x_driver); +MODULE_AUTHOR("Vishnuvardhanrao.Ravulapati@amd.com"); MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com"); MODULE_DESCRIPTION("AMD ACP3x PCI driver"); MODULE_LICENSE("GPL v2");
Creates Platform Device endpoints for multiple I2S instances: SP and BT endpoints device. Pass PCI resources like MMIO, irq to the platform devices. Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com> --- sound/soc/amd/raven/acp3x.h | 5 ++ sound/soc/amd/raven/pci-acp3x.c | 102 ++++++++++++++++++++++++++++------------ 2 files changed, 76 insertions(+), 31 deletions(-)