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

[PATCH V2 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_*()

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 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 closet one
instead of implementing the same locally. This increase readability.

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes from V1:
None

 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Thierry Reding]

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

On Thu, Apr 06, 2017 at 07:50:58PM +0530, Laxman Dewangan wrote:
> Use macro DIV_ROUND_CLOSEST_ULL() for 64bit division to closet one

"closest"

Thierry

> instead of implementing the same locally. This increase readability.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
> Changes from V1:
> None
> 
>  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
> 

[-- 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.

The tristate (high impedance of PWM pin form Tegra) also define
one of the state of PWM regulator which needs to be configure in
suspend state of system.

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes from v1:
- Use standard pinctrl names for sleep and active state.

 .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
index b4e7377..4128cdc 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:	Node handle for the default/active state of pi configurations.
+pinctrl-1:	Node handle for the sleep state of pin configurations.
+
 Example:
 
 	pwm: pwm@7000a000 {
@@ -29,3 +42,33 @@ Example:
 		resets = <&tegra_car 17>;
 		reset-names = "pwm";
 	};
+
+
+Example with the pin configuration for suspend and resume:
+=========================================================
+Pin PE7 is used as PWM interface.
+
+#include <dt-bindings/pinctrl/pinctrl-tegra.h>
+
+	pinmux@70000868 {
+		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 06/04/17 15:21, 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.
> 
> The tristate (high impedance of PWM pin form Tegra) also define

s/form/from/
s/define/defines/

> one of the state of PWM regulator which needs to be configure in
> suspend state of system.

It maybe clearer to say that when the system enters suspend the
regulator requires 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.
> 
>  .../devicetree/bindings/pwm/nvidia,tegra20-pwm.txt | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt b/Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt
> index b4e7377..4128cdc 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:	Node handle for the default/active state of pi configurations.

s/pi/pin/
s/Node handle/phandle/

> +pinctrl-1:	Node handle for the sleep state of pin configurations.
> +
>  Example:
>  
>  	pwm: pwm@7000a000 {
> @@ -29,3 +42,33 @@ Example:
>  		resets = <&tegra_car 17>;
>  		reset-names = "pwm";
>  	};
> +
> +
> +Example with the pin configuration for suspend and resume:
> +=========================================================
> +Pin PE7 is used as PWM interface.

Nit-pick. On what devices? Sounds like this is verbatim. Maybe state
what device this is an example for.

Jon

-- 
nvpublic

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

[Laxman Dewangan]

On Thursday 06 April 2017 08:56 PM, Jon Hunter wrote:
> On 06/04/17 15:21, 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.
>>
>> The tristate (high impedance of PWM pin form Tegra) also define
> s/form/from/
> s/define/defines/
>
>> one of the state of PWM regulator which needs to be configure in
>> suspend state of system.
> It maybe clearer to say that when the system enters suspend the
> regulator requires the pwm output to be tristated.

Not necessarily that every PWM regulator interfaces needs it.  It 
depends on the devices.
So I will say:

When system enters suspend, in some of PWM regulator interface, it is 
required to to set the PWM output to be tristated.


>   	pwm: pwm@7000a000 {
> @@ -29,3 +42,33 @@ Example:
>   		resets = <&tegra_car 17>;
>   		reset-names = "pwm";
>   	};
> +
> +
> +Example with the pin configuration for suspend and resume:
> +=========================================================
> +Pin PE7 is used as PWM interface.
> Nit-pick. On what devices? Sounds like this is verbatim. Maybe state
> what device this is an example for.

Let me phrase it as:
Suppose pin PE7 (On tegra210) interfaced with the regulator device and 
this requires PWM output to be tristated when system enters suspend.
Following will be DT binding to achieve this:

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

[Jon Hunter]

On 06/04/17 17:48, Laxman Dewangan wrote:
> 
> On Thursday 06 April 2017 08:56 PM, Jon Hunter wrote:
>> On 06/04/17 15:21, 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.
>>>
>>> The tristate (high impedance of PWM pin form Tegra) also define
>> s/form/from/
>> s/define/defines/
>>
>>> one of the state of PWM regulator which needs to be configure in
>>> suspend state of system.
>> It maybe clearer to say that when the system enters suspend the
>> regulator requires the pwm output to be tristated.
> 
> Not necessarily that every PWM regulator interfaces needs it.  It
> depends on the devices.

Yes I understand that. I am just saying the description could be a
little clearer.

> So I will say:
> 
> When system enters suspend, in some of PWM regulator interface, it is
> required to to set the PWM output to be tristated.

Ok, but I think you should say why that is, because from the above
sentence alone it is not clear. Maybe you should say that some PWM
client/slave devices require the PWM output to be tristated.

Jon

-- 
nvpublic

[PATCH V4 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.

The tristate (high impedance of PWM pin form Tegra) also define
one of the state of PWM regulator which needs to be configure in
suspend state of system.

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

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes from v1:
- Use standard pinctrl names for sleep and active state.
- Use API pinctrl_pm_select_*()

 drivers/pwm/pwm-tegra.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index e9c4de5..af1bd4f 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>
 
@@ -256,6 +257,22 @@ 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)
+{
+	pinctrl_pm_select_sleep_state(dev);
+
+	return 0;
+}
+
+static int tegra_pwm_resume(struct device *dev)
+{
+	pinctrl_pm_select_default_state(dev);
+
+	return 0;
+}
+#endif
+
 static const struct tegra_pwm_soc tegra20_pwm_soc = {
 	.num_channels = 4,
 };
@@ -272,10 +289,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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jon Hunter]

On 06/04/17 15:21, 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.
> 
> The tristate (high impedance of PWM pin form Tegra) also define
> one of the state of PWM regulator which needs to be configure in
> suspend state of system.
> 
> 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_*()
> 
>  drivers/pwm/pwm-tegra.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index e9c4de5..af1bd4f 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>
>  
> @@ -256,6 +257,22 @@ 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)
> +{
> +	pinctrl_pm_select_sleep_state(dev);

Why not return the error code here?

> +
> +	return 0;
> +}
> +
> +static int tegra_pwm_resume(struct device *dev)
> +{
> +	pinctrl_pm_select_default_state(dev);

And here.

By the way, do you plan to include patches to populate the bindings for
the pwm devices?

Cheers
Jon

-- 
nvpublic

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

[Laxman Dewangan]

Oops, it was actually v2.

On Thursday 06 April 2017 08:47 PM, Jon Hunter wrote:
> On 06/04/17 15:21, 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.
>>
>> The tristate (high impedance of PWM pin form Tegra) also define
>> one of the state of PWM regulator which needs to be configure in
>> suspend state of system.
>>
>> 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_*()
>>
>>   drivers/pwm/pwm-tegra.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
>> index e9c4de5..af1bd4f 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>
>>   
>> @@ -256,6 +257,22 @@ 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)
>> +{
>> +	pinctrl_pm_select_sleep_state(dev);
> Why not return the error code here?

As the pin state in suspend is optional, I dont  want to return error if 
the sleep state is not available.

However, it seems pinctrl take care of retuning success if there is no 
sleep state. By seeing code.
Let me test this on different condition and it it works fine then we can 
return the return of pinctrl_pm_select_*()


BTW, it should be OK to have pwm_tegra_resume/suspend wrapper, not 
directly use the pinctrl_pm_select_* in pm ops suspend/resume. The 
prototype matches.



>
> By the way, do you plan to include patches to populate the bindings for
> the pwm devices?

I am planning to populate the GPU regulator which is PWM based. This 
will only populate the regulator.

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

[Jon Hunter]

On 06/04/17 17:40, Laxman Dewangan wrote:
> Oops, it was actually v2.
> 
> On Thursday 06 April 2017 08:47 PM, Jon Hunter wrote:
>> On 06/04/17 15:21, 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.
>>>
>>> The tristate (high impedance of PWM pin form Tegra) also define
>>> one of the state of PWM regulator which needs to be configure in
>>> suspend state of system.
>>>
>>> Add support to configure the pin state via pinctrl frameworks in
>>> suspend and active state of the system.
>>>
>>> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>> Changes from v1:
>>> - Use standard pinctrl names for sleep and active state.
>>> - Use API pinctrl_pm_select_*()
>>>
>>>   drivers/pwm/pwm-tegra.c | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
>>> index e9c4de5..af1bd4f 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>
>>>   @@ -256,6 +257,22 @@ 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)
>>> +{
>>> +    pinctrl_pm_select_sleep_state(dev);
>> Why not return the error code here?
> 
> As the pin state in suspend is optional, I dont  want to return error if
> the sleep state is not available.
> 
> However, it seems pinctrl take care of retuning success if there is no
> sleep state. By seeing code.

Exactly, that is what I did for i2c because pinctrl it is also optional
for i2c.

> Let me test this on different condition and it it works fine then we can
> return the return of pinctrl_pm_select_*()
> 
> 
> BTW, it should be OK to have pwm_tegra_resume/suspend wrapper, not
> directly use the pinctrl_pm_select_* in pm ops suspend/resume. The
> prototype matches.

I think that I would keep the wrapper.

>> By the way, do you plan to include patches to populate the bindings for
>> the pwm devices?
> 
> I am planning to populate the GPU regulator which is PWM based. This
> will only populate the regulator.

Ok.

Jon

-- 
nvpublic

[PATCH V2 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.
Changing this to have more precision as:
	hz = DIV_ROUND_CLOSE(NSEC_PER_SEC * 100, period_ns);
	rate = DIV_ROUND_CLOSE(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 = 60, rate = 3333
	Based on new formula:
		hz = 5951, rate = 3360

	The rate of 3360 is more near to requested period then the 3333.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
- None

 drivers/pwm/pwm-tegra.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 0a688da..e9c4de5 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -76,6 +76,8 @@ 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;
+	unsigned long precision = 100; /* Consider 2 digit precision */
 	u32 val = 0;
 	int err;
 
@@ -94,9 +96,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 *= precision;
+	hz = DIV_ROUND_CLOSEST_ULL(ns100, period_ns);
+	rate = DIV_ROUND_CLOSEST(rate * precision, hz);
 
 	/*
 	 * Since the actual PWM divider is the register's frequency divider
-- 
2.1.4

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

[Thierry Reding]

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

On Thu, Apr 06, 2017 at 07:50:59PM +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.
> Changing this to have more precision as:
> 	hz = DIV_ROUND_CLOSE(NSEC_PER_SEC * 100, period_ns);
> 	rate = DIV_ROUND_CLOSE(rate * 100, hz)

DIV_ROUND_CLOSEST(). And I much prefer this to the actual code below. I
don't think it's necessary to have a local variable for the precision.

Thierry

> 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 = 60, rate = 3333
> 	Based on new formula:
> 		hz = 5951, rate = 3360
> 
> 	The rate of 3360 is more near to requested period then the 3333.
> 
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Changes from V1:
> - None
> 
>  drivers/pwm/pwm-tegra.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 0a688da..e9c4de5 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -76,6 +76,8 @@ 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;
> +	unsigned long precision = 100; /* Consider 2 digit precision */
>  	u32 val = 0;
>  	int err;
>  
> @@ -94,9 +96,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 *= precision;
> +	hz = DIV_ROUND_CLOSEST_ULL(ns100, period_ns);
> +	rate = DIV_ROUND_CLOSEST(rate * precision, hz);
>  
>  	/*
>  	 * Since the actual PWM divider is the register's frequency divider
> -- 
> 2.1.4
> 

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

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

[Laxman Dewangan]

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


On Thursday 06 April 2017 09:54 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Apr 06, 2017 at 07:50:59PM +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.
>> Changing this to have more precision as:
>> 	hz = DIV_ROUND_CLOSE(NSEC_PER_SEC * 100, period_ns);
>> 	rate = DIV_ROUND_CLOSE(rate * 100, hz)
> DIV_ROUND_CLOSEST(). And I much prefer this to the actual code below. I
> don't think it's necessary to have a local variable for the precision.
Do you suggest to use DIV_ROUND_CLOSEST() instead of 
DIV_ROUND_CLOSEST_ULL()?

This is ULL division and so we need to use this one.

Or am I missing something

[-- Attachment #2: Type: text/html, Size: 1288 bytes --]

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

[Thierry Reding]

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

On Thu, Apr 06, 2017 at 07:50:59PM +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.
> Changing this to have more precision as:
> 	hz = DIV_ROUND_CLOSE(NSEC_PER_SEC * 100, period_ns);
> 	rate = DIV_ROUND_CLOSE(rate * 100, hz)

DIV_ROUND_CLOSEST(). Also I very much prefer this notation over the
actual code below. I don't think we need a local variable to hold the
precision.

Thierry

> 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 = 60, rate = 3333
> 	Based on new formula:
> 		hz = 5951, rate = 3360
> 
> 	The rate of 3360 is more near to requested period then the 3333.
> 
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Changes from V1:
> - None
> 
>  drivers/pwm/pwm-tegra.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 0a688da..e9c4de5 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -76,6 +76,8 @@ 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;
> +	unsigned long precision = 100; /* Consider 2 digit precision */
>  	u32 val = 0;
>  	int err;
>  
> @@ -94,9 +96,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 *= precision;
> +	hz = DIV_ROUND_CLOSEST_ULL(ns100, period_ns);
> +	rate = DIV_ROUND_CLOSEST(rate * precision, hz);
>  
>  	/*
>  	 * Since the actual PWM divider is the register's frequency divider
> -- 
> 2.1.4
> 

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