[PATCH] acerhdf: convert to dev_pm_ops

[PATCH] acerhdf: convert to dev_pm_ops

[Borislav Petkov]

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/platform/x86/acerhdf.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index aa298d6..561b471 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
 };
 
 /* suspend / resume functionality */
-static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
+static int acerhdf_suspend(struct device *dev)
 {
 	if (kernelmode)
 		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
@@ -446,7 +446,7 @@ static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
 	return 0;
 }
 
-static int acerhdf_resume(struct platform_device *device)
+static int acerhdf_resume(struct device *dev)
 {
 	if (verbose)
 		pr_notice("resuming\n");
@@ -464,15 +464,19 @@ static int acerhdf_remove(struct platform_device *device)
 	return 0;
 }
 
+static struct dev_pm_ops acerhdf_pm_ops = {
+	.suspend = acerhdf_suspend,
+	.resume	 = acerhdf_resume,
+};
+
 static struct platform_driver acerhdf_driver = {
 	.driver = {
-		.name = "acerhdf",
+		.name  = "acerhdf",
 		.owner = THIS_MODULE,
+		.pm    = &acerhdf_pm_ops,
 	},
 	.probe = acerhdf_probe,
 	.remove = acerhdf_remove,
-	.suspend = acerhdf_suspend,
-	.resume = acerhdf_resume,
 };
 
 
-- 
1.6.3.3

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Peter Feuerer]

Works fine here, thanks!

Acked-by: Peter Feuerer <peter@piie.net>

Borislav Petkov writes:

> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> ---
>  drivers/platform/x86/acerhdf.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index aa298d6..561b471 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
>  };
>  
>  /* suspend / resume functionality */
> -static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
> +static int acerhdf_suspend(struct device *dev)
>  {
>  	if (kernelmode)
>  		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
> @@ -446,7 +446,7 @@ static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
>  	return 0;
>  }
>  
> -static int acerhdf_resume(struct platform_device *device)
> +static int acerhdf_resume(struct device *dev)
>  {
>  	if (verbose)
>  		pr_notice("resuming\n");
> @@ -464,15 +464,19 @@ static int acerhdf_remove(struct platform_device *device)
>  	return 0;
>  }
>  
> +static struct dev_pm_ops acerhdf_pm_ops = {
> +	.suspend = acerhdf_suspend,
> +	.resume	 = acerhdf_resume,
> +};
> +
>  static struct platform_driver acerhdf_driver = {
>  	.driver = {
> -		.name = "acerhdf",
> +		.name  = "acerhdf",
>  		.owner = THIS_MODULE,
> +		.pm    = &acerhdf_pm_ops,
>  	},
>  	.probe = acerhdf_probe,
>  	.remove = acerhdf_remove,
> -	.suspend = acerhdf_suspend,
> -	.resume = acerhdf_resume,
>  };
>  
>  
> -- 
> 1.6.3.3
> 

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Dmitry Torokhov]

On Wed, Jul 15, 2009 at 08:36:43PM +0200, Peter Feuerer wrote:
> Works fine here, thanks!
>
> Acked-by: Peter Feuerer <peter@piie.net>

Umm, did you test suspend-to-disk? As far as I understand the new
suspend() and resume() methods are onluy used for S2R and for S2D you need
to use freeze() and thaw().

