gpio-intel-mid: fix the incorrect return of idle callback

PATCH In review xinhui.pan 3 participants 10 messages Jan 28, 2014 – Jan 29, 2014 GPIO

lore

Share & tools

lore

xinhui.pan

author

2014-01-28 08:51 UTC #0 lore raw

PATCH 1/1 gpio-intel-mid: fix the incorrect return of idle callback

From: "xinhui.pan" <[email protected]>

intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.

Signed-off-by bo.he <[email protected]>

Signed-off-by xinhui.pan <[email protected]>

---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpio/gpio-intel-mid.c +3 -1

index d1b50ef..05749a3 100644
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;
 }

Signature · 1 line

1.7.9.5

David Cohen

Acked-by on v1 1/1

2014-01-28 16:42 UTC #1 lore raw

Hi,

Thanks for the patch :)

Quoted 7 lines · xinhui.pan

From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>

Acked-by David Cohen <[email protected]>

Felipe Balbi

2014-01-28 16:50 UTC #2 lore raw

In reply to xinhui.pan

Quoted 16 lines · xinhui.pan

From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>
---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-intel-mid.c +0 -0
index d1b50ef..05749a3 100644

drivers/gpio/gpio-intel-mid.c static const struct irq_domain_ops intel_gpio_irq_ops = { · line 400

Quoted 8 lines

drivers/gpio/gpio-intel-mid.c +3 -1

@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;

wait, is it only me or this would look a lot better as:

static int intel_gpio_runtime_idle(struct device *dev)

{
	return pm_schedule_suspend(dev, 500);
}

cheers

Signature · 1 line

balbi

David Cohen

2014-01-28 17:19 UTC #3 lore raw

Quoted 28 lines · Felipe Balbi

Quoted 16 lines · xinhui.pan
From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>
---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-intel-mid.c +0 -0
index d1b50ef..05749a3 100644
drivers/gpio/gpio-intel-mid.c static const struct irq_domain_ops intel_gpio_irq_ops = { · line 400
Quoted 8 lines
drivers/gpio/gpio-intel-mid.c +3 -1
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;
wait, is it only me or this would look a lot better as:

Quoted 4 lines

static int intel_gpio_runtime_idle(struct device *dev)
{
	return pm_schedule_suspend(dev, 500);
}

The reply to your suggestion is probably in this commit :)

---
commit 45f0a85c8258741d11bda25c0a5669c06267204a

Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Jun 3 21:49:52 2013 +0200

PM / Runtime: Rework the "runtime idle" helper routine
---

We won't return 0 from here.

Br, David

Quoted 5 lines

cheers
Signature · 1 line
balbi

Felipe Balbi

2014-01-28 18:13 UTC #4 lore raw

Quoted 38 lines · David Cohen

Quoted 28 lines · Felipe Balbi
Quoted 16 lines · xinhui.pan
From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>
---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-intel-mid.c +0 -0
index d1b50ef..05749a3 100644
drivers/gpio/gpio-intel-mid.c static const struct irq_domain_ops intel_gpio_irq_ops = { · line 400
Quoted 8 lines
drivers/gpio/gpio-intel-mid.c +3 -1
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;
wait, is it only me or this would look a lot better as:
Quoted 4 lines
static int intel_gpio_runtime_idle(struct device *dev)
{
	return pm_schedule_suspend(dev, 500);
}
The reply to your suggestion is probably in this commit :)
---
commit 45f0a85c8258741d11bda25c0a5669c06267204a

Quoted 7 lines

Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Jun 3 21:49:52 2013 +0200
PM / Runtime: Rework the "runtime idle" helper routine
---
We won't return 0 from here.

so you never want to return 0, why don't you, then:

static int intel_gpio_runtime_idle(struct device *dev)

{
	pm_schedule_suspend(dev, 500);
	return -EBUSY;
}

just like drivers/tty/serial/mfd.c::serial_hsu_runtime_idle() is doing ?

Signature · 1 line

balbi

David Cohen

2014-01-29 00:08 UTC #5 lore raw

Quoted 47 lines · Felipe Balbi

Quoted 38 lines · David Cohen
Quoted 28 lines · Felipe Balbi
Quoted 16 lines · xinhui.pan
From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>
---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-intel-mid.c +0 -0
index d1b50ef..05749a3 100644
drivers/gpio/gpio-intel-mid.c static const struct irq_domain_ops intel_gpio_irq_ops = { · line 400
Quoted 8 lines
drivers/gpio/gpio-intel-mid.c +3 -1
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;
wait, is it only me or this would look a lot better as:
Quoted 4 lines
static int intel_gpio_runtime_idle(struct device *dev)
{
	return pm_schedule_suspend(dev, 500);
}
The reply to your suggestion is probably in this commit :)
---
commit 45f0a85c8258741d11bda25c0a5669c06267204a
Quoted 7 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Jun 3 21:49:52 2013 +0200
PM / Runtime: Rework the "runtime idle" helper routine
---
We won't return 0 from here.

Quoted 7 lines

so you never want to return 0, why don't you, then:
static int intel_gpio_runtime_idle(struct device *dev)
{
	pm_schedule_suspend(dev, 500);
	return -EBUSY;
}

That's how it is currently :)

But this patch is making the function to return a different code in case of error. IMHO there is not much fuctional gain with it, but I see perhaps one extra info for tracing during development.

Anyway, I'll let Xinhui to do further comment since he's the author.

Br, David

Quoted 5 lines

just like drivers/tty/serial/mfd.c::serial_hsu_runtime_idle() is doing ?
Signature · 1 line
balbi

xinhui.pan

author

2014-01-29 07:23 UTC #6 lore raw

Quoted 58 lines · David Cohen

Quoted 47 lines · Felipe Balbi
Quoted 38 lines · David Cohen
Quoted 28 lines · Felipe Balbi
Quoted 16 lines · xinhui.pan
From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>
---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-intel-mid.c +0 -0
index d1b50ef..05749a3 100644
drivers/gpio/gpio-intel-mid.c static const struct irq_domain_ops intel_gpio_irq_ops = { · line 400
Quoted 8 lines
drivers/gpio/gpio-intel-mid.c +3 -1
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;
wait, is it only me or this would look a lot better as:
Quoted 4 lines
static int intel_gpio_runtime_idle(struct device *dev)
{
	return pm_schedule_suspend(dev, 500);
}
The reply to your suggestion is probably in this commit :)
---
commit 45f0a85c8258741d11bda25c0a5669c06267204a
Quoted 7 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Jun 3 21:49:52 2013 +0200
PM / Runtime: Rework the "runtime idle" helper routine
---
We won't return 0 from here.
Quoted 7 lines
so you never want to return 0, why don't you, then:
static int intel_gpio_runtime_idle(struct device *dev)
{
	pm_schedule_suspend(dev, 500);
	return -EBUSY;
}
That's how it is currently :)

Quoted 8 lines

But this patch is making the function to return a different code in case of error. IMHO there is not much fuctional gain with it, but I see perhaps one extra info for tracing during development.
Anyway, I'll let Xinhui to do further comment since he's the author.
Br, David

hi ,David & Balbi
I checked several drivers yesterday to see how they use pm_schedule_suspend
then found one bug in i2c. Also I noticed gpio.
I think returning a correct error code is important.So I change -EBUSY to *err*. To be honest,current code works well.

Quoted 7 lines

Quoted 5 lines
just like drivers/tty/serial/mfd.c::serial_hsu_runtime_idle() is doing ?
Signature · 1 line
balbi

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Cohen

2014-01-29 19:07 UTC #7 lore raw

Quoted 66 lines · xinhui.pan

Quoted 64 lines · David Cohen
Quoted 47 lines · Felipe Balbi
Quoted 38 lines · David Cohen
Quoted 28 lines · Felipe Balbi
Quoted 16 lines · xinhui.pan
From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>
---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-intel-mid.c +0 -0
index d1b50ef..05749a3 100644
drivers/gpio/gpio-intel-mid.c static const struct irq_domain_ops intel_gpio_irq_ops = { · line 400
Quoted 8 lines
drivers/gpio/gpio-intel-mid.c +3 -1
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;
wait, is it only me or this would look a lot better as:
Quoted 4 lines
static int intel_gpio_runtime_idle(struct device *dev)
{
	return pm_schedule_suspend(dev, 500);
}
The reply to your suggestion is probably in this commit :)
---
commit 45f0a85c8258741d11bda25c0a5669c06267204a
Quoted 7 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Jun 3 21:49:52 2013 +0200
PM / Runtime: Rework the "runtime idle" helper routine
---
We won't return 0 from here.
Quoted 7 lines
so you never want to return 0, why don't you, then:
static int intel_gpio_runtime_idle(struct device *dev)
{
	pm_schedule_suspend(dev, 500);
	return -EBUSY;
}
That's how it is currently :)
But this patch is making the function to return a different code in case of error. IMHO there is not much fuctional gain with it, but I see perhaps one extra info for tracing during development.
Anyway, I'll let Xinhui to do further comment since he's the author.

Quoted 7 lines

Quoted 2 lines
Br, David
hi ,David & Balbi
I checked several drivers yesterday to see how they use pm_schedule_suspend
then found one bug in i2c. Also I noticed gpio.
I think returning a correct error code is important.So I change -EBUSY to *err*. To be honest,current code works well.

In my experience, when I'm using fancy things like lauterbach a proper error code may save couple of minutes in my life :)

I keep my ack here.

Br, David

Quoted 7 lines

Quoted 7 lines
Quoted 5 lines
just like drivers/tty/serial/mfd.c::serial_hsu_runtime_idle() is doing ?
Signature · 1 line
balbi

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Felipe Balbi

2014-01-29 19:53 UTC #8 lore raw

Quoted 75 lines · David Cohen

Quoted 66 lines · xinhui.pan
Quoted 64 lines · David Cohen
Quoted 47 lines · Felipe Balbi
Quoted 38 lines · David Cohen
Quoted 28 lines · Felipe Balbi
Quoted 16 lines · xinhui.pan
From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>
---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-intel-mid.c +0 -0
index d1b50ef..05749a3 100644
drivers/gpio/gpio-intel-mid.c static const struct irq_domain_ops intel_gpio_irq_ops = { · line 400
Quoted 8 lines
drivers/gpio/gpio-intel-mid.c +3 -1
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;
wait, is it only me or this would look a lot better as:
Quoted 4 lines
static int intel_gpio_runtime_idle(struct device *dev)
{
	return pm_schedule_suspend(dev, 500);
}
The reply to your suggestion is probably in this commit :)
---
commit 45f0a85c8258741d11bda25c0a5669c06267204a
Quoted 7 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Jun 3 21:49:52 2013 +0200
PM / Runtime: Rework the "runtime idle" helper routine
---
We won't return 0 from here.
Quoted 7 lines
so you never want to return 0, why don't you, then:
static int intel_gpio_runtime_idle(struct device *dev)
{
	pm_schedule_suspend(dev, 500);
	return -EBUSY;
}
That's how it is currently :)
But this patch is making the function to return a different code in case of error. IMHO there is not much fuctional gain with it, but I see perhaps one extra info for tracing during development.
Anyway, I'll let Xinhui to do further comment since he's the author.
Quoted 7 lines
Quoted 2 lines
Br, David
hi ,David & Balbi
I checked several drivers yesterday to see how they use pm_schedule_suspend
then found one bug in i2c. Also I noticed gpio.
I think returning a correct error code is important.So I change -EBUSY to *err*. To be honest,current code works well.

Quoted 4 lines

In my experience, when I'm using fancy things like lauterbach a proper error code may save couple of minutes in my life :)
I keep my ack here.

fair enough, sorry for the noise ;-) It could still be simplified a bit:

	return err ?: -EBUSY;

