[PATCH 0/4] pwm: omap-dmtimer: Allow for dynamic pwm period updates

[PATCH 0/4] pwm: omap-dmtimer: Allow for dynamic pwm period updates

[Lokesh Vutla]

This series fixes minor issues in config callback and allows for on the
fly updates for pwm period and duty cycle. This is mainly intended to
allow pwm omap dmtimer to be used to generate a 1PPS signal that can be
syncronized to PTP clock in CPTS module in AM335x and AM57xx SoCs.

Series tested after applying the following series:
- https://patchwork.kernel.org/patch/11379875/
- https://patchwork.kernel.org/project/linux-omap/list/?series=246183 

Full dependencies can be seen here:
https://github.com/lokeshvutla/linux/tree/devel/pwm-1pps-generation

Lokesh Vutla (4):
  pwm: omap-dmtimer: Drop unused header file
  pwm: omap-dmtimer: Fix pwm enabling sequence
  pwm: omap-dmtimer: Do not disable pwm before changing
    period/duty_cycle
  pwm: omap-dmtimer: Implement .apply callback

 drivers/pwm/pwm-omap-dmtimer.c                | 163 +++++++++---------
 include/clocksource/timer-ti-dm.h             |   3 +-
 .../linux/platform_data/pwm_omap_dmtimer.h    |  90 ----------
 3 files changed, 84 insertions(+), 172 deletions(-)
 delete mode 100644 include/linux/platform_data/pwm_omap_dmtimer.h

-- 
2.23.0

[PATCH 1/4] pwm: omap-dmtimer: Drop unused header file

[Lokesh Vutla]

pwm_omap_dmtimer.h is used only to typedef struct omap_dm_timer to
pwm_omap_dmtimer. Rest of the file is pretty mush unsed. So reuse
omap_dm_timer in pwm-omap-dmtimer.c and delete the header file.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c                | 16 ++--
 include/clocksource/timer-ti-dm.h             |  3 +-
 .../linux/platform_data/pwm_omap_dmtimer.h    | 90 -------------------
 3 files changed, 8 insertions(+), 101 deletions(-)
 delete mode 100644 include/linux/platform_data/pwm_omap_dmtimer.h

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 88a3c5690fea..2e35bf9a7936 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -20,8 +20,8 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <clocksource/timer-ti-dm.h>
 #include <linux/platform_data/dmtimer-omap.h>
-#include <linux/platform_data/pwm_omap_dmtimer.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
@@ -34,7 +34,7 @@
 struct pwm_omap_dmtimer_chip {
 	struct pwm_chip chip;
 	struct mutex mutex;
-	pwm_omap_dmtimer *dm_timer;
+	struct omap_dm_timer *dm_timer;
 	const struct omap_dm_timer_ops *pdata;
 	struct platform_device *dm_timer_pdev;
 };
@@ -190,9 +190,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 		load_value, load_value,	match_value, match_value);
 
 	omap->pdata->set_pwm(omap->dm_timer,
-			      pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
-			      true,
-			      PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
+			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
 
 	/* If config was called while timer was running it must be reenabled. */
 	if (timer_active)
@@ -220,9 +219,8 @@ static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
 	 */
 	mutex_lock(&omap->mutex);
 	omap->pdata->set_pwm(omap->dm_timer,
-			      polarity == PWM_POLARITY_INVERSED,
-			      true,
-			      PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+			     polarity == PWM_POLARITY_INVERSED,
+			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
 	mutex_unlock(&omap->mutex);
 
 	return 0;
@@ -244,7 +242,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 	struct pwm_omap_dmtimer_chip *omap;
 	struct dmtimer_platform_data *timer_pdata;
 	const struct omap_dm_timer_ops *pdata;
-	pwm_omap_dmtimer *dm_timer;
+	struct omap_dm_timer *dm_timer;
 	u32 v;
 	int ret = 0;
 
diff --git a/include/clocksource/timer-ti-dm.h b/include/clocksource/timer-ti-dm.h
index 7d9598dc578d..e346020e2762 100644
--- a/include/clocksource/timer-ti-dm.h
+++ b/include/clocksource/timer-ti-dm.h
@@ -248,8 +248,7 @@ int omap_dm_timers_active(void);
 
 /*
  * The below are inlined to optimize code size for system timers. Other code
- * should not need these at all, see
- * include/linux/platform_data/pwm_omap_dmtimer.h
+ * should not need these at all.
  */
 #if defined(CONFIG_ARCH_OMAP1) || defined(CONFIG_ARCH_OMAP2PLUS)
 static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
diff --git a/include/linux/platform_data/pwm_omap_dmtimer.h b/include/linux/platform_data/pwm_omap_dmtimer.h
deleted file mode 100644
index e7d521e48855..000000000000
--- a/include/linux/platform_data/pwm_omap_dmtimer.h
+++ /dev/null
@@ -1,90 +0,0 @@
-/*
- * include/linux/platform_data/pwm_omap_dmtimer.h
- *
- * OMAP Dual-Mode Timer PWM platform data
- *
- * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com/
- * Tarun Kanti DebBarma <tarun.kanti@ti.com>
- * Thara Gopinath <thara@ti.com>
- *
- * Platform device conversion and hwmod support.
- *
- * Copyright (C) 2005 Nokia Corporation
- * Author: Lauri Leukkunen <lauri.leukkunen@nokia.com>
- * PWM and clock framework support by Timo Teras.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
- *
- * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
- * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN
- * NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
- * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
- * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
- * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- *
- * You should have received a copy of the  GNU General Public License along
- * with this program; if not, write  to the Free Software Foundation, Inc.,
- * 675 Mass Ave, Cambridge, MA 02139, USA.
- */
-
-#ifndef __PWM_OMAP_DMTIMER_PDATA_H
-#define __PWM_OMAP_DMTIMER_PDATA_H
-
-/* clock sources */
-#define PWM_OMAP_DMTIMER_SRC_SYS_CLK			0x00
-#define PWM_OMAP_DMTIMER_SRC_32_KHZ			0x01
-#define PWM_OMAP_DMTIMER_SRC_EXT_CLK			0x02
-
-/* timer interrupt enable bits */
-#define PWM_OMAP_DMTIMER_INT_CAPTURE			(1 << 2)
-#define PWM_OMAP_DMTIMER_INT_OVERFLOW			(1 << 1)
-#define PWM_OMAP_DMTIMER_INT_MATCH			(1 << 0)
-
-/* trigger types */
-#define PWM_OMAP_DMTIMER_TRIGGER_NONE			0x00
-#define PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW		0x01
-#define PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE	0x02
-
-struct omap_dm_timer;
-typedef struct omap_dm_timer pwm_omap_dmtimer;
-
-struct pwm_omap_dmtimer_pdata {
-	pwm_omap_dmtimer *(*request_by_node)(struct device_node *np);
-	pwm_omap_dmtimer *(*request_specific)(int timer_id);
-	pwm_omap_dmtimer *(*request)(void);
-
-	int	(*free)(pwm_omap_dmtimer *timer);
-
-	void	(*enable)(pwm_omap_dmtimer *timer);
-	void	(*disable)(pwm_omap_dmtimer *timer);
-
-	int	(*get_irq)(pwm_omap_dmtimer *timer);
-	int	(*set_int_enable)(pwm_omap_dmtimer *timer, unsigned int value);
-	int	(*set_int_disable)(pwm_omap_dmtimer *timer, u32 mask);
-
-	struct clk *(*get_fclk)(pwm_omap_dmtimer *timer);
-
-	int	(*start)(pwm_omap_dmtimer *timer);
-	int	(*stop)(pwm_omap_dmtimer *timer);
-	int	(*set_source)(pwm_omap_dmtimer *timer, int source);
-
-	int	(*set_load)(pwm_omap_dmtimer *timer, int autoreload,
-			unsigned int value);
-	int	(*set_match)(pwm_omap_dmtimer *timer, int enable,
-			unsigned int match);
-	int	(*set_pwm)(pwm_omap_dmtimer *timer, int def_on,
-			int toggle, int trigger);
-	int	(*set_prescaler)(pwm_omap_dmtimer *timer, int prescaler);
-
-	unsigned int (*read_counter)(pwm_omap_dmtimer *timer);
-	int	(*write_counter)(pwm_omap_dmtimer *timer, unsigned int value);
-	unsigned int (*read_status)(pwm_omap_dmtimer *timer);
-	int	(*write_status)(pwm_omap_dmtimer *timer, unsigned int value);
-};
-
-#endif /* __PWM_OMAP_DMTIMER_PDATA_H */
-- 
2.23.0

Re: [PATCH 1/4] pwm: omap-dmtimer: Drop unused header file

[Uwe Kleine-König]

Hello,

On Mon, Feb 24, 2020 at 10:51:32AM +0530, Lokesh Vutla wrote:
> @@ -190,9 +190,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  		load_value, load_value,	match_value, match_value);
>  
>  	omap->pdata->set_pwm(omap->dm_timer,
> -			      pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> -			      true,
> -			      PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);

This is unrelated.

>  
>  	/* If config was called while timer was running it must be reenabled. */
>  	if (timer_active)
> @@ -220,9 +219,8 @@ static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
>  	 */
>  	mutex_lock(&omap->mutex);
>  	omap->pdata->set_pwm(omap->dm_timer,
> -			      polarity == PWM_POLARITY_INVERSED,
> -			      true,
> -			      PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> +			     polarity == PWM_POLARITY_INVERSED,
> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);

ditto

>  	mutex_unlock(&omap->mutex);
>  
>  	return 0;
> @@ -244,7 +242,7 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
>  	struct pwm_omap_dmtimer_chip *omap;
>  	struct dmtimer_platform_data *timer_pdata;
>  	const struct omap_dm_timer_ops *pdata;
> -	pwm_omap_dmtimer *dm_timer;
> +	struct omap_dm_timer *dm_timer;
>  	u32 v;
>  	int ret = 0;
>  

Other than that looks fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 1/4] pwm: omap-dmtimer: Drop unused header file

