[PATCH V3 0/4] pwm: tegra: Pin configuration in suspend/resume and cleanups

[PATCH V3 0/4] pwm: tegra: Pin configuration in suspend/resume and cleanups

[Laxman Dewangan]

This patch series have following fixes:
- Add more precession in PWM period register value calculation
  for lower pwm frequency.
- Add support to configure PWM pins in different state in the
  suspend/resume.

Changes from v1:
- Use standard pinctrl names for sleep and active state.
- Use API pinctrl_pm_select_*()

Changes from V2:
- Type fixes, rephrases commit message and use pinctrl_pm_state* return
  value.

Laxman Dewangan (4):
  pwm: tegra: Use DIV_ROUND_CLOSEST_ULL() instead of local
    implementation
  pwm: tegra: Increase precision in pwm rate calculation
  pwm: tegra: Add DT binding details to configure pin in suspends/resume
  pwm: tegra: Add support to configure pin state in suspends/resume

 .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 43 ++++++++++++
 drivers/pwm/pwm-tegra.c                            | 77 ++++++++++++++++++++--
 2 files changed, 116 insertions(+), 4 deletions(-)

-- 
2.1.4

[PATCH V3 1/4] pwm: tegra: Use DIV_ROUND_CLOSEST_ULL() instead of local implementation

[Laxman Dewangan]

Use macro DIV_ROUND_CLOSEST_ULL() for 64bit division to closest one
instead of implementing the same locally. This increase readability.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

---
Changes from v1:
- None

Changes from V2:
- Fix typo in commit message.
---
 drivers/pwm/pwm-tegra.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index e464784..0a688da 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -85,8 +85,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * nearest integer during division.
 	 */
 	c *= (1 << PWM_DUTY_WIDTH);
-	c += period_ns / 2;
-	do_div(c, period_ns);
+	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);
 
 	val = (u32)c << PWM_DUTY_SHIFT;
 
-- 
2.1.4

[PATCH V3 2/4] pwm: tegra: Increase precision in pwm rate calculation

[Laxman Dewangan]

The rate of the PWM calculated as follows:
	hz = NSEC_PER_SEC / period_ns;
 	rate = (rate + (hz / 2)) / hz;

This has the precision loss in lower PWM rate.

Change this to have more precision as:
	hz = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC * 100, period_ns);
	rate = DIV_ROUND_CLOSEST(rate * 100, hz)

Example:
1. period_ns = 16672000, PWM clock rate is 200KHz.
	Based on old formula
		hz = NSEC_PER_SEC / period_ns
		   = 1000000000ul/16672000
		   = 59 (59.98)
		rate = (200K + 59/2)/59 = 3390

	Based on new method:
		hz = 5998
		rate = DIV_ROUND_CLOSE(200000*100, 5998) = 3334

	If we measure the PWM signal rate, we will get more accurate period
	with rate value of 3334 instead of 3390.

2.  period_ns = 16803898, PWM clock rate is 200KHz.
	Based on old formula:
		hz = 59, rate = 3390
	Based on new formula:
		hz = 5951, rate = 3360

	The PWM signal rate of 3360 is more near to requested period than 3333.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

---
Changes from v1:
- None

Changes from V2:
- Fix the commit message with exact formula used.
---
 drivers/pwm/pwm-tegra.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 0a688da..21518be 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -76,6 +76,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
 	unsigned long long c = duty_ns;
 	unsigned long rate, hz;
+	unsigned long long ns100 = NSEC_PER_SEC;
 	u32 val = 0;
 	int err;
 
@@ -94,9 +95,11 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * cycles at the PWM clock rate will take period_ns nanoseconds.
 	 */
 	rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
-	hz = NSEC_PER_SEC / period_ns;
 
-	rate = (rate + (hz / 2)) / hz;
+	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
+	ns100 *= 100;
+	hz = DIV_ROUND_CLOSEST_ULL(ns100, period_ns);
+	rate = DIV_ROUND_CLOSEST(rate * 100, hz);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
-- 
2.1.4

Re: [PATCH V3 2/4] pwm: tegra: Increase precision in pwm rate calculation

[Thierry Reding]

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

