[PATCH 1/2] pwm: lpss: Move struct pwm_lpss_chip definition to the header file

[PATCH 1/2] pwm: lpss: Move struct pwm_lpss_chip definition to the header file

[Hans de Goede]

Move struct pwm_lpss_chip definition from pwm-lpss.c to pwm-lpss.h,
so that the pci/platform drivers can access the info member
(struct pwm_lpss_boardinfo *).

This is a preparation patch for adding platform specific quirks, which
the drivers need access to, to pwm_lpss_boardinfo.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss.c | 9 ---------
 drivers/pwm/pwm-lpss.h | 9 ++++++++-
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 4721a264bac2..e602835fd6de 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -32,15 +32,6 @@
 /* Size of each PWM register space if multiple */
 #define PWM_SIZE			0x400
 
-#define MAX_PWMS			4
-
-struct pwm_lpss_chip {
-	struct pwm_chip chip;
-	void __iomem *regs;
-	const struct pwm_lpss_boardinfo *info;
-	u32 saved_ctrl[MAX_PWMS];
-};
-
 static inline struct pwm_lpss_chip *to_lpwm(struct pwm_chip *chip)
 {
 	return container_of(chip, struct pwm_lpss_chip, chip);
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 7a4238ad1fcb..8f029ed263af 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -16,7 +16,14 @@
 #include <linux/device.h>
 #include <linux/pwm.h>
 
-struct pwm_lpss_chip;
+#define MAX_PWMS			4
+
+struct pwm_lpss_chip {
+	struct pwm_chip chip;
+	void __iomem *regs;
+	const struct pwm_lpss_boardinfo *info;
+	u32 saved_ctrl[MAX_PWMS];
+};
 
 struct pwm_lpss_boardinfo {
 	unsigned long clk_rate;
-- 
2.19.0.rc0

[PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Hans de Goede]

The _PS0 method for the integrated graphics on some Cherry Trail devices
(observed on a HP Pavilion X2 10-p0XX) turns on the PWM chip (puts it in
D0), causing an inconsistency between the state the pm-core thinks it is
in (left runtime suspended as it was before the suspend/resume) and the
state it actually is in.

Interestingly enough this is done on a device where the pwm controller is
not used for the backlight at all, since it uses an eDP panel. On devices
where the PWM is used this is not a problem since we will resume it
ourselves anyways.

This inconsistency causes us to never suspend the pwm controller again,
which causes the device to not be able to reach S0ix states when suspended.

This commit adds a resume-complete handler, which when we think the device
is still run-time suspended checks the actual power-state and if necessary
updates the rpm-core's internal state.

This fixes the Pavilion X2 10-p0XX not reaching S0ix states when suspended.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss-platform.c | 26 +++++++++++++++++++++++---
 drivers/pwm/pwm-lpss.h          |  2 ++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index 7304f36ee715..00b2b18c8f6d 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -30,6 +30,7 @@ static const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
 	.clk_rate = 19200000,
 	.npwm = 1,
 	.base_unit_bits = 16,
+	.check_power_on_resume = true,
 };
 
 /* Broxton */
@@ -74,9 +75,28 @@ static int pwm_lpss_remove_platform(struct platform_device *pdev)
 	return pwm_lpss_remove(lpwm);
 }
 
-static SIMPLE_DEV_PM_OPS(pwm_lpss_platform_pm_ops,
-			 pwm_lpss_suspend,
-			 pwm_lpss_resume);
+static void pwm_lpss_complete(struct device *dev)
+{
+	struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev);
+	unsigned long long psc;
+	acpi_status status;
+
+	/* The PWM may be turned on by AML code, update our state to match */
+	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
+		status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_PSC",
+					       NULL, &psc);
+		if (ACPI_SUCCESS(status) && psc == ACPI_STATE_D0) {
+			pm_runtime_disable(dev);
+			pm_runtime_set_active(dev);
+			pm_runtime_enable(dev);
+		}
+	}
+}
+
+static const struct dev_pm_ops pwm_lpss_platform_pm_ops = {
+	.complete = pwm_lpss_complete,
+	SET_SYSTEM_SLEEP_PM_OPS(pwm_lpss_suspend, pwm_lpss_resume)
+};
 
 static const struct acpi_device_id pwm_lpss_acpi_match[] = {
 	{ "80860F09", (unsigned long)&pwm_lpss_byt_info },
diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
index 8f029ed263af..1a2575d25bea 100644
--- a/drivers/pwm/pwm-lpss.h
+++ b/drivers/pwm/pwm-lpss.h
@@ -30,6 +30,8 @@ struct pwm_lpss_boardinfo {
 	unsigned int npwm;
 	unsigned long base_unit_bits;
 	bool bypass;
+	/* Some devices have AML code messing with the state underneath us */
+	bool check_power_on_resume;
 };
 
 struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
-- 
2.19.0.rc0

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Andy Shevchenko]

