Re: [PATCH v3 6/6] gpio: max77620: Initialize hardware state of interrupts

[Laxman Dewangan]

On Thursday 09 July 2020 01:53 AM, Dmitry Osipenko wrote:

External email: Use caution opening links or attachments


I noticed on Nexus 7 that after rebooting from downstream kernel to
upstream, the GPIO interrupt is triggering non-stop despite interrupts
being disabled for all of GPIOs. This happens because Nexus 7 uses a
soft-reboot, meaning that bootloader should take care of resetting
hardware, but the bootloader doesn't do it well. As a result, GPIO
interrupt may be left ON at a boot time. Let's mask all GPIO interrupts
at the driver's initialization time in order to resolve the issue.

Signed-off-by: Dmitry Osipenko 




Looks good to me.
Acked-by: Laxman Dewangan 

Re: [PATCH v3 5/6] gpio: max77620: Use irqchip template

[Laxman Dewangan]

On Thursday 09 July 2020 01:53 AM, Dmitry Osipenko wrote:

External email: Use caution opening links or attachments


This change addresses one of the GPIO-core TODOs for the MAX77620 driver
which requires modern drivers to use the irqchip template. Instead of
using the GPIO's irqchip-helpers for creating the IRQ domain, the
gpio_irq_chip structure is now filled by the driver itself and then
gpiochip_add_data() takes care of instantiating the IRQ domain for us.

Suggested-by: Andy Shevchenko 
Signed-off-by: Dmitry Osipenko 




Looks good to me.
Acked-by: Laxman Dewangan 

Re: [PATCH v3 4/6] gpio: max77620: Don't shadow error code of platform_get_irq()

[Laxman Dewangan]

On Thursday 09 July 2020 01:53 AM, Dmitry Osipenko wrote:

External email: Use caution opening links or attachments


The platform_get_irq() returns a positive interrupt number on success and
negative error code on failure (zero shouldn't ever happen in practice, it
would produce a noisy warning). Hence let's return the error code directly
instead of overriding it with -ENODEV.

Suggested-by: Andy Shevchenko 
Signed-off-by: Dmitry Osipenko 




Looks good to me.
Acked-by: Laxman Dewangan 

Re: [PATCH v3 3/6] gpio: max77620: Don't set of_node

[Laxman Dewangan]

On Thursday 09 July 2020 01:53 AM, Dmitry Osipenko wrote:

External email: Use caution opening links or attachments


The gpiochip_add_data() takes care of setting the of_node to the parent's
device of_node, hence there is no need to do it manually in the driver's
code. This patch corrects the parent's device pointer and removes the
unnecessary setting of the of_node.

Suggested-by: Andy Shevchenko 
Signed-off-by: Dmitry Osipenko 




Looks good to me.
Acked-by: Laxman Dewangan 

Re: [PATCH v3 2/6] gpio: max77620: Fix missing release of interrupt

[Laxman Dewangan]

On Thursday 09 July 2020 01:53 AM, Dmitry Osipenko wrote:

External email: Use caution opening links or attachments


The requested interrupt is never released by the driver. Fix this by
using the resource-managed variant of request_threaded_irq().

Fixes: ab3dd9cc24d4 ("gpio: max77620: Fix interrupt handling")
Signed-off-by: Dmitry Osipenko 



Looks good to me.
Acked-by: Laxman Dewangan 

Re: [PATCH v3 1/6] gpio: max77620: Replace 8 with MAX77620_GPIO_NR

[Laxman Dewangan]

On Thursday 09 July 2020 01:53 AM, Dmitry Osipenko wrote:

External email: Use caution opening links or attachments


The MAX77620_GPIO_NR enum value represents the total number of GPIOs,
let's use it instead of a raw value in order to improve the code's
readability a tad.

Reviewed-by: Andy Shevchenko 



Looks good to me.
Acked-by: Laxman Dewangan 

Re: [PATCH V9] i2c: tegra: remove BUG() macro

[Laxman Dewangan]

On Tuesday 18 June 2019 04:39 PM, Bitan Biswas wrote:

The usage of BUG() macro is generally discouraged in kernel, unless
it's a problem that results in a physical damage or loss of data.
This patch removes unnecessary BUG() macros and replaces the rest
with warning.

Signed-off-by: Bitan Biswas 


Acked By: Laxman Dewangan 

Thanks,
Laxman

Re: [PATCH V5 6/7] i2c: tegra: fix PIO rx/tx residual transfer check

[Laxman Dewangan]

On Wednesday 12 June 2019 03:54 PM, Wolfram Sang wrote:

On Tue, Jun 11, 2019 at 03:51:13AM -0700, Bitan Biswas wrote:

Fix expression for residual bytes(less than word) transfer
in I2C PIO mode RX/TX.

Signed-off-by: Bitan Biswas 

I applied patches 1-5 to my for-next tree now. No need to resend them
anymore, you can focus on the remaining patches now.

Question: The nominal maintainer for this driver is

 Laxman Dewangan  (supporter:TEGRA I2C DRIVER)

I wonder if he is still around and interested?

That aside, thanks a lot Dmitry for the review of this series!

Most of patches are coming from the downstream as part of upstream 
effort. Hence not reviewing explicitly.

Re: [PATCH] regulator: rc5t583: Get rid of struct rc5t583_regulator

[Laxman Dewangan]

On Wednesday 27 March 2019 05:24 PM, Axel Lin wrote:

The struct rc5t583_regulator only has 2 members, the *rdev is no longer
used because this driver is using devm_regulator_register now. After remove
*rdev, only *reg_info left. We can use struct rc5t583_regulator_info
directly, so remove struct rc5t583_regulator.

Signed-off-by: Axel Lin 



Acked-by: Laxman Dewangan 

Re: [PATCH 2/2] regulator: as3722: Slightly improve readability

[Laxman Dewangan]

On Wednesday 27 March 2019 06:59 AM, Axel Lin wrote:

Add a local variable *desc to avoid too many change lines due to over 80
characters.

Signed-off-by: Axel Lin 


Acked-by: Laxman Dewangan 

Re: [PATCH 1/2] regulator: as3722: Convert to use regulator_set/get_current_limit_regmap

[Laxman Dewangan]

On Wednesday 27 March 2019 06:59 AM, Axel Lin wrote:

Use regulator_set/get_current_limit_regmap helpers to save some code.

Signed-off-by: Axel Lin 


Acked-by: Laxman Dewangan 

Re: [PATCH 12/18] mfd: tps80031: Make it explicitly non-modular

[Laxman Dewangan]

On Tuesday 18 December 2018 02:01 AM, Paul Gortmaker wrote:

The Kconfig currently controlling compilation of this code is:

drivers/mfd/Kconfig:config MFD_TPS80031
drivers/mfd/Kconfig:bool "TI TPS80031/TPS80032 Power Management chips"

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

We explicitly disallow a driver unbind, since that doesn't have a
sensible use case anyway, and it allows us to drop the ".remove"
code for non-modular drivers.

Since module_init was not in use by this code, the init ordering
remains unchanged with this commit.

We don't replace module.h with init.h since the file already has that.

Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.

We also delete the MODULE_LICENSE tag etc. since all that information
is already contained at the top of the file in the comments.

Cc: Lee Jones 
Cc: Laxman Dewangan 
Signed-off-by: Paul Gortmaker 
Acked-by: Linus Walleij 



Acked-by: Laxman Dewangan 

Re: [PATCH 07/18] mfd: rc5t583: Make it explicitly non-modular

[Laxman Dewangan]

On Tuesday 18 December 2018 02:01 AM, Paul Gortmaker wrote:

The Kconfig currently controlling compilation of this code is:

drivers/mfd/Kconfig:config MFD_RC5T583
drivers/mfd/Kconfig:bool "Ricoh RC5T583 Power Management system device"

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

Since module_init was not in use by this code, the init ordering
remains unchanged with this commit.

Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.

We also delete the MODULE_LICENSE tag etc. since all that information
is already contained at the top of the file in the comments.

Cc: Lee Jones 
Cc: Laxman Dewangan 
Signed-off-by: Paul Gortmaker 
Acked-by: Linus Walleij 


Acked-by: Laxman Dewangan 

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Laxman Dewangan]

On Monday 19 November 2018 10:44 PM, Tony Lindgren wrote:

Hi,

* Tony Lindgren  [181119 16:19]:

* Peter Ujfalusi  [181119 10:16]:

On 2018-11-13 20:06, Tony Lindgren wrote:

Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
should be fixed with IRQ_TYPE_HIGH.

No idea about why palmas interrupts would stop working though,
Peter, do you have any ideas on this one?

No, I don't.
The INT polarity can be changed in Palmas.
based on the pdata->irq_flags (queried via irqd_get_trigger_type())
the code  configures it:

if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
else
reg = 0;

and we pass the same irq_flags to the regmap_add_irq_chip()
IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x0004

A change in DT should be enough, no need to patch palmas.c, imho.

But it's not. I'm now wondering if wakeupgen is inverting the
polarity for this interrupt?

GIC docs say this about SPI interrupts:

"SPI is triggered on a rising edge or is active-HIGH level-sensitive."

So when setting IRQ_TYPE_LEVEL_HIGH in dts, we still must not
invert the polarity in palmas while tegra needs to. So either
tegra114 hardware is inverting the polarity, or omap5 wakeupgen
is.

Does the palmas trm say which way PALMAS_POLARITY_CTRL
triggers if PALMAS_POLARITY_CTRL_INT_POLARITY is set?

Also note that dra7 is using a gpio for palmas interrupt.

Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
Tegra114 PMIC interrupt") states that tegra114 inverts the
polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.