On Fri, Apr 07, 2017 at 03:04:00PM +0530, Laxman Dewangan wrote:
> The rate of the PWM calculated as follows:
> 	hz = NSEC_PER_SEC / period_ns;
>  	rate = (rate + (hz / 2)) / hz;
> 
> This has the precision loss in lower PWM rate.
> 
> Change this to have more precision as:
> 	hz = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC * 100, period_ns);
> 	rate = DIV_ROUND_CLOSEST(rate * 100, hz)
> 
> Example:
> 1. period_ns = 16672000, PWM clock rate is 200KHz.
> 	Based on old formula
> 		hz = NSEC_PER_SEC / period_ns
> 		   = 1000000000ul/16672000
> 		   = 59 (59.98)
> 		rate = (200K + 59/2)/59 = 3390
> 
> 	Based on new method:
> 		hz = 5998
> 		rate = DIV_ROUND_CLOSE(200000*100, 5998) = 3334
> 
> 	If we measure the PWM signal rate, we will get more accurate period
> 	with rate value of 3334 instead of 3390.
> 
> 2.  period_ns = 16803898, PWM clock rate is 200KHz.
> 	Based on old formula:
> 		hz = 59, rate = 3390
> 	Based on new formula:
> 		hz = 5951, rate = 3360
> 
> 	The PWM signal rate of 3360 is more near to requested period than 3333.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> 
> ---
> Changes from v1:
> - None
> 
> Changes from V2:
> - Fix the commit message with exact formula used.
> ---
>  drivers/pwm/pwm-tegra.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 0a688da..21518be 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -76,6 +76,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
>  	unsigned long long c = duty_ns;
>  	unsigned long rate, hz;
> +	unsigned long long ns100 = NSEC_PER_SEC;
>  	u32 val = 0;
>  	int err;
>  
> @@ -94,9 +95,11 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * cycles at the PWM clock rate will take period_ns nanoseconds.
>  	 */
>  	rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
> -	hz = NSEC_PER_SEC / period_ns;
>  
> -	rate = (rate + (hz / 2)) / hz;
> +	/* Consider precision in PWM_SCALE_WIDTH rate calculation */
> +	ns100 *= 100;
> +	hz = DIV_ROUND_CLOSEST_ULL(ns100, period_ns);

I think hz could overflow for small enough values of period_ns. I've
sent a patch that makes hz unsigned long long. While at it, the patch
also removes the ns100 variable which isn't really necessary here.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V3 3/4] pwm: tegra: Add DT binding details to configure pin in suspends/resume

[Laxman Dewangan]

In some of NVIDIA Tegra's platform, PWM controller is used to
control the PWM controlled regulators. PWM signal is connected to
the VID pin of the regulator where duty cycle of PWM signal decide
the voltage level of the regulator output.

When system enters suspend, some PWM client/slave regulator devices
require the PWM output to be tristated.

Add DT binding details to provide the pin configuration state
from PWM and pinctrl DT node in suspend and active state of
the system.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

---
Changes from v1:
- Use standard pinctrl names for sleep and active state.

Changes from V2:
- Fix the commit message and details
---
 .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
index b4e7377..c57e11b 100644
--- a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
@@ -19,6 +19,19 @@ Required properties:
 - reset-names: Must include the following entries:
   - pwm
 
+Optional properties:
+============================
+In some of the interface like PWM based regulator device, it is required
+to configure the pins differently in different states, especially in suspend
+state of the system. The configuration of pin is provided via the pinctrl
+DT node as detailed in the pinctrl DT binding document
+	Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+
+The PWM node will have following optional properties.
+pinctrl-names:	Pin state names. Must be "default" and "sleep".
+pinctrl-0:	phandle for the default/active state of pin configurations.
+pinctrl-1:	phandle for the sleep state of pin configurations.
+
 Example:
 
 	pwm: pwm@7000a000 {
@@ -29,3 +42,35 @@ Example:
 		resets = <&tegra_car 17>;
 		reset-names = "pwm";
 	};
+
+
+Example with the pin configuration for suspend and resume:
+=========================================================
+Suppose pin PE7 (On Tegra210) interfaced with the regulator device and
+it requires PWM output to be tristated when system enters suspend.
+Following will be DT binding to achieve this:
+
+#include <dt-bindings/pinctrl/pinctrl-tegra.h>
+
+	pinmux@700008d4 {
+		pwm_active_state: pwm_active_state {
+                        pe7 {
+                                nvidia,pins = "pe7";
+                                nvidia,tristate = <TEGRA_PIN_DISABLE>;
+			};
+		};
+
+		pwm_sleep_state: pwm_sleep_state {
+                        pe7 {
+                                nvidia,pins = "pe7";
+                                nvidia,tristate = <TEGRA_PIN_ENABLE>;
+			};
+		};
+	};
+
+	pwm@7000a000 {
+		/* Mandatory PWM properties */
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&pwm_active_state>;
+		pinctrl-1 = <&pwm_sleep_state>;
+	};
-- 
2.1.4

Re: [PATCH V3 3/4] pwm: tegra: Add DT binding details to configure pin in suspends/resume

[Jon Hunter]