>
> Borislav Petkov writes:
>
>> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
>> ---
>>  drivers/platform/x86/acerhdf.c |   14 +++++++++-----
>>  1 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
>> index aa298d6..561b471 100644
>> --- a/drivers/platform/x86/acerhdf.c
>> +++ b/drivers/platform/x86/acerhdf.c
>> @@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
>>  };
>>   /* suspend / resume functionality */
>> -static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
>> +static int acerhdf_suspend(struct device *dev)
>>  {
>>  	if (kernelmode)
>>  		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
>> @@ -446,7 +446,7 @@ static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
>>  	return 0;
>>  }
>>  -static int acerhdf_resume(struct platform_device *device)
>> +static int acerhdf_resume(struct device *dev)
>>  {
>>  	if (verbose)
>>  		pr_notice("resuming\n");
>> @@ -464,15 +464,19 @@ static int acerhdf_remove(struct platform_device *device)
>>  	return 0;
>>  }
>>  +static struct dev_pm_ops acerhdf_pm_ops = {
>> +	.suspend = acerhdf_suspend,
>> +	.resume	 = acerhdf_resume,
>> +};
>> +
>>  static struct platform_driver acerhdf_driver = {
>>  	.driver = {
>> -		.name = "acerhdf",
>> +		.name  = "acerhdf",
>>  		.owner = THIS_MODULE,
>> +		.pm    = &acerhdf_pm_ops,
>>  	},
>>  	.probe = acerhdf_probe,
>>  	.remove = acerhdf_remove,
>> -	.suspend = acerhdf_suspend,
>> -	.resume = acerhdf_resume,
>>  };
>>   

-- 
Dmitry

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Peter Feuerer]

Hi Dmitry,

Dmitry Torokhov writes:

> On Wed, Jul 15, 2009 at 08:36:43PM +0200, Peter Feuerer wrote:
>> Works fine here, thanks!
>>
>> Acked-by: Peter Feuerer <peter@piie.net>
> 
> Umm, did you test suspend-to-disk? As far as I understand the new
> suspend() and resume() methods are onluy used for S2R and for S2D you need
> to use freeze() and thaw().

No, I didn't test S2D, will have a look at this issue. Thanks for pointing 
out!

--peter

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Borislav Petkov]

On Thu, Jul 23, 2009 at 10:52 AM, Peter Feuerer<peter@piie.net> wrote:
> Hi Dmitry,
>
> Dmitry Torokhov writes:
>
>> On Wed, Jul 15, 2009 at 08:36:43PM +0200, Peter Feuerer wrote:
>>>
>>> Works fine here, thanks!
>>>
>>> Acked-by: Peter Feuerer <peter@piie.net>
>>
>> Umm, did you test suspend-to-disk? As far as I understand the new
>> suspend() and resume() methods are onluy used for S2R and for S2D you need
>> to use freeze() and thaw().
>
> No, I didn't test S2D, will have a look at this issue. Thanks for pointing
> out!

Dmitry's right, suspend() and resume() are used only for S2R. Since
the only thing we do when suspending/hibernating is set fan speed to
auto, we might just as well replicate those functions to .freeze() and
.thaw().

/me testing...

-- 
Regards/Gruss,
Boris

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Borislav Petkov]

> Dmitry's right, suspend() and resume() are used only for S2R. Since
> the only thing we do when suspending/hibernating is set fan speed to
> auto, we might just as well replicate those functions to .freeze() and
> .thaw().

here's a fix:

--
From: Borislav Petkov <petkovbb@gmail.com>
Date: Wed, 15 Jul 2009 17:33:32 +0200
Subject: [PATCH] acerhdf: convert to dev_pm_ops

v 1.1:
Add .freeze/.thaw func ptrs to support
suspend-to-disk, as suggested by Dmitry Torokhov.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/platform/x86/acerhdf.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index aa298d6..8cd7a7b 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
 };

 /* suspend / resume functionality */
-static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
+static int acerhdf_suspend(struct device *dev)
 {
 	if (kernelmode)
 		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
@@ -446,7 +446,7 @@ static int acerhdf_suspend(struct platform_device
*dev, pm_message_t state)
 	return 0;
 }

-static int acerhdf_resume(struct platform_device *device)
+static int acerhdf_resume(struct device *dev)
 {
 	if (verbose)
 		pr_notice("resuming\n");
@@ -464,15 +464,21 @@ static int acerhdf_remove(struct platform_device *device)
 	return 0;
 }