On Tue, Sep 11, 2018 at 07:30:50PM +0200, Hans de Goede wrote:
> The _PS0 method for the integrated graphics on some Cherry Trail devices
> (observed on a HP Pavilion X2 10-p0XX) turns on the PWM chip (puts it in
> D0), causing an inconsistency between the state the pm-core thinks it is
> in (left runtime suspended as it was before the suspend/resume) and the
> state it actually is in.
> 
> Interestingly enough this is done on a device where the pwm controller is
> not used for the backlight at all, since it uses an eDP panel. On devices
> where the PWM is used this is not a problem since we will resume it
> ourselves anyways.
> 
> This inconsistency causes us to never suspend the pwm controller again,
> which causes the device to not be able to reach S0ix states when suspended.
> 
> This commit adds a resume-complete handler, which when we think the device
> is still run-time suspended checks the actual power-state and if necessary
> updates the rpm-core's internal state.
> 
> This fixes the Pavilion X2 10-p0XX not reaching S0ix states when suspended.
> 

I also Cc Rafael.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/pwm-lpss-platform.c | 26 +++++++++++++++++++++++---
>  drivers/pwm/pwm-lpss.h          |  2 ++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
> index 7304f36ee715..00b2b18c8f6d 100644
> --- a/drivers/pwm/pwm-lpss-platform.c
> +++ b/drivers/pwm/pwm-lpss-platform.c
> @@ -30,6 +30,7 @@ static const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
>  	.clk_rate = 19200000,
>  	.npwm = 1,
>  	.base_unit_bits = 16,
> +	.check_power_on_resume = true,
>  };
>  
>  /* Broxton */
> @@ -74,9 +75,28 @@ static int pwm_lpss_remove_platform(struct platform_device *pdev)
>  	return pwm_lpss_remove(lpwm);
>  }
>  
> -static SIMPLE_DEV_PM_OPS(pwm_lpss_platform_pm_ops,
> -			 pwm_lpss_suspend,
> -			 pwm_lpss_resume);
> +static void pwm_lpss_complete(struct device *dev)
> +{
> +	struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev);
> +	unsigned long long psc;
> +	acpi_status status;
> +
> +	/* The PWM may be turned on by AML code, update our state to match */
> +	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {

> +		status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_PSC",
> +					       NULL, &psc);

AFAIU this is a standard power source method for ACPI, shouldn't ACPI core take
care of being in sync?

> +		if (ACPI_SUCCESS(status) && psc == ACPI_STATE_D0) {
> +			pm_runtime_disable(dev);
> +			pm_runtime_set_active(dev);
> +			pm_runtime_enable(dev);
> +		}
> +	}
> +}
> +
> +static const struct dev_pm_ops pwm_lpss_platform_pm_ops = {
> +	.complete = pwm_lpss_complete,
> +	SET_SYSTEM_SLEEP_PM_OPS(pwm_lpss_suspend, pwm_lpss_resume)
> +};
>  
>  static const struct acpi_device_id pwm_lpss_acpi_match[] = {
>  	{ "80860F09", (unsigned long)&pwm_lpss_byt_info },
> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
> index 8f029ed263af..1a2575d25bea 100644
> --- a/drivers/pwm/pwm-lpss.h
> +++ b/drivers/pwm/pwm-lpss.h
> @@ -30,6 +30,8 @@ struct pwm_lpss_boardinfo {
>  	unsigned int npwm;
>  	unsigned long base_unit_bits;
>  	bool bypass;
> +	/* Some devices have AML code messing with the state underneath us */
> +	bool check_power_on_resume;
>  };
>  
>  struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
> -- 
> 2.19.0.rc0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Hans de Goede]

Hi,