So it seems that commit df545d1cd01a ("mfd: palmas: Provide
irq flags through DT/platform data") wrongly sets the
PALMAS_POLARITY_CTRL_INT_POLARITY on IRQ_TYPE_LEVEL_HIGH
while it should set it on IRQ_TYPE_LEVEL_LOW.


When I implemented, ARM GIC interrupt driver did not support the 
IRQ_TYPE_LEVEL_LOW. If we set this then it produces warning.

[Commit ID commit df545d1cd01aab3ba3f687d5423e6c3687b069d8
mfd: palmas: Provide irq flags through DT/platform data]

So from DT we can not really set the IRQ_TYPE_LEVEL_LOW as irq flag.

Re: omap5 fixing palmas IRQ_TYPE_NONE warning leads to gpadc timeouts

[Laxman Dewangan]

On Monday 19 November 2018 10:44 PM, Tony Lindgren wrote:

Hi,

* Tony Lindgren  [181119 16:19]:

* Peter Ujfalusi  [181119 10:16]:

On 2018-11-13 20:06, Tony Lindgren wrote:

Looks like the IRQ_TYPE_NONE issue still is there for omap5 and
should be fixed with IRQ_TYPE_HIGH.

No idea about why palmas interrupts would stop working though,
Peter, do you have any ideas on this one?

No, I don't.
The INT polarity can be changed in Palmas.
based on the pdata->irq_flags (queried via irqd_get_trigger_type())
the code  configures it:

if (pdata->irq_flags & IRQ_TYPE_LEVEL_HIGH)
reg = PALMAS_POLARITY_CTRL_INT_POLARITY;
else
reg = 0;

and we pass the same irq_flags to the regmap_add_irq_chip()
IRQ_TYPE_LEVEL_HIGH == IRQF_TRIGGER_HIGH == 0x0004

A change in DT should be enough, no need to patch palmas.c, imho.

But it's not. I'm now wondering if wakeupgen is inverting the
polarity for this interrupt?

GIC docs say this about SPI interrupts:

"SPI is triggered on a rising edge or is active-HIGH level-sensitive."

So when setting IRQ_TYPE_LEVEL_HIGH in dts, we still must not
invert the polarity in palmas while tegra needs to. So either
tegra114 hardware is inverting the polarity, or omap5 wakeupgen
is.

Does the palmas trm say which way PALMAS_POLARITY_CTRL
triggers if PALMAS_POLARITY_CTRL_INT_POLARITY is set?

Also note that dra7 is using a gpio for palmas interrupt.

Well so commit 7e9d474954f4 ("ARM: tegra: Correct polarity for
Tegra114 PMIC interrupt") states that tegra114 inverts the
polarity of the PMIC interrupt. So adding Jon and Thierry to Cc.

So it seems that commit df545d1cd01a ("mfd: palmas: Provide
irq flags through DT/platform data") wrongly sets the
PALMAS_POLARITY_CTRL_INT_POLARITY on IRQ_TYPE_LEVEL_HIGH
while it should set it on IRQ_TYPE_LEVEL_LOW.


When I implemented, ARM GIC interrupt driver did not support the 
IRQ_TYPE_LEVEL_LOW. If we set this then it produces warning.

[Commit ID commit df545d1cd01aab3ba3f687d5423e6c3687b069d8
mfd: palmas: Provide irq flags through DT/platform data]

So from DT we can not really set the IRQ_TYPE_LEVEL_LOW as irq flag.

Re: [PATCH] pinctrl: max77620: Use define directive for max77620_pinconf_param values

[Laxman Dewangan]

On Friday 09 November 2018 02:31 PM, Linus Walleij wrote:

On Thu, Nov 1, 2018 at 1:51 AM Nathan Chancellor
 wrote:


Clang warns when one enumerated type is implicitly converted to another:

drivers/pinctrl/pinctrl-max77620.c:56:12: warning: implicit conversion
from enumeration type 'enum max77620_pinconf_param' to different
enumeration type 'enum pin_config_param' [-Wenum-conversion]
 .param = MAX77620_ACTIVE_FPS_SOURCE,
  ^~

It is expected that pinctrl drivers can extend pin_config_param because
of the gap between PIN_CONFIG_END and PIN_CONFIG_MAX so this conversion
isn't an issue. Most drivers that take advantage of this define the
PIN_CONFIG variables as constants, rather than enumerated values. Do the
same thing here so that Clang no longer warns.

Link: https://github.com/ClangBuiltLinux/linux/issues/139
Signed-off-by: Nathan Chancellor 

Patch tentatively applied.

This seems to be the direction we need to be going with a lot
of CLANG business.

Laxman: you weren't CCed, so tell us if you dislike it for some
reason.

Looked changes and it is great. MISRA-C also happy with macros instead 
of enum.


Acked-by: Laxman Dewangan 

Re: [PATCH] pinctrl: max77620: Use define directive for max77620_pinconf_param values

[Laxman Dewangan]

On Friday 09 November 2018 02:31 PM, Linus Walleij wrote:

On Thu, Nov 1, 2018 at 1:51 AM Nathan Chancellor
 wrote:


Clang warns when one enumerated type is implicitly converted to another:

drivers/pinctrl/pinctrl-max77620.c:56:12: warning: implicit conversion
from enumeration type 'enum max77620_pinconf_param' to different
enumeration type 'enum pin_config_param' [-Wenum-conversion]
 .param = MAX77620_ACTIVE_FPS_SOURCE,
  ^~

It is expected that pinctrl drivers can extend pin_config_param because
of the gap between PIN_CONFIG_END and PIN_CONFIG_MAX so this conversion
isn't an issue. Most drivers that take advantage of this define the
PIN_CONFIG variables as constants, rather than enumerated values. Do the
same thing here so that Clang no longer warns.

Link: https://github.com/ClangBuiltLinux/linux/issues/139
Signed-off-by: Nathan Chancellor 

Patch tentatively applied.

This seems to be the direction we need to be going with a lot
of CLANG business.

Laxman: you weren't CCed, so tell us if you dislike it for some
reason.

Looked changes and it is great. MISRA-C also happy with macros instead 
of enum.


Acked-by: Laxman Dewangan 

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Laxman Dewangan]

On Tuesday 29 May 2018 11:36 PM, Wolfram Sang wrote:

Our I2C driver is based on the interrupt. So we have converted the
suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the
transfer when system interrupt disabled in downstream.
   SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume)

In shutdown path, where interrupt disabled and still need i2c, we use the
bit-bang method via GPIO for i2c transfer.

In the current upstream kernel suspend/resume can't be simply moved to the
'noirq' stage because resume invokes tegra_i2c_init() which uses runtime PM and
that doesn't work with the IRQ's being disabled. But things do not work even
with the tegra_i2c_init() changed to work with the disabled IRQ's, like I wrote
above the I2C transfer fails (due to timeout) and a "fix" for that failure was
to remove reset_control_assert/deassert from the tegra_i2c_init(). So I'd go for
a complete suspend/resume removal for now as it is causes problem.

Laxman, are you convinced or do you have still objections?

Fine with me. Please add my Ack

Acked-by: Laxman Dewangan 

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Laxman Dewangan]

On Tuesday 29 May 2018 11:36 PM, Wolfram Sang wrote:

Our I2C driver is based on the interrupt. So we have converted the
suspend/resume to suspend_noirq and reseume_noirq so that we will not allow the
transfer when system interrupt disabled in downstream.
   SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, tegra_i2c_resume)

In shutdown path, where interrupt disabled and still need i2c, we use the
bit-bang method via GPIO for i2c transfer.

In the current upstream kernel suspend/resume can't be simply moved to the
'noirq' stage because resume invokes tegra_i2c_init() which uses runtime PM and
that doesn't work with the IRQ's being disabled. But things do not work even
with the tegra_i2c_init() changed to work with the disabled IRQ's, like I wrote
above the I2C transfer fails (due to timeout) and a "fix" for that failure was
to remove reset_control_assert/deassert from the tegra_i2c_init(). So I'd go for
a complete suspend/resume removal for now as it is causes problem.

Laxman, are you convinced or do you have still objections?

Fine with me. Please add my Ack

Acked-by: Laxman Dewangan 

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Laxman Dewangan]

On Monday 14 May 2018 05:29 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote:

Nothing prevents I2C clients to access I2C while Tegra's driver is being
suspended, this results in -EBUSY error returned to the clients and that
may have unfortunate consequences. In particular this causes problems
for the TPS6586x MFD driver which emits hundreds of "failed to read
interrupt status" error messages on resume from suspend. This happens if
TPS6586X is used to wake system from suspend by the expired RTC alarm
timer because TPS6586X is an I2C device driver and its IRQ handler reads
the status register while Tegra's I2C driver is suspended, i.e. just after
kernel enabled IRQ's during of resume-from-suspend process.

Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which
performs HW reset. That seems was also not entirely correct because moving
tegra_i2c_resume to an earlier stage of resume-from-suspend process causes
I2C transfer to fail in the case of TPS6586X. It is fine to remove the
HW-reinitialization for now because it should be only needed in a case of
using lowest power-mode during suspend, which upstream kernel doesn't
support.

Signed-off-by: Dmitry Osipenko 
Cc: 
---
  drivers/i2c/busses/i2c-tegra.c | 33 -
  1 file changed, 33 deletions(-)

Shardar, Laxman, any thoughts on this? The is_suspended thing looks to
me like a workaround of some sort that may not be needed if clients have
proper suspend/resume implementations. Even without suspend/resume
support in client drivers, the driver core should resume devices in the
right order (I2C adapter before any of the clients), so I don't see any
cases where the is_suspended logic would be useful.



Our I2C driver is based on the interrupt. So we have converted the 
suspend/resume to suspend_noirq and reseume_noirq so that we will not 
allow the transfer when system interrupt disabled in downstream.
  SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, 
tegra_i2c_resume)


In shutdown path, where interrupt disabled and still need i2c, we use 
the bit-bang method via GPIO for i2c transfer.

Re: [PATCH v1] i2c: tegra: Remove suspend-resume

[Laxman Dewangan]

On Monday 14 May 2018 05:29 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Mon, May 14, 2018 at 12:13:47AM +0300, Dmitry Osipenko wrote:

Nothing prevents I2C clients to access I2C while Tegra's driver is being
suspended, this results in -EBUSY error returned to the clients and that
may have unfortunate consequences. In particular this causes problems
for the TPS6586x MFD driver which emits hundreds of "failed to read
interrupt status" error messages on resume from suspend. This happens if
TPS6586X is used to wake system from suspend by the expired RTC alarm
timer because TPS6586X is an I2C device driver and its IRQ handler reads
the status register while Tegra's I2C driver is suspended, i.e. just after
kernel enabled IRQ's during of resume-from-suspend process.

Note that the removed tegra_i2c_resume() invoked tegra_i2c_init() which
performs HW reset. That seems was also not entirely correct because moving
tegra_i2c_resume to an earlier stage of resume-from-suspend process causes
I2C transfer to fail in the case of TPS6586X. It is fine to remove the
HW-reinitialization for now because it should be only needed in a case of
using lowest power-mode during suspend, which upstream kernel doesn't
support.

Signed-off-by: Dmitry Osipenko 
Cc: 
---
  drivers/i2c/busses/i2c-tegra.c | 33 -
  1 file changed, 33 deletions(-)

Shardar, Laxman, any thoughts on this? The is_suspended thing looks to
me like a workaround of some sort that may not be needed if clients have
proper suspend/resume implementations. Even without suspend/resume
support in client drivers, the driver core should resume devices in the
right order (I2C adapter before any of the clients), so I don't see any
cases where the is_suspended logic would be useful.



Our I2C driver is based on the interrupt. So we have converted the 
suspend/resume to suspend_noirq and reseume_noirq so that we will not 
allow the transfer when system interrupt disabled in downstream.
  SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_i2c_suspend, 
tegra_i2c_resume)


In shutdown path, where interrupt disabled and still need i2c, we use 
the bit-bang method via GPIO for i2c transfer.

Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

[Laxman Dewangan]

On Thursday 08 March 2018 08:01 PM, Guenter Roeck wrote:

On 03/07/2018 10:06 PM, Laxman Dewangan wrote:



The RPM is measured speed via PWM signal capture  which is output 
from fan.

So should we have the fan[1..n]_output_rpm?



No. I hear you clearly that you for some reason dislike fan[1..n]_input.
While ABIs are not always to our liking, that doesn't mean that we get
to change them at our whim. If that is not acceptable for you, I can't
help you. And you can't change inX_input to inX_voltage either, sorry.


My opinion is only to not use "input" as this is not really the input to 
fan.

Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

[Laxman Dewangan]

On Thursday 08 March 2018 08:01 PM, Guenter Roeck wrote:

On 03/07/2018 10:06 PM, Laxman Dewangan wrote:



The RPM is measured speed via PWM signal capture  which is output 
from fan.

So should we have the fan[1..n]_output_rpm?



No. I hear you clearly that you for some reason dislike fan[1..n]_input.
While ABIs are not always to our liking, that doesn't mean that we get
to change them at our whim. If that is not acceptable for you, I can't
help you. And you can't change inX_input to inX_voltage either, sorry.


My opinion is only to not use "input" as this is not really the input to 
fan.

Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

[Laxman Dewangan]

On Wednesday 07 March 2018 07:50 PM, Guenter Roeck wrote:

On 03/07/2018 01:47 AM, Rajkumar Rampelli wrote:




While I am not opposed to ABI changes, the merits of those would 
need to be

discussed on the mailing list. But replacing "fan1_input" with "rpm" is
not an acceptable ABI change, even if it may measure something that 
turns

but isn't a fan.

If this _is_ in fact supposed to be used for something else but 
fans, we
would have to discuss what that might be, and if hwmon is the 
appropriate
subsystem to measure and report it. This does to some degree lead 
back to
my concern of having the "fan" part of this patch series in the pwm 
core.

I am still not sure if that makes sense.

Thanks,
Guenter
I am planning to add tachometer support in pwm-fan.c driver 
(drivers/hwmon/) instead of adding new generic-pwm-tachometer.c 
driver. Measuring RPM value will be done in pwm-fan driver itself 
using pwm capture feature and will add new sysfs attributes under 
this driver to report rpm value of fan.


There is an existing attribute to report the RPM of fans. It is called 
fan[1..n]_input.


"replacing "fan1_input" with "rpm" is not an acceptable ABI change"

Preemptive NACK.


The RPM is measured speed via PWM signal capture  which is output from fan.
So should we have the fan[1..n]_output_rpm?

Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

[Laxman Dewangan]

On Wednesday 07 March 2018 07:50 PM, Guenter Roeck wrote:

On 03/07/2018 01:47 AM, Rajkumar Rampelli wrote:




While I am not opposed to ABI changes, the merits of those would 
need to be

discussed on the mailing list. But replacing "fan1_input" with "rpm" is
not an acceptable ABI change, even if it may measure something that 
turns