+static struct dev_pm_ops acerhdf_pm_ops = {
+	.suspend = acerhdf_suspend,
+	.resume  = acerhdf_resume,
+	.freeze  = acerhdf_suspend,
+	.thaw	 = acerhdf_resume,
+};
+
 static struct platform_driver acerhdf_driver = {
 	.driver = {
-		.name = "acerhdf",
+		.name  = "acerhdf",
 		.owner = THIS_MODULE,
+		.pm    = &acerhdf_pm_ops,
 	},
 	.probe = acerhdf_probe,
 	.remove = acerhdf_remove,
-	.suspend = acerhdf_suspend,
-	.resume = acerhdf_resume,
 };


-- 
1.6.3.3

-- 
Regards/Gruss,
Boris

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Peter Feuerer]

Hi Boris,

I can't apply this patch, tried different methods to get the patch to ensure 
my mailclient doesn't screw the patch up.

The last one I tried was from:
http://lkml.org/lkml/2009/7/23/106

When patching I'm getting this error:
patch: **** malformed patch at line 15: *dev, pm_message_t state)

Could you please fix and resend the patch? Unfortunatelly can't I test s2d 
as I do have the 8gb ssd version of the acer aspire. But I'll try to use 
a sd-card for s2d, maybe it'll work this way.

kind regards,
--peter


Borislav Petkov writes:

>> Dmitry's right, suspend() and resume() are used only for S2R. Since
>> the only thing we do when suspending/hibernating is set fan speed to
>> auto, we might just as well replicate those functions to .freeze() and
>> .thaw().
> 
> here's a fix:
> 
> --
> From: Borislav Petkov <petkovbb@gmail.com>
> Date: Wed, 15 Jul 2009 17:33:32 +0200
> Subject: [PATCH] acerhdf: convert to dev_pm_ops
> 
> v 1.1:
> Add .freeze/.thaw func ptrs to support
> suspend-to-disk, as suggested by Dmitry Torokhov.
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> ---
>  drivers/platform/x86/acerhdf.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index aa298d6..8cd7a7b 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
>  };
> 
>  /* suspend / resume functionality */
> -static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
> +static int acerhdf_suspend(struct device *dev)
>  {
>  	if (kernelmode)
>  		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
> @@ -446,7 +446,7 @@ static int acerhdf_suspend(struct platform_device
> *dev, pm_message_t state)
>  	return 0;
>  }
> 
> -static int acerhdf_resume(struct platform_device *device)
> +static int acerhdf_resume(struct device *dev)
>  {
>  	if (verbose)
>  		pr_notice("resuming\n");
> @@ -464,15 +464,21 @@ static int acerhdf_remove(struct platform_device *device)
>  	return 0;
>  }
> 
> +static struct dev_pm_ops acerhdf_pm_ops = {
> +	.suspend = acerhdf_suspend,
> +	.resume  = acerhdf_resume,
> +	.freeze  = acerhdf_suspend,
> +	.thaw	 = acerhdf_resume,
> +};
> +
>  static struct platform_driver acerhdf_driver = {
>  	.driver = {
> -		.name = "acerhdf",
> +		.name  = "acerhdf",
>  		.owner = THIS_MODULE,
> +		.pm    = &acerhdf_pm_ops,
>  	},
>  	.probe = acerhdf_probe,
>  	.remove = acerhdf_remove,
> -	.suspend = acerhdf_suspend,
> -	.resume = acerhdf_resume,
>  };
> 
> 
> -- 
> 1.6.3.3
> 
> -- 
> Regards/Gruss,
> Boris

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Dmitry Torokhov]