[Lokesh Vutla]

Hi Uwe,

On 24/02/20 1:23 PM, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Feb 24, 2020 at 10:51:32AM +0530, Lokesh Vutla wrote:
>> @@ -190,9 +190,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>  		load_value, load_value,	match_value, match_value);
>>  
>>  	omap->pdata->set_pwm(omap->dm_timer,
>> -			      pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
>> -			      true,
>> -			      PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE);
>> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
>> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> 
> This is unrelated.

PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE is deleted along with header file
and used OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE. I should have mentioned this
in my commit description. Will fix it in v2.

Thanks and regards,
Lokesh

Re: [PATCH 1/4] pwm: omap-dmtimer: Drop unused header file

[Uwe Kleine-König]

On Tue, Feb 25, 2020 at 10:32:04AM +0530, Lokesh Vutla wrote:
> Hi Uwe,
> 
> On 24/02/20 1:23 PM, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Feb 24, 2020 at 10:51:32AM +0530, Lokesh Vutla wrote:
> >> @@ -190,9 +190,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
> >>  		load_value, load_value,	match_value, match_value);
> >>  
> >>  	omap->pdata->set_pwm(omap->dm_timer,
> >> -			      pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> >> -			      true,
> >> -			      PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> >> +			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> >> +			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> > 
> > This is unrelated.
> 
> PWM_OMAP_DMTIMER_TRIGGER_OVERFLOW_AND_COMPARE is deleted along with header file
> and used OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE. I should have mentioned this
> in my commit description. Will fix it in v2.

Ah, indeed I missed that the register name was changed in that hunk and
considered that an indention only change.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH 2/4] pwm: omap-dmtimer: Fix pwm enabling sequence

[Lokesh Vutla]

To configure DM timer is pwm mode the following needs to be set in
OMAP_TIMER_CTRL_REG using set_pwm callback:
- Set toggle mode on PORTIMERPWM output pin
- Set trigger on overflow and match on PORTIMERPWM output pin.
- Set auto reload

This is a one time configuration and needs to be set before the start of
the dm timer. But the current driver tries to set the same configuration
for every period/duty cycle update, which is not needed. So move the pwm
setup before enabling timer and do not update it in pwm_omap_dmtimer_config.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 2e35bf9a7936..f13be7216847 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -73,6 +73,10 @@ static int pwm_omap_dmtimer_enable(struct pwm_chip *chip,
 	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
 
 	mutex_lock(&omap->mutex);
+	omap->pdata->set_pwm(omap->dm_timer,
+			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
+			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+
 	pwm_omap_dmtimer_start(omap);
 	mutex_unlock(&omap->mutex);
 
@@ -189,10 +193,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
 		load_value, load_value,	match_value, match_value);
 
-	omap->pdata->set_pwm(omap->dm_timer,
-			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
-			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
-
 	/* If config was called while timer was running it must be reenabled. */
 	if (timer_active)
 		pwm_omap_dmtimer_start(omap);
-- 
2.23.0

Re: [PATCH 2/4] pwm: omap-dmtimer: Fix pwm enabling sequence

[Uwe Kleine-König]

On Mon, Feb 24, 2020 at 10:51:33AM +0530, Lokesh Vutla wrote:
> To configure DM timer is pwm mode the following needs to be set in
> OMAP_TIMER_CTRL_REG using set_pwm callback:
> - Set toggle mode on PORTIMERPWM output pin
> - Set trigger on overflow and match on PORTIMERPWM output pin.
> - Set auto reload
> 
> This is a one time configuration and needs to be set before the start of
> the dm timer. But the current driver tries to set the same configuration
> for every period/duty cycle update, which is not needed. So move the pwm
> setup before enabling timer and do not update it in pwm_omap_dmtimer_config.

Is this change kind of moot with the conversion to .apply in the next
patch?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 2/4] pwm: omap-dmtimer: Fix pwm enabling sequence

[Lokesh Vutla]

Hi Uwe,

On 24/02/20 2:19 PM, Uwe Kleine-König wrote:
> On Mon, Feb 24, 2020 at 10:51:33AM +0530, Lokesh Vutla wrote:
>> To configure DM timer is pwm mode the following needs to be set in
>> OMAP_TIMER_CTRL_REG using set_pwm callback:
>> - Set toggle mode on PORTIMERPWM output pin
>> - Set trigger on overflow and match on PORTIMERPWM output pin.
>> - Set auto reload
>>
>> This is a one time configuration and needs to be set before the start of
>> the dm timer. But the current driver tries to set the same configuration
>> for every period/duty cycle update, which is not needed. So move the pwm
>> setup before enabling timer and do not update it in pwm_omap_dmtimer_config.
> 
> Is this change kind of moot with the conversion to .apply in the next
> patch?

Yes, but I didn't want to club it with the conversion to .apply as this is
functional change wrt to the existing driver.

Thanks and regards,
Lokesh

[PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Only the Timer control register(TCLR) can be updated only when the timer
is stopped. Registers like Counter register(TCRR), loader register(TLDR),
match register(TMAR) can be updated when the counter is running. Since
TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
timer for period/duty_cycle update.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index f13be7216847..58c61559e72f 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	u32 load_value, match_value;
 	struct clk *fclk;
 	unsigned long clk_rate;
-	bool timer_active;
 
 	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
 		duty_ns, period_ns);
@@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	load_value = (DM_TIMER_MAX - period_cycles) + 1;
 	match_value = load_value + duty_cycles - 1;
 
-	/*
-	 * We MUST stop the associated dual-mode timer before attempting to
-	 * write its registers, but calls to omap_dm_timer_start/stop must
-	 * be balanced so check if timer is active before calling timer_stop.
-	 */
-	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
-	if (timer_active)
-		omap->pdata->stop(omap->dm_timer);
-
 	omap->pdata->set_load(omap->dm_timer, true, load_value);
 	omap->pdata->set_match(omap->dm_timer, true, match_value);
 
 	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
 		load_value, load_value,	match_value, match_value);
 
