On Wed, Jan 7, 2015 at 3:12 PM, Takashi Iwai tiwai@suse.de wrote:
At Wed, 7 Jan 2015 14:20:26 +0200, Andy Shevchenko wrote:
On Wed, Jan 7, 2015 at 1:27 PM, Takashi Iwai tiwai@suse.de wrote:
At Wed, 7 Jan 2015 12:54:40 +0200, Andy Shevchenko wrote:
On Wed, Jan 7, 2015 at 9:11 AM, Takashi Iwai tiwai@suse.de wrote:
At Wed, 7 Jan 2015 01:40:17 +0200, Andy Shevchenko wrote:
The managed functions allow to get ->probe() and ->remove() simplier. This patch converts code to use managed functions.
This doesn't work well because of the order of release calls in device_release(). devres_release_all() is called at first before anything else. Thus the card's free callback would be called after it, and snd_fm801_free() touches the hardware before disabling.
Hmm… I didn't get what make those calls. Sound core? The driver is a normal PCI driver, so resources will be freed *after* ->remove() of driver was called. Please, elaborate.
Ah, OK, this seems working, as this is the managed *pci* device that is a parent of the card device, thus it shall be released after the children.
But any reason not to use pcim_iomap_regions_request_all()?
Yes. I was doubt to put a comment about this.
Well, pcim_iomap_regions_request_all() can just request regions without actually iomapping via passing 0 to mask. But, now looking at the code again, I couldn't find the place releasing the non-mmio regions. Hmm... it might be a leak?
It's in snd_fm801_free(). pci_release_regions(chip->pci);
So, the initial idea is to move to MMIO instead of IO access. That's why you may see previous patch about fm801_{read,write}w() macros. I would like to do that later.
Yeah, of course, rewriting to MMIO would be nicer. I guess you're testing with the real hardware, so I'm for this rewrite.
thanks,
Takashi
thanks,
Takashi
Takashi
While here remove the dead code and fix the value printed in error message.
Signed-off-by: Andy Shevchenko andy.shevchenko@gmail.com
sound/pci/fm801.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c index dcda3c1..2708500 100644 --- a/sound/pci/fm801.c +++ b/sound/pci/fm801.c @@ -1178,12 +1178,8 @@ static int snd_fm801_free(struct fm801 *chip) v4l2_device_unregister(&chip->v4l2_dev); } #endif
if (chip->irq >= 0)free_irq(chip->irq, chip); pci_release_regions(chip->pci);pci_disable_device(chip->pci);kfree(chip); return 0;}
@@ -1206,28 +1202,23 @@ static int snd_fm801_create(struct snd_card *card, };
*rchip = NULL;
if ((err = pci_enable_device(pci)) < 0)
if ((err = pcim_enable_device(pci)) < 0) return err;
chip = kzalloc(sizeof(*chip), GFP_KERNEL);if (chip == NULL) {pci_disable_device(pci);
chip = devm_kzalloc(&pci->dev, sizeof(*chip), GFP_KERNEL);if (chip == NULL) return -ENOMEM;
} spin_lock_init(&chip->reg_lock); chip->card = card; chip->pci = pci; chip->irq = -1; chip->tea575x_tuner = tea575x_tuner;if ((err = pci_request_regions(pci, "FM801")) < 0) {kfree(chip);pci_disable_device(pci);
if ((err = pci_request_regions(pci, "FM801")) < 0) return err;
} chip->port = pci_resource_start(pci, 0); if ((tea575x_tuner & TUNER_ONLY) == 0) {if (request_irq(pci->irq, snd_fm801_interrupt, IRQF_SHARED,KBUILD_MODNAME, chip)) {dev_err(card->dev, "unable to grab IRQ %d\n", chip->irq);
if (devm_request_irq(&pci->dev, pci->irq, snd_fm801_interrupt,IRQF_SHARED, KBUILD_MODNAME, chip)) {dev_err(card->dev, "unable to grab IRQ %d\n", pci->irq); snd_fm801_free(chip); return -EBUSY; }@@ -1242,12 +1233,6 @@ static int snd_fm801_create(struct snd_card *card, /* init might set tuner access method */ tea575x_tuner = chip->tea575x_tuner;
if (chip->irq >= 0 && (tea575x_tuner & TUNER_ONLY)) {pci_clear_master(pci);free_irq(chip->irq, chip);chip->irq = -1;}if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) { snd_fm801_free(chip); return err;-- 1.8.3.101.g727a46b
-- With Best Regards, Andy Shevchenko
-- With Best Regards, Andy Shevchenko