On 24-09-18 11:02, Andy Shevchenko wrote:
> On Tue, Sep 11, 2018 at 07:30:50PM +0200, Hans de Goede wrote:
>> The _PS0 method for the integrated graphics on some Cherry Trail devices
>> (observed on a HP Pavilion X2 10-p0XX) turns on the PWM chip (puts it in
>> D0), causing an inconsistency between the state the pm-core thinks it is
>> in (left runtime suspended as it was before the suspend/resume) and the
>> state it actually is in.
>>
>> Interestingly enough this is done on a device where the pwm controller is
>> not used for the backlight at all, since it uses an eDP panel. On devices
>> where the PWM is used this is not a problem since we will resume it
>> ourselves anyways.
>>
>> This inconsistency causes us to never suspend the pwm controller again,
>> which causes the device to not be able to reach S0ix states when suspended.
>>
>> This commit adds a resume-complete handler, which when we think the device
>> is still run-time suspended checks the actual power-state and if necessary
>> updates the rpm-core's internal state.
>>
>> This fixes the Pavilion X2 10-p0XX not reaching S0ix states when suspended.
>>
> 
> I also Cc Rafael.
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/pwm/pwm-lpss-platform.c | 26 +++++++++++++++++++++++---
>>   drivers/pwm/pwm-lpss.h          |  2 ++
>>   2 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
>> index 7304f36ee715..00b2b18c8f6d 100644
>> --- a/drivers/pwm/pwm-lpss-platform.c
>> +++ b/drivers/pwm/pwm-lpss-platform.c
>> @@ -30,6 +30,7 @@ static const struct pwm_lpss_boardinfo pwm_lpss_bsw_info = {
>>   	.clk_rate = 19200000,
>>   	.npwm = 1,
>>   	.base_unit_bits = 16,
>> +	.check_power_on_resume = true,
>>   };
>>   
>>   /* Broxton */
>> @@ -74,9 +75,28 @@ static int pwm_lpss_remove_platform(struct platform_device *pdev)
>>   	return pwm_lpss_remove(lpwm);
>>   }
>>   
>> -static SIMPLE_DEV_PM_OPS(pwm_lpss_platform_pm_ops,
>> -			 pwm_lpss_suspend,
>> -			 pwm_lpss_resume);
>> +static void pwm_lpss_complete(struct device *dev)
>> +{
>> +	struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev);
>> +	unsigned long long psc;
>> +	acpi_status status;
>> +
>> +	/* The PWM may be turned on by AML code, update our state to match */
>> +	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
> 
>> +		status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_PSC",
>> +					       NULL, &psc);
> 
> AFAIU this is a standard power source method for ACPI, shouldn't ACPI core take
> care of being in sync?

This is not about ACPI power-resources, this is about the power state (D0 or D3)
of the device itself. The ACPI core does not expect the state of devices to
magically change underneath it when using s2idle, since then everything is
under the kernel's control. But the _PS0 method of the GPU messing with the PWM
controller (hurray for firmware) messes things up.

Regards,

Hans


> 
>> +		if (ACPI_SUCCESS(status) && psc == ACPI_STATE_D0) {
>> +			pm_runtime_disable(dev);
>> +			pm_runtime_set_active(dev);
>> +			pm_runtime_enable(dev);
>> +		}
>> +	}
>> +}
>> +
>> +static const struct dev_pm_ops pwm_lpss_platform_pm_ops = {
>> +	.complete = pwm_lpss_complete,
>> +	SET_SYSTEM_SLEEP_PM_OPS(pwm_lpss_suspend, pwm_lpss_resume)
>> +};
>>   
>>   static const struct acpi_device_id pwm_lpss_acpi_match[] = {
>>   	{ "80860F09", (unsigned long)&pwm_lpss_byt_info },
>> diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h
>> index 8f029ed263af..1a2575d25bea 100644
>> --- a/drivers/pwm/pwm-lpss.h
>> +++ b/drivers/pwm/pwm-lpss.h
>> @@ -30,6 +30,8 @@ struct pwm_lpss_boardinfo {
>>   	unsigned int npwm;
>>   	unsigned long base_unit_bits;
>>   	bool bypass;
>> +	/* Some devices have AML code messing with the state underneath us */
>> +	bool check_power_on_resume;
>>   };
>>   
>>   struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r,
>> -- 
>> 2.19.0.rc0
>>
> 

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Andy Shevchenko]

On Mon, Sep 24, 2018 at 11:10:28AM +0200, Hans de Goede wrote:

> > > +	/* The PWM may be turned on by AML code, update our state to match */
> > > +	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
> > 
> > > +		status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_PSC",
> > > +					       NULL, &psc);
> > 
> > AFAIU this is a standard power source method for ACPI, shouldn't ACPI core take
> > care of being in sync?
> 
> This is not about ACPI power-resources, this is about the power state (D0 or D3)
> of the device itself. The ACPI core does not expect the state of devices to
> magically change underneath it when using s2idle, since then everything is
> under the kernel's control. But the _PS0 method of the GPU messing with the PWM
> controller (hurray for firmware) messes things up.