-	/* If config was called while timer was running it must be reenabled. */
-	if (timer_active)
-		pwm_omap_dmtimer_start(omap);
-
 	mutex_unlock(&omap->mutex);
 
 	return 0;
-- 
2.23.0

Re: [PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

Hello,

On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
> Only the Timer control register(TCLR) can be updated only when the timer
> is stopped. Registers like Counter register(TCRR), loader register(TLDR),
> match register(TMAR) can be updated when the counter is running. Since
> TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
> timer for period/duty_cycle update.
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index f13be7216847..58c61559e72f 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	u32 load_value, match_value;
>  	struct clk *fclk;
>  	unsigned long clk_rate;
> -	bool timer_active;
>  
>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
>  		duty_ns, period_ns);
> @@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
>  	match_value = load_value + duty_cycles - 1;
>  
> -	/*
> -	 * We MUST stop the associated dual-mode timer before attempting to
> -	 * write its registers, but calls to omap_dm_timer_start/stop must
> -	 * be balanced so check if timer is active before calling timer_stop.
> -	 */
> -	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
> -	if (timer_active)
> -		omap->pdata->stop(omap->dm_timer);
> -
>  	omap->pdata->set_load(omap->dm_timer, true, load_value);
>  	omap->pdata->set_match(omap->dm_timer, true, match_value);

(Without having looked into the depths of the driver I assume
.set_load() sets the period of the PWM and .set_match() the duty cycle.)

What happens on a running PWM if you change the period? Consider you
change from duty_cycle = 1000, period = 5000 to duty_cycle = 4000,
period = 10000. As you set the period first, can it happen the hardware
produces a cycle with duty_cycle = 1000, period = 10000?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Uwe,

On 24/02/20 2:25 PM, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
>> Only the Timer control register(TCLR) can be updated only when the timer
>> is stopped. Registers like Counter register(TCRR), loader register(TLDR),
>> match register(TMAR) can be updated when the counter is running. Since
>> TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
>> timer for period/duty_cycle update.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
>>  1 file changed, 14 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
>> index f13be7216847..58c61559e72f 100644
>> --- a/drivers/pwm/pwm-omap-dmtimer.c
>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>> @@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>  	u32 load_value, match_value;
>>  	struct clk *fclk;
>>  	unsigned long clk_rate;
>> -	bool timer_active;
>>  
>>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
>>  		duty_ns, period_ns);
>> @@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
>>  	match_value = load_value + duty_cycles - 1;
>>  
>> -	/*
>> -	 * We MUST stop the associated dual-mode timer before attempting to
>> -	 * write its registers, but calls to omap_dm_timer_start/stop must
>> -	 * be balanced so check if timer is active before calling timer_stop.
>> -	 */
>> -	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
>> -	if (timer_active)
>> -		omap->pdata->stop(omap->dm_timer);
>> -
>>  	omap->pdata->set_load(omap->dm_timer, true, load_value);
>>  	omap->pdata->set_match(omap->dm_timer, true, match_value);
> 
> (Without having looked into the depths of the driver I assume
> .set_load() sets the period of the PWM and .set_match() the duty cycle.)

Right.

> 
> What happens on a running PWM if you change the period? Consider you
> change from duty_cycle = 1000, period = 5000 to duty_cycle = 4000,
> period = 10000. As you set the period first, can it happen the hardware
> produces a cycle with duty_cycle = 1000, period = 10000?

No. So, the current cycle is un affected with duty_cycle = 1000 and period =
5000. Starting from next cycle new settings gets reflected with duty_cycle =
4000 and period = 10000.

Thanks and regards,
Lokesh

Re: [PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

Hello,

On Tue, Feb 25, 2020 at 10:32:42AM +0530, Lokesh Vutla wrote:
> On 24/02/20 2:25 PM, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
> >> Only the Timer control register(TCLR) can be updated only when the timer
> >> is stopped. Registers like Counter register(TCRR), loader register(TLDR),
> >> match register(TMAR) can be updated when the counter is running. Since
> >> TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
> >> timer for period/duty_cycle update.
> >>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >> ---
> >>  drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
> >>  1 file changed, 14 deletions(-)
> >>
> >> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> >> index f13be7216847..58c61559e72f 100644
> >> --- a/drivers/pwm/pwm-omap-dmtimer.c
> >> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> >> @@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
> >>  	u32 load_value, match_value;
> >>  	struct clk *fclk;
> >>  	unsigned long clk_rate;
> >> -	bool timer_active;
> >>  
> >>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
> >>  		duty_ns, period_ns);
> >> @@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
> >>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
> >>  	match_value = load_value + duty_cycles - 1;
> >>  
> >> -	/*
> >> -	 * We MUST stop the associated dual-mode timer before attempting to
> >> -	 * write its registers, but calls to omap_dm_timer_start/stop must
> >> -	 * be balanced so check if timer is active before calling timer_stop.
> >> -	 */
> >> -	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
> >> -	if (timer_active)
> >> -		omap->pdata->stop(omap->dm_timer);
> >> -
> >>  	omap->pdata->set_load(omap->dm_timer, true, load_value);
> >>  	omap->pdata->set_match(omap->dm_timer, true, match_value);
> > 
> > (Without having looked into the depths of the driver I assume
> > .set_load() sets the period of the PWM and .set_match() the duty cycle.)
> 
> Right.
> 
> > 
> > What happens on a running PWM if you change the period? Consider you
> > change from duty_cycle = 1000, period = 5000 to duty_cycle = 4000,
> > period = 10000. As you set the period first, can it happen the hardware
> > produces a cycle with duty_cycle = 1000, period = 10000?
> 
> No. So, the current cycle is un affected with duty_cycle = 1000 and period =
> 5000. Starting from next cycle new settings gets reflected with duty_cycle =
> 4000 and period = 10000.

Is the reference manual for this hardware publically available?

So the .set_load callback just writes a shadow register and .set_match
latches it into hardware atomically with its own register changes? A
comment in the source code about this would be good. Also if .set_load
doesn't work without .set_match I wonder if it is sane to put their
logic in two different functions.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Uwe,