On Thu, Jul 23, 2009 at 02:53:17PM +0200, Borislav Petkov wrote:
> > Dmitry's right, suspend() and resume() are used only for S2R. Since
> > the only thing we do when suspending/hibernating is set fan speed to
> > auto, we might just as well replicate those functions to .freeze() and
> > .thaw().
> 
> here's a fix:
> 
> --
> From: Borislav Petkov <petkovbb@gmail.com>
> Date: Wed, 15 Jul 2009 17:33:32 +0200
> Subject: [PATCH] acerhdf: convert to dev_pm_ops
> 
> v 1.1:
> Add .freeze/.thaw func ptrs to support
> suspend-to-disk, as suggested by Dmitry Torokhov.
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> ---
>  drivers/platform/x86/acerhdf.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index aa298d6..8cd7a7b 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
>  };
> 
>  /* suspend / resume functionality */
> -static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
> +static int acerhdf_suspend(struct device *dev)
>  {
>  	if (kernelmode)
>  		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
> @@ -446,7 +446,7 @@ static int acerhdf_suspend(struct platform_device
> *dev, pm_message_t state)
>  	return 0;
>  }
> 
> -static int acerhdf_resume(struct platform_device *device)
> +static int acerhdf_resume(struct device *dev)
>  {
>  	if (verbose)
>  		pr_notice("resuming\n");
> @@ -464,15 +464,21 @@ static int acerhdf_remove(struct platform_device *device)
>  	return 0;
>  }
> 
> +static struct dev_pm_ops acerhdf_pm_ops = {
> +	.suspend = acerhdf_suspend,
> +	.resume  = acerhdf_resume,
> +	.freeze  = acerhdf_suspend,
> +	.thaw	 = acerhdf_resume,
> +};
> +

Hmm, looking at the driver I think the only function that actually is
needed is poweroff() that would turn the fan in automatic mode before
shutting down. The driver does not perform any actions when resuming so
why bother?

BTW, I wasn't entirely correct, for S2D if you need freeze() and thaw()
you also need restore() because that's the one that is called after
restoring hibernation image.

-- 
Dmitry

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Borislav Petkov]

On Mon, Jul 27, 2009 at 08:55:51AM -0700, Dmitry Torokhov wrote:
> On Thu, Jul 23, 2009 at 02:53:17PM +0200, Borislav Petkov wrote:
> > > Dmitry's right, suspend() and resume() are used only for S2R. Since
> > > the only thing we do when suspending/hibernating is set fan speed to
> > > auto, we might just as well replicate those functions to .freeze() and
> > > .thaw().
> > 
> > here's a fix:
> > 
> > --
> > From: Borislav Petkov <petkovbb@gmail.com>
> > Date: Wed, 15 Jul 2009 17:33:32 +0200
> > Subject: [PATCH] acerhdf: convert to dev_pm_ops
> > 
> > v 1.1:
> > Add .freeze/.thaw func ptrs to support
> > suspend-to-disk, as suggested by Dmitry Torokhov.
> > 
> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> > ---
> >  drivers/platform/x86/acerhdf.c |   16 +++++++++++-----
> >  1 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> > index aa298d6..8cd7a7b 100644
> > --- a/drivers/platform/x86/acerhdf.c
> > +++ b/drivers/platform/x86/acerhdf.c
> > @@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
> >  };
> > 
> >  /* suspend / resume functionality */
> > -static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
> > +static int acerhdf_suspend(struct device *dev)
> >  {
> >  	if (kernelmode)
> >  		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
> > @@ -446,7 +446,7 @@ static int acerhdf_suspend(struct platform_device
> > *dev, pm_message_t state)
> >  	return 0;
> >  }
> > 
> > -static int acerhdf_resume(struct platform_device *device)
> > +static int acerhdf_resume(struct device *dev)
> >  {
> >  	if (verbose)
> >  		pr_notice("resuming\n");
> > @@ -464,15 +464,21 @@ static int acerhdf_remove(struct platform_device *device)
> >  	return 0;
> >  }
> > 
> > +static struct dev_pm_ops acerhdf_pm_ops = {
> > +	.suspend = acerhdf_suspend,
> > +	.resume  = acerhdf_resume,
> > +	.freeze  = acerhdf_suspend,
> > +	.thaw	 = acerhdf_resume,
> > +};
> > +
> 
> Hmm, looking at the driver I think the only function that actually is
> needed is poweroff() that would turn the fan in automatic mode before
> shutting down. The driver does not perform any actions when resuming so
> why bother?

Agreed.