What I mean is shouldn't we care about this on a ACPI core level to be sure
that states are kept in sync on OS level?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Hans de Goede]

Hi,

On 24-09-18 11:18, Andy Shevchenko wrote:
> On Mon, Sep 24, 2018 at 11:10:28AM +0200, Hans de Goede wrote:
> 
>>>> +	/* The PWM may be turned on by AML code, update our state to match */
>>>> +	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
>>>
>>>> +		status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_PSC",
>>>> +					       NULL, &psc);
>>>
>>> AFAIU this is a standard power source method for ACPI, shouldn't ACPI core take
>>> care of being in sync?
>>
>> This is not about ACPI power-resources, this is about the power state (D0 or D3)
>> of the device itself. The ACPI core does not expect the state of devices to
>> magically change underneath it when using s2idle, since then everything is
>> under the kernel's control. But the _PS0 method of the GPU messing with the PWM
>> controller (hurray for firmware) messes things up.
> 
> What I mean is shouldn't we care about this on a ACPI core level to be sure
> that states are kept in sync on OS level?

The ACPI / pm core does care about this when doing a firmware supported suspend/resume
(so going to regular S3) because the firmware can then make all sort of changes
to the device state.

But with s2idle the kernel is fully in control and we never hand control over to the
firmware, so checking the device state then should not be necessary and is a somewhat
expensive operation (esp. to do on all devices), while one of the advantages of s2idle
is supposed to be shorter suspend/resume times.

IOW for s2idle it is undesirable for the core to check the power-state of all
devices after a resume, since it simply should not have changed, with the PWM
device being a nasty exception because of the _PS0 method for *another* device
mucking with it.

Anyways lets see what Rafael has to say about this.

Regards,

Hans

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Rafael J. Wysocki]

On Monday, September 24, 2018 11:40:14 AM CEST Hans de Goede wrote:
> Hi,
> 
> On 24-09-18 11:18, Andy Shevchenko wrote:
> > On Mon, Sep 24, 2018 at 11:10:28AM +0200, Hans de Goede wrote:
> > 
> >>>> +	/* The PWM may be turned on by AML code, update our state to match */
> >>>> +	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
> >>>
> >>>> +		status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_PSC",
> >>>> +					       NULL, &psc);
> >>>
> >>> AFAIU this is a standard power source method for ACPI, shouldn't ACPI core take
> >>> care of being in sync?
> >>
> >> This is not about ACPI power-resources, this is about the power state (D0 or D3)
> >> of the device itself. The ACPI core does not expect the state of devices to
> >> magically change underneath it when using s2idle, since then everything is
> >> under the kernel's control. But the _PS0 method of the GPU messing with the PWM
> >> controller (hurray for firmware) messes things up.
> > 
> > What I mean is shouldn't we care about this on a ACPI core level to be sure
> > that states are kept in sync on OS level?
> 
> The ACPI / pm core does care about this when doing a firmware supported suspend/resume
> (so going to regular S3) because the firmware can then make all sort of changes
> to the device state.
> 
> But with s2idle the kernel is fully in control and we never hand control over to the
> firmware, so checking the device state then should not be necessary and is a somewhat
> expensive operation (esp. to do on all devices), while one of the advantages of s2idle
> is supposed to be shorter suspend/resume times.
> 
> IOW for s2idle it is undesirable for the core to check the power-state of all
> devices after a resume, since it simply should not have changed, with the PWM
> device being a nasty exception because of the _PS0 method for *another* device
> mucking with it.
> 
> Anyways lets see what Rafael has to say about this.

It is OK to check the device power state in the driver in that case IMO,
although I would add a comment explaining why it is needed to the code.

Also, why don't you use acpi_device_get_power() instead of evaluating
_PSC directly?  It should make no difference if there are no power
resources, should it?

In addition, I would disable runtime PM before checking the power state,
then check it, set the correct runtime PM status and re-enable runtime PM.

Thanks,
Rafael

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Hans de Goede]

Hi,