but isn't a fan.

If this _is_ in fact supposed to be used for something else but 
fans, we
would have to discuss what that might be, and if hwmon is the 
appropriate
subsystem to measure and report it. This does to some degree lead 
back to
my concern of having the "fan" part of this patch series in the pwm 
core.

I am still not sure if that makes sense.

Thanks,
Guenter
I am planning to add tachometer support in pwm-fan.c driver 
(drivers/hwmon/) instead of adding new generic-pwm-tachometer.c 
driver. Measuring RPM value will be done in pwm-fan driver itself 
using pwm capture feature and will add new sysfs attributes under 
this driver to report rpm value of fan.


There is an existing attribute to report the RPM of fans. It is called 
fan[1..n]_input.


"replacing "fan1_input" with "rpm" is not an acceptable ABI change"

Preemptive NACK.


The RPM is measured speed via PWM signal capture  which is output from fan.
So should we have the fan[1..n]_output_rpm?

Re: [PATCH] spi: tegra20-slink: use true and false for boolean values

[Laxman Dewangan]

On Tuesday 06 March 2018 05:23 AM, Gustavo A. R. Silva wrote:

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>



Acked-by: Laxman Dewangan <ldewan...@nvidia.com>

Re: [PATCH] spi: tegra20-slink: use true and false for boolean values

[Laxman Dewangan]

On Tuesday 06 March 2018 05:23 AM, Gustavo A. R. Silva wrote:

Assign true or false to boolean variables instead of an integer value.

This issue was detected with the help of Coccinelle.

Signed-off-by: Gustavo A. R. Silva 



Acked-by: Laxman Dewangan 

Re: [PATCH v3 01/11] regulator: core: add API to get voltage constraints

[Laxman Dewangan]

On Wednesday 07 February 2018 09:07 PM, Mark Brown wrote:

On Wed, Feb 07, 2018 at 05:20:45PM +0200, Peter De Schrijver wrote:

On Wed, Feb 07, 2018 at 03:01:55PM +, Mark Brown wrote:

I can't really tell what you're saying here.  If the driver needs to
know if it can set the a given voltage there's already an API for doing
that as I said.  If you're trying to convey this minimum and maximum
voltage via the constraints that sounds like an abuse of the constraints.

No, what I want is the voltage which the regulator will output for a given
regulator_set_voltage request taking constraints, regulator step etc into
account.

Knowing the range of the constaints is going to tell you nothing useful
about that, it has zero information on steps or anything.  The way to
find out what voltages can be set is to enumerate the voltages that can
be set through the existing API and then if you want to set a specific
voltage that you've confirmed is available you can set exactly that
voltage via the normal voltage setting interface, no need to provide a
range.


Hi,
I added this API in downstream for the purpose of finding whether rail 
output can be changed or not(fixed) based on constraints.
If min and max is same then it can not change. I used this information 
to set the pad voltage of SOC automatically.


I dont know other usage when I added this API. May be Peter is needed here.

Re: [PATCH v3 01/11] regulator: core: add API to get voltage constraints

[Laxman Dewangan]

On Wednesday 07 February 2018 09:07 PM, Mark Brown wrote:

On Wed, Feb 07, 2018 at 05:20:45PM +0200, Peter De Schrijver wrote:

On Wed, Feb 07, 2018 at 03:01:55PM +, Mark Brown wrote:

I can't really tell what you're saying here.  If the driver needs to
know if it can set the a given voltage there's already an API for doing
that as I said.  If you're trying to convey this minimum and maximum
voltage via the constraints that sounds like an abuse of the constraints.

No, what I want is the voltage which the regulator will output for a given
regulator_set_voltage request taking constraints, regulator step etc into
account.

Knowing the range of the constaints is going to tell you nothing useful
about that, it has zero information on steps or anything.  The way to
find out what voltages can be set is to enumerate the voltages that can
be set through the existing API and then if you want to set a specific
voltage that you've confirmed is available you can set exactly that
voltage via the normal voltage setting interface, no need to provide a
range.


Hi,
I added this API in downstream for the purpose of finding whether rail 
output can be changed or not(fixed) based on constraints.
If min and max is same then it can not change. I used this information 
to set the pad voltage of SOC automatically.


I dont know other usage when I added this API. May be Peter is needed here.

Re: [PATCH 1/2] serial: tegra: Delete an error message for a failed memory allocation in tegra_uart_probe()

[Laxman Dewangan]

On Friday 08 December 2017 01:49 AM, SF Markus Elfring wrote:

From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Thu, 7 Dec 2017 21:00:05 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.



Acked-by: Laxman Dewangan <ldewan...@nvidia.com>

Re: [PATCH 1/2] serial: tegra: Delete an error message for a failed memory allocation in tegra_uart_probe()

[Laxman Dewangan]

On Friday 08 December 2017 01:49 AM, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Thu, 7 Dec 2017 21:00:05 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.



Acked-by: Laxman Dewangan 

Re: [PATCH 2/2] serial: tegra: Fix a typo in a comment line

[Laxman Dewangan]

On Friday 08 December 2017 01:51 AM, SF Markus Elfring wrote:

From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Thu, 7 Dec 2017 21:06:25 +0100

Delete a duplicate character in a word of this description.



Acked-by: Laxman Dewangan <ldewan...@nvidia.com>

Re: [PATCH 2/2] serial: tegra: Fix a typo in a comment line

[Laxman Dewangan]

On Friday 08 December 2017 01:51 AM, SF Markus Elfring wrote:

From: Markus Elfring 
Date: Thu, 7 Dec 2017 21:06:25 +0100

Delete a duplicate character in a word of this description.



Acked-by: Laxman Dewangan 

Re: [PATCH V2] thermal/drivers/generic-iio-adc: Switch tz request to devm version

[Laxman Dewangan]

On Friday 08 September 2017 12:31 PM, Daniel Lezcano wrote:

Everything mentionned here:
  https://lkml.org/lkml/2016/4/20/850

This driver was added before the devm_iio_channel_get() function version was
merged. The sensor should be released before the iio channel, thus we had to
use the non-devm version of thermal_zone_of_sensor_register().

Now the devm_iio_channel_get() is available, do the corresponding change in
this driver and remove gadc_thermal_remove().




Acked-by: Laxman Dewangan <ldewan...@nvidia.com>

Thanks,
Laxman

Re: [PATCH V2] thermal/drivers/generic-iio-adc: Switch tz request to devm version

[Laxman Dewangan]

On Friday 08 September 2017 12:31 PM, Daniel Lezcano wrote:

Everything mentionned here:
  https://lkml.org/lkml/2016/4/20/850

This driver was added before the devm_iio_channel_get() function version was
merged. The sensor should be released before the iio channel, thus we had to
use the non-devm version of thermal_zone_of_sensor_register().

Now the devm_iio_channel_get() is available, do the corresponding change in
this driver and remove gadc_thermal_remove().




Acked-by: Laxman Dewangan 

Thanks,
Laxman

