[PATCH 1/2] ASoC: rt5645: Fix missing free_irq

[PATCH 1/2] ASoC: rt5645: Fix missing free_irq

[Koro Chen]

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(-)

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);
-- 
1.8.1.1.dirty

[PATCH 2/2] ASoC: rt5645: Add regulator support

[Koro Chen]

This adds basic regulator support for rt5645.

Signed-off-by: Koro Chen <koro.chen@mediatek.com>
---
 sound/soc/codecs/rt5645.c | 71 +++++++++++++++++++++++++++++++++++++++++++----
 sound/soc/codecs/rt5645.h | 26 -----------------
 2 files changed, 66 insertions(+), 31 deletions(-)

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index f9f2db8..7713e2b 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -21,6 +21,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/acpi.h>
 #include <linux/dmi.h>
+#include <linux/regulator/consumer.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -223,6 +224,38 @@ static const struct reg_default rt5645_reg[] = {
 	{ 0xff, 0x6308 },
 };
 
+static const char *const rt5645_supply_names[] = {
+	"avdd",
+	"cpvdd",
+};
+
+struct rt5645_priv {
+	struct snd_soc_codec *codec;
+	struct rt5645_platform_data pdata;
+	struct regmap *regmap;
+	struct i2c_client *i2c;
+	struct gpio_desc *gpiod_hp_det;
+	struct snd_soc_jack *hp_jack;
+	struct snd_soc_jack *mic_jack;
+	struct snd_soc_jack *btn_jack;
+	struct delayed_work jack_detect_work;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(rt5645_supply_names)];
+
+	int codec_type;
+	int sysclk;
+	int sysclk_src;
+	int lrck[RT5645_AIFS];
+	int bclk[RT5645_AIFS];
+	int master[RT5645_AIFS];
+
+	int pll_src;
+	int pll_in;
+	int pll_out;
+
+	int jack_type;
+	bool en_button_func;
+};
+
 static int rt5645_reset(struct snd_soc_codec *codec)
 {
 	return snd_soc_write(codec, RT5645_RESET, 0);
@@ -3247,7 +3280,7 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 {
 	struct rt5645_platform_data *pdata = dev_get_platdata(&i2c->dev);
 	struct rt5645_priv *rt5645;
-	int ret;
+	int ret, i;
 	unsigned int val;
 
 	rt5645 = devm_kzalloc(&i2c->dev, sizeof(struct rt5645_priv),
@@ -3281,6 +3314,24 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	for (i = 0; i < ARRAY_SIZE(rt5645->supplies); i++)
+		rt5645->supplies[i].supply = rt5645_supply_names[i];
+
+	ret = devm_regulator_bulk_get(&i2c->dev,
+				      ARRAY_SIZE(rt5645->supplies),
+				      rt5645->supplies);
+	if (ret) {
+		dev_err(&i2c->dev, "Failed to request supplies: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(rt5645->supplies),
+				    rt5645->supplies);
+	if (ret) {
+		dev_err(&i2c->dev, "Failed to enable supplies: %d\n", ret);
+		return ret;
+	}
+
 	regmap_read(rt5645->regmap, RT5645_VENDOR_ID2, &val);
 
 	switch (val) {
@@ -3294,7 +3345,8 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 		dev_err(&i2c->dev,
 			"Device with ID register %#x is not rt5645 or rt5650\n",
 			val);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto err_enable;
 	}
 
 	if (rt5645->codec_type == CODEC_TYPE_RT5650) {
@@ -3434,12 +3486,20 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
 						IRQF_ONESHOT, "rt5645", rt5645);
 		if (ret) {
 			dev_err(&i2c->dev, "Failed to reguest IRQ: %d\n", ret);
-			return ret;
+			goto err_enable;
 		}
 	}
 
-	return snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5645,
-				      rt5645_dai, ARRAY_SIZE(rt5645_dai));
+	ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt5645,
+				     rt5645_dai, ARRAY_SIZE(rt5645_dai));
+	if (ret)
+		goto err_enable;
+
+	return 0;
+
+err_enable:
+	regulator_bulk_disable(ARRAY_SIZE(rt5645->supplies), rt5645->supplies);
+	return ret;
 }
 
 static int rt5645_i2c_remove(struct i2c_client *i2c)
@@ -3449,6 +3509,7 @@ static int rt5645_i2c_remove(struct i2c_client *i2c)
 	cancel_delayed_work_sync(&rt5645->jack_detect_work);
 
 	snd_soc_unregister_codec(&i2c->dev);
+	regulator_bulk_disable(ARRAY_SIZE(rt5645->supplies), rt5645->supplies);
 
 	return 0;
 }
diff --git a/sound/soc/codecs/rt5645.h b/sound/soc/codecs/rt5645.h
index 0353a6a..199b22f 100644
--- a/sound/soc/codecs/rt5645.h
+++ b/sound/soc/codecs/rt5645.h
@@ -2177,32 +2177,6 @@ enum {
 int rt5645_sel_asrc_clk_src(struct snd_soc_codec *codec,
 		unsigned int filter_mask, unsigned int clk_src);
 
-struct rt5645_priv {
-	struct snd_soc_codec *codec;
-	struct rt5645_platform_data pdata;
-	struct regmap *regmap;
-	struct i2c_client *i2c;
-	struct gpio_desc *gpiod_hp_det;
-	struct snd_soc_jack *hp_jack;
-	struct snd_soc_jack *mic_jack;
-	struct snd_soc_jack *btn_jack;
-	struct delayed_work jack_detect_work;
-
-	int codec_type;
-	int sysclk;
-	int sysclk_src;
-	int lrck[RT5645_AIFS];
-	int bclk[RT5645_AIFS];
-	int master[RT5645_AIFS];
-
-	int pll_src;
-	int pll_in;
-	int pll_out;
-
-	int jack_type;
-	bool en_button_func;
-};
-
 int rt5645_set_jack_detect(struct snd_soc_codec *codec,
 	struct snd_soc_jack *hp_jack, struct snd_soc_jack *mic_jack,
 	struct snd_soc_jack *btn_jack);
-- 
1.8.1.1.dirty

Re: [PATCH 1/2] ASoC: rt5645: Fix missing free_irq

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] ASoC: rt5645: Fix missing free_irq

[Koro Chen]

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?
 

Re: [PATCH 1/2] ASoC: rt5645: Fix missing free_irq

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

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().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] ASoC: rt5645: Fix missing free_irq

[Koro Chen]

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?
 

Re: [PATCH 1/2] ASoC: rt5645: Fix missing free_irq

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]