On 25/02/20 12:18 PM, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Feb 25, 2020 at 10:32:42AM +0530, Lokesh Vutla wrote:
>> On 24/02/20 2:25 PM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
>>>> Only the Timer control register(TCLR) can be updated only when the timer
>>>> is stopped. Registers like Counter register(TCRR), loader register(TLDR),
>>>> match register(TMAR) can be updated when the counter is running. Since
>>>> TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
>>>> timer for period/duty_cycle update.
>>>>
>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>>>> ---
>>>>  drivers/pwm/pwm-omap-dmtimer.c | 14 --------------
>>>>  1 file changed, 14 deletions(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
>>>> index f13be7216847..58c61559e72f 100644
>>>> --- a/drivers/pwm/pwm-omap-dmtimer.c
>>>> +++ b/drivers/pwm/pwm-omap-dmtimer.c
>>>> @@ -102,7 +102,6 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>>>  	u32 load_value, match_value;
>>>>  	struct clk *fclk;
>>>>  	unsigned long clk_rate;
>>>> -	bool timer_active;
>>>>  
>>>>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
>>>>  		duty_ns, period_ns);
>>>> @@ -178,25 +177,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>>>>  	load_value = (DM_TIMER_MAX - period_cycles) + 1;
>>>>  	match_value = load_value + duty_cycles - 1;
>>>>  
>>>> -	/*
>>>> -	 * We MUST stop the associated dual-mode timer before attempting to
>>>> -	 * write its registers, but calls to omap_dm_timer_start/stop must
>>>> -	 * be balanced so check if timer is active before calling timer_stop.
>>>> -	 */
>>>> -	timer_active = pm_runtime_active(&omap->dm_timer_pdev->dev);
>>>> -	if (timer_active)
>>>> -		omap->pdata->stop(omap->dm_timer);
>>>> -
>>>>  	omap->pdata->set_load(omap->dm_timer, true, load_value);
>>>>  	omap->pdata->set_match(omap->dm_timer, true, match_value);
>>>
>>> (Without having looked into the depths of the driver I assume
>>> .set_load() sets the period of the PWM and .set_match() the duty cycle.)
>>
>> Right.
>>
>>>
>>> What happens on a running PWM if you change the period? Consider you
>>> change from duty_cycle = 1000, period = 5000 to duty_cycle = 4000,
>>> period = 10000. As you set the period first, can it happen the hardware
>>> produces a cycle with duty_cycle = 1000, period = 10000?
>>
>> No. So, the current cycle is un affected with duty_cycle = 1000 and period =
>> 5000. Starting from next cycle new settings gets reflected with duty_cycle =
>> 4000 and period = 10000.
> 
> Is the reference manual for this hardware publically available?

AM335x TRM [0] Section 20.1.3.5 Pulse-Width Modulation (Page 4445).

[0] http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf

> 
> So the .set_load callback just writes a shadow register and .set_match
> latches it into hardware atomically with its own register changes? A
> comment in the source code about this would be good. Also if .set_load
> doesn't work without .set_match I wonder if it is sane to put their
> logic in two different functions.

Just to give a little bit of background:
- The omap timer is an upward counter that can be started and stopped at any time.
- Once the timer counter overflows, it gets loaded with a predefined load
value.(Or can be configured to not re load at all).
- Timer has a configurable output pin which can be toggled in the following two
cases:
	- When the counter overflows
	- When the counter matches with a predefined register(match register).

Using this o/p pin the driver tries to generate a PWM with period = (OVERFLOW -
LOAD_VALUE) and duty_cycle = (MATCH_VALUE - LOAD_VALUE).

.set_load will configure the load value .set_match will configure the match
value. The configured values gets effected only in the next cycle of PWM.

I hope this is clear. Let me know if you need more details.

Thanks and regards,
Lokesh

Re: [PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

Hello Lokesh,

On Tue, Feb 25, 2020 at 01:29:57PM +0530, Lokesh Vutla wrote:
> On 25/02/20 12:18 PM, Uwe Kleine-König wrote:
> > On Tue, Feb 25, 2020 at 10:32:42AM +0530, Lokesh Vutla wrote:
> >> On 24/02/20 2:25 PM, Uwe Kleine-König wrote:
> >>> On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
> >>>>  	omap->pdata->set_load(omap->dm_timer, true, load_value);
> >>>>  	omap->pdata->set_match(omap->dm_timer, true, match_value);
> >>>
> >>> (Without having looked into the depths of the driver I assume
> >>> .set_load() sets the period of the PWM and .set_match() the duty cycle.)
> >>
> >> Right.
> >>
> >>>
> >>> What happens on a running PWM if you change the period? Consider you
> >>> change from duty_cycle = 1000, period = 5000 to duty_cycle = 4000,
> >>> period = 10000. As you set the period first, can it happen the hardware
> >>> produces a cycle with duty_cycle = 1000, period = 10000?
> >>
> >> No. So, the current cycle is un affected with duty_cycle = 1000 and period =
> >> 5000. Starting from next cycle new settings gets reflected with duty_cycle =
> >> 4000 and period = 10000.
> > 
> > Is the reference manual for this hardware publically available?
> 
> AM335x TRM [0] Section 20.1.3.5 Pulse-Width Modulation (Page 4445).
> 
> [0] http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf

Great. This is BTW an opportunity to increase your patch count: Create a
patch that adds a reference to this document at the top of the driver.

> > So the .set_load callback just writes a shadow register and .set_match
> > latches it into hardware atomically with its own register changes? A
> > comment in the source code about this would be good. Also if .set_load
> > doesn't work without .set_match I wonder if it is sane to put their
> > logic in two different functions.
> 
> Just to give a little bit of background:

Thanks, very appreciated.

> - The omap timer is an upward counter that can be started and stopped at any time.
> - Once the timer counter overflows, it gets loaded with a predefined load
> value.(Or can be configured to not re load at all).
> - Timer has a configurable output pin which can be toggled in the following two
> cases:
> 	- When the counter overflows
> 	- When the counter matches with a predefined register(match register).
> 
> Using this o/p pin the driver tries to generate a PWM with period = (OVERFLOW -
> LOAD_VALUE) and duty_cycle = (MATCH_VALUE - LOAD_VALUE).
> 
> .set_load will configure the load value .set_match will configure the match
> value. The configured values gets effected only in the next cycle of PWM.

Ah, so back to my original question: If you change from
duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 and
after you set the period but before you set the duty_cycle a period
happens to end, you get indeed a cycle with mixed settings, right?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Lokesh Vutla]

Hi Uwe,

On 25/02/20 2:08 PM, Uwe Kleine-König wrote:
> Hello Lokesh,
> 
> On Tue, Feb 25, 2020 at 01:29:57PM +0530, Lokesh Vutla wrote:
>> On 25/02/20 12:18 PM, Uwe Kleine-König wrote:
>>> On Tue, Feb 25, 2020 at 10:32:42AM +0530, Lokesh Vutla wrote:
>>>> On 24/02/20 2:25 PM, Uwe Kleine-König wrote:
>>>>> On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
>>>>>>  	omap->pdata->set_load(omap->dm_timer, true, load_value);
>>>>>>  	omap->pdata->set_match(omap->dm_timer, true, match_value);
>>>>>
>>>>> (Without having looked into the depths of the driver I assume
>>>>> .set_load() sets the period of the PWM and .set_match() the duty cycle.)
>>>>
>>>> Right.
>>>>
>>>>>
>>>>> What happens on a running PWM if you change the period? Consider you
>>>>> change from duty_cycle = 1000, period = 5000 to duty_cycle = 4000,
>>>>> period = 10000. As you set the period first, can it happen the hardware
>>>>> produces a cycle with duty_cycle = 1000, period = 10000?
>>>>
>>>> No. So, the current cycle is un affected with duty_cycle = 1000 and period =
>>>> 5000. Starting from next cycle new settings gets reflected with duty_cycle =
>>>> 4000 and period = 10000.
>>>
>>> Is the reference manual for this hardware publically available?
>>
>> AM335x TRM [0] Section 20.1.3.5 Pulse-Width Modulation (Page 4445).
>>
>> [0] http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
> 
> Great. This is BTW an opportunity to increase your patch count: Create a
> patch that adds a reference to this document at the top of the driver.
> 
>>> So the .set_load callback just writes a shadow register and .set_match
>>> latches it into hardware atomically with its own register changes? A
>>> comment in the source code about this would be good. Also if .set_load
>>> doesn't work without .set_match I wonder if it is sane to put their
>>> logic in two different functions.
>>
>> Just to give a little bit of background:
> 
> Thanks, very appreciated.
> 
>> - The omap timer is an upward counter that can be started and stopped at any time.
>> - Once the timer counter overflows, it gets loaded with a predefined load
>> value.(Or can be configured to not re load at all).
>> - Timer has a configurable output pin which can be toggled in the following two
>> cases:
>> 	- When the counter overflows
>> 	- When the counter matches with a predefined register(match register).
>>
>> Using this o/p pin the driver tries to generate a PWM with period = (OVERFLOW -
>> LOAD_VALUE) and duty_cycle = (MATCH_VALUE - LOAD_VALUE).
>>
>> .set_load will configure the load value .set_match will configure the match
>> value. The configured values gets effected only in the next cycle of PWM.
> 
> Ah, so back to my original question: If you change from
> duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 and
> after you set the period but before you set the duty_cycle a period
> happens to end, you get indeed a cycle with mixed settings, right?

