| Message ID | 1436343951-3482-1-git-send-email-koro.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 08, 2015 at 04:25:50PM +0800, Koro Chen wrote: > The driver does not free irq if snd_soc_register_codec fails. > It does not return error when request irq failed, either. > Fix this by using devm_request_threaded_irq(), and returns when error. Unfortunately this isn't safe... > - if (i2c->irq) > - free_irq(i2c->irq, rt5645); > - > cancel_delayed_work_sync(&rt5645->jack_detect_work); This work item is queued up by the interrupt handler so we need to unregister the interrupt before we cancel any pending work otherwise it's possible that the interrupt may fire after we cancelled the work.
On Wed, 2015-07-08 at 12:14 +0100, Mark Brown wrote: > On Wed, Jul 08, 2015 at 04:25:50PM +0800, Koro Chen wrote: > > > The driver does not free irq if snd_soc_register_codec fails. > > It does not return error when request irq failed, either. > > Fix this by using devm_request_threaded_irq(), and returns when error. > > Unfortunately this isn't safe... > > > - if (i2c->irq) > > - free_irq(i2c->irq, rt5645); > > - > > cancel_delayed_work_sync(&rt5645->jack_detect_work); > > This work item is queued up by the interrupt handler so we need to > unregister the interrupt before we cancel any pending work otherwise > it's possible that the interrupt may fire after we cancelled the work. Thank you for seeing this, I didn't notice the delayed work. Do you think I should use devm_request_threaded_irq(), and change free_irq to devm_free_irq in remove? Or I should keep the original request_thread_irq(), and just add a free_irq() during probe failed?
On Thu, Jul 09, 2015 at 09:48:13AM +0800, Koro Chen wrote: > Do you think I should use devm_request_threaded_irq(), and change > free_irq to devm_free_irq in remove? Or I should keep the original > request_thread_irq(), and just add a free_irq() during probe failed? I'd just keep request_threaded_irq() and use free_irq().
On Thu, 2015-07-09 at 12:10 +0100, Mark Brown wrote: > On Thu, Jul 09, 2015 at 09:48:13AM +0800, Koro Chen wrote: > > > Do you think I should use devm_request_threaded_irq(), and change > > free_irq to devm_free_irq in remove? Or I should keep the original > > request_thread_irq(), and just add a free_irq() during probe failed? > > I'd just keep request_threaded_irq() and use free_irq(). OK, thanks, I will send a new patch. And about the patch 2/2 regulator support, is there any problem I should also fix?
On Fri, Jul 10, 2015 at 10:43:21PM +0800, Koro Chen wrote: > And about the patch 2/2 regulator support, is there any problem I should > also fix? Should be fine, though I might've missed something.
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index 9dfa431..f9f2db8 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3427,11 +3427,15 @@ static int rt5645_i2c_probe(struct i2c_client *i2c, INIT_DELAYED_WORK(&rt5645->jack_detect_work, rt5645_jack_detect_work); if (rt5645->i2c->irq) { - ret = request_threaded_irq(rt5645->i2c->irq, NULL, rt5645_irq, - IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING - | IRQF_ONESHOT, "rt5645", rt5645); - if (ret) + ret = devm_request_threaded_irq(&i2c->dev, rt5645->i2c->irq, + NULL, rt5645_irq, + IRQF_TRIGGER_RISING | + IRQF_TRIGGER_FALLING | + IRQF_ONESHOT, "rt5645", rt5645); + if (ret) { dev_err(&i2c->dev, "Failed to reguest IRQ: %d\n", ret); + return ret; + } } return snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5645, @@ -3442,9 +3446,6 @@ static int rt5645_i2c_remove(struct i2c_client *i2c) { struct rt5645_priv *rt5645 = i2c_get_clientdata(i2c); - if (i2c->irq) - free_irq(i2c->irq, rt5645); - cancel_delayed_work_sync(&rt5645->jack_detect_work); snd_soc_unregister_codec(&i2c->dev);
The driver does not free irq if snd_soc_register_codec fails. It does not return error when request irq failed, either. Fix this by using devm_request_threaded_irq(), and returns when error. Signed-off-by: Koro Chen <koro.chen@mediatek.com> --- sound/soc/codecs/rt5645.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)