serial8250: update to dev_pm_ops
>From dmesg:
Platform driver 'serial8250' needs updating - please use dev_pm_ops
Is this how it should be done? I have not tested it, but it compiles fine.
Should I send in similar patches for other drivers that also give this
message?
Signed-off-by: Erik Ekman <[email protected]>
---
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index fb867a9..dcbc6da 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2982,42 +2982,46 @@ static int __devexit serial8250_remove(struct platform_device *dev)
return 0;
}
-static int serial8250_suspend(struct platform_device *dev, pm_message_t state)
+static int serial8250_suspend(struct device *dev)
{
int i;
for (i = 0; i < UART_NR; i++) {
struct uart_8250_port *up = &serial8250_ports[i];
- if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
+ if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
uart_suspend_port(&serial8250_reg, &up->port);
}
return 0;
}
-static int serial8250_resume(struct platform_device *dev)
+static int serial8250_resume(struct device *dev)
{
int i;
for (i = 0; i < UART_NR; i++) {
struct uart_8250_port *up = &serial8250_ports[i];
- if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
+ if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
serial8250_resume_port(i);
}
return 0;
}
+static struct dev_pm_ops serial8250_pm_ops = {
+ .suspend = serial8250_suspend,
+ .resume = serial8250_resume,
+};
+
static struct platform_driver serial8250_isa_driver = {
.probe = serial8250_probe,
.remove = __devexit_p(serial8250_remove),
- .suspend = serial8250_suspend,
- .resume = serial8250_resume,
.driver = {
.name = "serial8250",
.owner = THIS_MODULE,
+ .pm = &serial8250_pm_ops,
},
};
Adding some CCs as there's no reply yet.
> From dmesg:> Platform driver 'serial8250' needs updating - please use dev_pm_ops>> Is this how it should be done?
Alan or Rafael can probably tell.
One comment from me at the bottom.
> I have not tested it, but it compiles fine. Should I send in similar> patches for other drivers that also give this message?
Note that comments and questions that should not go in the commit logshould go between the separator after the Signed-off-by line and thepatch.
> Signed-off-by: Erik Ekman <[email protected]>> ---
[Comments and questions go here]
>diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c>index fb867a9..dcbc6da 100644>--- a/drivers/serial/8250.c>+++ b/drivers/serial/8250.c>@@ -2982,42 +2982,46 @@ static int __devexit serial8250_remove(struct platform_device *dev)>????????return 0;>?}>?>-static int serial8250_suspend(struct platform_device *dev, pm_message_t state)>+static int serial8250_suspend(struct device *dev)>?{>????????int i;>?>????????for (i = 0; i < UART_NR; i++) {>????????????????struct uart_8250_port *up = &serial8250_ports[i];>?>-???????????????if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)>+???????????????if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)>????????????????????????uart_suspend_port(&serial8250_reg, &up->port);>????????}>?>????????return 0;>?}>?>-static int serial8250_resume(struct platform_device *dev)>+static int serial8250_resume(struct device *dev)>?{>????????int i;>?>????????for (i = 0; i < UART_NR; i++) {>????????????????struct uart_8250_port *up = &serial8250_ports[i];>?>-???????????????if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)>+???????????????if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)>????????????????????????serial8250_resume_port(i);>????????}>?>????????return 0;>?}>?>+static struct dev_pm_ops serial8250_pm_ops = {>+???????.suspend = serial8250_suspend,>+???????.resume ?= serial8250_resume,>+};>+>?static struct platform_driver serial8250_isa_driver = {>????????.probe??????????= serial8250_probe,>????????.remove?????????= __devexit_p(serial8250_remove),>-???????.suspend????????= serial8250_suspend,>-???????.resume?????????= serial8250_resume,>????????.driver?????????= {>????????????????.name???= "serial8250",>????????????????.owner??= THIS_MODULE,>+???????????????.pm ? ? = &serial8250_pm_ops,
Git warns that there is whitespace at the end of this line...
>????????},>?};
Cheers,FJP????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, 8 Jul 2009 13:10:51 +0200
Frans Pop <[email protected]> wrote:
> Adding some CCs as there's no reply yet.
>
> > From dmesg:
> > Platform driver 'serial8250' needs updating - please use dev_pm_ops
> >
> > Is this how it should be done?
>
> Alan or Rafael can probably tell.
No idea. I've not looked at this so your guess is as good as mine (but
looks quite believable. I don't plan to do anything until someone who
needs platform 8250 support sends a tested version of the change
however.
Alan
h
On Wednesday 08 July 2009, Alan Cox wrote:
> On Wed, 8 Jul 2009 Frans Pop <[email protected]> wrote:
> > > From dmesg:
> > > Platform driver 'serial8250' needs updating - please use dev_pm_ops
> > >
> > > Is this how it should be done?
> >
> > Alan or Rafael can probably tell.
>
> No idea. I've not looked at this so your guess is as good as mine (but
> looks quite believable. I don't plan to do anything until someone who
> needs platform 8250 support sends a tested version of the change
> however.
I have done a few of these myself now and followed all other similar
updates for other drivers. From that experience I can say that this update
is both trivial and correct. Hope you will reconsider your reluctance on
that basis.
Below Erik's patch with the commit log cleaned up, one whitespace problem
fixed and my Reviewed-by added.
Cheers,
FJP
---
From: Erik Ekman <[email protected]>
Subject: serial8250: update to dev_pm_ops
>From dmesg:
Platform driver 'serial8250' needs updating - please use dev_pm_ops
Signed-off-by: Erik Ekman <[email protected]>
Reviewed-by: Frans Pop <[email protected]>
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index fb867a9..496c85f 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2982,42 +2982,46 @@ static int __devexit serial8250_remove(struct platform_device *dev)
return 0;
}
-static int serial8250_suspend(struct platform_device *dev, pm_message_t state)
+static int serial8250_suspend(struct device *dev)
{
int i;
for (i = 0; i < UART_NR; i++) {
struct uart_8250_port *up = &serial8250_ports[i];
- if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
+ if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
uart_suspend_port(&serial8250_reg, &up->port);
}
return 0;
}
-static int serial8250_resume(struct platform_device *dev)
+static int serial8250_resume(struct device *dev)
{
int i;
for (i = 0; i < UART_NR; i++) {
struct uart_8250_port *up = &serial8250_ports[i];
- if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
+ if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
serial8250_resume_port(i);
}
return 0;
}
+static struct dev_pm_ops serial8250_pm_ops = {
+ .suspend = serial8250_suspend,
+ .resume = serial8250_resume,
+};
+
static struct platform_driver serial8250_isa_driver = {
.probe = serial8250_probe,
.remove = __devexit_p(serial8250_remove),
- .suspend = serial8250_suspend,
- .resume = serial8250_resume,
.driver = {
.name = "serial8250",
.owner = THIS_MODULE,
+ .pm = &serial8250_pm_ops,
},
};
On Saturday 25 July 2009, Frans Pop wrote:
> On Wednesday 08 July 2009, Alan Cox wrote:
> > On Wed, 8 Jul 2009 Frans Pop <[email protected]> wrote:
> > > > From dmesg:
> > > > Platform driver 'serial8250' needs updating - please use dev_pm_ops
> > > >
> > > > Is this how it should be done?
> > >
> > > Alan or Rafael can probably tell.
> >
> > No idea. I've not looked at this so your guess is as good as mine (but
> > looks quite believable. I don't plan to do anything until someone who
> > needs platform 8250 support sends a tested version of the change
> > however.
>
> I have done a few of these myself now and followed all other similar
> updates for other drivers. From that experience I can say that this update
> is both trivial and correct. Hope you will reconsider your reluctance on
> that basis.
>
> Below Erik's patch with the commit log cleaned up, one whitespace problem
> fixed and my Reviewed-by added.
>
> Cheers,
> FJP
>
> ---
> From: Erik Ekman <[email protected]>
> Subject: serial8250: update to dev_pm_ops
>
> From dmesg:
> Platform driver 'serial8250' needs updating - please use dev_pm_ops
>
> Signed-off-by: Erik Ekman <[email protected]>
> Reviewed-by: Frans Pop <[email protected]>
>
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index fb867a9..496c85f 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -2982,42 +2982,46 @@ static int __devexit serial8250_remove(struct platform_device *dev)
> return 0;
> }
>
> -static int serial8250_suspend(struct platform_device *dev, pm_message_t state)
> +static int serial8250_suspend(struct device *dev)
> {
> int i;
>
> for (i = 0; i < UART_NR; i++) {
> struct uart_8250_port *up = &serial8250_ports[i];
>
> - if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
> + if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
> uart_suspend_port(&serial8250_reg, &up->port);
> }
>
> return 0;
> }
>
> -static int serial8250_resume(struct platform_device *dev)
> +static int serial8250_resume(struct device *dev)
> {
> int i;
>
> for (i = 0; i < UART_NR; i++) {
> struct uart_8250_port *up = &serial8250_ports[i];
>
> - if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
> + if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
> serial8250_resume_port(i);
> }
>
> return 0;
> }
>
> +static struct dev_pm_ops serial8250_pm_ops = {
> + .suspend = serial8250_suspend,
> + .resume = serial8250_resume,
Please don't forget about hibernation!
> +};
> +
> static struct platform_driver serial8250_isa_driver = {
> .probe = serial8250_probe,
> .remove = __devexit_p(serial8250_remove),
> - .suspend = serial8250_suspend,
> - .resume = serial8250_resume,
> .driver = {
> .name = "serial8250",
> .owner = THIS_MODULE,
> + .pm = &serial8250_pm_ops,
> },
> };
Best,
Rafael
On Saturday 25 July 2009, Frans Pop wrote:
> On Wednesday 08 July 2009, Alan Cox wrote:
> > On Wed, 8 Jul 2009 Frans Pop <[email protected]> wrote:
> > > > From dmesg:
> > > > Platform driver 'serial8250' needs updating - please use
> > > > dev_pm_ops
> > > >
> > > > Is this how it should be done?
> > >
> > > Alan or Rafael can probably tell.
> >
> > No idea. I've not looked at this so your guess is as good as mine
> > (but looks quite believable. I don't plan to do anything until
> > someone who needs platform 8250 support sends a tested version of the
> > change however.
>
> I have done a few of these myself now and followed all other similar
> updates for other drivers. From that experience I can say that this
> update is both trivial and correct. Hope you will reconsider your
> reluctance on that basis.
Please cancel that. It turns out that the old system also supported
suspend to disk, which most driver conversions so far did not allow for.
Kudo's to Dmitry Torokhov for bringing this up.
Sorry.
serial8250: update to dev_pm_ops
>From dmesg:
Platform driver 'serial8250' needs updating - please use dev_pm_ops
Signed-off-by: Erik Ekman <[email protected]>
--
Updated to handle hibernation as understood based on info from Rafael J. Wysocki.
Please let me know if any special handling is needed.
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index fb867a9..e93222c 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2982,42 +2982,50 @@ static int __devexit serial8250_remove(struct platform_device *dev)
return 0;
}
-static int serial8250_suspend(struct platform_device *dev, pm_message_t state)
+static int serial8250_suspend(struct device *dev)
{
int i;
for (i = 0; i < UART_NR; i++) {
struct uart_8250_port *up = &serial8250_ports[i];
- if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
+ if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
uart_suspend_port(&serial8250_reg, &up->port);
}
return 0;
}
-static int serial8250_resume(struct platform_device *dev)
+static int serial8250_resume(struct device *dev)
{
int i;
for (i = 0; i < UART_NR; i++) {
struct uart_8250_port *up = &serial8250_ports[i];
- if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
+ if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
serial8250_resume_port(i);
}
return 0;
}
+static struct dev_pm_ops serial8250_pm_ops = {
+ .resume = serial8250_resume,
+ .suspend = serial8250_suspend,
+ .freeze = serial8250_resume,
+ .thaw = serial8250_suspend,
+ .restore = serial8250_resume,
+ .poweroff = serial8250_suspend,
+};
+
static struct platform_driver serial8250_isa_driver = {
.probe = serial8250_probe,
.remove = __devexit_p(serial8250_remove),
- .suspend = serial8250_suspend,
- .resume = serial8250_resume,
.driver = {
.name = "serial8250",
.owner = THIS_MODULE,
+ .pm = &serial8250_pm_ops,
},
};
On Jul 25, 2009, at 1:59 PM, Erik Ekman <[email protected]> wrote:
> serial8250: update to dev_pm_ops
>
> From dmesg:
> Platform driver 'serial8250' needs updating - please use dev_pm_ops
>
> Signed-off-by: Erik Ekman <[email protected]>
> --
> Updated to handle hibernation as understood based on info from
> Rafael J. Wysocki.
> Please let me know if any special handling is needed.
>
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index fb867a9..e93222c 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -2982,42 +2982,50 @@ static int __devexit serial8250_remove
> (struct platform_device *dev)
> return 0;
> }
>
> -static int serial8250_suspend(struct platform_device *dev,
> pm_message_t state)
> +static int serial8250_suspend(struct device *dev)
> {
> int i;
>
> for (i = 0; i < UART_NR; i++) {
> struct uart_8250_port *up = &serial8250_ports[i];
>
> - if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev-
> >dev)
> + if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
> uart_suspend_port(&serial8250_reg, &up->port);
> }
>
> return 0;
> }
>
> -static int serial8250_resume(struct platform_device *dev)
> +static int serial8250_resume(struct device *dev)
> {
> int i;
>
> for (i = 0; i < UART_NR; i++) {
> struct uart_8250_port *up = &serial8250_ports[i];
>
> - if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev-
> >dev)
> + if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
> serial8250_resume_port(i);
> }
>
> return 0;
> }
>
> +static struct dev_pm_ops serial8250_pm_ops = {
> + .resume = serial8250_resume,
> + .suspend = serial8250_suspend,
> + .freeze = serial8250_resume,
> + .thaw = serial8250_suspend,
> + .restore = serial8250_resume,
> + .poweroff = serial8250_suspend,
> +};
Do we really both freeze and poweroff for serial port? I am getting
wary of these mechanical conversions...
--
Dmitry
On Sunday 26 July 2009, Dmitry Torokhov wrote:
> On Jul 25, 2009, at 1:59 PM, Erik Ekman <[email protected]> wrote:
>
> > serial8250: update to dev_pm_ops
> >
> > From dmesg:
> > Platform driver 'serial8250' needs updating - please use dev_pm_ops
> >
> > Signed-off-by: Erik Ekman <[email protected]>
> > --
> > Updated to handle hibernation as understood based on info from
> > Rafael J. Wysocki.
> > Please let me know if any special handling is needed.
> >
> > diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> > index fb867a9..e93222c 100644
> > --- a/drivers/serial/8250.c
> > +++ b/drivers/serial/8250.c
> > @@ -2982,42 +2982,50 @@ static int __devexit serial8250_remove
> > (struct platform_device *dev)
> > return 0;
> > }
> >
> > -static int serial8250_suspend(struct platform_device *dev,
> > pm_message_t state)
> > +static int serial8250_suspend(struct device *dev)
> > {
> > int i;
> >
> > for (i = 0; i < UART_NR; i++) {
> > struct uart_8250_port *up = &serial8250_ports[i];
> >
> > - if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev-
> > >dev)
> > + if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
> > uart_suspend_port(&serial8250_reg, &up->port);
> > }
> >
> > return 0;
> > }
> >
> > -static int serial8250_resume(struct platform_device *dev)
> > +static int serial8250_resume(struct device *dev)
> > {
> > int i;
> >
> > for (i = 0; i < UART_NR; i++) {
> > struct uart_8250_port *up = &serial8250_ports[i];
> >
> > - if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev-
> > >dev)
> > + if (up->port.type != PORT_UNKNOWN && up->port.dev == dev)
> > serial8250_resume_port(i);
> > }
> >
> > return 0;
> > }
> >
> > +static struct dev_pm_ops serial8250_pm_ops = {
> > + .resume = serial8250_resume,
> > + .suspend = serial8250_suspend,
> > + .freeze = serial8250_resume,
> > + .thaw = serial8250_suspend,
> > + .restore = serial8250_resume,
> > + .poweroff = serial8250_suspend,
> > +};
>
>
> Do we really both freeze and poweroff for serial port? I am getting
> wary of these mechanical conversions...
And this one is incorrect, because I made a mistake in one of my previous
messages. Obviously _suspend() should not be used for .thaw(), but for
.freeze(), and analogously with _resume().
Also, .poweroff() in probably unnecessary and .freeze() is only necessary if
there's anything to be saved before hibernation and restored during the
subsequent resume.
In fact, I wouldn't advise anyone to do these conversions mechanically.
Best,
Rafael
On Sunday 26 July 2009, Rafael J. Wysocki wrote:
> In fact, I wouldn't advise anyone to do these conversions mechanically.
With hindsight I'd say you are correct.
In that case would it maybe have been better to have a build time warning
about the need to convert drivers rather than a run time one? Run time
warnings have a much higher annoyance factor and will thus "invite" more
people into having a shot at getting rid of them.
But I guess partly it's also just the nature of this change: it looks
trivial due to the corresponding terms.
On Sunday 26 July 2009, Frans Pop wrote:
> On Sunday 26 July 2009, Rafael J. Wysocki wrote:
> > In fact, I wouldn't advise anyone to do these conversions mechanically.
>
> With hindsight I'd say you are correct.
>
> In that case would it maybe have been better to have a build time warning
> about the need to convert drivers rather than a run time one? Run time
> warnings have a much higher annoyance factor and will thus "invite" more
> people into having a shot at getting rid of them.
>
> But I guess partly it's also just the nature of this change: it looks
> trivial due to the corresponding terms.
Well, in fact the patch that added the warning wasn't mine. :-)
Anyway, the purpose was to get the attention of the people who maintain
those drivers mostly, AFAICS.
Best,
Rafael
On Mon, Jul 27, 2009 at 5:52 AM, Frans Pop<[email protected]> wrote:
> On Sunday 26 July 2009, Rafael J. Wysocki wrote:
>> In fact, I wouldn't advise anyone to do these conversions mechanically.
>
> With hindsight I'd say you are correct.
>
> In that case would it maybe have been better to have a build time warning
> about the need to convert drivers rather than a run time one? Run time
> warnings have a much higher annoyance factor and will thus "invite" more
> people into having a shot at getting rid of them.
I'm the one who added the runtime warning and I agree that a build
time warning would have been more suitable. Exacly how to implement
that in this specific case is another question. I'm open to
suggestions. =)
The purpose was simply to move over to the new interface sooner than later.
> But I guess partly it's also just the nature of this change: it looks
> trivial due to the corresponding terms.
It should be pretty trivial. The tricky part is what gets used when in
the cases of CONFIG_HIBERNATION and CONFIG_SUSPEND. The dev_pm_ops
comment in include/linux/pm.h should guide people in the right
direction.
Thanks for your help!
/ magnus