hmm..you are right but the mixed period happens in a bit different case. Let me
explain in bit more detail.

For omap dm timer, the load_value that gets set in the current period, will be
reflected only in next cycle, as timer counter has to overflow to load this
value. But in case of match register(which determines the duty cycle), the timer
counter is continuously matched to it. So below are the cases where a mixed
period can happen:
1) When signal is high and new match value is > current timer counter. Then the
duty cycle gets reflected in the current cycle.(Duty_cycle for current period=
new match value -  previous load  value).
2) When signal is high and new match value is < current timer counter. Then the
period and duty cycle for the current cycle gets effected as well. Because the
signal is pulled down only when counter matches with match register, and this
happens only in the next cycle(after timer counter overflows). Then:
	- new Period for current cycle = (current period + new period)
	- new duty cycle for current cycle =  (current period + new duty_cycle).

I am able to observe the above mentioned 2 behaviors on the scope using beagle
bone black. So the problem is with updating duty cycle when the signal is high.
but when signal is low, nothing gets effected to the current cycle.

How do you want to go about this? Should we describe this as limitation in the
driver as you asked?

Thanks and regards,
Lokesh

Re: [PATCH 3/4] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Uwe Kleine-König]

On Tue, Feb 25, 2020 at 04:56:02PM +0530, Lokesh Vutla wrote:
> Hi Uwe,
> 
> On 25/02/20 2:08 PM, Uwe Kleine-König wrote:
> > Hello Lokesh,
> > 
> > On Tue, Feb 25, 2020 at 01:29:57PM +0530, Lokesh Vutla wrote:
> >> On 25/02/20 12:18 PM, Uwe Kleine-König wrote:
> >>> On Tue, Feb 25, 2020 at 10:32:42AM +0530, Lokesh Vutla wrote:
> >>>> On 24/02/20 2:25 PM, Uwe Kleine-König wrote:
> >>>>> On Mon, Feb 24, 2020 at 10:51:34AM +0530, Lokesh Vutla wrote:
> >>>>>>  	omap->pdata->set_load(omap->dm_timer, true, load_value);
> >>>>>>  	omap->pdata->set_match(omap->dm_timer, true, match_value);
> >>>>>
> >>>>> (Without having looked into the depths of the driver I assume
> >>>>> .set_load() sets the period of the PWM and .set_match() the duty cycle.)
> >>>>
> >>>> Right.
> >>>>
> >>>>>
> >>>>> What happens on a running PWM if you change the period? Consider you
> >>>>> change from duty_cycle = 1000, period = 5000 to duty_cycle = 4000,
> >>>>> period = 10000. As you set the period first, can it happen the hardware
> >>>>> produces a cycle with duty_cycle = 1000, period = 10000?
> >>>>
> >>>> No. So, the current cycle is un affected with duty_cycle = 1000 and period =
> >>>> 5000. Starting from next cycle new settings gets reflected with duty_cycle =
> >>>> 4000 and period = 10000.
> >>>
> >>> Is the reference manual for this hardware publically available?
> >>
> >> AM335x TRM [0] Section 20.1.3.5 Pulse-Width Modulation (Page 4445).
> >>
> >> [0] http://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
> > 
> > Great. This is BTW an opportunity to increase your patch count: Create a
> > patch that adds a reference to this document at the top of the driver.
> > 
> >>> So the .set_load callback just writes a shadow register and .set_match
> >>> latches it into hardware atomically with its own register changes? A
> >>> comment in the source code about this would be good. Also if .set_load
> >>> doesn't work without .set_match I wonder if it is sane to put their
> >>> logic in two different functions.
> >>
> >> Just to give a little bit of background:
> > 
> > Thanks, very appreciated.
> > 
> >> - The omap timer is an upward counter that can be started and stopped at any time.
> >> - Once the timer counter overflows, it gets loaded with a predefined load
> >> value.(Or can be configured to not re load at all).
> >> - Timer has a configurable output pin which can be toggled in the following two
> >> cases:
> >> 	- When the counter overflows
> >> 	- When the counter matches with a predefined register(match register).
> >>
> >> Using this o/p pin the driver tries to generate a PWM with period = (OVERFLOW -
> >> LOAD_VALUE) and duty_cycle = (MATCH_VALUE - LOAD_VALUE).
> >>
> >> .set_load will configure the load value .set_match will configure the match
> >> value. The configured values gets effected only in the next cycle of PWM.
> > 
> > Ah, so back to my original question: If you change from
> > duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 and
> > after you set the period but before you set the duty_cycle a period
> > happens to end, you get indeed a cycle with mixed settings, right?
> 
> hmm..you are right but the mixed period happens in a bit different case. Let me
> explain in bit more detail.
> 
> For omap dm timer, the load_value that gets set in the current period, will be
> reflected only in next cycle, as timer counter has to overflow to load this
> value. But in case of match register(which determines the duty cycle), the timer
> counter is continuously matched to it. So below are the cases where a mixed
> period can happen:
> 1) When signal is high and new match value is > current timer counter. Then the
> duty cycle gets reflected in the current cycle.(Duty_cycle for current period=
> new match value -  previous load  value).
> 2) When signal is high and new match value is < current timer counter. Then the
> period and duty cycle for the current cycle gets effected as well. Because the
> signal is pulled down only when counter matches with match register, and this
> happens only in the next cycle(after timer counter overflows). Then:
> 	- new Period for current cycle = (current period + new period)
> 	- new duty cycle for current cycle =  (current period + new duty_cycle).
> 
> I am able to observe the above mentioned 2 behaviors on the scope using beagle
> bone black. So the problem is with updating duty cycle when the signal is high.
> but when signal is low, nothing gets effected to the current cycle.
> 
> How do you want to go about this? Should we describe this as limitation in the
> driver as you asked?

OK, to sumarize: You have a counter that goes up. When it overflows it
gets reloaded with the load value and the output goes up. When
$counter = $match, the output goes down.

Having this is a comment at the top of the driver would be very welcome.

(I just noticed I duplicated this question about a racy update in
another end of this thread. This pops in my head too automatically :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH 4/4] pwm: omap-dmtimer: Implement .apply callback

[Lokesh Vutla]

Implement .apply callback and drop the legacy callbacks(enable, disable,
config, set_polarity).

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++--------------
 1 file changed, 80 insertions(+), 61 deletions(-)

diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
index 58c61559e72f..aeda4ab12385 100644
--- a/drivers/pwm/pwm-omap-dmtimer.c
+++ b/drivers/pwm/pwm-omap-dmtimer.c
@@ -31,8 +31,18 @@
 #define DM_TIMER_LOAD_MIN 0xfffffffe
 #define DM_TIMER_MAX      0xffffffff
 