Also, the fan comes out of warm and cold reboot in mode AUTO and
when the driver is enabled, the fan is turned off on the next run of
thermal_zone_device_update() when the read out temperature is within
limits.

Correct me if I'm wrong, but the only reason I see for setting the
fan to mode AUTO before suspending/hibernating/etc is if it is taking
a really long time to hibernate and write RAM image to disk and the
machine is getting hot during that process. Otherwise, we might just
as well do _nothing_ when suspending and remove all suspend/resume
functionality altogether, no?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Peter Feuerer]

Borislav Petkov writes:

> On Mon, Jul 27, 2009 at 08:55:51AM -0700, Dmitry Torokhov wrote:
>> On Thu, Jul 23, 2009 at 02:53:17PM +0200, Borislav Petkov wrote:
>> > > Dmitry's right, suspend() and resume() are used only for S2R. Since
>> > > the only thing we do when suspending/hibernating is set fan speed to
>> > > auto, we might just as well replicate those functions to .freeze() and
>> > > .thaw().
>> > 
>> > here's a fix:
>> > 
>> > --
>> > From: Borislav Petkov <petkovbb@gmail.com>
>> > Date: Wed, 15 Jul 2009 17:33:32 +0200
>> > Subject: [PATCH] acerhdf: convert to dev_pm_ops
>> > 
>> > v 1.1:
>> > Add .freeze/.thaw func ptrs to support
>> > suspend-to-disk, as suggested by Dmitry Torokhov.
>> > 
>> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
>> > ---
>> >  drivers/platform/x86/acerhdf.c |   16 +++++++++++-----
>> >  1 files changed, 11 insertions(+), 5 deletions(-)
>> > 
>> > diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
>> > index aa298d6..8cd7a7b 100644
>> > --- a/drivers/platform/x86/acerhdf.c
>> > +++ b/drivers/platform/x86/acerhdf.c
>> > @@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
>> >  };
>> > 
>> >  /* suspend / resume functionality */
>> > -static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
>> > +static int acerhdf_suspend(struct device *dev)
>> >  {
>> >  	if (kernelmode)
>> >  		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
>> > @@ -446,7 +446,7 @@ static int acerhdf_suspend(struct platform_device
>> > *dev, pm_message_t state)
>> >  	return 0;
>> >  }
>> > 
>> > -static int acerhdf_resume(struct platform_device *device)
>> > +static int acerhdf_resume(struct device *dev)
>> >  {
>> >  	if (verbose)
>> >  		pr_notice("resuming\n");
>> > @@ -464,15 +464,21 @@ static int acerhdf_remove(struct platform_device *device)
>> >  	return 0;
>> >  }
>> > 
>> > +static struct dev_pm_ops acerhdf_pm_ops = {
>> > +	.suspend = acerhdf_suspend,
>> > +	.resume  = acerhdf_resume,
>> > +	.freeze  = acerhdf_suspend,
>> > +	.thaw	 = acerhdf_resume,
>> > +};
>> > +
>> 
>> Hmm, looking at the driver I think the only function that actually is
>> needed is poweroff() that would turn the fan in automatic mode before
>> shutting down. The driver does not perform any actions when resuming so
>> why bother?
> 
> Agreed.
> 
> Also, the fan comes out of warm and cold reboot in mode AUTO and
> when the driver is enabled, the fan is turned off on the next run of
> thermal_zone_device_update() when the read out temperature is within
> limits.
> 
> Correct me if I'm wrong, but the only reason I see for setting the
> fan to mode AUTO before suspending/hibernating/etc is if it is taking
> a really long time to hibernate and write RAM image to disk and the
> machine is getting hot during that process. Otherwise, we might just
> as well do _nothing_ when suspending and remove all suspend/resume
> functionality altogether, no?

This is right, currently the only reason for calling the suspend / 
hibernate functions is to set the fan to auto to ensure it doesn't get too 
hot while the kernel prepares the machine to suspend / hibernate.

I don't see a way to remove those functionality completely and I would like 
to keep the resume function too. It's a nice to see what's happening with 
verbose=1.