On 07/04/17 10:34, Laxman Dewangan wrote:
> In some of NVIDIA Tegra's platform, PWM controller is used to
> control the PWM controlled regulators. PWM signal is connected to
> the VID pin of the regulator where duty cycle of PWM signal decide
> the voltage level of the regulator output.
> 
> When system enters suspend, some PWM client/slave regulator devices
> require the PWM output to be tristated.
> 
> Add DT binding details to provide the pin configuration state
> from PWM and pinctrl DT node in suspend and active state of
> the system.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> 
> ---
> Changes from v1:
> - Use standard pinctrl names for sleep and active state.
> 
> Changes from V2:
> - Fix the commit message and details
> ---
>  .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> index b4e7377..c57e11b 100644
> --- a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> @@ -19,6 +19,19 @@ Required properties:
>  - reset-names: Must include the following entries:
>    - pwm
>  
> +Optional properties:
> +============================
> +In some of the interface like PWM based regulator device, it is required
> +to configure the pins differently in different states, especially in suspend
> +state of the system. The configuration of pin is provided via the pinctrl
> +DT node as detailed in the pinctrl DT binding document
> +	Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +The PWM node will have following optional properties.
> +pinctrl-names:	Pin state names. Must be "default" and "sleep".
> +pinctrl-0:	phandle for the default/active state of pin configurations.
> +pinctrl-1:	phandle for the sleep state of pin configurations.
> +
>  Example:
>  
>  	pwm: pwm@7000a000 {
> @@ -29,3 +42,35 @@ Example:
>  		resets = <&tegra_car 17>;
>  		reset-names = "pwm";
>  	};
> +
> +
> +Example with the pin configuration for suspend and resume:
> +=========================================================
> +Suppose pin PE7 (On Tegra210) interfaced with the regulator device and
> +it requires PWM output to be tristated when system enters suspend.
> +Following will be DT binding to achieve this:
> +
> +#include <dt-bindings/pinctrl/pinctrl-tegra.h>
> +
> +	pinmux@700008d4 {
> +		pwm_active_state: pwm_active_state {
> +                        pe7 {
> +                                nvidia,pins = "pe7";
> +                                nvidia,tristate = <TEGRA_PIN_DISABLE>;
> +			};
> +		};
> +
> +		pwm_sleep_state: pwm_sleep_state {
> +                        pe7 {
> +                                nvidia,pins = "pe7";
> +                                nvidia,tristate = <TEGRA_PIN_ENABLE>;
> +			};
> +		};
> +	};
> +
> +	pwm@7000a000 {
> +		/* Mandatory PWM properties */

Maybe these are mandatory for the platform, but given these are
optional, its a bit confusing.

> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&pwm_active_state>;
> +		pinctrl-1 = <&pwm_sleep_state>;
> +	};

However, fine with me so ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

Re: [PATCH V3 3/4] pwm: tegra: Add DT binding details to configure pin in suspends/resume

[Rob Herring]

On Fri, Apr 07, 2017 at 03:04:01PM +0530, Laxman Dewangan wrote:
> In some of NVIDIA Tegra's platform, PWM controller is used to
> control the PWM controlled regulators. PWM signal is connected to
> the VID pin of the regulator where duty cycle of PWM signal decide
> the voltage level of the regulator output.
> 
> When system enters suspend, some PWM client/slave regulator devices
> require the PWM output to be tristated.
> 
> Add DT binding details to provide the pin configuration state
> from PWM and pinctrl DT node in suspend and active state of
> the system.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> 
> ---
> Changes from v1:
> - Use standard pinctrl names for sleep and active state.
> 
> Changes from V2:
> - Fix the commit message and details
> ---
>  .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)

Acked-by: Rob Herring <robh@kernel.org> 

[PATCH V3 4/4] pwm: tegra: Add support to configure pin state in suspends/resume

[Laxman Dewangan]

In some of NVIDIA Tegra's platform, PWM controller is used to
control the PWM controlled regulators. PWM signal is connected to
the VID pin of the regulator where duty cycle of PWM signal decide
the voltage level of the regulator output.

When system enters suspend, some PWM client/slave regulator devices
require the PWM output to be tristated.

Add support to configure the pin state via pinctrl frameworks in
suspend and active state of the system.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

---
Changes from v1:
- Use standard pinctrl names for sleep and active state.
- Use API pinctrl_pm_select_*()

Changes from V2:
- Use returns of pinctrl_pm_select_*()
- Rephrase commit message.
---
 drivers/pwm/pwm-tegra.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 21518be..9c7f180 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -29,6 +29,7 @@
 #include <linux/of_device.h>
 #include <linux/pwm.h>
 #include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/slab.h>
 #include <linux/reset.h>
 