+/**
+ * struct pwm_omap_dmtimer_chip - Structure representing a pwm chip
+ *				  corresponding to omap dmtimer.
+ * @chip:		PWM chip structure representing PWM controller
+ * @mutex:		Mutex to protect pwm apply state
+ * @dm_timer:		Pointer to omap dm timer.
+ * @pdata:		Pointer to omap dm timer ops.
+ * dm_timer_pdev:	Pointer to omap dm timer platform device
+ */
 struct pwm_omap_dmtimer_chip {
 	struct pwm_chip chip;
+	/* Mutex to protect pwm apply state */
 	struct mutex mutex;
 	struct omap_dm_timer *dm_timer;
 	const struct omap_dm_timer_ops *pdata;
@@ -45,11 +55,22 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip)
 	return container_of(chip, struct pwm_omap_dmtimer_chip, chip);
 }
 
+/**
+ * pwm_omap_dmtimer_get_clock_cycles() - Get clock cycles in a time frame
+ * @clk_rate:	pwm timer clock rate
+ * @ns:		time frame in nano seconds.
+ *
+ * Return number of clock cycles in a given period(ins ns).
+ */
 static u32 pwm_omap_dmtimer_get_clock_cycles(unsigned long clk_rate, int ns)
 {
 	return DIV_ROUND_CLOSEST_ULL((u64)clk_rate * ns, NSEC_PER_SEC);
 }
 
+/**
+ * pwm_omap_dmtimer_start() - Start the pwm omap dm timer in pwm mode
+ * @omap:	Pointer to pwm omap dm timer chip
+ */
 static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
 {
 	/*
@@ -67,32 +88,16 @@ static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
 	omap->pdata->start(omap->dm_timer);
 }
 
-static int pwm_omap_dmtimer_enable(struct pwm_chip *chip,
-				   struct pwm_device *pwm)
-{
-	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
-
-	mutex_lock(&omap->mutex);
-	omap->pdata->set_pwm(omap->dm_timer,
-			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
-			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
-
-	pwm_omap_dmtimer_start(omap);
-	mutex_unlock(&omap->mutex);
-
-	return 0;
-}
-
-static void pwm_omap_dmtimer_disable(struct pwm_chip *chip,
-				     struct pwm_device *pwm)
-{
-	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
-
-	mutex_lock(&omap->mutex);
-	omap->pdata->stop(omap->dm_timer);
-	mutex_unlock(&omap->mutex);
-}
-
+/**
+ * pwm_omap_dmtimer_config() - Update the configuration of pwm omap dm timer
+ * @chip:	Pointer to PWM controller
+ * @pwm:	Pointer to PWM channel
+ * @duty_ns:	New duty cycle in nano seconds
+ * @period_ns:	New period in nano seconds
+ *
+ * Return 0 if successfully changed the period/duty_cycle else appropriate
+ * error.
+ */
 static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 				   struct pwm_device *pwm,
 				   int duty_ns, int period_ns)
@@ -100,30 +105,26 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
 	u32 period_cycles, duty_cycles;
 	u32 load_value, match_value;
-	struct clk *fclk;
 	unsigned long clk_rate;
+	struct clk *fclk;
 
 	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
 		duty_ns, period_ns);
 
-	mutex_lock(&omap->mutex);
 	if (duty_ns == pwm_get_duty_cycle(pwm) &&
-	    period_ns == pwm_get_period(pwm)) {
-		/* No change - don't cause any transients. */
-		mutex_unlock(&omap->mutex);
+	    period_ns == pwm_get_period(pwm))
 		return 0;
-	}
 
 	fclk = omap->pdata->get_fclk(omap->dm_timer);
 	if (!fclk) {
 		dev_err(chip->dev, "invalid pmtimer fclk\n");
-		goto err_einval;
+		return -EINVAL;
 	}
 
 	clk_rate = clk_get_rate(fclk);
 	if (!clk_rate) {
 		dev_err(chip->dev, "invalid pmtimer fclk rate\n");
-		goto err_einval;
+		return -EINVAL;
 	}
 
 	dev_dbg(chip->dev, "clk rate: %luHz\n", clk_rate);
@@ -151,7 +152,7 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 		dev_info(chip->dev,
 			 "period %d ns too short for clock rate %lu Hz\n",
 			 period_ns, clk_rate);
-		goto err_einval;
+		return -EINVAL;
 	}
 
 	if (duty_cycles < 1) {
@@ -183,54 +184,72 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
 	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
 		load_value, load_value,	match_value, match_value);
 
-	mutex_unlock(&omap->mutex);
-
 	return 0;
-
-err_einval:
-	mutex_unlock(&omap->mutex);
-
-	return -EINVAL;
 }
 