kind regards,
--peter

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Borislav Petkov]

Hi,

On Tue, Jul 28, 2009 at 9:25 AM, Peter Feuerer<peter@piie.net> wrote:

[..]

>>> Hmm, looking at the driver I think the only function that actually
>>> is needed is poweroff() that would turn the fan in automatic mode
>>> before shutting down. The driver does not perform any actions when
>>> resuming so why bother?
>>
>> Agreed.
>>
>> Also, the fan comes out of warm and cold reboot in mode AUTO and
>> when the driver is enabled, the fan is turned off on the next run of
>> thermal_zone_device_update() when the read out temperature is within
>> limits.
>>
>> Correct me if I'm wrong, but the only reason I see for setting the
>> fan to mode AUTO before suspending/hibernating/etc is if it is taking
>> a really long time to hibernate and write RAM image to disk and the
>> machine is getting hot during that process. Otherwise, we might just
>> as well do _nothing_ when suspending and remove all suspend/resume
>> functionality altogether, no?
>
> This is right, currently the only reason for calling the suspend /
> hibernate functions is to set the fan to auto to ensure it doesn't get
> too hot while the kernel prepares the machine to suspend / hibernate.
>
> I would like to keep the resume function too. It's a nice to see
> what's happening with verbose=1.

That's not a reason for keeping code in the kernel and raising bloat
levels unnecessarily. If the driver doesn't need to do anything on
resume, then no function is needed.

-- 
Regards/Gruss,
Boris

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Peter Feuerer]

Hi,

Borislav Petkov writes:

> Hi,
> 
> On Tue, Jul 28, 2009 at 9:25 AM, Peter Feuerer<peter@piie.net> wrote:
> 
> [..]
> 
>>>> Hmm, looking at the driver I think the only function that actually
>>>> is needed is poweroff() that would turn the fan in automatic mode
>>>> before shutting down. The driver does not perform any actions when
>>>> resuming so why bother?
>>>
>>> Agreed.
>>>
>>> Also, the fan comes out of warm and cold reboot in mode AUTO and
>>> when the driver is enabled, the fan is turned off on the next run of
>>> thermal_zone_device_update() when the read out temperature is within
>>> limits.
>>>
>>> Correct me if I'm wrong, but the only reason I see for setting the
>>> fan to mode AUTO before suspending/hibernating/etc is if it is taking
>>> a really long time to hibernate and write RAM image to disk and the
>>> machine is getting hot during that process. Otherwise, we might just
>>> as well do _nothing_ when suspending and remove all suspend/resume
>>> functionality altogether, no?
>>
>> This is right, currently the only reason for calling the suspend /
>> hibernate functions is to set the fan to auto to ensure it doesn't get
>> too hot while the kernel prepares the machine to suspend / hibernate.
>>
>> I would like to keep the resume function too. It's a nice to see
>> what's happening with verbose=1.
> 
> That's not a reason for keeping code in the kernel and raising bloat
> levels unnecessarily. If the driver doesn't need to do anything on
> resume, then no function is needed.

I don't think the verbose message is useless. If an user has a problem with 
suspend / hibernate I can just ask him to load the module with verbose=1 and 
dmesg tells whether the module is waking up or not. Searching for such an 
error is much easier this way as the user doesn't have to recompile the 
kernelmodule. In my opinion keeping these 4 lines of code is worth it.

kind regards,
--peter

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Borislav Petkov]

On Tue, Jul 28, 2009 at 3:25 PM, Peter Feuerer<peter@piie.net> wrote:

[..]

>> That's not a reason for keeping code in the kernel and raising bloat
>> levels unnecessarily. If the driver doesn't need to do anything on
>> resume, then no function is needed.
>
> I don't think the verbose message is useless. If an user has a problem
> with suspend / hibernate I can just ask him to load the module with
> verbose=1 and dmesg tells whether the module is waking up or not.