Signature · 1 line

balbi

David Cohen

2014-01-29 21:01 UTC #9 lore raw

Quoted 76 lines · Felipe Balbi

Quoted 75 lines · David Cohen
Quoted 73 lines · xinhui.pan
Quoted 58 lines · David Cohen
Quoted 47 lines · Felipe Balbi
Quoted 38 lines · David Cohen
Quoted 28 lines · Felipe Balbi
Quoted 16 lines · xinhui.pan
From: "xinhui.pan" <[email protected]>
intel_gpio_runtime_idle should return correct error code if it do fail. make it more correct even though -EBUSY is the most possible return value.
Signed-off-by bo.he <[email protected]>
Signed-off-by xinhui.pan <[email protected]>
---
 drivers/gpio/gpio-intel-mid.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/gpio/gpio-intel-mid.c +0 -0
index d1b50ef..05749a3 100644
drivers/gpio/gpio-intel-mid.c static const struct irq_domain_ops intel_gpio_irq_ops = { · line 400
Quoted 8 lines
drivers/gpio/gpio-intel-mid.c +3 -1
@@ -394,7 +394,9 @@ static const struct irq_domain_ops intel_gpio_irq_ops = {
 
 static int intel_gpio_runtime_idle(struct device *dev)
 {
-	pm_schedule_suspend(dev, 500);
+	int err = pm_schedule_suspend(dev, 500);
+	if (err)
+		return err;
 	return -EBUSY;
wait, is it only me or this would look a lot better as:
Quoted 4 lines
static int intel_gpio_runtime_idle(struct device *dev)
{
	return pm_schedule_suspend(dev, 500);
}
The reply to your suggestion is probably in this commit :)
---
commit 45f0a85c8258741d11bda25c0a5669c06267204a
Quoted 7 lines
Author: Rafael J. Wysocki <[email protected]>
Date:   Mon Jun 3 21:49:52 2013 +0200
PM / Runtime: Rework the "runtime idle" helper routine
---
We won't return 0 from here.
Quoted 7 lines
so you never want to return 0, why don't you, then:
static int intel_gpio_runtime_idle(struct device *dev)
{
	pm_schedule_suspend(dev, 500);
	return -EBUSY;
}
That's how it is currently :)
Quoted 8 lines
But this patch is making the function to return a different code in case of error. IMHO there is not much fuctional gain with it, but I see perhaps one extra info for tracing during development.
Anyway, I'll let Xinhui to do further comment since he's the author.
Br, David
hi ,David & Balbi
I checked several drivers yesterday to see how they use pm_schedule_suspend
then found one bug in i2c. Also I noticed gpio.
I think returning a correct error code is important.So I change -EBUSY to *err*. To be honest,current code works well.

Quoted 8 lines

Quoted 4 lines
In my experience, when I'm using fancy things like lauterbach a proper error code may save couple of minutes in my life :)
I keep my ack here.
fair enough, sorry for the noise ;-) It could still be simplified a bit:
	return err ?: -EBUSY;

Agreed :)
Xinhui, could we have this suggestion in your patch?

Br, David

Quoted 3 lines

Signature · 2 lines
-- 
balbi

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html