Re: [PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high

[Laxman Dewangan]

On Wednesday 19 July 2017 06:55 PM, Johan Hovold wrote:

On Fri, Apr 07, 2017 at 12:25:49PM +0200, Linus Walleij wrote:

On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewan...@nvidia.com> wrote:


Currently, the GPIO interface is said to Open Drain if it is Single
Ended and active LOW. Similarly, it is said as Open Source if it is
Single Ended and active HIGH.

The active HIGH/LOW is used in the interface for setting the pin
state to HIGH or LOW when enabling/disabling the interface.

In Open Drain interface, pin is set to HIGH by putting pin in
high impedance and LOW by driving to the LOW.

In Open Source interface, pin is set to HIGH by driving pin to
HIGH and set to LOW by putting pin in high impedance.

With above, the Open Drain/Source is unrelated to the active LOW/HIGH
in interface. There is interface where the enable/disable of interface
is ether active LOW or HIGH but it is Open Drain type.

Hence decouple the Open Drain with Single Ended + Active LOW and
Open Source with Single Ended + Active HIGH.

Adding different flag for the Open Drain/Open Source which is valid
only when Single ended flag is enabled.

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

Patch applied.

Good that you found this and fixed it before someone git hurt.

Well, while decoupling single-endedness from polarity was the right
thing to do, this change did actually break the DT binary interface.

If you have an old compiled dtb whose source used GPIO_OPEN_DRAIN, you
now instead get *open-source* behaviour on 4.12:

GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW

=> active-low, but *open source*

while if you recompile that source against 4.12 you do get the expected
open-drain behaviour, but now with inverted polarity:

GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN

=> open drain, but *active high*

requiring the device tree to be updated by specifying

(GPIO_OPEN_DRAIN | GPIO_ACTIVE_LOW)

I guess the latter is fine, even if it is likely to amount to a fair bit
of debugging world wide.

Perhaps all this can still be avoided by adding further flags and
deprecating others before people start migrating to 4.12 (after all,
GPIO_OPEN_DRAIN has been around since 4.4 even if there are no in-kernel
users).

Or we accept the binary interface breakage -- it probably is pretty rare
that people update the kernel without updating the dtb. I can just
update the dts on the system that broke for me, and hopefully anyone
debugging this issue while updating to 4.12 will find this mail quickly.



Yes, it breaks the older DTS with new kernel. However,  this point was 
discussed before sending patch. As there was no user in the mainline DTs 
for these macros, we made change.

Re: [PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high

[Laxman Dewangan]

On Wednesday 19 July 2017 06:55 PM, Johan Hovold wrote:

On Fri, Apr 07, 2017 at 12:25:49PM +0200, Linus Walleij wrote:

On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan  wrote:


Currently, the GPIO interface is said to Open Drain if it is Single
Ended and active LOW. Similarly, it is said as Open Source if it is
Single Ended and active HIGH.

The active HIGH/LOW is used in the interface for setting the pin
state to HIGH or LOW when enabling/disabling the interface.

In Open Drain interface, pin is set to HIGH by putting pin in
high impedance and LOW by driving to the LOW.

In Open Source interface, pin is set to HIGH by driving pin to
HIGH and set to LOW by putting pin in high impedance.

With above, the Open Drain/Source is unrelated to the active LOW/HIGH
in interface. There is interface where the enable/disable of interface
is ether active LOW or HIGH but it is Open Drain type.

Hence decouple the Open Drain with Single Ended + Active LOW and
Open Source with Single Ended + Active HIGH.

Adding different flag for the Open Drain/Open Source which is valid
only when Single ended flag is enabled.

Signed-off-by: Laxman Dewangan 

Patch applied.

Good that you found this and fixed it before someone git hurt.

Well, while decoupling single-endedness from polarity was the right
thing to do, this change did actually break the DT binary interface.

If you have an old compiled dtb whose source used GPIO_OPEN_DRAIN, you
now instead get *open-source* behaviour on 4.12:

GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW

=> active-low, but *open source*

while if you recompile that source against 4.12 you do get the expected
open-drain behaviour, but now with inverted polarity:

GPIO_OPEN_DRAIN = GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN

=> open drain, but *active high*

requiring the device tree to be updated by specifying

(GPIO_OPEN_DRAIN | GPIO_ACTIVE_LOW)

I guess the latter is fine, even if it is likely to amount to a fair bit
of debugging world wide.

Perhaps all this can still be avoided by adding further flags and
deprecating others before people start migrating to 4.12 (after all,
GPIO_OPEN_DRAIN has been around since 4.4 even if there are no in-kernel
users).

Or we accept the binary interface breakage -- it probably is pretty rare
that people update the kernel without updating the dtb. I can just
update the dts on the system that broke for me, and hopefully anyone
debugging this issue while updating to 4.12 will find this mail quickly.



Yes, it breaks the older DTS with new kernel. However,  this point was 
discussed before sending patch. As there was no user in the mainline DTs 
for these macros, we made change.

Re: [PATCH V2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

On Tuesday 02 May 2017 11:13 PM, Laxman Dewangan wrote:


On Tuesday 02 May 2017 08:53 PM, Jon Hunter wrote:

On 02/05/17 15:05, Laxman Dewangan wrote:

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

 From HW team:
Before Tegra186, it is 38.4MHz.
In Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

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

---
Changes from V1:
- Set the 48MHz maximum frequency for Tegra210 and earlier.

I think that your changelog needs to be updated, because it still says
38.4MHz and not 48MHz.



Oops, thanks for pointing.

Thierry,
Do I need to recycle the patch or can be corrected when applying?
If there is any further review comment in code then I will recycle and 
correct it.


Thierry,
Can you please review?

Re: [PATCH V2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

On Tuesday 02 May 2017 11:13 PM, Laxman Dewangan wrote:


On Tuesday 02 May 2017 08:53 PM, Jon Hunter wrote:

On 02/05/17 15:05, Laxman Dewangan wrote:

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

 From HW team:
Before Tegra186, it is 38.4MHz.
In Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

Signed-off-by: Laxman Dewangan 

---
Changes from V1:
- Set the 48MHz maximum frequency for Tegra210 and earlier.

I think that your changelog needs to be updated, because it still says
38.4MHz and not 48MHz.



Oops, thanks for pointing.

Thierry,
Do I need to recycle the patch or can be corrected when applying?
If there is any further review comment in code then I will recycle and 
correct it.


Thierry,
Can you please review?

Re: [PATCH V2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

On Tuesday 02 May 2017 08:53 PM, Jon Hunter wrote:

On 02/05/17 15:05, Laxman Dewangan wrote:

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

 From HW team:
Before Tegra186, it is 38.4MHz.
In Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

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

---
Changes from V1:
- Set the 48MHz maximum frequency for Tegra210 and earlier.

I think that your changelog needs to be updated, because it still says
38.4MHz and not 48MHz.



Oops, thanks for pointing.

Thierry,
Do I need to recycle the patch or can be corrected when applying?
If there is any further review comment in code then I will recycle and 
correct it.

Re: [PATCH V2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

On Tuesday 02 May 2017 08:53 PM, Jon Hunter wrote:

On 02/05/17 15:05, Laxman Dewangan wrote:

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

 From HW team:
Before Tegra186, it is 38.4MHz.
In Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

Signed-off-by: Laxman Dewangan 

---
Changes from V1:
- Set the 48MHz maximum frequency for Tegra210 and earlier.

I think that your changelog needs to be updated, because it still says
38.4MHz and not 48MHz.



Oops, thanks for pointing.

Thierry,
Do I need to recycle the patch or can be corrected when applying?
If there is any further review comment in code then I will recycle and 
correct it.

[PATCH V2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

>From HW team:
Before Tegra186, it is 38.4MHz.
In Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

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

---
Changes from V1:
- Set the 48MHz maximum frequency for Tegra210 and earlier.
- Set the maximum frequency unconditionally as per V1 review comment.
---
 drivers/pwm/pwm-tegra.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 8c6ed55..e9b33f0 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -41,6 +41,9 @@
 
 struct tegra_pwm_soc {
unsigned int num_channels;
+
+   /* Maximum IP frequency for given SoCs */
+   unsigned long max_frequency;
 };
 
 struct tegra_pwm_chip {
@@ -201,7 +204,18 @@ static int tegra_pwm_probe(struct platform_device *pdev)
if (IS_ERR(pwm->clk))
return PTR_ERR(pwm->clk);
 
-   /* Read PWM clock rate from source */
+   /* Set maximum frequency of the IP */
+   ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to set max frequency: %d\n", ret);
+   return ret;
+   }
+
+   /*
+* The requested and configured frequency may differ due to
+* clock register resolutions. Get the configured frequency
+* so that PWM period can be calculated more accurately.
+*/
pwm->clk_rate = clk_get_rate(pwm->clk);
 
pwm->rst = devm_reset_control_get(>dev, "pwm");
@@ -273,10 +287,12 @@ static int tegra_pwm_resume(struct device *dev)
 
 static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
+   .max_frequency = 4800UL,
 };
 
 static const struct tegra_pwm_soc tegra186_pwm_soc = {
.num_channels = 1,
+   .max_frequency = 10200UL,
 };
 
 static const struct of_device_id tegra_pwm_of_match[] = {
-- 
2.1.4

[PATCH V2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

>From HW team:
Before Tegra186, it is 38.4MHz.
In Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

Signed-off-by: Laxman Dewangan 

---
Changes from V1:
- Set the 48MHz maximum frequency for Tegra210 and earlier.
- Set the maximum frequency unconditionally as per V1 review comment.
---
 drivers/pwm/pwm-tegra.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 8c6ed55..e9b33f0 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -41,6 +41,9 @@
 
 struct tegra_pwm_soc {
unsigned int num_channels;
+
+   /* Maximum IP frequency for given SoCs */
+   unsigned long max_frequency;
 };
 
 struct tegra_pwm_chip {
@@ -201,7 +204,18 @@ static int tegra_pwm_probe(struct platform_device *pdev)
if (IS_ERR(pwm->clk))
return PTR_ERR(pwm->clk);
 
-   /* Read PWM clock rate from source */
+   /* Set maximum frequency of the IP */
+   ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to set max frequency: %d\n", ret);
+   return ret;
+   }
+
+   /*
+* The requested and configured frequency may differ due to
+* clock register resolutions. Get the configured frequency
+* so that PWM period can be calculated more accurately.
+*/
pwm->clk_rate = clk_get_rate(pwm->clk);
 
pwm->rst = devm_reset_control_get(>dev, "pwm");
@@ -273,10 +287,12 @@ static int tegra_pwm_resume(struct device *dev)
 
 static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
+   .max_frequency = 4800UL,
 };
 
 static const struct tegra_pwm_soc tegra186_pwm_soc = {
.num_channels = 1,
+   .max_frequency = 10200UL,
 };
 
 static const struct of_device_id tegra_pwm_of_match[] = {
-- 
2.1.4

Re: [PATCH v2 1/2] regulator: DT: Add properties for asymmetric settling times

[Laxman Dewangan]

On Tuesday 02 May 2017 12:07 AM, Matthias Kaehlcke wrote:

Some regulators have different settling times for voltage increases and
decreases. Add DT properties to define separate settling times for up-
and downward voltage changes.

Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---


Acked-by: Laxman Dewangan <ldewan...@nvidia.com>

Re: [PATCH v2 1/2] regulator: DT: Add properties for asymmetric settling times

[Laxman Dewangan]

On Tuesday 02 May 2017 12:07 AM, Matthias Kaehlcke wrote:

Some regulators have different settling times for voltage increases and
decreases. Add DT properties to define separate settling times for up-
and downward voltage changes.

Signed-off-by: Matthias Kaehlcke 
---


Acked-by: Laxman Dewangan 

Re: [PATCH v2 2/2] regulator: Allow for asymmetric settling times

[Laxman Dewangan]

On Tuesday 02 May 2017 12:07 AM, Matthias Kaehlcke wrote:

Some regulators have different settling times for voltage increases and
decreases. To avoid a time penalty on the faster transition allow for
different settings for up- and downward transitions.

Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
---


Acked-by: Laxman Dewangan <ldewan...@nvidia.com>

Re: [PATCH v2 2/2] regulator: Allow for asymmetric settling times

[Laxman Dewangan]

On Tuesday 02 May 2017 12:07 AM, Matthias Kaehlcke wrote:

Some regulators have different settling times for voltage increases and
decreases. To avoid a time penalty on the faster transition allow for
different settings for up- and downward transitions.

Signed-off-by: Matthias Kaehlcke 
---


Acked-by: Laxman Dewangan 

Re: [PATCH] regulator: Allow for asymmetric settling times

[Laxman Dewangan]

On Saturday 29 April 2017 05:36 AM, Matthias Kaehlcke wrote:

Some regulators have different settling times for voltage increases and
decreases. To avoid a time penalty on the faster transition extend the
settling time property to allow for different settings for upward and
downward transitions.

Signed-off-by: Matthias Kaehlcke 
---
Dependencies (from broonie/regulator topic/settle):
  - regulator: DT: Add settling time property for non-linear voltage change
  - regulator: Add settling time for non-linear voltage transition

Sorry for not bringing this up during the review of the 'settling time'
patch, I just came across it when looking to revive a similar change I
sent out some time ago (https://patchwork.kernel.org/patch/9332051/).

  Documentation/devicetree/bindings/regulator/regulator.txt | 11 ---
  drivers/regulator/core.c  |  8 ++--
  drivers/regulator/of_regulator.c  |  9 +++--
  include/linux/regulator/machine.h |  9 ++---
  4 files changed, 27 insertions(+), 10 deletions(-)

I think DT change and code change go in different patches.


diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt 
b/Documentation/devicetree/bindings/regulator/regulator.txt
index d18edb075e1c..f21fead1c802 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -21,9 +21,14 @@ Optional properties:
design requires. This property describes the total system ramp time
required due to the combination of internal ramping of the regulator itself,
and board design issues such as trace capacitance and load on the supply.
-- regulator-settling-time-us: Settling time, in microseconds, for voltage
-  change if regulator have the constant time for any level voltage change.
-  This is useful when regulator have exponential voltage change.
+- regulator-settling-time-up-us: Settling time, in microseconds, for voltage
+  increase if the regulator needs a constant time to settle after voltage
+  increases of any level. This is useful for regulators with exponential
+  voltage changes.
+- regulator-settling-time-down-us: Settling time, in microseconds, for voltage
+  decrease if the regulator needs a constant time to settle after voltage
+  decreases of any level. This is useful for regulators with exponential
+  voltage changes.


Can we have regulator-settling-time-us also so if it is there then 
up/down same.

If up/down different then separate properties can be used.


Also in driver, if up/dn are not provided and only 
regulator-settling-time-us is provided then up/dn can take value from 
this property.

Re: [PATCH] regulator: Allow for asymmetric settling times

[Laxman Dewangan]

On Saturday 29 April 2017 05:36 AM, Matthias Kaehlcke wrote:

Some regulators have different settling times for voltage increases and
decreases. To avoid a time penalty on the faster transition extend the
settling time property to allow for different settings for upward and
downward transitions.

Signed-off-by: Matthias Kaehlcke 
---
Dependencies (from broonie/regulator topic/settle):
  - regulator: DT: Add settling time property for non-linear voltage change
  - regulator: Add settling time for non-linear voltage transition

Sorry for not bringing this up during the review of the 'settling time'
patch, I just came across it when looking to revive a similar change I
sent out some time ago (https://patchwork.kernel.org/patch/9332051/).

  Documentation/devicetree/bindings/regulator/regulator.txt | 11 ---
  drivers/regulator/core.c  |  8 ++--
  drivers/regulator/of_regulator.c  |  9 +++--
  include/linux/regulator/machine.h |  9 ++---
  4 files changed, 27 insertions(+), 10 deletions(-)

I think DT change and code change go in different patches.


diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt 
b/Documentation/devicetree/bindings/regulator/regulator.txt
index d18edb075e1c..f21fead1c802 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -21,9 +21,14 @@ Optional properties:
design requires. This property describes the total system ramp time
required due to the combination of internal ramping of the regulator itself,
and board design issues such as trace capacitance and load on the supply.
-- regulator-settling-time-us: Settling time, in microseconds, for voltage
-  change if regulator have the constant time for any level voltage change.
-  This is useful when regulator have exponential voltage change.
+- regulator-settling-time-up-us: Settling time, in microseconds, for voltage
+  increase if the regulator needs a constant time to settle after voltage
+  increases of any level. This is useful for regulators with exponential
+  voltage changes.
+- regulator-settling-time-down-us: Settling time, in microseconds, for voltage
+  decrease if the regulator needs a constant time to settle after voltage
+  decreases of any level. This is useful for regulators with exponential
+  voltage changes.


Can we have regulator-settling-time-us also so if it is there then 
up/down same.

If up/down different then separate properties can be used.


Also in driver, if up/dn are not provided and only 
regulator-settling-time-us is provided then up/dn can take value from 
this property.

Re: [PATCH] regulator: tps65132: fix platform_no_drv_owner.cocci warnings

[Laxman Dewangan]

On Friday 14 April 2017 02:27 AM, kbuild test robot wrote:

drivers/regulator/tps65132-regulator.c:274:3-8: No need to set .owner here. The 
core will do it.

  Remove .owner field if calls are used which set it automatically

Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci

CC: Venkat Reddy Talla <vreddyta...@nvidia.com>
Signed-off-by: Fengguang Wu <fengguang...@intel.com>
---

LGTM,
Acked-by: Laxman Dewangan <ldewan...@nvidia.com>

Re: [PATCH] regulator: tps65132: fix platform_no_drv_owner.cocci warnings

[Laxman Dewangan]

On Friday 14 April 2017 02:27 AM, kbuild test robot wrote:

drivers/regulator/tps65132-regulator.c:274:3-8: No need to set .owner here. The 
core will do it.

  Remove .owner field if calls are used which set it automatically

Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci

CC: Venkat Reddy Talla 
Signed-off-by: Fengguang Wu 
---

LGTM,
Acked-by: Laxman Dewangan 

Re: [PATCH 2/2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

On Thursday 13 April 2017 08:57 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Thu, Apr 13, 2017 at 07:40:28PM +0530, Laxman Dewangan wrote:

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

 From HW team:
For Tegra210, it is 38.4MHz.
For Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
---
  drivers/pwm/pwm-tegra.c | 28 
  1 file changed, 28 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 8c6ed55..7016c08 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -41,6 +41,9 @@
  
  struct tegra_pwm_soc {

unsigned int num_channels;
+
+   /* Maximum IP frequency for given SoCs */
+   unsigned long max_frequency;
  };
  
  struct tegra_pwm_chip {

@@ -204,6 +207,24 @@ static int tegra_pwm_probe(struct platform_device *pdev)
/* Read PWM clock rate from source */
pwm->clk_rate = clk_get_rate(pwm->clk);
  
+	/* Make sure clock source freqeuncy must less than IP supported */

+   if (pwm->soc->max_frequency &&
+   (pwm->soc->max_frequency < pwm->clk_rate)) {
+   ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to set max frequency: %d\n",
+   ret);
+   return ret;
+   }
+
+   /*
+* The requested and configured frequency may differ due to
+* clock register resolutions. Get the configured frequency
+* so that PWM period can be calculated more accurately.
+*/
+pwm->clk_rate = clk_get_rate(pwm->clk);
+   }

Is there a reason to conditionalize this? Couldn't we simply set the
clock to the maximum frequency in all cases? Higher frequency means
higher precision, right?


I think higher precision is not related directly to maximum frequency. 
Precision will much depends on the perfect multiples between period and 
clock source frequency.


If some usecases needed the perfect periods then the clock source 
frequency can be set via clock init table for clock driver to achieve 
the perfect PWM period. on this case, we should not change it in the PWM 
driver.

PWM driver should only worry about to limit the maximum.




  So just something like this perhaps:

ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
if (ret < 0) {
...
}

pwm->clk_rate = clk_get_rate(pwm->clk);

That of course means that we'd need to define a maximum frequency for
SoCs prior to Tegra210. Any chance we can get at them?



Getting information for the older SoCs are little bit difficult. Let me 
try if possible from HW team.


Otherwise, What we can do now as we can make 38.4MHz till T210 and 102M 
for T186.




+
pwm->rst = devm_reset_control_get(>dev, "pwm");
if (IS_ERR(pwm->rst)) {
ret = PTR_ERR(pwm->rst);
@@ -275,12 +296,19 @@ static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
  };
  
+static const struct tegra_pwm_soc tegra210_pwm_soc = {

+   .num_channels = 4,
+   .max_frequency = 3840UL, /* 38.4MHz */
+};
+
  static const struct tegra_pwm_soc tegra186_pwm_soc = {
.num_channels = 1,
+   .max_frequency = 10200UL, /* 102MHz */

I don't think we need these comments, it's fairly obvious what
frequencies you're specifying there. =)

Thierry

* Unknown Key
* 0x7F3EB3A1

Re: [PATCH 2/2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

On Thursday 13 April 2017 08:57 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Thu, Apr 13, 2017 at 07:40:28PM +0530, Laxman Dewangan wrote:

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

 From HW team:
For Tegra210, it is 38.4MHz.
For Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

Signed-off-by: Laxman Dewangan 
---
  drivers/pwm/pwm-tegra.c | 28 
  1 file changed, 28 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 8c6ed55..7016c08 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -41,6 +41,9 @@
  
  struct tegra_pwm_soc {

unsigned int num_channels;
+
+   /* Maximum IP frequency for given SoCs */
+   unsigned long max_frequency;
  };
  
  struct tegra_pwm_chip {

@@ -204,6 +207,24 @@ static int tegra_pwm_probe(struct platform_device *pdev)
/* Read PWM clock rate from source */
pwm->clk_rate = clk_get_rate(pwm->clk);
  
+	/* Make sure clock source freqeuncy must less than IP supported */

+   if (pwm->soc->max_frequency &&
+   (pwm->soc->max_frequency < pwm->clk_rate)) {
+   ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to set max frequency: %d\n",
+   ret);
+   return ret;
+   }
+
+   /*
+* The requested and configured frequency may differ due to
+* clock register resolutions. Get the configured frequency
+* so that PWM period can be calculated more accurately.
+*/
+pwm->clk_rate = clk_get_rate(pwm->clk);
+   }

Is there a reason to conditionalize this? Couldn't we simply set the
clock to the maximum frequency in all cases? Higher frequency means
higher precision, right?


I think higher precision is not related directly to maximum frequency. 
Precision will much depends on the perfect multiples between period and 
clock source frequency.


If some usecases needed the perfect periods then the clock source 
frequency can be set via clock init table for clock driver to achieve 
the perfect PWM period. on this case, we should not change it in the PWM 
driver.

PWM driver should only worry about to limit the maximum.




  So just something like this perhaps:

ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
if (ret < 0) {
...
}

pwm->clk_rate = clk_get_rate(pwm->clk);

That of course means that we'd need to define a maximum frequency for
SoCs prior to Tegra210. Any chance we can get at them?



Getting information for the older SoCs are little bit difficult. Let me 
try if possible from HW team.


Otherwise, What we can do now as we can make 38.4MHz till T210 and 102M 
for T186.




+
pwm->rst = devm_reset_control_get(>dev, "pwm");
if (IS_ERR(pwm->rst)) {
ret = PTR_ERR(pwm->rst);
@@ -275,12 +296,19 @@ static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
  };
  
+static const struct tegra_pwm_soc tegra210_pwm_soc = {

+   .num_channels = 4,
+   .max_frequency = 3840UL, /* 38.4MHz */
+};
+
  static const struct tegra_pwm_soc tegra186_pwm_soc = {
.num_channels = 1,
+   .max_frequency = 10200UL, /* 102MHz */

I don't think we need these comments, it's fairly obvious what
frequencies you're specifying there. =)

Thierry

* Unknown Key
* 0x7F3EB3A1

[PATCH 2/2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

>From HW team:
For Tegra210, it is 38.4MHz.
For Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
---
 drivers/pwm/pwm-tegra.c | 28 
 1 file changed, 28 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 8c6ed55..7016c08 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -41,6 +41,9 @@
 
 struct tegra_pwm_soc {
unsigned int num_channels;
+
+   /* Maximum IP frequency for given SoCs */
+   unsigned long max_frequency;
 };
 
 struct tegra_pwm_chip {
@@ -204,6 +207,24 @@ static int tegra_pwm_probe(struct platform_device *pdev)
/* Read PWM clock rate from source */
pwm->clk_rate = clk_get_rate(pwm->clk);
 
+   /* Make sure clock source freqeuncy must less than IP supported */
+   if (pwm->soc->max_frequency &&
+   (pwm->soc->max_frequency < pwm->clk_rate)) {
+   ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to set max frequency: %d\n",
+   ret);
+   return ret;
+   }
+
+   /*
+* The requested and configured frequency may differ due to
+* clock register resolutions. Get the configured frequency
+* so that PWM period can be calculated more accurately.
+*/
+pwm->clk_rate = clk_get_rate(pwm->clk);
+   }
+
pwm->rst = devm_reset_control_get(>dev, "pwm");
if (IS_ERR(pwm->rst)) {
ret = PTR_ERR(pwm->rst);
@@ -275,12 +296,19 @@ static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
 };
 
+static const struct tegra_pwm_soc tegra210_pwm_soc = {
+   .num_channels = 4,
+   .max_frequency = 3840UL, /* 38.4MHz */
+};
+
 static const struct tegra_pwm_soc tegra186_pwm_soc = {
.num_channels = 1,
+   .max_frequency = 10200UL, /* 102MHz */
 };
 
 static const struct of_device_id tegra_pwm_of_match[] = {
{ .compatible = "nvidia,tegra20-pwm", .data = _pwm_soc },
+   { .compatible = "nvidia,tegra210-pwm", .data = _pwm_soc },
{ .compatible = "nvidia,tegra186-pwm", .data = _pwm_soc },
{ }
 };
-- 
2.1.4

[PATCH 2/2] pwm: tegra: Set maximum pwm clock source per SoC tapeout

[Laxman Dewangan]

The PWM hardware IP is taped-out with different maximum frequency
on different SoCs.

>From HW team:
For Tegra210, it is 38.4MHz.
For Tegra186, it is 102MHz.

Add support to limit the clock source frequency to the maximum IP
supported frequency. Provide these values via SoC chipdata.

Signed-off-by: Laxman Dewangan 
---
 drivers/pwm/pwm-tegra.c | 28 
 1 file changed, 28 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 8c6ed55..7016c08 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -41,6 +41,9 @@
 
 struct tegra_pwm_soc {
unsigned int num_channels;
+
+   /* Maximum IP frequency for given SoCs */
+   unsigned long max_frequency;
 };
 
 struct tegra_pwm_chip {
@@ -204,6 +207,24 @@ static int tegra_pwm_probe(struct platform_device *pdev)
/* Read PWM clock rate from source */
pwm->clk_rate = clk_get_rate(pwm->clk);
 
+   /* Make sure clock source freqeuncy must less than IP supported */
+   if (pwm->soc->max_frequency &&
+   (pwm->soc->max_frequency < pwm->clk_rate)) {
+   ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
+   if (ret < 0) {
+   dev_err(>dev, "Failed to set max frequency: %d\n",
+   ret);
+   return ret;
+   }
+
+   /*
+* The requested and configured frequency may differ due to
+* clock register resolutions. Get the configured frequency
+* so that PWM period can be calculated more accurately.
+*/
+pwm->clk_rate = clk_get_rate(pwm->clk);
+   }
+
pwm->rst = devm_reset_control_get(>dev, "pwm");
if (IS_ERR(pwm->rst)) {
ret = PTR_ERR(pwm->rst);
@@ -275,12 +296,19 @@ static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
 };
 
+static const struct tegra_pwm_soc tegra210_pwm_soc = {
+   .num_channels = 4,
+   .max_frequency = 3840UL, /* 38.4MHz */
+};
+
 static const struct tegra_pwm_soc tegra186_pwm_soc = {
.num_channels = 1,
+   .max_frequency = 10200UL, /* 102MHz */
 };
 
 static const struct of_device_id tegra_pwm_of_match[] = {
{ .compatible = "nvidia,tegra20-pwm", .data = _pwm_soc },
+   { .compatible = "nvidia,tegra210-pwm", .data = _pwm_soc },
{ .compatible = "nvidia,tegra186-pwm", .data = _pwm_soc },
{ }
 };
-- 
2.1.4

[PATCH 1/2] pwm: tegra: Read PWM clock source rate in driver init

[Laxman Dewangan]

It is required to know the PWM clock source frequency to
calculate the PWM period.

In driver, the clock source frequency of the PWM does not
get change and, hence, get the clock source frequency in
driver initi. Get this values later for period calculation
from pwm_config().

This will help in avoiding the clock call for getting clock rate
in the pwm_config() each time.

Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
---
 drivers/pwm/pwm-tegra.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index c040f87..8c6ed55 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -50,6 +50,8 @@ struct tegra_pwm_chip {
struct clk *clk;
struct reset_control*rst;
 
+   unsigned long clk_rate;
+
void __iomem *regs;
 
const struct tegra_pwm_soc *soc;
@@ -94,7 +96,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct 
pwm_device *pwm,
 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 * cycles at the PWM clock rate will take period_ns nanoseconds.
 */
-   rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
+   rate = pc->clk_rate >> PWM_DUTY_WIDTH;
 
/* Consider precision in PWM_SCALE_WIDTH rate calculation */
hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
@@ -199,6 +201,9 @@ static int tegra_pwm_probe(struct platform_device *pdev)
if (IS_ERR(pwm->clk))
return PTR_ERR(pwm->clk);
 
+   /* Read PWM clock rate from source */
+   pwm->clk_rate = clk_get_rate(pwm->clk);
+
pwm->rst = devm_reset_control_get(>dev, "pwm");
if (IS_ERR(pwm->rst)) {
ret = PTR_ERR(pwm->rst);
-- 
2.1.4

[PATCH 1/2] pwm: tegra: Read PWM clock source rate in driver init

[Laxman Dewangan]

It is required to know the PWM clock source frequency to
calculate the PWM period.

In driver, the clock source frequency of the PWM does not
get change and, hence, get the clock source frequency in
driver initi. Get this values later for period calculation
from pwm_config().

This will help in avoiding the clock call for getting clock rate
in the pwm_config() each time.

Signed-off-by: Laxman Dewangan 
---
 drivers/pwm/pwm-tegra.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index c040f87..8c6ed55 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -50,6 +50,8 @@ struct tegra_pwm_chip {
struct clk *clk;
struct reset_control*rst;
 
+   unsigned long clk_rate;
+
void __iomem *regs;
 
const struct tegra_pwm_soc *soc;
@@ -94,7 +96,7 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct 
pwm_device *pwm,
 * Compute the prescaler value for which (1 << PWM_DUTY_WIDTH)
 * cycles at the PWM clock rate will take period_ns nanoseconds.
 */
-   rate = clk_get_rate(pc->clk) >> PWM_DUTY_WIDTH;
+   rate = pc->clk_rate >> PWM_DUTY_WIDTH;
 
/* Consider precision in PWM_SCALE_WIDTH rate calculation */
hz = DIV_ROUND_CLOSEST_ULL(100ULL * NSEC_PER_SEC, period_ns);
@@ -199,6 +201,9 @@ static int tegra_pwm_probe(struct platform_device *pdev)
if (IS_ERR(pwm->clk))
return PTR_ERR(pwm->clk);
 
+   /* Read PWM clock rate from source */
+   pwm->clk_rate = clk_get_rate(pwm->clk);
+
pwm->rst = devm_reset_control_get(>dev, "pwm");
if (IS_ERR(pwm->rst)) {
ret = PTR_ERR(pwm->rst);
-- 
2.1.4

Re: [PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high

[Laxman Dewangan]

On Friday 07 April 2017 03:55 PM, Linus Walleij wrote:

On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan <ldewan...@nvidia.com> wrote:


Currently, the GPIO interface is said to Open Drain if it is Single
Ended and active LOW. Similarly, it is said as Open Source if it is
Single Ended and active HIGH.

The active HIGH/LOW is used in the interface for setting the pin
state to HIGH or LOW when enabling/disabling the interface.

In Open Drain interface, pin is set to HIGH by putting pin in
high impedance and LOW by driving to the LOW.

In Open Source interface, pin is set to HIGH by driving pin to
HIGH and set to LOW by putting pin in high impedance.

With above, the Open Drain/Source is unrelated to the active LOW/HIGH
in interface. There is interface where the enable/disable of interface
is ether active LOW or HIGH but it is Open Drain type.

Hence decouple the Open Drain with Single Ended + Active LOW and
Open Source with Single Ended + Active HIGH.

Adding different flag for the Open Drain/Open Source which is valid
only when Single ended flag is enabled.

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

Patch applied.

Good that you found this and fixed it before someone git hurt.


Thanks you very much.


Now we are going to use the descriptor based GPIO APIs as this is very 
powerful and passing this info from DT is very useful.

Re: [PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high

[Laxman Dewangan]

On Friday 07 April 2017 03:55 PM, Linus Walleij wrote:

On Thu, Apr 6, 2017 at 3:35 PM, Laxman Dewangan  wrote:


Currently, the GPIO interface is said to Open Drain if it is Single
Ended and active LOW. Similarly, it is said as Open Source if it is
Single Ended and active HIGH.

The active HIGH/LOW is used in the interface for setting the pin
state to HIGH or LOW when enabling/disabling the interface.

In Open Drain interface, pin is set to HIGH by putting pin in
high impedance and LOW by driving to the LOW.

In Open Source interface, pin is set to HIGH by driving pin to
HIGH and set to LOW by putting pin in high impedance.

With above, the Open Drain/Source is unrelated to the active LOW/HIGH
in interface. There is interface where the enable/disable of interface
is ether active LOW or HIGH but it is Open Drain type.

Hence decouple the Open Drain with Single Ended + Active LOW and
Open Source with Single Ended + Active HIGH.

Adding different flag for the Open Drain/Open Source which is valid
only when Single ended flag is enabled.

Signed-off-by: Laxman Dewangan 

Patch applied.

Good that you found this and fixed it before someone git hurt.


Thanks you very much.


Now we are going to use the descriptor based GPIO APIs as this is very 
powerful and passing this info from DT is very useful.

[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 <ldewan...@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 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -255,6 +256,18 @@ static int tegra_pwm_remove(struct platform_device *pdev)
return pwmchip_remove(>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 = _pwm_pm_ops,
},
.probe = tegra_pwm_probe,
.remove = tegra_pwm_remove,
-- 
2.1.4

[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 

---
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 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -255,6 +256,18 @@ static int tegra_pwm_remove(struct platform_device *pdev)
return pwmchip_remove(>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 = _pwm_pm_ops,
},
.probe = tegra_pwm_probe,
.remove = tegra_pwm_remove,
-- 
2.1.4

[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 <ldewan...@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 = <_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 
+
+   pinmux@78d4 {
+   pwm_active_state: pwm_active_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+
+   pwm_sleep_state: pwm_sleep_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+   };
+
+   pwm@7000a000 {
+   /* Mandatory PWM properties */
+   pinctrl-names = "default", "sleep";
+   pinctrl-0 = <_active_state>;
+   pinctrl-1 = <_sleep_state>;
+   };
-- 
2.1.4

[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 

---
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 = <_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 
+
+   pinmux@78d4 {
+   pwm_active_state: pwm_active_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+
+   pwm_sleep_state: pwm_sleep_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+   };
+
+   pwm@7000a000 {
+   /* Mandatory PWM properties */
+   pinctrl-names = "default", "sleep";
+   pinctrl-0 = <_active_state>;
+   pinctrl-1 = <_sleep_state>;
+   };
-- 
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
   = 10ul/16672000
   = 59 (59.98)
rate = (200K + 59/2)/59 = 3390

Based on new method:
hz = 5998
rate = DIV_ROUND_CLOSE(20*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 .

Signed-off-by: Laxman Dewangan <ldewan...@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

[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
   = 10ul/16672000
   = 59 (59.98)
rate = (200K + 59/2)/59 = 3390

Based on new method:
hz = 5998
rate = DIV_ROUND_CLOSE(20*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 .

Signed-off-by: Laxman Dewangan 

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

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

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

Re: [PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high

[Laxman Dewangan]

On Thursday 06 April 2017 09:40 PM, Andy Shevchenko wrote:

On Thu, Apr 6, 2017 at 4:35 PM, Laxman Dewangan <ldewan...@nvidia.com> wrote:

Currently, the GPIO interface is said to Open Drain if it is Single
Ended and active LOW. Similarly, it is said as Open Source if it is
Single Ended and active HIGH.

The active HIGH/LOW is used in the interface for setting the pin
state to HIGH or LOW when enabling/disabling the interface.

In Open Drain interface, pin is set to HIGH by putting pin in
high impedance and LOW by driving to the LOW.

In Open Source interface, pin is set to HIGH by driving pin to
HIGH and set to LOW by putting pin in high impedance.

With above, the Open Drain/Source is unrelated to the active LOW/HIGH
in interface. There is interface where the enable/disable of interface
is ether active LOW or HIGH but it is Open Drain type.

Hence decouple the Open Drain with Single Ended + Active LOW and
Open Source with Single Ended + Active HIGH.

Adding different flag for the Open Drain/Open Source which is valid
only when Single ended flag is enabled.
 if (single_ended) {
-   if (active_low)
+   if (open_drain)

This breaks ACPI case, right?



In acpi case, single_ended is not handled. It only handles the active LOW.

From code:


   bool active_low = false;
bool single_ended = false;
int ret;

if (!fwnode)
return ERR_PTR(-EINVAL);

if (is_of_node(fwnode)) {
enum of_gpio_flags flags;

desc = of_get_named_gpiod_flags(to_of_node(fwnode), 
propname,

index, );
if (!IS_ERR(desc)) {
active_low = flags & OF_GPIO_ACTIVE_LOW;
single_ended = flags & OF_GPIO_SINGLE_ENDED;
}
} else if (is_acpi_node(fwnode)) {
struct acpi_gpio_info info;

desc = acpi_node_get_gpiod(fwnode, propname, index, );
if (!IS_ERR(desc))
active_low = info.polarity == GPIO_ACTIVE_LOW;
}

Re: [PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high

[Laxman Dewangan]

On Thursday 06 April 2017 09:40 PM, Andy Shevchenko wrote:

On Thu, Apr 6, 2017 at 4:35 PM, Laxman Dewangan  wrote:

Currently, the GPIO interface is said to Open Drain if it is Single
Ended and active LOW. Similarly, it is said as Open Source if it is
Single Ended and active HIGH.

The active HIGH/LOW is used in the interface for setting the pin
state to HIGH or LOW when enabling/disabling the interface.

In Open Drain interface, pin is set to HIGH by putting pin in
high impedance and LOW by driving to the LOW.

In Open Source interface, pin is set to HIGH by driving pin to
HIGH and set to LOW by putting pin in high impedance.

With above, the Open Drain/Source is unrelated to the active LOW/HIGH
in interface. There is interface where the enable/disable of interface
is ether active LOW or HIGH but it is Open Drain type.

Hence decouple the Open Drain with Single Ended + Active LOW and
Open Source with Single Ended + Active HIGH.

Adding different flag for the Open Drain/Open Source which is valid
only when Single ended flag is enabled.
 if (single_ended) {
-   if (active_low)
+   if (open_drain)

This breaks ACPI case, right?



In acpi case, single_ended is not handled. It only handles the active LOW.

From code:


   bool active_low = false;
bool single_ended = false;
int ret;

if (!fwnode)
return ERR_PTR(-EINVAL);

if (is_of_node(fwnode)) {
enum of_gpio_flags flags;

desc = of_get_named_gpiod_flags(to_of_node(fwnode), 
propname,

index, );
if (!IS_ERR(desc)) {
active_low = flags & OF_GPIO_ACTIVE_LOW;
single_ended = flags & OF_GPIO_SINGLE_ENDED;
}
} else if (is_acpi_node(fwnode)) {
struct acpi_gpio_info info;

desc = acpi_node_get_gpiod(fwnode, propname, index, );
if (!IS_ERR(desc))
active_low = info.polarity == GPIO_ACTIVE_LOW;
}

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 = <_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

[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 = <_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 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 <ldewan...@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 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -256,6 +257,22 @@ static int tegra_pwm_remove(struct platform_device *pdev)

return pwmchip_remove(>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

[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 
---
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 
  #include 
  #include 
+#include 
  #include 
  #include 
  
@@ -256,6 +257,22 @@ static int tegra_pwm_remove(struct platform_device *pdev)

return pwmchip_remove(>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.

[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

[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

[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
   = 10ul/16672000
   = 59 (59.98)
rate = (200K + 59/2)/59 = 3390

Based on new method:
hz = 5998
rate = DIV_ROUND_CLOSE(20*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 = 
Based on new formula:
hz = 5951, rate = 3360

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

Signed-off-by: Laxman Dewangan <ldewan...@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

[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
   = 10ul/16672000
   = 59 (59.98)
rate = (200K + 59/2)/59 = 3390

Based on new method:
hz = 5998
rate = DIV_ROUND_CLOSE(20*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 = 
Based on new formula:
hz = 5951, rate = 3360

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

Signed-off-by: Laxman Dewangan 
---
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

[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 <ldewan...@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 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -256,6 +257,22 @@ static int tegra_pwm_remove(struct platform_device *pdev)
return pwmchip_remove(>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 = _pwm_pm_ops,
},
.probe = tegra_pwm_probe,
.remove = tegra_pwm_remove,
-- 
2.1.4

[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 
---
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 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -256,6 +257,22 @@ static int tegra_pwm_remove(struct platform_device *pdev)
return pwmchip_remove(>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 = _pwm_pm_ops,
},
.probe = tegra_pwm_probe,
.remove = tegra_pwm_remove,
-- 
2.1.4

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

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

[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 <ldewan...@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.
+pinctrl-1: Node handle for the sleep state of pin configurations.
+
 Example:
 
pwm: pwm@7000a000 {
@@ -29,3 +42,33 @@ Example:
resets = <_car 17>;
reset-names = "pwm";
};
+
+
+Example with the pin configuration for suspend and resume:
+=
+Pin PE7 is used as PWM interface.
+
+#include 
+
+   pinmux@7868 {
+   pwm_active_state: pwm_active_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+
+   pwm_sleep_state: pwm_sleep_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+   };
+
+   pwm@7000a000 {
+   /* Mandatory PWM properties */
+   pinctrl-names = "default", "sleep";
+   pinctrl-0 = <_active_state>;
+   pinctrl-1 = <_sleep_state>;
+   };
-- 
2.1.4

[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 
---
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 = <_car 17>;
reset-names = "pwm";
};
+
+
+Example with the pin configuration for suspend and resume:
+=
+Pin PE7 is used as PWM interface.
+
+#include 
+
+   pinmux@7868 {
+   pwm_active_state: pwm_active_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+
+   pwm_sleep_state: pwm_sleep_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+   };
+
+   pwm@7000a000 {
+   /* Mandatory PWM properties */
+   pinctrl-names = "default", "sleep";
+   pinctrl-0 = <_active_state>;
+   pinctrl-1 = <_sleep_state>;
+   };
-- 
2.1.4

[PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high

[Laxman Dewangan]

Currently, the GPIO interface is said to Open Drain if it is Single
Ended and active LOW. Similarly, it is said as Open Source if it is
Single Ended and active HIGH.

The active HIGH/LOW is used in the interface for setting the pin
state to HIGH or LOW when enabling/disabling the interface.

In Open Drain interface, pin is set to HIGH by putting pin in
high impedance and LOW by driving to the LOW.

In Open Source interface, pin is set to HIGH by driving pin to
HIGH and set to LOW by putting pin in high impedance.

With above, the Open Drain/Source is unrelated to the active LOW/HIGH
in interface. There is interface where the enable/disable of interface
is ether active LOW or HIGH but it is Open Drain type.

Hence decouple the Open Drain with Single Ended + Active LOW and
Open Source with Single Ended + Active HIGH.

Adding different flag for the Open Drain/Open Source which is valid
only when Single ended flag is enabled.

Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
---
 drivers/gpio/gpiolib-of.c   |  2 +-
 drivers/gpio/gpiolib.c  |  4 +++-
 include/dt-bindings/gpio/gpio.h | 12 
 include/linux/of_gpio.h |  1 +
 4 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 975b9f6..b13b7c7 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -147,7 +147,7 @@ struct gpio_desc *of_find_gpio(struct device *dev, const 
char *con_id,
*flags |= GPIO_ACTIVE_LOW;
 
if (of_flags & OF_GPIO_SINGLE_ENDED) {
-   if (of_flags & OF_GPIO_ACTIVE_LOW)
+   if (of_flags & OF_GPIO_OPEN_DRAIN)
*flags |= GPIO_OPEN_DRAIN;
else
*flags |= GPIO_OPEN_SOURCE;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4aa1e78..c22e572 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -,6 +,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct 
fwnode_handle *fwnode,
unsigned long lflags = 0;
bool active_low = false;
bool single_ended = false;
+   bool open_drain = false;
int ret;
 
if (!fwnode)
@@ -3346,6 +3347,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct 
fwnode_handle *fwnode,
if (!IS_ERR(desc)) {
active_low = flags & OF_GPIO_ACTIVE_LOW;
single_ended = flags & OF_GPIO_SINGLE_ENDED;
+   open_drain = flags & OF_GPIO_OPEN_DRAIN;
}
} else if (is_acpi_node(fwnode)) {
struct acpi_gpio_info info;
@@ -3366,7 +3368,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct 
fwnode_handle *fwnode,
lflags |= GPIO_ACTIVE_LOW;
 
if (single_ended) {
-   if (active_low)
+   if (open_drain)
lflags |= GPIO_OPEN_DRAIN;
else
lflags |= GPIO_OPEN_SOURCE;
diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index c673d2c..b4f54da 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -17,11 +17,15 @@
 #define GPIO_PUSH_PULL 0
 #define GPIO_SINGLE_ENDED 2
 
+/* Bit 2 express Open drain or open source */
+#define GPIO_LINE_OPEN_SOURCE 0
+#define GPIO_LINE_OPEN_DRAIN 4
+
 /*
- * Open Drain/Collector is the combination of single-ended active low,
- * Open Source/Emitter is the combination of single-ended active high.
+ * Open Drain/Collector is the combination of single-ended open drain 
interface.
+ * Open Source/Emitter is the combination of single-ended open source 
interface.
  */
-#define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW)
-#define GPIO_OPEN_SOURCE (GPIO_SINGLE_ENDED | GPIO_ACTIVE_HIGH)
+#define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
+#define GPIO_OPEN_SOURCE (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_SOURCE)
 
 #endif
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 3f87ea5..1e089d5 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -30,6 +30,7 @@ struct device_node;
 enum of_gpio_flags {
OF_GPIO_ACTIVE_LOW = 0x1,
OF_GPIO_SINGLE_ENDED = 0x2,
+   OF_GPIO_OPEN_DRAIN = 0x4,
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.1.4

[PATCH 1/1] gpio: core: Decouple open drain/source flag with active low/high

[Laxman Dewangan]

Currently, the GPIO interface is said to Open Drain if it is Single
Ended and active LOW. Similarly, it is said as Open Source if it is
Single Ended and active HIGH.

The active HIGH/LOW is used in the interface for setting the pin
state to HIGH or LOW when enabling/disabling the interface.

In Open Drain interface, pin is set to HIGH by putting pin in
high impedance and LOW by driving to the LOW.

In Open Source interface, pin is set to HIGH by driving pin to
HIGH and set to LOW by putting pin in high impedance.

With above, the Open Drain/Source is unrelated to the active LOW/HIGH
in interface. There is interface where the enable/disable of interface
is ether active LOW or HIGH but it is Open Drain type.

Hence decouple the Open Drain with Single Ended + Active LOW and
Open Source with Single Ended + Active HIGH.

Adding different flag for the Open Drain/Open Source which is valid
only when Single ended flag is enabled.

Signed-off-by: Laxman Dewangan 
---
 drivers/gpio/gpiolib-of.c   |  2 +-
 drivers/gpio/gpiolib.c  |  4 +++-
 include/dt-bindings/gpio/gpio.h | 12 
 include/linux/of_gpio.h |  1 +
 4 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 975b9f6..b13b7c7 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -147,7 +147,7 @@ struct gpio_desc *of_find_gpio(struct device *dev, const 
char *con_id,
*flags |= GPIO_ACTIVE_LOW;
 
if (of_flags & OF_GPIO_SINGLE_ENDED) {
-   if (of_flags & OF_GPIO_ACTIVE_LOW)
+   if (of_flags & OF_GPIO_OPEN_DRAIN)
*flags |= GPIO_OPEN_DRAIN;
else
*flags |= GPIO_OPEN_SOURCE;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4aa1e78..c22e572 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -,6 +,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct 
fwnode_handle *fwnode,
unsigned long lflags = 0;
bool active_low = false;
bool single_ended = false;
+   bool open_drain = false;
int ret;
 
if (!fwnode)
@@ -3346,6 +3347,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct 
fwnode_handle *fwnode,
if (!IS_ERR(desc)) {
active_low = flags & OF_GPIO_ACTIVE_LOW;
single_ended = flags & OF_GPIO_SINGLE_ENDED;
+   open_drain = flags & OF_GPIO_OPEN_DRAIN;
}
} else if (is_acpi_node(fwnode)) {
struct acpi_gpio_info info;
@@ -3366,7 +3368,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct 
fwnode_handle *fwnode,
lflags |= GPIO_ACTIVE_LOW;
 
if (single_ended) {
-   if (active_low)
+   if (open_drain)
lflags |= GPIO_OPEN_DRAIN;
else
lflags |= GPIO_OPEN_SOURCE;
diff --git a/include/dt-bindings/gpio/gpio.h b/include/dt-bindings/gpio/gpio.h
index c673d2c..b4f54da 100644
--- a/include/dt-bindings/gpio/gpio.h
+++ b/include/dt-bindings/gpio/gpio.h
@@ -17,11 +17,15 @@
 #define GPIO_PUSH_PULL 0
 #define GPIO_SINGLE_ENDED 2
 
+/* Bit 2 express Open drain or open source */
+#define GPIO_LINE_OPEN_SOURCE 0
+#define GPIO_LINE_OPEN_DRAIN 4
+
 /*
- * Open Drain/Collector is the combination of single-ended active low,
- * Open Source/Emitter is the combination of single-ended active high.
+ * Open Drain/Collector is the combination of single-ended open drain 
interface.
+ * Open Source/Emitter is the combination of single-ended open source 
interface.
  */
-#define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW)
-#define GPIO_OPEN_SOURCE (GPIO_SINGLE_ENDED | GPIO_ACTIVE_HIGH)
+#define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_DRAIN)
+#define GPIO_OPEN_SOURCE (GPIO_SINGLE_ENDED | GPIO_LINE_OPEN_SOURCE)
 
 #endif
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 3f87ea5..1e089d5 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -30,6 +30,7 @@ struct device_node;
 enum of_gpio_flags {
OF_GPIO_ACTIVE_LOW = 0x1,
OF_GPIO_SINGLE_ENDED = 0x2,
+   OF_GPIO_OPEN_DRAIN = 0x4,
 };
 
 #ifdef CONFIG_OF_GPIO
-- 
2.1.4

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

[Laxman Dewangan]

On Thursday 06 April 2017 06:33 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Thu, Apr 06, 2017 at 09:57:09AM +0100, Jon Hunter wrote:

On 05/04/17 15:13, Laxman Dewangan wrote:

+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 "suspend" and "resume".

Why not just use the pre-defined names here? There is a pre-defined name
for "default", "idle" and "sleep" and then you can use the following
APIs and avoid the lookup of the state ...

pinctrl_pm_select_default_state()
pinctrl_pm_select_idle_state()
pinctrl_pm_select_sleep_state()

Note for i2c [0][1], I used "default" as the active/on state (which I
know is not that descriptive) and then used 'idle' as the suspended
state. This way we don't need any custom names.

Agreed, I think that's how these states are meant to be used.
I did quick grep for the pinctrl_pm_select_* functions in the code tree 
and found usage of these APIs in some of the places.
I am taking the reference of i2c-st, i2c-nomadic and 
extcon/extcon-usb-gpio.c drivers and from this the interpretation is


default state: When interface active and transfer need to be done in IO 
interface.
idle state: Active state of the system but interface is not active, put 
in non-active state of the interface.
sleep state: When system entering into suspend and IO interface is going 
to be inactive.


So in PWM case, we will need the "default" and "sleep" state.

In suspend(), set the "sleep" state and in resume, set the "default" state.

+ Linus W as I refereed his st/nomadik driver for reference.

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

[Laxman Dewangan]

On Thursday 06 April 2017 06:33 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Thu, Apr 06, 2017 at 09:57:09AM +0100, Jon Hunter wrote:

On 05/04/17 15:13, Laxman Dewangan wrote:

+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 "suspend" and "resume".

Why not just use the pre-defined names here? There is a pre-defined name
for "default", "idle" and "sleep" and then you can use the following
APIs and avoid the lookup of the state ...

pinctrl_pm_select_default_state()
pinctrl_pm_select_idle_state()
pinctrl_pm_select_sleep_state()

Note for i2c [0][1], I used "default" as the active/on state (which I
know is not that descriptive) and then used 'idle' as the suspended
state. This way we don't need any custom names.

Agreed, I think that's how these states are meant to be used.
I did quick grep for the pinctrl_pm_select_* functions in the code tree 
and found usage of these APIs in some of the places.
I am taking the reference of i2c-st, i2c-nomadic and 
extcon/extcon-usb-gpio.c drivers and from this the interpretation is


default state: When interface active and transfer need to be done in IO 
interface.
idle state: Active state of the system but interface is not active, put 
in non-active state of the interface.
sleep state: When system entering into suspend and IO interface is going 
to be inactive.


So in PWM case, we will need the "default" and "sleep" state.

In suspend(), set the "sleep" state and in resume, set the "default" state.

+ Linus W as I refereed his st/nomadik driver for reference.

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

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

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 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 <ldewan...@nvidia.com>
---
 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 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 
---
 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 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
   = 10ul/16672000
   = 59 (59.98)
rate = (200K + 59/2)/59 = 3390

Based on new method:
hz = 5998
rate = DIV_ROUND_CLOSE(20*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 = 
Based on new formula:
hz = 5951, rate = 3360

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

Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
---
 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

[PATCH 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
   = 10ul/16672000
   = 59 (59.98)
rate = (200K + 59/2)/59 = 3390

Based on new method:
hz = 5998
rate = DIV_ROUND_CLOSE(20*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 = 
Based on new formula:
hz = 5951, rate = 3360

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

Signed-off-by: Laxman Dewangan 
---
 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

[PATCH 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 <ldewan...@nvidia.com>
---
 drivers/pwm/pwm-tegra.c | 66 +
 1 file changed, 66 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index e9c4de5..60ed522 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -52,6 +53,9 @@ struct tegra_pwm_chip {
void __iomem *regs;
 
const struct tegra_pwm_soc *soc;
+   struct pinctrl  *pinctrl;
+   struct pinctrl_state*suspend_state;
+   struct pinctrl_state*resume_state;
 };
 
 static inline struct tegra_pwm_chip *to_tegra_pwm_chip(struct pwm_chip *chip)
@@ -215,6 +219,27 @@ static int tegra_pwm_probe(struct platform_device *pdev)
pwm->chip.base = -1;
pwm->chip.npwm = pwm->soc->num_channels;
 
+   pwm->pinctrl = devm_pinctrl_get(>dev);
+   if (!IS_ERR(pwm->pinctrl)) {
+   pwm->suspend_state = pinctrl_lookup_state(pwm->pinctrl,
+ "suspend");
+   if (IS_ERR(pwm->suspend_state)) {
+   /* Ignore error other than PROBE_DEFER */
+   ret = PTR_ERR(pwm->suspend_state);
+   if (ret == -EPROBE_DEFER)
+   return ret;
+   }
+
+   pwm->resume_state = pinctrl_lookup_state(pwm->pinctrl,
+"resume");
+   if (IS_ERR(pwm->resume_state)) {
+   /* Ignore error other than PROBE_DEFER */
+   ret = PTR_ERR(pwm->resume_state);
+   if (ret == -EPROBE_DEFER)
+   return ret;
+   }
+   }
+
ret = pwmchip_add(>chip);
if (ret < 0) {
dev_err(>dev, "pwmchip_add() failed: %d\n", ret);
@@ -256,6 +281,42 @@ static int tegra_pwm_remove(struct platform_device *pdev)
return pwmchip_remove(>chip);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int tegra_pwm_suspend(struct device *dev)
+{
+   struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
+   int ret;
+
+   if (IS_ERR(pc->pinctrl) || IS_ERR(pc->suspend_state))
+   return 0;
+
+   ret = pinctrl_select_state(pc->pinctrl, pc->suspend_state);
+   if (ret < 0) {
+   dev_err(dev, "Failed to set pin into suspend state:%d\n", ret);
+   return ret;
+   }
+
+   return 0;
+}
+
+static int tegra_pwm_resume(struct device *dev)
+{
+   struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
+   int ret;
+
+   if (IS_ERR(pc->pinctrl) || IS_ERR(pc->resume_state))
+   return 0;
+
+   ret = pinctrl_select_state(pc->pinctrl, pc->resume_state);
+   if (ret < 0) {
+   dev_err(dev, "Failed to set pin into resume state:%d\n", ret);
+   return ret;
+   }
+
+   return 0;
+}
+#endif
+
 static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
 };
@@ -272,10 +333,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 = _pwm_pm_ops,
},
.probe = tegra_pwm_probe,
.remove = tegra_pwm_remove,
-- 
2.1.4

[PATCH 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 
---
 drivers/pwm/pwm-tegra.c | 66 +
 1 file changed, 66 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index e9c4de5..60ed522 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -52,6 +53,9 @@ struct tegra_pwm_chip {
void __iomem *regs;
 
const struct tegra_pwm_soc *soc;
+   struct pinctrl  *pinctrl;
+   struct pinctrl_state*suspend_state;
+   struct pinctrl_state*resume_state;
 };
 
 static inline struct tegra_pwm_chip *to_tegra_pwm_chip(struct pwm_chip *chip)
@@ -215,6 +219,27 @@ static int tegra_pwm_probe(struct platform_device *pdev)
pwm->chip.base = -1;
pwm->chip.npwm = pwm->soc->num_channels;
 
+   pwm->pinctrl = devm_pinctrl_get(>dev);
+   if (!IS_ERR(pwm->pinctrl)) {
+   pwm->suspend_state = pinctrl_lookup_state(pwm->pinctrl,
+ "suspend");
+   if (IS_ERR(pwm->suspend_state)) {
+   /* Ignore error other than PROBE_DEFER */
+   ret = PTR_ERR(pwm->suspend_state);
+   if (ret == -EPROBE_DEFER)
+   return ret;
+   }
+
+   pwm->resume_state = pinctrl_lookup_state(pwm->pinctrl,
+"resume");
+   if (IS_ERR(pwm->resume_state)) {
+   /* Ignore error other than PROBE_DEFER */
+   ret = PTR_ERR(pwm->resume_state);
+   if (ret == -EPROBE_DEFER)
+   return ret;
+   }
+   }
+
ret = pwmchip_add(>chip);
if (ret < 0) {
dev_err(>dev, "pwmchip_add() failed: %d\n", ret);
@@ -256,6 +281,42 @@ static int tegra_pwm_remove(struct platform_device *pdev)
return pwmchip_remove(>chip);
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int tegra_pwm_suspend(struct device *dev)
+{
+   struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
+   int ret;
+
+   if (IS_ERR(pc->pinctrl) || IS_ERR(pc->suspend_state))
+   return 0;
+
+   ret = pinctrl_select_state(pc->pinctrl, pc->suspend_state);
+   if (ret < 0) {
+   dev_err(dev, "Failed to set pin into suspend state:%d\n", ret);
+   return ret;
+   }
+
+   return 0;
+}
+
+static int tegra_pwm_resume(struct device *dev)
+{
+   struct tegra_pwm_chip *pc = dev_get_drvdata(dev);
+   int ret;
+
+   if (IS_ERR(pc->pinctrl) || IS_ERR(pc->resume_state))
+   return 0;
+
+   ret = pinctrl_select_state(pc->pinctrl, pc->resume_state);
+   if (ret < 0) {
+   dev_err(dev, "Failed to set pin into resume state:%d\n", ret);
+   return ret;
+   }
+
+   return 0;
+}
+#endif
+
 static const struct tegra_pwm_soc tegra20_pwm_soc = {
.num_channels = 4,
 };
@@ -272,10 +333,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 = _pwm_pm_ops,
},
.probe = tegra_pwm_probe,
.remove = tegra_pwm_remove,
-- 
2.1.4

[PATCH 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 <ldewan...@nvidia.com>
---
 .../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..145c323 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 regualator device, it is required
+to configure the pins diffrently in different states, specially 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 "suspend" and "resume".
+pinctrl-0: Node handle of the suspend state configuration of pins.
+pinctrl-1: Node handle of the resume state configuration of pins.
+
 Example:
 
pwm: pwm@7000a000 {
@@ -29,3 +42,33 @@ Example:
resets = <_car 17>;
reset-names = "pwm";
};
+
+
+Example with the pin configuration for suspend and resume:
+=
+Here Pin PE7 is used as PWM.
+
+#include 
+
+   pinmux@7868 {
+   pwm_suspend: pwm_suspend_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+
+   pwm_resume: pwm_resume_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+   };
+
+   pwm@7000a000 {
+   /* Mandatory pwm properties */
+   pinctrl-names = "suspend", "resume";
+   pinctrl-0 = <_suspend>;
+   pinctrl-1 = <_resume>;
+   };
-- 
2.1.4

[PATCH 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 
---
 .../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..145c323 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 regualator device, it is required
+to configure the pins diffrently in different states, specially 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 "suspend" and "resume".
+pinctrl-0: Node handle of the suspend state configuration of pins.
+pinctrl-1: Node handle of the resume state configuration of pins.
+
 Example:
 
pwm: pwm@7000a000 {
@@ -29,3 +42,33 @@ Example:
resets = <_car 17>;
reset-names = "pwm";
};
+
+
+Example with the pin configuration for suspend and resume:
+=
+Here Pin PE7 is used as PWM.
+
+#include 
+
+   pinmux@7868 {
+   pwm_suspend: pwm_suspend_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+
+   pwm_resume: pwm_resume_state {
+pe7 {
+nvidia,pins = "pe7";
+nvidia,tristate = ;
+   };
+   };
+   };
+
+   pwm@7000a000 {
+   /* Mandatory pwm properties */
+   pinctrl-names = "suspend", "resume";
+   pinctrl-0 = <_suspend>;
+   pinctrl-1 = <_resume>;
+   };
-- 
2.1.4

[PATCH V4 2/2] regulator: Add settling time for non-linear voltage transition

[Laxman Dewangan]

Some regulators (some PWM regulators) have the voltage transition
non-linear i.e. exponentially. On such cases, the settling time
for voltage transition can not be presented in the voltage-ramp-delay.

Add new property for non-linear voltage transition and handle this
in getting the voltage settling time.

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

---
This patch is continuation of discussion on patch
regulator: pwm: Fix regulator ramp delay for continuous mode
https://patchwork.kernel.org/patch/9216857/
where is it discussed to have separate property for PWM which has
exponential voltage transition.

Changes from V1:
- Use new DT property to finding that voltage ramp is exponential or
  not and use flag for having fixed delay for all voltage change.

Changes from V2:
- Based on review comment from V1, make the settling time property
  independent of the regulator-ramp-delay and move this to core
  framework instead of handling in PWM regulator.

Changes from V3:
- The change was sent long back and resuming this patch.
- Rebase on linux-next tag 20170330 for resend.
---
 drivers/regulator/core.c  | 2 ++
 drivers/regulator/of_regulator.c  | 4 
 include/linux/regulator/machine.h | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0753635..7303454 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2773,6 +2773,8 @@ static int _regulator_set_voltage_time(struct 
regulator_dev *rdev,
ramp_delay = rdev->constraints->ramp_delay;
else if (rdev->desc->ramp_delay)
ramp_delay = rdev->desc->ramp_delay;
+   else if (rdev->constraints->settling_time)
+   return rdev->constraints->settling_time;
 
if (ramp_delay == 0) {
rdev_dbg(rdev, "ramp_delay not set\n");
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 4f613ec..09d677d 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -86,6 +86,10 @@ static void of_get_regulation_constraints(struct device_node 
*np,
constraints->ramp_disable = true;
}
 
+   ret = of_property_read_u32(np, "regulator-settling-time-us", );
+   if (!ret)
+   constraints->settling_time = pval;
+
ret = of_property_read_u32(np, "regulator-enable-ramp-delay", );
if (!ret)
constraints->enable_time = pval;
diff --git a/include/linux/regulator/machine.h 
b/include/linux/regulator/machine.h
index c9f795e..117699d 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -108,6 +108,8 @@ struct regulator_state {
  * @initial_state: Suspend state to set by default.
  * @initial_mode: Mode to set at startup.
  * @ramp_delay: Time to settle down after voltage change (unit: uV/us)
+ * @settling_time: Time to settle down after voltage change when voltage
+ *change is non-linear (unit: microseconds).
  * @active_discharge: Enable/disable active discharge. The enum
  *   regulator_active_discharge values are used for
  *   initialisation.
@@ -149,6 +151,7 @@ struct regulation_constraints {
unsigned int initial_mode;
 
unsigned int ramp_delay;
+   unsigned int settling_time;
unsigned int enable_time;
 
unsigned int active_discharge;
-- 
2.1.4