Since the driver doesn't do anything upon resume, it _is_ _going_ to
resume just fine. If not, then the problem is located somewhere else,
i.e. you can safely assume that you are resuming ok. Also, it's not like
this is the only verbose printk you have in the driver to not be able to
follow what's going on...

-- 
Regards/Gruss,
Boris

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Peter Feuerer]

Hi Boris,

Borislav Petkov writes:

> On Tue, Jul 28, 2009 at 3:25 PM, Peter Feuerer<peter@piie.net> wrote:
> 
> [..]
> 
>>> That's not a reason for keeping code in the kernel and raising bloat
>>> levels unnecessarily. If the driver doesn't need to do anything on
>>> resume, then no function is needed.
>>
>> I don't think the verbose message is useless. If an user has a problem
>> with suspend / hibernate I can just ask him to load the module with
>> verbose=1 and dmesg tells whether the module is waking up or not.
> 
> Since the driver doesn't do anything upon resume, it _is_ _going_ to
> resume just fine. If not, then the problem is located somewhere else,
> i.e. you can safely assume that you are resuming ok. Also, it's not like
> this is the only verbose printk you have in the driver to not be able to
> follow what's going on...

Ok, I think you are right. So we will remove the resume functionality and 
keep just the suspend + freeze calls?

Btw, should I resend the 0.5.13 → 0.5.16 patch splitted up, so that Len can 
apply them?

kind regards,
--peter

Re: [PATCH] acerhdf: convert to dev_pm_ops

[Borislav Petkov]

Hi,

On Wed, Jul 29, 2009 at 9:25 AM, Peter Feuerer<peter@piie.net> wrote:
> Ok, I think you are right. So we will remove the resume functionality and
> keep just the suspend + freeze calls?

Yep, see below.

> Btw, should I resend the 0.5.13 → 0.5.16 patch splitted up, so that Len can
> apply them?

No need for that - just add my S-O-B to it and make sure all changes are
properly documented in the commit message.

Thanks.

--
From: Borislav Petkov <petkovbb@gmail.com>
Date: Wed, 15 Jul 2009 17:33:32 +0200
Subject: [PATCH] acerhdf: convert to dev_pm_ops

v 1.1:
Add .freeze func ptr to support suspend-to-disk,
as suggested by Dmitry Torokhov.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/platform/x86/acerhdf.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
index aa298d6..97e473c 100644
--- a/drivers/platform/x86/acerhdf.c
+++ b/drivers/platform/x86/acerhdf.c
@@ -435,7 +435,7 @@ struct thermal_cooling_device_ops acerhdf_cooling_ops = {
 };

 /* suspend / resume functionality */
-static int acerhdf_suspend(struct platform_device *dev, pm_message_t state)
+static int acerhdf_suspend(struct device *dev)
 {
 	if (kernelmode)
 		acerhdf_change_fanstate(ACERHDF_FAN_AUTO);
@@ -446,14 +446,6 @@ static int acerhdf_suspend(struct platform_device
*dev, pm_message_t state)
 	return 0;
 }

-static int acerhdf_resume(struct platform_device *device)
-{
-	if (verbose)
-		pr_notice("resuming\n");
-
-	return 0;
-}
-
 static int __devinit acerhdf_probe(struct platform_device *device)
 {
 	return 0;
@@ -464,15 +456,19 @@ static int acerhdf_remove(struct platform_device *device)
 	return 0;
 }

+static struct dev_pm_ops acerhdf_pm_ops = {
+	.suspend = acerhdf_suspend,
+	.freeze  = acerhdf_suspend,
+};
+
 static struct platform_driver acerhdf_driver = {
 	.driver = {
-		.name = "acerhdf",
+		.name  = "acerhdf",
 		.owner = THIS_MODULE,
+		.pm    = &acerhdf_pm_ops,
 	},
 	.probe = acerhdf_probe,
 	.remove = acerhdf_remove,
-	.suspend = acerhdf_suspend,
-	.resume = acerhdf_resume,
 };


-- 
1.6.3.3


-- 
Regards/Gruss,
Boris