On 03-10-18 11:22, Rafael J. Wysocki wrote:
> On Monday, September 24, 2018 11:40:14 AM CEST Hans de Goede wrote:
>> Hi,
>>
>> On 24-09-18 11:18, Andy Shevchenko wrote:
>>> On Mon, Sep 24, 2018 at 11:10:28AM +0200, Hans de Goede wrote:
>>>
>>>>>> +	/* The PWM may be turned on by AML code, update our state to match */
>>>>>> +	if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
>>>>>
>>>>>> +		status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_PSC",
>>>>>> +					       NULL, &psc);
>>>>>
>>>>> AFAIU this is a standard power source method for ACPI, shouldn't ACPI core take
>>>>> care of being in sync?
>>>>
>>>> This is not about ACPI power-resources, this is about the power state (D0 or D3)
>>>> of the device itself. The ACPI core does not expect the state of devices to
>>>> magically change underneath it when using s2idle, since then everything is
>>>> under the kernel's control. But the _PS0 method of the GPU messing with the PWM
>>>> controller (hurray for firmware) messes things up.
>>>
>>> What I mean is shouldn't we care about this on a ACPI core level to be sure
>>> that states are kept in sync on OS level?
>>
>> The ACPI / pm core does care about this when doing a firmware supported suspend/resume
>> (so going to regular S3) because the firmware can then make all sort of changes
>> to the device state.
>>
>> But with s2idle the kernel is fully in control and we never hand control over to the
>> firmware, so checking the device state then should not be necessary and is a somewhat
>> expensive operation (esp. to do on all devices), while one of the advantages of s2idle
>> is supposed to be shorter suspend/resume times.
>>
>> IOW for s2idle it is undesirable for the core to check the power-state of all
>> devices after a resume, since it simply should not have changed, with the PWM
>> device being a nasty exception because of the _PS0 method for *another* device
>> mucking with it.
>>
>> Anyways lets see what Rafael has to say about this.
> 
> It is OK to check the device power state in the driver in that case IMO,
> although I would add a comment explaining why it is needed to the code.

There already are comments:

drivers/pwm/pwm-lpss.h

         /* Some devices have AML code messing with the state underneath us */
         bool check_power_on_resume;

drivers/pwm/pwm-lpss-platform.c: pwm_lpss_complete():

         /* The PWM may be turned on by AML code, update our state to match */
         if (pm_runtime_suspended(dev) && lpwm->info->check_power_on_resume) {
                 status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_PSC",
                                                NULL, &psc);
                 if (ACPI_SUCCESS(status) && psc == ACPI_STATE_D0) {
                         pm_runtime_disable(dev);
                         pm_runtime_set_active(dev);
                         pm_runtime_enable(dev);
                 }
         }
	
Or do you want the comment to go into more detail ?

> Also, why don't you use acpi_device_get_power() instead of evaluating
> _PSC directly?  It should make no difference if there are no power
> resources, should it?

2 reasons:

1) I really want to check the D3 enabled bit from the devices configs regs,
as that is what block S0ix, this is exactly what the _PSC method does

2) acpi_device_get_power() is not exported to modules

> In addition, I would disable runtime PM before checking the power state,
> then check it, set the correct runtime PM status and re-enable runtime PM.

Ok I will send a v2 with this changed.

Regards,

Hans

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Andy Shevchenko]

On Sat, Oct 06, 2018 at 10:55:41AM +0200, Hans de Goede wrote:
> On 03-10-18 11:22, Rafael J. Wysocki wrote:
> > On Monday, September 24, 2018 11:40:14 AM CEST Hans de Goede wrote:

> > Also, why don't you use acpi_device_get_power() instead of evaluating
> > _PSC directly?  It should make no difference if there are no power
> > resources, should it?

> 2) acpi_device_get_power() is not exported to modules

Do we have any side effects if we just export it?
I would think exporting is better than open coding.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Hans de Goede]

Hi,

On 06-10-18 16:16, Andy Shevchenko wrote:
> On Sat, Oct 06, 2018 at 10:55:41AM +0200, Hans de Goede wrote:
>> On 03-10-18 11:22, Rafael J. Wysocki wrote:
>>> On Monday, September 24, 2018 11:40:14 AM CEST Hans de Goede wrote:
> 
>>> Also, why don't you use acpi_device_get_power() instead of evaluating
>>> _PSC directly?  It should make no difference if there are no power
>>> resources, should it?
> 
>> 2) acpi_device_get_power() is not exported to modules
> 
> Do we have any side effects if we just export it?
> I would think exporting is better than open coding.

acpi_device_get_power() does a lot more then just call the _PSC
method, while as explained we really just want the _PSC method
as that checks the actual D3 bit which we are interested in.

