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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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.
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:
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
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 <[email protected]>
>>> ---
>>> 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