On Thu, Jul 15, 2021 at 09:59:01AM +0200, Takashi Iwai wrote:
> This patch converts the resource management in PCI korg1212 driver
> with devres as a clean up. Each manual resource management is
> converted with the corresponding devres helper, the page allocations
> are done with the devres helper, and the card object release is
> managed now via card->private_free instead of a lowlevel snd_device.
>
> This should give no user-visible functional changes.
>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> sound/pci/korg1212/korg1212.c | 211 +++++++++-------------------------
> 1 file changed, 55 insertions(+), 156 deletions(-)
>
> diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
> index 030e01b062e4..7872abbd4587 100644
> --- a/sound/pci/korg1212/korg1212.c
> +++ b/sound/pci/korg1212/korg1212.c
> @@ -320,10 +320,10 @@ struct snd_korg1212 {
> unsigned long inIRQ;
> void __iomem *iobase;
>
> - struct snd_dma_buffer dma_dsp;
> - struct snd_dma_buffer dma_play;
> - struct snd_dma_buffer dma_rec;
> - struct snd_dma_buffer dma_shared;
> + struct snd_dma_buffer *dma_dsp;
> + struct snd_dma_buffer *dma_play;
> + struct snd_dma_buffer *dma_rec;
> + struct snd_dma_buffer *dma_shared;
>
> u32 DataBufsSize;
>
> @@ -1200,8 +1200,8 @@ static int snd_korg1212_downloadDSPCode(struct snd_korg1212 *korg1212)
> snd_korg1212_setCardState(korg1212, K1212_STATE_DSP_IN_PROCESS);
>
> rc = snd_korg1212_Send1212Command(korg1212, K1212_DB_StartDSPDownload,
> - UpperWordSwap(korg1212->dma_dsp.addr),
> - 0, 0, 0);
> + UpperWordSwap(korg1212->dma_dsp->addr),
> + 0, 0, 0);
> if (rc)
> K1212_DEBUG_PRINTK("K1212_DEBUG: Start DSP Download RC = %d [%s]\n",
> rc, stateName[korg1212->cardState]);
> @@ -1382,7 +1382,7 @@ static int snd_korg1212_playback_open(struct snd_pcm_substream *substream)
> snd_korg1212_OpenCard(korg1212);
>
> runtime->hw = snd_korg1212_playback_info;
> - snd_pcm_set_runtime_buffer(substream, &korg1212->dma_play);
> + snd_pcm_set_runtime_buffer(substream, korg1212->dma_play);
>
> spin_lock_irqsave(&korg1212->lock, flags);
>
> @@ -1413,7 +1413,7 @@ static int snd_korg1212_capture_open(struct snd_pcm_substream *substream)
> snd_korg1212_OpenCard(korg1212);
>
> runtime->hw = snd_korg1212_capture_info;
> - snd_pcm_set_runtime_buffer(substream, &korg1212->dma_rec);
> + snd_pcm_set_runtime_buffer(substream, korg1212->dma_rec);
>
> spin_lock_irqsave(&korg1212->lock, flags);
>
> @@ -2080,71 +2080,16 @@ static void snd_korg1212_proc_init(struct snd_korg1212 *korg1212)
> snd_korg1212_proc_read);
> }
>
> -static int
> -snd_korg1212_free(struct snd_korg1212 *korg1212)
> +static void
> +snd_korg1212_free(struct snd_card *card)
> {
> - snd_korg1212_TurnOffIdleMonitor(korg1212);
> -
> - if (korg1212->irq >= 0) {
> - snd_korg1212_DisableCardInterrupts(korg1212);
> - free_irq(korg1212->irq, korg1212);
> - korg1212->irq = -1;
> - }
> -
> - if (korg1212->iobase != NULL) {
> - iounmap(korg1212->iobase);
> - korg1212->iobase = NULL;
> - }
> -
> - pci_release_regions(korg1212->pci);
> -
> - // ----------------------------------------------------
> - // free up memory resources used for the DSP download.
> - // ----------------------------------------------------
> - if (korg1212->dma_dsp.area) {
> - snd_dma_free_pages(&korg1212->dma_dsp);
> - korg1212->dma_dsp.area = NULL;
> - }
> -
> -#ifndef K1212_LARGEALLOC
> -
> - // ------------------------------------------------------
> - // free up memory resources used for the Play/Rec Buffers
> - // ------------------------------------------------------
> - if (korg1212->dma_play.area) {
> - snd_dma_free_pages(&korg1212->dma_play);
> - korg1212->dma_play.area = NULL;
> - }
> + struct snd_korg1212 *korg1212 = card->private_data;
>
> - if (korg1212->dma_rec.area) {
> - snd_dma_free_pages(&korg1212->dma_rec);
> - korg1212->dma_rec.area = NULL;
> - }
> -
> -#endif
> -
> - // ----------------------------------------------------
> - // free up memory resources used for the Shared Buffers
> - // ----------------------------------------------------
> - if (korg1212->dma_shared.area) {
> - snd_dma_free_pages(&korg1212->dma_shared);
> - korg1212->dma_shared.area = NULL;
> - }
> -
> - pci_disable_device(korg1212->pci);
> - kfree(korg1212);
> - return 0;
> -}
> -
> -static int snd_korg1212_dev_free(struct snd_device *device)
> -{
> - struct snd_korg1212 *korg1212 = device->device_data;
> - K1212_DEBUG_PRINTK("K1212_DEBUG: Freeing device\n");
> - return snd_korg1212_free(korg1212);
> + snd_korg1212_TurnOffIdleMonitor(korg1212);
> + snd_korg1212_DisableCardInterrupts(korg1212);
> }
>
> -static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> - struct snd_korg1212 **rchip)
> +static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci)
>
> {
> int err, rc;
> @@ -2152,24 +2097,13 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> unsigned iomem_size;
> __maybe_unused unsigned ioport_size;
> __maybe_unused unsigned iomem2_size;
> - struct snd_korg1212 * korg1212;
> + struct snd_korg1212 *korg1212 = card->private_data;
> const struct firmware *dsp_code;
>
> - static const struct snd_device_ops ops = {
> - .dev_free = snd_korg1212_dev_free,
> - };
> -
> - * rchip = NULL;
> - err = pci_enable_device(pci);
> + err = pcim_enable_device(pci);
> if (err < 0)
> return err;
>
> - korg1212 = kzalloc(sizeof(*korg1212), GFP_KERNEL);
> - if (korg1212 == NULL) {
> - pci_disable_device(pci);
> - return -ENOMEM;
> - }
> -
> korg1212->card = card;
> korg1212->pci = pci;
>
> @@ -2198,12 +2132,9 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> for (i=0; i<kAudioChannels; i++)
> korg1212->volumePhase[i] = 0;
>
> - err = pci_request_regions(pci, "korg1212");
> - if (err < 0) {
> - kfree(korg1212);
> - pci_disable_device(pci);
> + err = pcim_iomap_regions_request_all(pci, 1 << 0, "korg1212");
> + if (err < 0)
> return err;
> - }
>
> korg1212->iomem = pci_resource_start(korg1212->pci, 0);
> korg1212->ioport = pci_resource_start(korg1212->pci, 1);
> @@ -2223,26 +2154,20 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> korg1212->iomem2, iomem2_size,
> stateName[korg1212->cardState]);
>
> - korg1212->iobase = ioremap(korg1212->iomem, iomem_size);
> - if (!korg1212->iobase) {
> - snd_printk(KERN_ERR "korg1212: unable to remap memory region 0x%lx-0x%lx\n", korg1212->iomem,
> - korg1212->iomem + iomem_size - 1);
> - snd_korg1212_free(korg1212);
> - return -EBUSY;
> - }
> + korg1212->iobase = pcim_iomap_table(pci)[0];
>
> - err = request_irq(pci->irq, snd_korg1212_interrupt,
> + err = devm_request_irq(&pci->dev, pci->irq, snd_korg1212_interrupt,
> IRQF_SHARED,
> KBUILD_MODNAME, korg1212);
>
> if (err) {
> snd_printk(KERN_ERR "korg1212: unable to grab IRQ %d\n", pci->irq);
> - snd_korg1212_free(korg1212);
> return -EBUSY;
> }
>
> korg1212->irq = pci->irq;
> card->sync_irq = korg1212->irq;
> + card->private_free = snd_korg1212_free;
>
> pci_set_master(korg1212->pci);
>
> @@ -2281,41 +2206,36 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> korg1212->idRegPtr,
> stateName[korg1212->cardState]);
>
> - if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> - sizeof(struct KorgSharedBuffer), &korg1212->dma_shared) < 0) {
> - snd_printk(KERN_ERR "korg1212: can not allocate shared buffer memory (%zd bytes)\n", sizeof(struct KorgSharedBuffer));
> - snd_korg1212_free(korg1212);
> - return -ENOMEM;
> - }
> - korg1212->sharedBufferPtr = (struct KorgSharedBuffer *)korg1212->dma_shared.area;
> - korg1212->sharedBufferPhy = korg1212->dma_shared.addr;
> + korg1212->dma_shared = snd_devm_alloc_pages(&pci->dev,
> + SNDRV_DMA_TYPE_DEV,
> + sizeof(struct KorgSharedBuffer));
> + if (!korg1212->dma_shared)
> + return -ENOMEM;
> + korg1212->sharedBufferPtr = (struct KorgSharedBuffer *)korg1212->dma_shared->area;
> + korg1212->sharedBufferPhy = korg1212->dma_shared->addr;
>
> K1212_DEBUG_PRINTK("K1212_DEBUG: Shared Buffer Area = 0x%p (0x%08lx), %d bytes\n", korg1212->sharedBufferPtr, korg1212->sharedBufferPhy, sizeof(struct KorgSharedBuffer));
>
> #ifndef K1212_LARGEALLOC
> -
> korg1212->DataBufsSize = sizeof(struct KorgAudioBuffer) * kNumBuffers;
> + korg1212->dma_play = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV,
> + korg1212->DataBufsSize);
> + if (!korg1212->dma_play)
> + return -ENOMEM;
>
> - if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> - korg1212->DataBufsSize, &korg1212->dma_play) < 0) {
> - snd_printk(KERN_ERR "korg1212: can not allocate play data buffer memory (%d bytes)\n", korg1212->DataBufsSize);
> - snd_korg1212_free(korg1212);
> - return -ENOMEM;
> - }
> - korg1212->playDataBufsPtr = (struct KorgAudioBuffer *)korg1212->dma_play.area;
> - korg1212->PlayDataPhy = korg1212->dma_play.addr;
> + korg1212->playDataBufsPtr = (struct KorgAudioBuffer *)korg1212->dma_play->area;
> + korg1212->PlayDataPhy = korg1212->dma_play->addr;
>
> K1212_DEBUG_PRINTK("K1212_DEBUG: Play Data Area = 0x%p (0x%08x), %d bytes\n",
> korg1212->playDataBufsPtr, korg1212->PlayDataPhy, korg1212->DataBufsSize);
>
> - if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> - korg1212->DataBufsSize, &korg1212->dma_rec) < 0) {
> - snd_printk(KERN_ERR "korg1212: can not allocate record data buffer memory (%d bytes)\n", korg1212->DataBufsSize);
> - snd_korg1212_free(korg1212);
> - return -ENOMEM;
> - }
> - korg1212->recordDataBufsPtr = (struct KorgAudioBuffer *)korg1212->dma_rec.area;
> - korg1212->RecDataPhy = korg1212->dma_rec.addr;
> + korg1212->dma_rec = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV,
> + korg1212->DataBufsSize);
> + if (!korg1212->dma_rec)
> + return -ENOMEM;
> +
> + korg1212->recordDataBufsPtr = (struct KorgAudioBuffer *)korg1212->dma_rec->area;
> + korg1212->RecDataPhy = korg1212->dma_rec->addr;
>
> K1212_DEBUG_PRINTK("K1212_DEBUG: Record Data Area = 0x%p (0x%08x), %d bytes\n",
> korg1212->recordDataBufsPtr, korg1212->RecDataPhy, korg1212->DataBufsSize);
> @@ -2336,26 +2256,22 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> korg1212->AdatTimeCodePhy = korg1212->sharedBufferPhy +
> offsetof(struct KorgSharedBuffer, AdatTimeCode);
>
> + korg1212->dma_dsp = snd_devm_alloc_pages(&pci->dev, SNDRV_DMA_TYPE_DEV,
> + dsp_code->size);
> + if (!korg1212->dma_dsp)
> + return -ENOMEM;
> +
Should this section be moved below the next one?
sound/pci/korg1212/korg1212.c:2260:8: error: variable 'dsp_code' is uninitialized when used here [-Werror,-Wuninitialized]
dsp_code->size);
^~~~~~~~
sound/pci/korg1212/korg1212.c:2101:33: note: initialize the variable 'dsp_code' to silence this warning
const struct firmware *dsp_code;
^
= NULL
1 error generated.
> err = request_firmware(&dsp_code, "korg/k1212.dsp", &pci->dev);
> if (err < 0) {
> snd_printk(KERN_ERR "firmware not available\n");
> - snd_korg1212_free(korg1212);
> return err;
> }
>
> - if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> - dsp_code->size, &korg1212->dma_dsp) < 0) {
> - snd_printk(KERN_ERR "korg1212: cannot allocate dsp code memory (%zd bytes)\n", dsp_code->size);
> - snd_korg1212_free(korg1212);
> - release_firmware(dsp_code);
> - return -ENOMEM;
> - }
> -
> K1212_DEBUG_PRINTK("K1212_DEBUG: DSP Code area = 0x%p (0x%08x) %d bytes [%s]\n",
> - korg1212->dma_dsp.area, korg1212->dma_dsp.addr, dsp_code->size,
> + korg1212->dma_dsp->area, korg1212->dma_dsp->addr, dsp_code->size,
> stateName[korg1212->cardState]);
>
> - memcpy(korg1212->dma_dsp.area, dsp_code->data, dsp_code->size);
> + memcpy(korg1212->dma_dsp->area, dsp_code->data, dsp_code->size);
>
> release_firmware(dsp_code);
>
> @@ -2364,12 +2280,6 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> if (rc)
> K1212_DEBUG_PRINTK("K1212_DEBUG: Reboot Card - RC = %d [%s]\n", rc, stateName[korg1212->cardState]);
>
> - err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, korg1212, &ops);
> - if (err < 0) {
> - snd_korg1212_free(korg1212);
> - return err;
> - }
> -
> snd_korg1212_EnableCardInterrupts(korg1212);
>
> mdelay(CARD_BOOT_DELAY_IN_MS);
> @@ -2411,10 +2321,8 @@ static int snd_korg1212_create(struct snd_card *card, struct pci_dev *pci,
> }
>
> snd_korg1212_proc_init(korg1212);
> -
> - * rchip = korg1212;
> - return 0;
>
> + return 0;
> }
>
> /*
> @@ -2437,16 +2345,15 @@ snd_korg1212_probe(struct pci_dev *pci,
> dev++;
> return -ENOENT;
> }
> - err = snd_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE,
> - 0, &card);
> + err = snd_devm_card_new(&pci->dev, index[dev], id[dev], THIS_MODULE,
> + sizeof(*korg1212), &card);
> if (err < 0)
> return err;
> + korg1212 = card->private_data;
>
> - err = snd_korg1212_create(card, pci, &korg1212);
> - if (err < 0) {
> - snd_card_free(card);
> + err = snd_korg1212_create(card, pci);
> + if (err < 0)
> return err;
> - }
>
> strcpy(card->driver, "korg1212");
> strcpy(card->shortname, "korg1212");
> @@ -2456,25 +2363,17 @@ snd_korg1212_probe(struct pci_dev *pci,
> K1212_DEBUG_PRINTK("K1212_DEBUG: %s\n", card->longname);
>
> err = snd_card_register(card);
> - if (err < 0) {
> - snd_card_free(card);
> + if (err < 0)
> return err;
> - }
> pci_set_drvdata(pci, card);
> dev++;
> return 0;
> }
>
> -static void snd_korg1212_remove(struct pci_dev *pci)
> -{
> - snd_card_free(pci_get_drvdata(pci));
> -}
> -
> static struct pci_driver korg1212_driver = {
> .name = KBUILD_MODNAME,
> .id_table = snd_korg1212_ids,
> .probe = snd_korg1212_probe,
> - .remove = snd_korg1212_remove,
> };
>
> module_pci_driver(korg1212_driver);
> --
> 2.26.2