But as Rafael mentioned if the DSDT does not define power-resources
(which in the 3 DSDTs I just checked it doesn't for the PWM node) then
acpi_device_get_power() is equivalent.

So I guess I could change this, but I would prefer to stick with
the direct _PSC call.

Regards,

Hans

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Andy Shevchenko]

On Wed, Oct 10, 2018 at 01:14:36PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06-10-18 16:16, Andy Shevchenko wrote:
> > On Sat, Oct 06, 2018 at 10:55:41AM +0200, Hans de Goede wrote:
> > > On 03-10-18 11:22, Rafael J. Wysocki wrote:
> > > > On Monday, September 24, 2018 11:40:14 AM CEST Hans de Goede wrote:
> > 
> > > > Also, why don't you use acpi_device_get_power() instead of evaluating
> > > > _PSC directly?  It should make no difference if there are no power
> > > > resources, should it?
> > 
> > > 2) acpi_device_get_power() is not exported to modules
> > 
> > Do we have any side effects if we just export it?
> > I would think exporting is better than open coding.
> 
> acpi_device_get_power() does a lot more then just call the _PSC
> method, while as explained we really just want the _PSC method
> as that checks the actual D3 bit which we are interested in.
> 
> But as Rafael mentioned if the DSDT does not define power-resources
> (which in the 3 DSDTs I just checked it doesn't for the PWM node) then
> acpi_device_get_power() is equivalent.
> 
> So I guess I could change this, but I would prefer to stick with
> the direct _PSC call.

Up to Rafael, my personal point of view that acpi_device_get_power() is a good replacement here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Rafael J. Wysocki]

On Wed, Oct 10, 2018 at 1:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 06-10-18 16:16, Andy Shevchenko wrote:
> > On Sat, Oct 06, 2018 at 10:55:41AM +0200, Hans de Goede wrote:
> >> On 03-10-18 11:22, Rafael J. Wysocki wrote:
> >>> On Monday, September 24, 2018 11:40:14 AM CEST Hans de Goede wrote:
> >
> >>> Also, why don't you use acpi_device_get_power() instead of evaluating
> >>> _PSC directly?  It should make no difference if there are no power
> >>> resources, should it?
> >
> >> 2) acpi_device_get_power() is not exported to modules
> >
> > Do we have any side effects if we just export it?
> > I would think exporting is better than open coding.
>
> acpi_device_get_power() does a lot more then just call the _PSC
> method, while as explained we really just want the _PSC method
> as that checks the actual D3 bit which we are interested in.
>
> But as Rafael mentioned if the DSDT does not define power-resources
> (which in the 3 DSDTs I just checked it doesn't for the PWM node) then
> acpi_device_get_power() is equivalent.
>
> So I guess I could change this, but I would prefer to stick with
> the direct _PSC call.

And I would prefer acpi_device_get_power() to be used as long as it
works in this case.

I really don't want every driver to evaluate ACPI methods directly at will.

Thanks,
Rafael

Re: [PATCH 2/2] pwm: lpss: Check PWM powerstate after resume on Cherry Trail devices

[Hans de Goede]

Hi,

On 11-10-18 16:00, Rafael J. Wysocki wrote:
> On Wed, Oct 10, 2018 at 1:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 06-10-18 16:16, Andy Shevchenko wrote:
>>> On Sat, Oct 06, 2018 at 10:55:41AM +0200, Hans de Goede wrote:
>>>> On 03-10-18 11:22, Rafael J. Wysocki wrote:
>>>>> On Monday, September 24, 2018 11:40:14 AM CEST Hans de Goede wrote:
>>>
>>>>> Also, why don't you use acpi_device_get_power() instead of evaluating
>>>>> _PSC directly?  It should make no difference if there are no power
>>>>> resources, should it?
>>>
>>>> 2) acpi_device_get_power() is not exported to modules
>>>
>>> Do we have any side effects if we just export it?
>>> I would think exporting is better than open coding.
>>
>> acpi_device_get_power() does a lot more then just call the _PSC
>> method, while as explained we really just want the _PSC method
>> as that checks the actual D3 bit which we are interested in.
>>
>> But as Rafael mentioned if the DSDT does not define power-resources
>> (which in the 3 DSDTs I just checked it doesn't for the PWM node) then
>> acpi_device_get_power() is equivalent.
>>
>> So I guess I could change this, but I would prefer to stick with
>> the direct _PSC call.
> 
> And I would prefer acpi_device_get_power() to be used as long as it
> works in this case.
> 
> I really don't want every driver to evaluate ACPI methods directly at will.

Ok, I will prepare a v4 of this series using acpi_device_get_power()

Regards,

Hans