@@ -255,6 +256,18 @@ static int tegra_pwm_remove(struct platform_device *pdev)
 	return pwmchip_remove(&pc->chip);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int tegra_pwm_suspend(struct device *dev)
+{
+	return pinctrl_pm_select_sleep_state(dev);
+}
+
+static int tegra_pwm_resume(struct device *dev)
+{
+	return pinctrl_pm_select_default_state(dev);
+}
+#endif
+
 static const struct tegra_pwm_soc tegra20_pwm_soc = {
 	.num_channels = 4,
 };
@@ -271,10 +284,15 @@ static const struct of_device_id tegra_pwm_of_match[] = {
 
 MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
 
+static const struct dev_pm_ops tegra_pwm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
+};
+
 static struct platform_driver tegra_pwm_driver = {
 	.driver = {
 		.name = "tegra-pwm",
 		.of_match_table = tegra_pwm_of_match,
+		.pm = &tegra_pwm_pm_ops,
 	},
 	.probe = tegra_pwm_probe,
 	.remove = tegra_pwm_remove,
-- 
2.1.4

Re: [PATCH V3 4/4] pwm: tegra: Add support to configure pin state in suspends/resume

[Jon Hunter]

On 07/04/17 10:34, Laxman Dewangan wrote:
> In some of NVIDIA Tegra's platform, PWM controller is used to
> control the PWM controlled regulators. PWM signal is connected to
> the VID pin of the regulator where duty cycle of PWM signal decide
> the voltage level of the regulator output.
> 
> When system enters suspend, some PWM client/slave regulator devices
> require the PWM output to be tristated.
> 
> Add support to configure the pin state via pinctrl frameworks in
> suspend and active state of the system.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> 
> ---
> Changes from v1:
> - Use standard pinctrl names for sleep and active state.
> - Use API pinctrl_pm_select_*()
> 
> Changes from V2:
> - Use returns of pinctrl_pm_select_*()
> - Rephrase commit message.
> ---
>  drivers/pwm/pwm-tegra.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 21518be..9c7f180 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -29,6 +29,7 @@
>  #include <linux/of_device.h>
>  #include <linux/pwm.h>
>  #include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/reset.h>
>  
> @@ -255,6 +256,18 @@ static int tegra_pwm_remove(struct platform_device *pdev)
>  	return pwmchip_remove(&pc->chip);
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_pwm_suspend(struct device *dev)
> +{
> +	return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int tegra_pwm_resume(struct device *dev)
> +{
> +	return pinctrl_pm_select_default_state(dev);
> +}
> +#endif
> +
>  static const struct tegra_pwm_soc tegra20_pwm_soc = {
>  	.num_channels = 4,
>  };
> @@ -271,10 +284,15 @@ static const struct of_device_id tegra_pwm_of_match[] = {
>  
>  MODULE_DEVICE_TABLE(of, tegra_pwm_of_match);
>  
> +static const struct dev_pm_ops tegra_pwm_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tegra_pwm_suspend, tegra_pwm_resume)
> +};
> +
>  static struct platform_driver tegra_pwm_driver = {
>  	.driver = {
>  		.name = "tegra-pwm",
>  		.of_match_table = tegra_pwm_of_match,
> +		.pm = &tegra_pwm_pm_ops,
>  	},
>  	.probe = tegra_pwm_probe,
>  	.remove = tegra_pwm_remove,

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

Re: [PATCH V3 0/4] pwm: tegra: Pin configuration in suspend/resume and cleanups

[Thierry Reding]

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

On Fri, Apr 07, 2017 at 03:03:58PM +0530, Laxman Dewangan wrote:
> This patch series have following fixes:
> - Add more precession in PWM period register value calculation
>   for lower pwm frequency.
> - Add support to configure PWM pins in different state in the
>   suspend/resume.
> 
> Changes from v1:
> - Use standard pinctrl names for sleep and active state.
> - Use API pinctrl_pm_select_*()
> 
> Changes from V2:
> - Type fixes, rephrases commit message and use pinctrl_pm_state* return
>   value.
> 
> Laxman Dewangan (4):
>   pwm: tegra: Use DIV_ROUND_CLOSEST_ULL() instead of local
>     implementation
>   pwm: tegra: Increase precision in pwm rate calculation
>   pwm: tegra: Add DT binding details to configure pin in suspends/resume
>   pwm: tegra: Add support to configure pin state in suspends/resume
> 
>  .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 43 ++++++++++++
>  drivers/pwm/pwm-tegra.c                            | 77 ++++++++++++++++++++--
>  2 files changed, 116 insertions(+), 4 deletions(-)

All four patches applied to for-4.12/drivers, thanks.

I've slightly modified the commit messages of some patches for "pwm" ->
"PWM".

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]