-static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
-					 struct pwm_device *pwm,
-					 enum pwm_polarity polarity)
+/**
+ * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer.
+ * @chip:	Pointer to PWM controller
+ * @pwm:	Pointer to PWM channel
+ * @state:	New sate to apply
+ *
+ * Return 0 if successfully changed the state else appropriate error.
+ */
+static int pwm_omap_dmtimer_apply(struct pwm_chip *chip,
+				  struct pwm_device *pwm,
+				  const struct pwm_state *state)
 {
 	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
+	int ret = 0;
 
-	/*
-	 * PWM core will not call set_polarity while PWM is enabled so it's
-	 * safe to reconfigure the timer here without stopping it first.
-	 */
 	mutex_lock(&omap->mutex);
-	omap->pdata->set_pwm(omap->dm_timer,
-			     polarity == PWM_POLARITY_INVERSED,
-			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+
+	if (pwm_is_enabled(pwm) && !state->enabled) {
+		omap->pdata->stop(omap->dm_timer);
+		goto unlock_mutex;
+	}
+
+	if (pwm_get_polarity(pwm) != state->polarity)
+		omap->pdata->set_pwm(omap->dm_timer,
+				     state->polarity == PWM_POLARITY_INVERSED,
+				     true,
+				     OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+
+	ret = pwm_omap_dmtimer_config(chip, pwm, state->duty_cycle,
+				      state->period);
+	if (ret)
+		goto unlock_mutex;
+
+	if (!pwm_is_enabled(pwm) && state->enabled) {
+		omap->pdata->set_pwm(omap->dm_timer,
+				     state->polarity == PWM_POLARITY_INVERSED,
+				     true,
+				     OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
+		pwm_omap_dmtimer_start(omap);
+	}
+
+unlock_mutex:
 	mutex_unlock(&omap->mutex);
 
-	return 0;
+	return ret;
 }
 
 static const struct pwm_ops pwm_omap_dmtimer_ops = {
-	.enable	= pwm_omap_dmtimer_enable,
-	.disable = pwm_omap_dmtimer_disable,
-	.config	= pwm_omap_dmtimer_config,
-	.set_polarity = pwm_omap_dmtimer_set_polarity,
+	.apply = pwm_omap_dmtimer_apply,
 	.owner = THIS_MODULE,
 };
 
 static int pwm_omap_dmtimer_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *timer;
-	struct platform_device *timer_pdev;
-	struct pwm_omap_dmtimer_chip *omap;
 	struct dmtimer_platform_data *timer_pdata;
 	const struct omap_dm_timer_ops *pdata;
+	struct platform_device *timer_pdev;
+	struct pwm_omap_dmtimer_chip *omap;
 	struct omap_dm_timer *dm_timer;
-	u32 v;
+	struct device_node *timer;
 	int ret = 0;
+	u32 v;
 
 	timer = of_parse_phandle(np, "ti,timers", 0);
 	if (!timer)
-- 
2.23.0

Re: [PATCH 4/4] pwm: omap-dmtimer: Implement .apply callback

[Uwe Kleine-König]

On Mon, Feb 24, 2020 at 10:51:35AM +0530, Lokesh Vutla wrote:
> Implement .apply callback and drop the legacy callbacks(enable, disable,
> config, set_polarity).
> 
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
>  drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++--------------
>  1 file changed, 80 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c
> index 58c61559e72f..aeda4ab12385 100644
> --- a/drivers/pwm/pwm-omap-dmtimer.c
> +++ b/drivers/pwm/pwm-omap-dmtimer.c
> @@ -31,8 +31,18 @@
>  #define DM_TIMER_LOAD_MIN 0xfffffffe
>  #define DM_TIMER_MAX      0xffffffff
>  
> +/**
> + * struct pwm_omap_dmtimer_chip - Structure representing a pwm chip
> + *				  corresponding to omap dmtimer.
> + * @chip:		PWM chip structure representing PWM controller
> + * @mutex:		Mutex to protect pwm apply state
> + * @dm_timer:		Pointer to omap dm timer.
> + * @pdata:		Pointer to omap dm timer ops.
> + * dm_timer_pdev:	Pointer to omap dm timer platform device
> + */
>  struct pwm_omap_dmtimer_chip {
>  	struct pwm_chip chip;
> +	/* Mutex to protect pwm apply state */
>  	struct mutex mutex;
>  	struct omap_dm_timer *dm_timer;
>  	const struct omap_dm_timer_ops *pdata;
> @@ -45,11 +55,22 @@ to_pwm_omap_dmtimer_chip(struct pwm_chip *chip)
>  	return container_of(chip, struct pwm_omap_dmtimer_chip, chip);
>  }
>  
> +/**
> + * pwm_omap_dmtimer_get_clock_cycles() - Get clock cycles in a time frame
> + * @clk_rate:	pwm timer clock rate
> + * @ns:		time frame in nano seconds.
> + *
> + * Return number of clock cycles in a given period(ins ns).
> + */
>  static u32 pwm_omap_dmtimer_get_clock_cycles(unsigned long clk_rate, int ns)
>  {
>  	return DIV_ROUND_CLOSEST_ULL((u64)clk_rate * ns, NSEC_PER_SEC);
>  }
>  
> +/**
> + * pwm_omap_dmtimer_start() - Start the pwm omap dm timer in pwm mode
> + * @omap:	Pointer to pwm omap dm timer chip
> + */
>  static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
>  {
>  	/*
> @@ -67,32 +88,16 @@ static void pwm_omap_dmtimer_start(struct pwm_omap_dmtimer_chip *omap)
>  	omap->pdata->start(omap->dm_timer);
>  }
>  
> -static int pwm_omap_dmtimer_enable(struct pwm_chip *chip,
> -				   struct pwm_device *pwm)
> -{
> -	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
> -
> -	mutex_lock(&omap->mutex);
> -	omap->pdata->set_pwm(omap->dm_timer,
> -			     pwm_get_polarity(pwm) == PWM_POLARITY_INVERSED,
> -			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> -
> -	pwm_omap_dmtimer_start(omap);
> -	mutex_unlock(&omap->mutex);
> -
> -	return 0;
> -}
> -
> -static void pwm_omap_dmtimer_disable(struct pwm_chip *chip,
> -				     struct pwm_device *pwm)
> -{
> -	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
> -
> -	mutex_lock(&omap->mutex);
> -	omap->pdata->stop(omap->dm_timer);
> -	mutex_unlock(&omap->mutex);
> -}
> -
> +/**
> + * pwm_omap_dmtimer_config() - Update the configuration of pwm omap dm timer
> + * @chip:	Pointer to PWM controller
> + * @pwm:	Pointer to PWM channel
> + * @duty_ns:	New duty cycle in nano seconds
> + * @period_ns:	New period in nano seconds
> + *
> + * Return 0 if successfully changed the period/duty_cycle else appropriate
> + * error.
> + */
>  static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  				   struct pwm_device *pwm,
>  				   int duty_ns, int period_ns)
> @@ -100,30 +105,26 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
>  	u32 period_cycles, duty_cycles;
>  	u32 load_value, match_value;
> -	struct clk *fclk;
>  	unsigned long clk_rate;
> +	struct clk *fclk;
>  
>  	dev_dbg(chip->dev, "requested duty cycle: %d ns, period: %d ns\n",
>  		duty_ns, period_ns);
>  
> -	mutex_lock(&omap->mutex);
>  	if (duty_ns == pwm_get_duty_cycle(pwm) &&
> -	    period_ns == pwm_get_period(pwm)) {
> -		/* No change - don't cause any transients. */
> -		mutex_unlock(&omap->mutex);
> +	    period_ns == pwm_get_period(pwm))
>  		return 0;
> -	}
>  
>  	fclk = omap->pdata->get_fclk(omap->dm_timer);
>  	if (!fclk) {
>  		dev_err(chip->dev, "invalid pmtimer fclk\n");
> -		goto err_einval;
> +		return -EINVAL;
>  	}
>  
>  	clk_rate = clk_get_rate(fclk);
>  	if (!clk_rate) {
>  		dev_err(chip->dev, "invalid pmtimer fclk rate\n");
> -		goto err_einval;
> +		return -EINVAL;
>  	}
>  
>  	dev_dbg(chip->dev, "clk rate: %luHz\n", clk_rate);
> @@ -151,7 +152,7 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  		dev_info(chip->dev,
>  			 "period %d ns too short for clock rate %lu Hz\n",
>  			 period_ns, clk_rate);
> -		goto err_einval;
> +		return -EINVAL;
>  	}
>  
>  	if (duty_cycles < 1) {
> @@ -183,54 +184,72 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip,
>  	dev_dbg(chip->dev, "load value: %#08x (%d), match value: %#08x (%d)\n",
>  		load_value, load_value,	match_value, match_value);
>  
> -	mutex_unlock(&omap->mutex);
> -
>  	return 0;
> -
> -err_einval:
> -	mutex_unlock(&omap->mutex);
> -
> -	return -EINVAL;
>  }
>  
> -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
> -					 struct pwm_device *pwm,
> -					 enum pwm_polarity polarity)
> +/**
> + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer.
> + * @chip:	Pointer to PWM controller
> + * @pwm:	Pointer to PWM channel
> + * @state:	New sate to apply
> + *
> + * Return 0 if successfully changed the state else appropriate error.
> + */
> +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip,
> +				  struct pwm_device *pwm,
> +				  const struct pwm_state *state)
>  {
>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
> +	int ret = 0;
>  
> -	/*
> -	 * PWM core will not call set_polarity while PWM is enabled so it's
> -	 * safe to reconfigure the timer here without stopping it first.
> -	 */
>  	mutex_lock(&omap->mutex);
> -	omap->pdata->set_pwm(omap->dm_timer,
> -			     polarity == PWM_POLARITY_INVERSED,
> -			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> +
> +	if (pwm_is_enabled(pwm) && !state->enabled) {

In my book calling PWM API functions (designed for PWM consumers) is not
so nice. I would prefer you checking the hardware registers or cache the
state locally instead of relying on the core here.

It would be great to have a general description at the top of the driver
(like for example drivers/pwm/pwm-sifive.c) that answers things like:

 - Does calling .stop completes the currently running period (it
   should)?
 - Does changing polarity, duty_cycle and period complete the running
   period?
 - How does the hardware behave on disable? (i.e. does it output the
   state the pin is at in that moment? Does it go High-Z?)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 4/4] pwm: omap-dmtimer: Implement .apply callback

[Lokesh Vutla]

Hi Uwe,

On 24/02/20 2:37 PM, Uwe Kleine-König wrote:
> On Mon, Feb 24, 2020 at 10:51:35AM +0530, Lokesh Vutla wrote:
>> Implement .apply callback and drop the legacy callbacks(enable, disable,
>> config, set_polarity).
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++--------------
>>  1 file changed, 80 insertions(+), 61 deletions(-)
>>

[..snip..]

>> -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
>> -					 struct pwm_device *pwm,
>> -					 enum pwm_polarity polarity)
>> +/**
>> + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer.
>> + * @chip:	Pointer to PWM controller
>> + * @pwm:	Pointer to PWM channel
>> + * @state:	New sate to apply
>> + *
>> + * Return 0 if successfully changed the state else appropriate error.
>> + */
>> +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip,
>> +				  struct pwm_device *pwm,
>> +				  const struct pwm_state *state)
>>  {
>>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
>> +	int ret = 0;
>>  
>> -	/*
>> -	 * PWM core will not call set_polarity while PWM is enabled so it's
>> -	 * safe to reconfigure the timer here without stopping it first.
>> -	 */
>>  	mutex_lock(&omap->mutex);
>> -	omap->pdata->set_pwm(omap->dm_timer,
>> -			     polarity == PWM_POLARITY_INVERSED,
>> -			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
>> +
>> +	if (pwm_is_enabled(pwm) && !state->enabled) {
> 
> In my book calling PWM API functions (designed for PWM consumers) is not
> so nice. I would prefer you checking the hardware registers or cache the
> state locally instead of relying on the core here.

.start and .stop apis does read the hardware registers and check the state
before making any changes. Do you want to drop off the pwm_is_enabled(pwm) check
here?

> 
> It would be great to have a general description at the top of the driver
> (like for example drivers/pwm/pwm-sifive.c) that answers things like:
> 
>  - Does calling .stop completes the currently running period (it
>    should)?

Existing driver implementation abruptly stops the cycle. I can make changes such
that it completes the currently running period.

>  - Does changing polarity, duty_cycle and period complete the running
>    period?

- Polarity can be changed only when the pwm is not running. Ill add extra guards
to reflect this behavior.
- Changing duty_cycle and period does complete the running period and new values
gets reflected in next cycle.

>  - How does the hardware behave on disable? (i.e. does it output the
>    state the pin is at in that moment? Does it go High-Z?)

Now that I am making changes to complete the current period on disable, the pin
goes to Low after disabling(completing the cycle).

Ill add all these points as you mentioned in v2.

Thanks and regards,
Lokesh

Re: [PATCH 4/4] pwm: omap-dmtimer: Implement .apply callback

[Uwe Kleine-König]

Hello Lokesh,

On Tue, Feb 25, 2020 at 10:31:45AM +0530, Lokesh Vutla wrote:
> Hi Uwe,
> 
> On 24/02/20 2:37 PM, Uwe Kleine-König wrote:
> > On Mon, Feb 24, 2020 at 10:51:35AM +0530, Lokesh Vutla wrote:
> >> Implement .apply callback and drop the legacy callbacks(enable, disable,
> >> config, set_polarity).
> >>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >> ---
> >>  drivers/pwm/pwm-omap-dmtimer.c | 141 +++++++++++++++++++--------------
> >>  1 file changed, 80 insertions(+), 61 deletions(-)
> >>
> 
> [..snip..]
> 
> >> -static int pwm_omap_dmtimer_set_polarity(struct pwm_chip *chip,
> >> -					 struct pwm_device *pwm,
> >> -					 enum pwm_polarity polarity)
> >> +/**
> >> + * pwm_omap_dmtimer_apply() - Changes the state of the pwm omap dm timer.
> >> + * @chip:	Pointer to PWM controller
> >> + * @pwm:	Pointer to PWM channel
> >> + * @state:	New sate to apply
> >> + *
> >> + * Return 0 if successfully changed the state else appropriate error.
> >> + */
> >> +static int pwm_omap_dmtimer_apply(struct pwm_chip *chip,
> >> +				  struct pwm_device *pwm,
> >> +				  const struct pwm_state *state)
> >>  {
> >>  	struct pwm_omap_dmtimer_chip *omap = to_pwm_omap_dmtimer_chip(chip);
> >> +	int ret = 0;
> >>  
> >> -	/*
> >> -	 * PWM core will not call set_polarity while PWM is enabled so it's
> >> -	 * safe to reconfigure the timer here without stopping it first.
> >> -	 */
> >>  	mutex_lock(&omap->mutex);
> >> -	omap->pdata->set_pwm(omap->dm_timer,
> >> -			     polarity == PWM_POLARITY_INVERSED,
> >> -			     true, OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE);
> >> +
> >> +	if (pwm_is_enabled(pwm) && !state->enabled) {
> > 
> > In my book calling PWM API functions (designed for PWM consumers) is not
> > so nice. I would prefer you checking the hardware registers or cache the
> > state locally instead of relying on the core here.
> 
> .start and .stop apis does read the hardware registers and check the state
> before making any changes. Do you want to drop off the pwm_is_enabled(pwm) check
> here?

The IMHO more natural approach would be to look into the hardware
registers instead of asking the framework.

> > It would be great to have a general description at the top of the driver
> > (like for example drivers/pwm/pwm-sifive.c) that answers things like:
> > 
> >  - Does calling .stop completes the currently running period (it
> >    should)?
> 
> Existing driver implementation abruptly stops the cycle. I can make changes such
> that it completes the currently running period.

That would be good for correctness.

> >  - Does changing polarity, duty_cycle and period complete the running
> >    period?
> 
> - Polarity can be changed only when the pwm is not running. Ill add extra guards
> to reflect this behavior.
> - Changing duty_cycle and period does complete the running period and new values
> gets reflected in next cycle.

Is there are race with the hardware? I.e. can it happen that when a new
cycle starts just when you configured the new period but not the
duty_cycle yet a mixed cycle is output?

> >  - How does the hardware behave on disable? (i.e. does it output the
> >    state the pin is at in that moment? Does it go High-Z?)
> 
> Now that I am making changes to complete the current period on disable, the pin
> goes to Low after disabling(completing the cycle).
> 
> Ill add all these points as you mentioned in v2.

Great

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH 0/4] pwm: omap-dmtimer: Allow for dynamic pwm period updates

[Tony Lindgren]

* Lokesh Vutla <lokeshvutla@ti.com> [200224 05:23]:
> This series fixes minor issues in config callback and allows for on the
> fly updates for pwm period and duty cycle. This is mainly intended to
> allow pwm omap dmtimer to be used to generate a 1PPS signal that can be
> syncronized to PTP clock in CPTS module in AM335x and AM57xx SoCs.
> 
> Series tested after applying the following series:
> - https://patchwork.kernel.org/patch/11379875/
> - https://patchwork.kernel.org/project/linux-omap/list/?series=246183 
> 
> Full dependencies can be seen here:
> https://github.com/lokeshvutla/linux/tree/devel/pwm-1pps-generation
> 
> Lokesh Vutla (4):
>   pwm: omap-dmtimer: Drop unused header file
>   pwm: omap-dmtimer: Fix pwm enabling sequence
>   pwm: omap-dmtimer: Do not disable pwm before changing
>     period/duty_cycle
>   pwm: omap-dmtimer: Implement .apply callback

FYI, things seem to work for me after this set, but as it looks like
there will be v2 set of patches I want to test again with v2 out
before any Tested-by.

Thanks,

Tony