Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 01/06/2013 03:12 PM, NeilBrown wrote:
[snip]
> I've been playing with off-mode and discovered that the first attempt to set
> the PWM after resume didn't work, but subsequent ones did.
> I did some digging and came up with the following patch.
> With this in place, the omap_pwm_suspend() above is definitely pointless (was
> wasn't really useful even without it).
Thanks for sending. I have given this patch a try on omap3 and I am still
some some failures with my timer read test. I need to dig into that further,
but I am guessing not related to your patch as there were problems there
before :-(
Some minor comments below ...
> NeilBrown
>
>
> From: NeilBrown
> Date: Mon, 7 Jan 2013 07:53:03 +1100
> Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
Nit, subject should formatted "ARM: OMAP: blah blah blah"
Also, may be worth calling this "fix context-loss" as this is really
fixing something that is broken.
> The context loss handling in dmtimer appears to assume that
>omap_dm_timer_set_load_start() or omap_dm_timer_start()
> and
>omap_dm_timer_stop()
>
> bracket all interactions. Only the first two restore the context and
> the last updates the context loss counter.
> However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
> reasonably be called outside this bracketing, and the fact that they
> call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
> is expected.
>
> So if, after a transition into and out of off-mode which would cause
> the dm timer to loose all state, omap_dm_timer_set_match() is called
> before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
> will be 'wrong' and this wrong value will be stored context.tclr so
> a subsequent omap_dm_timer_start() can fail (As the control register
> is wrong).
>
> Simplify this be doing the restore-from-context in
> omap_dm_timer_enable() so that whenever the timer is enabled, the
> context is correct.
> Also update the ctx_loss_count at the same time as we notice it is
> wrong - these is no value in delaying this until the
> omap_dm_timer_disable() as it cannot change while the timer is
> enabled.
>
> Signed-off-by: NeilBrown
>
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index 938b50a..c216696 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
> void omap_dm_timer_enable(struct omap_dm_timer *timer)
> {
> pm_runtime_get_sync(>pdev->dev);
> +
> + if (!(timer->capability & OMAP_TIMER_ALWON)) {
> + int loss_count =
> + omap_pm_get_dev_context_loss_count(>pdev->dev);
> + if (loss_count != timer->ctx_loss_count) {
> + omap_timer_restore_context(timer);
> + timer->ctx_loss_count = loss_count;
> + }
> + }
> }
Can you rebase on v3.8-rc2? We no longer call
omap_pm_get_dev_context_loss_count() directly and so this
does not apply. Should be something like ...
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..2c48182 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -315,7 +315,19 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
+ int c;
+
pm_runtime_get_sync(>pdev->dev);
+
+ if (!(timer->capability & OMAP_TIMER_ALWON)) {
+ if (timer->get_context_loss_count) {
+ c = timer->get_context_loss_count(>pdev->dev);
+ if (c != timer->ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer->ctx_loss_count = c;
+ }
+ }
+ }
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 01/06/2013 03:12 PM, NeilBrown wrote:
[snip]
I've been playing with off-mode and discovered that the first attempt to set
the PWM after resume didn't work, but subsequent ones did.
I did some digging and came up with the following patch.
With this in place, the omap_pwm_suspend() above is definitely pointless (was
wasn't really useful even without it).
Thanks for sending. I have given this patch a try on omap3 and I am still
some some failures with my timer read test. I need to dig into that further,
but I am guessing not related to your patch as there were problems there
before :-(
Some minor comments below ...
NeilBrown
From: NeilBrown ne...@suse.de
Date: Mon, 7 Jan 2013 07:53:03 +1100
Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
Nit, subject should formatted ARM: OMAP: blah blah blah
Also, may be worth calling this fix context-loss as this is really
fixing something that is broken.
The context loss handling in dmtimer appears to assume that
omap_dm_timer_set_load_start() or omap_dm_timer_start()
and
omap_dm_timer_stop()
bracket all interactions. Only the first two restore the context and
the last updates the context loss counter.
However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
reasonably be called outside this bracketing, and the fact that they
call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
is expected.
So if, after a transition into and out of off-mode which would cause
the dm timer to loose all state, omap_dm_timer_set_match() is called
before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
will be 'wrong' and this wrong value will be stored context.tclr so
a subsequent omap_dm_timer_start() can fail (As the control register
is wrong).
Simplify this be doing the restore-from-context in
omap_dm_timer_enable() so that whenever the timer is enabled, the
context is correct.
Also update the ctx_loss_count at the same time as we notice it is
wrong - these is no value in delaying this until the
omap_dm_timer_disable() as it cannot change while the timer is
enabled.
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c216696 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
pm_runtime_get_sync(timer-pdev-dev);
+
+ if (!(timer-capability OMAP_TIMER_ALWON)) {
+ int loss_count =
+ omap_pm_get_dev_context_loss_count(timer-pdev-dev);
+ if (loss_count != timer-ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer-ctx_loss_count = loss_count;
+ }
+ }
}
Can you rebase on v3.8-rc2? We no longer call
omap_pm_get_dev_context_loss_count() directly and so this
does not apply. Should be something like ...
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..2c48182 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -315,7 +315,19 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
+ int c;
+
pm_runtime_get_sync(timer-pdev-dev);
+
+ if (!(timer-capability OMAP_TIMER_ALWON)) {
+ if (timer-get_context_loss_count) {
+ c = timer-get_context_loss_count(timer-pdev-dev);
+ if (c != timer-ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer-ctx_loss_count = c;
+ }
+ }
+ }
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, 13 Dec 2012 11:07:25 -0600 Jon Hunter wrote:
>
> On 12/12/2012 09:06 PM, NeilBrown wrote:
> >
> >>> +
> >>> +#if CONFIG_PM
> >>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t
> >>> state)
> >>> +{
> >>> + struct omap_chip *omap = platform_get_drvdata(pdev);
> >>> + /* No one preserve these values during suspend so reset them
> >>> + * Otherwise driver leaves PWM unconfigured if same values
> >>> + * passed to pwm_config
> >>> + */
> >>> + omap->period_ns = 0;
> >>> + omap->duty_ns = 0;
> >>
> >>
> >> Hmmm, looks like you are trying to force a reconfiguration after suspend
> >> if the same values are used. Is there an underlying problem here that
> >> you are trying to workaround?
> >
> > I copied that from pwm-samsung.c.
> >
> > The key question is: does a dmtimer preserve all register values over
> > suspend.
> > If so, then I guess we don't need this.
> > If not, we do (because omap_pwm_config short circuits if it thinks the
> > config
> > hasn't changed).
>
> I gave it a quick test on omap3/4 when just operating the timer as a
> counter (not driving a pwm output) and suspend/resume works fine.
> However, it does not work if I enable off mode (via the debugfs). This
> is not enabled by default and may be I should put that on my to-do list
> as well.
I've been playing with off-mode and discovered that the first attempt to set
the PWM after resume didn't work, but subsequent ones did.
I did some digging and came up with the following patch.
With this in place, the omap_pwm_suspend() above is definitely pointless (was
wasn't really useful even without it).
NeilBrown
From: NeilBrown
Date: Mon, 7 Jan 2013 07:53:03 +1100
Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
The context loss handling in dmtimer appears to assume that
omap_dm_timer_set_load_start() or omap_dm_timer_start()
and
omap_dm_timer_stop()
bracket all interactions. Only the first two restore the context and
the last updates the context loss counter.
However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
reasonably be called outside this bracketing, and the fact that they
call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
is expected.
So if, after a transition into and out of off-mode which would cause
the dm timer to loose all state, omap_dm_timer_set_match() is called
before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
will be 'wrong' and this wrong value will be stored context.tclr so
a subsequent omap_dm_timer_start() can fail (As the control register
is wrong).
Simplify this be doing the restore-from-context in
omap_dm_timer_enable() so that whenever the timer is enabled, the
context is correct.
Also update the ctx_loss_count at the same time as we notice it is
wrong - these is no value in delaying this until the
omap_dm_timer_disable() as it cannot change while the timer is
enabled.
Signed-off-by: NeilBrown
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c216696 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
pm_runtime_get_sync(>pdev->dev);
+
+ if (!(timer->capability & OMAP_TIMER_ALWON)) {
+ int loss_count =
+ omap_pm_get_dev_context_loss_count(>pdev->dev);
+ if (loss_count != timer->ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer->ctx_loss_count = loss_count;
+ }
+ }
}
EXPORT_SYMBOL_GPL(omap_dm_timer_enable);
@@ -347,12 +356,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer)
omap_dm_timer_enable(timer);
- if (!(timer->capability & OMAP_TIMER_ALWON)) {
- if (omap_pm_get_dev_context_loss_count(>pdev->dev) !=
- timer->ctx_loss_count)
- omap_timer_restore_context(timer);
- }
-
l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
if (!(l & OMAP_TIMER_CTRL_ST)) {
l |= OMAP_TIMER_CTRL_ST;
@@ -377,10 +380,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
__omap_dm_timer_stop(timer, timer->posted, rate);
- if (!(timer->capability & OMAP_TIMER_ALWON))
- timer->ctx_loss_count =
- omap_pm_get_dev_context_loss_count(>pdev->dev);
-
/*
* Since the register values are computed and written within
* __omap_dm_timer_stop, we need to use read to retrieve the
@@ -494,12 +493,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer
*timer, int autoreload,
omap_dm_timer_enable(timer);
- if (!(timer->capability & OMAP_TIMER_ALWON)) {
- if (omap_pm_get_dev_context_loss_count(>pdev->dev) !=
- timer->ctx_loss_count)
-
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, 13 Dec 2012 11:07:25 -0600 Jon Hunter jon-hun...@ti.com wrote:
On 12/12/2012 09:06 PM, NeilBrown wrote:
+
+#if CONFIG_PM
+static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t
state)
+{
+ struct omap_chip *omap = platform_get_drvdata(pdev);
+ /* No one preserve these values during suspend so reset them
+ * Otherwise driver leaves PWM unconfigured if same values
+ * passed to pwm_config
+ */
+ omap-period_ns = 0;
+ omap-duty_ns = 0;
Hmmm, looks like you are trying to force a reconfiguration after suspend
if the same values are used. Is there an underlying problem here that
you are trying to workaround?
I copied that from pwm-samsung.c.
The key question is: does a dmtimer preserve all register values over
suspend.
If so, then I guess we don't need this.
If not, we do (because omap_pwm_config short circuits if it thinks the
config
hasn't changed).
I gave it a quick test on omap3/4 when just operating the timer as a
counter (not driving a pwm output) and suspend/resume works fine.
However, it does not work if I enable off mode (via the debugfs). This
is not enabled by default and may be I should put that on my to-do list
as well.
I've been playing with off-mode and discovered that the first attempt to set
the PWM after resume didn't work, but subsequent ones did.
I did some digging and came up with the following patch.
With this in place, the omap_pwm_suspend() above is definitely pointless (was
wasn't really useful even without it).
NeilBrown
From: NeilBrown ne...@suse.de
Date: Mon, 7 Jan 2013 07:53:03 +1100
Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
The context loss handling in dmtimer appears to assume that
omap_dm_timer_set_load_start() or omap_dm_timer_start()
and
omap_dm_timer_stop()
bracket all interactions. Only the first two restore the context and
the last updates the context loss counter.
However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
reasonably be called outside this bracketing, and the fact that they
call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
is expected.
So if, after a transition into and out of off-mode which would cause
the dm timer to loose all state, omap_dm_timer_set_match() is called
before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
will be 'wrong' and this wrong value will be stored context.tclr so
a subsequent omap_dm_timer_start() can fail (As the control register
is wrong).
Simplify this be doing the restore-from-context in
omap_dm_timer_enable() so that whenever the timer is enabled, the
context is correct.
Also update the ctx_loss_count at the same time as we notice it is
wrong - these is no value in delaying this until the
omap_dm_timer_disable() as it cannot change while the timer is
enabled.
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c216696 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
pm_runtime_get_sync(timer-pdev-dev);
+
+ if (!(timer-capability OMAP_TIMER_ALWON)) {
+ int loss_count =
+ omap_pm_get_dev_context_loss_count(timer-pdev-dev);
+ if (loss_count != timer-ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer-ctx_loss_count = loss_count;
+ }
+ }
}
EXPORT_SYMBOL_GPL(omap_dm_timer_enable);
@@ -347,12 +356,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer)
omap_dm_timer_enable(timer);
- if (!(timer-capability OMAP_TIMER_ALWON)) {
- if (omap_pm_get_dev_context_loss_count(timer-pdev-dev) !=
- timer-ctx_loss_count)
- omap_timer_restore_context(timer);
- }
-
l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
if (!(l OMAP_TIMER_CTRL_ST)) {
l |= OMAP_TIMER_CTRL_ST;
@@ -377,10 +380,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
__omap_dm_timer_stop(timer, timer-posted, rate);
- if (!(timer-capability OMAP_TIMER_ALWON))
- timer-ctx_loss_count =
- omap_pm_get_dev_context_loss_count(timer-pdev-dev);
-
/*
* Since the register values are computed and written within
* __omap_dm_timer_stop, we need to use read to retrieve the
@@ -494,12 +493,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer
*timer, int autoreload,
omap_dm_timer_enable(timer);
- if (!(timer-capability OMAP_TIMER_ALWON)) {
- if (omap_pm_get_dev_context_loss_count(timer-pdev-dev) !=
- timer-ctx_loss_count)
-
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, 13 Dec 2012 11:42:18 -0600 Jon Hunter wrote:
>
> On 12/12/2012 10:33 PM, NeilBrown wrote:
> > On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown wrote:
> >
> +omap_dm_timer_enable(omap->dm_timer);
> >>>
> >>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match
> >>> will enable the timer. So this should not be necessary.
> >>
> >> True. That is what you get for copying someone else's code and not
> >> understanding it fully.
> >
> > However omap_dm_timer_write_counter *doesn't* enable the timer, and
> > explicitly checks that it is already runtime-enabled.
> >
> > Does that mean I don't need to call omap_dm_timer_write_counter here? Or
> > does it mean that I do need the enable/disable pair?
>
> Typically, omap_dm_timer_write_counter() is used to update the counter
> value while the counter is running and hence is enabled.
>
> Looking at the code, some more I now see what they are trying to do. It
> seems that they are trying to force an overflow to occur as soon as they
> enable the timer. This will cause the timer to load the count value from
> the timer load register into the timer counter register. So that does
> make sense to me. However, this should not be necessary as
> omap_dm_timer_set_load should do this for you. Therefore, I think that
> you could accomplish the same thing by doing ...
>
> omap_pwm_config
> --> omap_dm_timer_set_load()
> --> omap_dm_timer_set_match()
> --> omap_dm_timer_set_pwm()
>
> omap_pwm_enable
> --> omap_dm_timer_start()
>
> If we call _set_load in config then we don't need to call _load_start in
> the enable, we can just call _start.
>
> Can you try this and see if this is working ok?
Seems to work, and is much neater. Thanks.
Below is my current patch.
Unresolved issues are:
- it uses
omap_dm_timer_request_specific()
which apparently isn't ideal.
- It still zeros things in the suspend routine. I haven't explored this at
all yet
Thanks,
NeilBrown
From 69ed735d1bc377e8e65345792997f809e60b5fbf Mon Sep 17 00:00:00 2001
From: NeilBrown
Date: Sun, 2 Dec 2012 14:53:20 +1100
Subject: [PATCH] pwm: omap: Add PWM support using dual-mode timers
This patch is based on an earlier patch by Grant Erickson
which provided PWM devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Platform data must be provided to identify which dmtimer to use.
Lots of cleanup and inprovements thanks to Thierry Reding
and Jon Hunter.
Cc: Grant Erickson
Signed-off-by: NeilBrown
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..32c1253 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate "OMAP PWM support"
+ depends on ARCH_OMAP && OMAP_DM_TIMER
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate "PKUnity NetBook-0916 PWM support"
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..344072c
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,271 @@
+/*
+ *Copyright (c) 2012 NeilBrown
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP support for the generic, Linux
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define DM_TIMER_LOAD_MIN 0xFFFE
+
+struct omap_chip {
+ struct omap_dm_timer*dm_timer;
+ enum pwm_polarity polarity;
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+/**
+ * pwm_calc_value - Determine the counter value for a clock rate and period.
+ *
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, 13 Dec 2012 11:42:18 -0600 Jon Hunter jon-hun...@ti.com wrote:
On 12/12/2012 10:33 PM, NeilBrown wrote:
On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown ne...@suse.de wrote:
+omap_dm_timer_enable(omap-dm_timer);
Do you need to call omap_dm_timer_enable here? _set_load and _set_match
will enable the timer. So this should not be necessary.
True. That is what you get for copying someone else's code and not
understanding it fully.
However omap_dm_timer_write_counter *doesn't* enable the timer, and
explicitly checks that it is already runtime-enabled.
Does that mean I don't need to call omap_dm_timer_write_counter here? Or
does it mean that I do need the enable/disable pair?
Typically, omap_dm_timer_write_counter() is used to update the counter
value while the counter is running and hence is enabled.
Looking at the code, some more I now see what they are trying to do. It
seems that they are trying to force an overflow to occur as soon as they
enable the timer. This will cause the timer to load the count value from
the timer load register into the timer counter register. So that does
make sense to me. However, this should not be necessary as
omap_dm_timer_set_load should do this for you. Therefore, I think that
you could accomplish the same thing by doing ...
omap_pwm_config
-- omap_dm_timer_set_load()
-- omap_dm_timer_set_match()
-- omap_dm_timer_set_pwm()
omap_pwm_enable
-- omap_dm_timer_start()
If we call _set_load in config then we don't need to call _load_start in
the enable, we can just call _start.
Can you try this and see if this is working ok?
Seems to work, and is much neater. Thanks.
Below is my current patch.
Unresolved issues are:
- it uses
omap_dm_timer_request_specific()
which apparently isn't ideal.
- It still zeros things in the suspend routine. I haven't explored this at
all yet
Thanks,
NeilBrown
From 69ed735d1bc377e8e65345792997f809e60b5fbf Mon Sep 17 00:00:00 2001
From: NeilBrown ne...@suse.de
Date: Sun, 2 Dec 2012 14:53:20 +1100
Subject: [PATCH] pwm: omap: Add PWM support using dual-mode timers
This patch is based on an earlier patch by Grant Erickson
which provided PWM devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Platform data must be provided to identify which dmtimer to use.
Lots of cleanup and inprovements thanks to Thierry Reding
and Jon Hunter.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..32c1253 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP PWM support
+ depends on ARCH_OMAP OMAP_DM_TIMER
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate PKUnity NetBook-0916 PWM support
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..344072c
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,271 @@
+/*
+ *Copyright (c) 2012 NeilBrown ne...@suse.de
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson maratho...@gmail.com
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP support for the generic, Linux
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ */
+
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+#include linux/platform_data/omap-pwm.h
+
+#include plat/dmtimer.h
+
+#define DM_TIMER_LOAD_MIN 0xFFFE
+
+struct omap_chip {
+ struct omap_dm_timer*dm_timer;
+ enum pwm_polarity polarity;
+ unsigned intduty_ns, period_ns;
+ struct
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 10:33 PM, NeilBrown wrote: > On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown wrote: > + omap_dm_timer_enable(omap->dm_timer); >>> >>> Do you need to call omap_dm_timer_enable here? _set_load and _set_match >>> will enable the timer. So this should not be necessary. >> >> True. That is what you get for copying someone else's code and not >> understanding it fully. > > However omap_dm_timer_write_counter *doesn't* enable the timer, and > explicitly checks that it is already runtime-enabled. > > Does that mean I don't need to call omap_dm_timer_write_counter here? Or > does it mean that I do need the enable/disable pair? Typically, omap_dm_timer_write_counter() is used to update the counter value while the counter is running and hence is enabled. Looking at the code, some more I now see what they are trying to do. It seems that they are trying to force an overflow to occur as soon as they enable the timer. This will cause the timer to load the count value from the timer load register into the timer counter register. So that does make sense to me. However, this should not be necessary as omap_dm_timer_set_load should do this for you. Therefore, I think that you could accomplish the same thing by doing ... omap_pwm_config --> omap_dm_timer_set_load() --> omap_dm_timer_set_match() --> omap_dm_timer_set_pwm() omap_pwm_enable --> omap_dm_timer_start() If we call _set_load in config then we don't need to call _load_start in the enable, we can just call _start. Can you try this and see if this is working ok? Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
* NeilBrown [121212 19:09]: > On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter wrote: > > On 12/12/2012 02:24 AM, NeilBrown wrote: > > > + > > > + /* Request the OMAP dual-mode timer that will be bound to and > > > + * associated with this generic PWM. > > > + */ > > > + > > > + omap->dm_timer = omap_dm_timer_request_specific(timer); > > > > I would recommend that you use omap_dm_timer_request_by_cap() (new for > > v3.8 so you should be able to use once v3.8-rc1 is out) here to request > > a timer that supports the PWM output. The above function will not be > > supported when booting with device-tree. > > I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do > this now. > Would you object to the patch being submitted with the current call, then an > update when I do move on to 3.8? > > However I may be misunderstanding something, but I want a timer to drive > a particular output pin - GPIO-57. And I thought that it could only be > driver by GPT11. So I need to explicitly request number 11 don't I? Yes I believe it needs to tied to a specific GPT instance to have access to the pin. We should not export omap_dm_timer_request_specific(), that information should become from platform_data and device tree. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
* Jon Hunter [121213 09:11]: > On 12/12/2012 09:06 PM, NeilBrown wrote: > > On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter wrote: > >> On 12/12/2012 02:24 AM, NeilBrown wrote: > >>> + > >>> +#include > >> > >> This is going to be a problem for the single zImage work, because we > >> cannot include any plat headers in driver code any more. Therefore, > >> although it is not ideal, one way to handle this is pass function > >> pointers to the various dmtimer APIs that are needed via the platform > >> data. Painful I know ... > > > > But that doesn't work with devicetree does it? > > Ugh, you are right! This is becoming an increasing problem. > > > Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or > > something? > > I can ask Tony if he thinks we could do that. Yeah we need to fix this somehow. First we need to limit that header to the minimum and have most of it in a local header file for the clocksource and clockevent. Then let's move the minimal header to include/linux/omap-dmtimer.h until we have something Linux generic available for doing things like this. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 09:06 PM, NeilBrown wrote:
>
> [Thierry: question for you near the end - thanks]
>
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter wrote:
>
>> Hi Neil,
>>
>> On 12/12/2012 02:24 AM, NeilBrown wrote:
>>>
>>>
>>> This patch is based on an earlier patch by Grant Erickson
>>> which provided pwm devices using the 'legacy' interface.
>>>
>>> This driver instead uses the new framework interface.
>>>
>>> Cc: Grant Erickson
>>> Signed-off-by: NeilBrown
>>>
>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>> index ed81720..7df573a 100644
>>> --- a/drivers/pwm/Kconfig
>>> +++ b/drivers/pwm/Kconfig
>>> @@ -85,6 +85,15 @@ config PWM_MXS
>>> To compile this driver as a module, choose M here: the module
>>> will be called pwm-mxs.
>>>
>>> +config PWM_OMAP
>>> + tristate "OMAP pwm support"
>>> + depends on ARCH_OMAP
>>
>> We should probably have depends on or selects OMAP_DM_TIMER here too.
>
> Sounds sensible - fixed.
>
>>
>>> + help
>>> + Generic PWM framework driver for OMAP
>>> +
>>> + To compile this driver as a module, choose M here: the module
>>> + will be called pwm-omap
>>> +
>>> config PWM_PUV3
>>> tristate "PKUnity NetBook-0916 PWM support"
>>> depends on ARCH_PUV3
>>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>>> index acfe482..f5d200d 100644
>>> --- a/drivers/pwm/Makefile
>>> +++ b/drivers/pwm/Makefile
>>> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
>>> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
>>> obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
>>> obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
>>> +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
>>> obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
>>> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
>>> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
>>> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
>>> new file mode 100644
>>> index 000..e3dbce3
>>> --- /dev/null
>>> +++ b/drivers/pwm/pwm-omap.c
>>> @@ -0,0 +1,318 @@
>>> +/*
>>> + *Copyright (c) 2012 NeilBrown
>>> + *Heavily based on earlier code which is:
>>> + *Copyright (c) 2010 Grant Erickson
>>> + *
>>> + *Also based on pwm-samsung.c
>>> + *
>>> + *This program is free software; you can redistribute it and/or
>>> + *modify it under the terms of the GNU General Public License
>>> + *version 2 as published by the Free Software Foundation.
>>> + *
>>> + *Description:
>>> + * This file is the core OMAP2/3 support for the generic, Linux
>>
>> I would drop the OMAP2/3 and just say OMAP here. Potentially this should
>> work for OMAP1-5.
>>
>
> Done.
>
>
>>> + * PWM driver / controller, using the OMAP's dual-mode timers.
>>> + *
>>> + *The 'id' number for the device encodes the number of the dm timer
>>> + *to use, and the polarity of the output.
>>> + *lsb is '1' of active-high, and '0' for active low
>>> + *remaining bit a timer number and need to be shifted down before use.
>>> + */
>>> +
>>> +#define pr_fmt(fmt) "pwm-omap: " fmt
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include
>>
>> This is going to be a problem for the single zImage work, because we
>> cannot include any plat headers in driver code any more. Therefore,
>> although it is not ideal, one way to handle this is pass function
>> pointers to the various dmtimer APIs that are needed via the platform
>> data. Painful I know ...
>
> But that doesn't work with devicetree does it?
Ugh, you are right! This is becoming an increasing problem.
> Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
I can ask Tony if he thinks we could do that.
> It only included other things from include/linux, so it should be safe.
>
>>
>>> +#define DM_TIMER_LOAD_MIN 0xFFFE
>>> +
>>> +struct omap_chip {
>>> + struct platform_device *pdev;
>>> +
>>> + struct omap_dm_timer*dm_timer;
>>> + unsigned intpolarity;
>>> + const char *label;
>>> +
>>> + unsigned intduty_ns, period_ns;
>>> + struct pwm_chip chip;
>>> +};
>>> +
>>> +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
>>> +
>>> +#definepwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
>>> +
>>> +/**
>>> + * pwm_calc_value - determines the counter value for a clock rate and
>>> period.
>>> + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute
>>> the
>>> + *counter value for.
>>> + * @ns: The period, in nanoseconds, to computer the counter value for.
>>> + *
>>> + * Returns the PWM counter value for the specified clock rate and period.
>>> + */
>>> +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
>>> +{
>>> + const unsigned long nanoseconds_per_second = 10;
>>> + int cycles;
>>> + __u64 c;
>>> +
>>> + c =
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 09:06 PM, NeilBrown wrote:
[Thierry: question for you near the end - thanks]
On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter jon-hun...@ti.com wrote:
Hi Neil,
On 12/12/2012 02:24 AM, NeilBrown wrote:
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP pwm support
+ depends on ARCH_OMAP
We should probably have depends on or selects OMAP_DM_TIMER here too.
Sounds sensible - fixed.
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate PKUnity NetBook-0916 PWM support
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..e3dbce3
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,318 @@
+/*
+ *Copyright (c) 2012 NeilBrown ne...@suse.de
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson maratho...@gmail.com
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP2/3 support for the generic, Linux
I would drop the OMAP2/3 and just say OMAP here. Potentially this should
work for OMAP1-5.
Done.
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
+ */
+
+#define pr_fmt(fmt) pwm-omap: fmt
+
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+
+#include plat/dmtimer.h
This is going to be a problem for the single zImage work, because we
cannot include any plat headers in driver code any more. Therefore,
although it is not ideal, one way to handle this is pass function
pointers to the various dmtimer APIs that are needed via the platform
data. Painful I know ...
But that doesn't work with devicetree does it?
Ugh, you are right! This is becoming an increasing problem.
Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
I can ask Tony if he thinks we could do that.
It only included other things from include/linux, so it should be safe.
+#define DM_TIMER_LOAD_MIN 0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
+
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
+ const char *label;
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#definepwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and
period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute
the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
+ int cycles;
+ __u64 c;
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device
Re: [PATCH] OMAP: add pwm driver using dmtimers.
* Jon Hunter jon-hun...@ti.com [121213 09:11]: On 12/12/2012 09:06 PM, NeilBrown wrote: On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter jon-hun...@ti.com wrote: On 12/12/2012 02:24 AM, NeilBrown wrote: + +#include plat/dmtimer.h This is going to be a problem for the single zImage work, because we cannot include any plat headers in driver code any more. Therefore, although it is not ideal, one way to handle this is pass function pointers to the various dmtimer APIs that are needed via the platform data. Painful I know ... But that doesn't work with devicetree does it? Ugh, you are right! This is becoming an increasing problem. Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something? I can ask Tony if he thinks we could do that. Yeah we need to fix this somehow. First we need to limit that header to the minimum and have most of it in a local header file for the clocksource and clockevent. Then let's move the minimal header to include/linux/omap-dmtimer.h until we have something Linux generic available for doing things like this. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
* NeilBrown ne...@suse.de [121212 19:09]: On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter jon-hun...@ti.com wrote: On 12/12/2012 02:24 AM, NeilBrown wrote: + + /* Request the OMAP dual-mode timer that will be bound to and + * associated with this generic PWM. + */ + + omap-dm_timer = omap_dm_timer_request_specific(timer); I would recommend that you use omap_dm_timer_request_by_cap() (new for v3.8 so you should be able to use once v3.8-rc1 is out) here to request a timer that supports the PWM output. The above function will not be supported when booting with device-tree. I wasn't planning on rushing into working on 3.8-rcX so I'd rather not do this now. Would you object to the patch being submitted with the current call, then an update when I do move on to 3.8? However I may be misunderstanding something, but I want a timer to drive a particular output pin - GPIO-57. And I thought that it could only be driver by GPT11. So I need to explicitly request number 11 don't I? Yes I believe it needs to tied to a specific GPT instance to have access to the pin. We should not export omap_dm_timer_request_specific(), that information should become from platform_data and device tree. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 10:33 PM, NeilBrown wrote: On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown ne...@suse.de wrote: + omap_dm_timer_enable(omap-dm_timer); Do you need to call omap_dm_timer_enable here? _set_load and _set_match will enable the timer. So this should not be necessary. True. That is what you get for copying someone else's code and not understanding it fully. However omap_dm_timer_write_counter *doesn't* enable the timer, and explicitly checks that it is already runtime-enabled. Does that mean I don't need to call omap_dm_timer_write_counter here? Or does it mean that I do need the enable/disable pair? Typically, omap_dm_timer_write_counter() is used to update the counter value while the counter is running and hence is enabled. Looking at the code, some more I now see what they are trying to do. It seems that they are trying to force an overflow to occur as soon as they enable the timer. This will cause the timer to load the count value from the timer load register into the timer counter register. So that does make sense to me. However, this should not be necessary as omap_dm_timer_set_load should do this for you. Therefore, I think that you could accomplish the same thing by doing ... omap_pwm_config -- omap_dm_timer_set_load() -- omap_dm_timer_set_match() -- omap_dm_timer_set_pwm() omap_pwm_enable -- omap_dm_timer_start() If we call _set_load in config then we don't need to call _load_start in the enable, we can just call _start. Can you try this and see if this is working ok? Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, Dec 13, 2012 at 01:38:28PM +1100, NeilBrown wrote:
> On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding
> wrote:
>
> > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[...]
> > > + struct omap_dm_timer*dm_timer;
> > > + unsigned intpolarity;
> >
> > The PWM subsystem already has enum pwm_polarity for this.
> >
>
> I'll use that then and as there is a pwm_set_polarity() interface, that
> probably means that I don't need to configure the polarity via the platform
> data? That would be a lot cleaner.
I guess the answer to that question is: it depends. If the user can set
the polarity (via platform or other means), then yes, you don't have to
pass it in here. However there may be users that don't support setting
the polarity or there may even be situations where the PWM goes through
an additional inverter on the board and therefore doesn't need the
polarity inversed after all, even if the user driver requests it.
Generally though I think that it is up to the user drivers to take care
of this and call pwm_set_polarity() as appropriate, so yes, I don't
think you have to explicitly pass it via platform data at all.
> > > + if (omap->duty_ns == duty_ns &&
> > > + omap->period_ns == period_ns)
> > > + /* No change - don't cause any transients */
> > > + return 0;
> >
> > Note to self: this might be a candidate to put in the core.
>
> might be useful, though the core doesn't currently "know" the current values.
Yes, but that can be changed. PWM is still a very young subsystem and
I'm trying to be cautious not to add too much cruft to it unless it's
really worth it.
> > > + omap_dm_timer_set_pwm(omap->dm_timer,
> > > + !omap->polarity,
> > > + toggle,
> > > + trigger);
> >
> > This doesn't either. Also you should be explicit about the polarity
> > parameter, since enum pwm_polarity is an enum and therefore negating it
> > isn't very nice (it should work though).
> >
> > You could solve this by doing something like:
> >
> > if (omap->polarity == PWM_POLARITY_NORMAL)
> > polarity = 1;
> > else
> > polarity = 0;
>
> (omap->polarity == PWM_POLARITY_NORMAL)
>
> would have the same effect.
Yes, that should work as well. However I'm not a friend of using such
expressions in a function call. But since you'll probably be reworking
this anyway because of the pwm_set_polarity() comments from above you
might just want to stick the proper value into omap->polarity in your
.set_polarity() implementation and not need the extra negation here.
> > > +static int __devinit omap_pwm_probe(struct platform_device *pdev)
> >
> > No more __devinit, please.
>
> If you say so (having no idea what it did :-)
This was used to mark functions depending on whether HOTPLUG was enabled
or not. For instance functions marked __devinit could be discarded after
the init stage if HOTPLUG was disabled because it would be guaranteed to
not be called after the init stage. Recently however HOTPLUG was changed
to be always enabled because the gains were very small and most people
would get them wrong anyway.
> > > +#if CONFIG_PM
> > > +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t
> > > state)
> > > +{
> > > + struct omap_chip *omap = platform_get_drvdata(pdev);
> > > + /* No one preserve these values during suspend so reset them
> > > + * Otherwise driver leaves PWM unconfigured if same values
> > > + * passed to pwm_config
> > > + */
> > > + omap->period_ns = 0;
> > > + omap->duty_ns = 0;
> > > +
> > > + return 0;
> > > +}
> > > +#else
> > > +#define omap_pwm_suspend NULL
> > > +#endif
> >
> > This doesn't look right. You should implement .resume() if you really
> > care, in which case the resume callback would have to reconfigure with
> > the cached values. In that case maybe you should switch to dev_pm_ops
> > and SIMPLE_DEV_PM_OPS() as well.
> >
> > If you don't, just resetting these values will not make the PWM work
> > properly after resume either since it will have to be explicitly
> > reconfigured.
>
> I just copied that from pwm-samsung.c
>
> I think the point is to avoid the "no transients" short-circuit in
> omap_pwm_config if the config is unchanged.
>
> The assumption is that pwm_disable() will be called before suspend and
> pwm_config()/pwm_enable() after resume. So there is no point actually
> configuring anything in .resume() - it makes sense to wait until pwm_config()
> is called (if ever). But we want to make sure that pwm_config actually does
> something.
Okay, that makes sense. User drivers should actually be better suited to
reset PWM devices to their proper state on resume.
> > > +MODULE_AUTHOR("Grant Erickson ");
> > > +MODULE_AUTHOR("NeilBrown ");
> >
> > Shouldn't this be "Neil Brown"? I noticed you use the concatenated form
> > in the email address as well, so maybe that's on purpose?
>
> Yes, it is on purpose.
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, Dec 13, 2012 at 02:06:35PM +1100, NeilBrown wrote:
>
> [Thierry: question for you near the end - thanks]
>
> On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter wrote:
>
> > Hi Neil,
> >
> > On 12/12/2012 02:24 AM, NeilBrown wrote:
[...]
> > > +{
> > > + struct omap_chip *omap = platform_get_drvdata(pdev);
> > > + int status = 0;
> > > +
> > > + status = pwmchip_remove(>chip);
> > > + if (status < 0)
> > > + goto done;
> > > +
> > > + omap_dm_timer_free(omap->dm_timer);
> >
> > Is it guaranteed that the timer will be disabled at this point?
>
> Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
> might not be disabled.
> Thierry: should pwm_put do that, or do I need a 'free' function in my chip
> ops to do that?
To be honest, I haven't decided yet. =) There have been discussions that
resulted in a request to run pwm_disable() from pwmchip_remove() on all
PWM devices a chip provides.
This isn't implemented yet and I'm not sure about all the side-effects.
I think for now the best way would be to implement .free() within this
driver, or even do an explicit pwm_disable() in the driver's .remove()
function to do this. When I've come to a decision I'll refactor all of
that in one patch across the whole subsystem.
Thierry
pgpKbq0nitgDD.pgp
Description: PGP signature
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown wrote: > > > + omap_dm_timer_enable(omap->dm_timer); > > > > Do you need to call omap_dm_timer_enable here? _set_load and _set_match > > will enable the timer. So this should not be necessary. > > True. That is what you get for copying someone else's code and not > understanding it fully. However omap_dm_timer_write_counter *doesn't* enable the timer, and explicitly checks that it is already runtime-enabled. Does that mean I don't need to call omap_dm_timer_write_counter here? Or does it mean that I do need the enable/disable pair? > > > > > > + omap_dm_timer_set_load(omap->dm_timer, autoreload, load_value); > > > + omap_dm_timer_set_match(omap->dm_timer, enable, match_value); > > > + > > > + dev_dbg(chip->dev, > > > + "load value: %#08x (%d), " > > > + "match value: %#08x (%d)\n", > > > + load_value, load_value, > > > + match_value, match_value); > > > + > > > + omap_dm_timer_set_pwm(omap->dm_timer, > > > + !omap->polarity, > > > + toggle, > > > + trigger); > > > + > > > + /* Set the counter to generate an overflow event immediately. */ > > > + > > > + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN); > > > + > > > + /* Now that we're done configuring the dual-mode timer, disable it > > > + * again. We'll enable and start it later, when requested. > > > + */ > > > + > > > + omap_dm_timer_disable(omap->dm_timer); > > > > Similarly the disable should not be needed here either. > > Thanks, NeilBrown signature.asc Description: PGP signature
Re: [PATCH] OMAP: add pwm driver using dmtimers.
[Thierry: question for you near the end - thanks]
On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter wrote:
> Hi Neil,
>
> On 12/12/2012 02:24 AM, NeilBrown wrote:
> >
> >
> > This patch is based on an earlier patch by Grant Erickson
> > which provided pwm devices using the 'legacy' interface.
> >
> > This driver instead uses the new framework interface.
> >
> > Cc: Grant Erickson
> > Signed-off-by: NeilBrown
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..7df573a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -85,6 +85,15 @@ config PWM_MXS
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-mxs.
> >
> > +config PWM_OMAP
> > + tristate "OMAP pwm support"
> > + depends on ARCH_OMAP
>
> We should probably have depends on or selects OMAP_DM_TIMER here too.
Sounds sensible - fixed.
>
> > + help
> > + Generic PWM framework driver for OMAP
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-omap
> > +
> > config PWM_PUV3
> > tristate "PKUnity NetBook-0916 PWM support"
> > depends on ARCH_PUV3
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index acfe482..f5d200d 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
> > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
> > obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
> > +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
> > obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
> > new file mode 100644
> > index 000..e3dbce3
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-omap.c
> > @@ -0,0 +1,318 @@
> > +/*
> > + *Copyright (c) 2012 NeilBrown
> > + *Heavily based on earlier code which is:
> > + *Copyright (c) 2010 Grant Erickson
> > + *
> > + *Also based on pwm-samsung.c
> > + *
> > + *This program is free software; you can redistribute it and/or
> > + *modify it under the terms of the GNU General Public License
> > + *version 2 as published by the Free Software Foundation.
> > + *
> > + *Description:
> > + * This file is the core OMAP2/3 support for the generic, Linux
>
> I would drop the OMAP2/3 and just say OMAP here. Potentially this should
> work for OMAP1-5.
>
Done.
> > + * PWM driver / controller, using the OMAP's dual-mode timers.
> > + *
> > + *The 'id' number for the device encodes the number of the dm timer
> > + *to use, and the polarity of the output.
> > + *lsb is '1' of active-high, and '0' for active low
> > + *remaining bit a timer number and need to be shifted down before use.
> > + */
> > +
> > +#define pr_fmt(fmt) "pwm-omap: " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
>
> This is going to be a problem for the single zImage work, because we
> cannot include any plat headers in driver code any more. Therefore,
> although it is not ideal, one way to handle this is pass function
> pointers to the various dmtimer APIs that are needed via the platform
> data. Painful I know ...
But that doesn't work with devicetree does it?
Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
It only included other things from include/linux, so it should be safe.
>
> > +#define DM_TIMER_LOAD_MIN 0xFFFE
> > +
> > +struct omap_chip {
> > + struct platform_device *pdev;
> > +
> > + struct omap_dm_timer*dm_timer;
> > + unsigned intpolarity;
> > + const char *label;
> > +
> > + unsigned intduty_ns, period_ns;
> > + struct pwm_chip chip;
> > +};
> > +
> > +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
> > +
> > +#definepwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
> > +
> > +/**
> > + * pwm_calc_value - determines the counter value for a clock rate and
> > period.
> > + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute
> > the
> > + *counter value for.
> > + * @ns: The period, in nanoseconds, to computer the counter value for.
> > + *
> > + * Returns the PWM counter value for the specified clock rate and period.
> > + */
> > +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
> > +{
> > + const unsigned long nanoseconds_per_second = 10;
> > + int cycles;
> > + __u64 c;
> > +
> > + c = (__u64)clk_rate * ns;
> > + do_div(c, nanoseconds_per_second);
> > + cycles = c;
> > +
> > + return DM_TIMER_LOAD_MIN - cycles;
> > +}
> > +
> > +static int omap_pwm_enable(struct pwm_chip
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Wed, 12 Dec 2012 10:20:34 -0600 Jon Hunter wrote:
>
> On 12/12/2012 05:31 AM, Thierry Reding wrote:
> > On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
>
> [snip]
>
> >> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >> +{
> >> + struct omap_chip *omap = to_omap_chip(chip);
> >> + int status = 0;
> >> +
> >> + /* Enable the counter--always--before attempting to write its
> >> + * registers and then set the timer to its minimum load value to
> >> + * ensure we get an overflow event right away once we start it.
> >> + */
> >
> > Block comments should be in the following format:
> >
> > /*
> > * foo...
> > * bar...
> > */
> >
> >> +
> >> + omap_dm_timer_enable(omap->dm_timer);
> >> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> >> + omap_dm_timer_start(omap->dm_timer);
> >> + omap_dm_timer_disable(omap->dm_timer);
> >
> > So omap_dm_timer_disable() doesn't actually stop the timer? It just
> > disables the access to the registers?
>
> I thought this looked odd too ;-)
>
> So what is going on here is that omap_dm_timer_start() calls
> omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the
> last disable really just complements the first enable (ie. decrements
> the use count), but the timer will not actually be disabled, because the
> start has called an extra enable.
>
> These four function calls can be replaced by one call to
> omap_dm_timer_set_load_start() and I think that will be much clearer and
> concise.
So it now reads:
/*
* Set the timer to its minimum load value to ensure we get an
* overflow event right away once we start it.
*/
omap_dm_timer_set_load_start(omap->dm_timer, true, DM_TIMER_LOAD_MIN);
Certainly more concise - thanks.
>
> In general, it should not be necessary to call these
> omap_dm_timer_enable/disable APIs directly. I am not sure what the
> history is or if there is a use-case that really requires this. So in
> the future may be I should make them static so they cannot be used
> directly :-)
I've removed the other instance of these calls - in omap_pwm_config.
Thanks,
NeilBrown
signature.asc
Description: PGP signature
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding
wrote:
> On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
> >
> >
> > This patch is based on an earlier patch by Grant Erickson
> > which provided pwm devices using the 'legacy' interface.
> >
> > This driver instead uses the new framework interface.
>
> I'd prefer some kind of description about the driver here.
I'm not really sure what more there is to say. There was a bit of text in a
comment at the top of the file which I've copied to the commit comment.
>Also the
> subject should be something like:
>
> pwm: Add OMAP support using dual-mode timers
>
> or
>
> pwm: omap: Add PWM support using dual-mode timers
Done - I chose the second.
>
> I take this description to mean that OMAP doesn't have dedicated PWM
> hardware? Otherwise it might be bad to call this pwm-omap.
Correct. The timers can be used for a number of things which explicitly
includes PWM.
>
> Also please use all-caps when referring to PWM devices in prose. A few
> other comments inline below.
OK.
>
> > Cc: Grant Erickson
> > Signed-off-by: NeilBrown
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index ed81720..7df573a 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -85,6 +85,15 @@ config PWM_MXS
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-mxs.
> >
> > +config PWM_OMAP
> > + tristate "OMAP pwm support"
>
> "OMAP PWM support"
Fixed.
>
> > diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
> [...]
> > + *The 'id' number for the device encodes the number of the dm timer
> > + *to use, and the polarity of the output.
> > + *lsb is '1' of active-high, and '0' for active low
> > + *remaining bit a timer number and need to be shifted down before use.
>
> I don't know if this is such a good idea. Usually you number platform
> devices sequentially, while this would leave gaps in the numbering. I
> know that adding platform data may sound a bit like overkill, but I
> really think the added clarity and consistency is worth it.
I guess so. No other PWM driver seems to use platform data, and I needed so
little...
I'll see what I can do.
>
> > +#define pr_fmt(fmt) "pwm-omap: " fmt
>
> You don't seem to be using any of the pr_*() logging functions, so this
> isn't needed.
Gone now, thanks.
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#define DM_TIMER_LOAD_MIN 0xFFFE
> > +
> > +struct omap_chip {
> > + struct platform_device *pdev;
>
> I don't see this field being used anywhere.
No. Gone.
>
> > + struct omap_dm_timer*dm_timer;
> > + unsigned intpolarity;
>
> The PWM subsystem already has enum pwm_polarity for this.
>
I'll use that then and as there is a pwm_set_polarity() interface, that
probably means that I don't need to configure the polarity via the platform
data? That would be a lot cleaner.
> > + const char *label;
>
> This isn't used anywhere either.
Gone.
>
> > +
> > + unsigned intduty_ns, period_ns;
> > + struct pwm_chip chip;
> > +};
> > +
> > +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
> > +
> > +#definepwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
>
> This is never used.
:-) There is a theme here.
>
> > +
> > +/**
> > + * pwm_calc_value - determines the counter value for a clock rate and
> > period.
>
> Nit: You should either start the sentence with a capital or not
> terminate it with a full stop.
In this case the sentence really includes the function name which is
case-sensitive so cannot be capitalised ;-)
I'll rephrase a bit and find something to capitalise.
>
> > + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute
> > the
> > + *counter value for.
> > + * @ns: The period, in nanoseconds, to computer the counter value for.
>
> "compute"
Yep.
>
> > + *
> > + * Returns the PWM counter value for the specified clock rate and period.
> > + */
> > +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
> > +{
> > + const unsigned long nanoseconds_per_second = 10;
>
> Maybe use NSEC_PER_SEC instead?
Good idea, thanks.
>
> > + int cycles;
> > + __u64 c;
>
> I think for in-kernel use, the custom is to stick with simply u64.
It is, yes.
>
> > +
> > + c = (__u64)clk_rate * ns;
> > + do_div(c, nanoseconds_per_second);
> > + cycles = c;
> > +
> > + return DM_TIMER_LOAD_MIN - cycles;
>
> Can't you just do "DM_TIMER_LOAD_MIN - c" and get rid of the cycles
> variable altogether?
Yep.
>
> > +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct omap_chip *omap =
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 05:31 AM, Thierry Reding wrote:
> On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[snip]
>> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +struct omap_chip *omap = to_omap_chip(chip);
>> +int status = 0;
>> +
>> +/* Enable the counter--always--before attempting to write its
>> + * registers and then set the timer to its minimum load value to
>> + * ensure we get an overflow event right away once we start it.
>> + */
>
> Block comments should be in the following format:
>
> /*
>* foo...
>* bar...
>*/
>
>> +
>> +omap_dm_timer_enable(omap->dm_timer);
>> +omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
>> +omap_dm_timer_start(omap->dm_timer);
>> +omap_dm_timer_disable(omap->dm_timer);
>
> So omap_dm_timer_disable() doesn't actually stop the timer? It just
> disables the access to the registers?
I thought this looked odd too ;-)
So what is going on here is that omap_dm_timer_start() calls
omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the
last disable really just complements the first enable (ie. decrements
the use count), but the timer will not actually be disabled, because the
start has called an extra enable.
These four function calls can be replaced by one call to
omap_dm_timer_set_load_start() and I think that will be much clearer and
concise.
In general, it should not be necessary to call these
omap_dm_timer_enable/disable APIs directly. I am not sure what the
history is or if there is a use-case that really requires this. So in
the future may be I should make them static so they cannot be used
directly :-)
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
Hi Neil,
On 12/12/2012 02:24 AM, NeilBrown wrote:
>
>
> This patch is based on an earlier patch by Grant Erickson
> which provided pwm devices using the 'legacy' interface.
>
> This driver instead uses the new framework interface.
>
> Cc: Grant Erickson
> Signed-off-by: NeilBrown
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..7df573a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -85,6 +85,15 @@ config PWM_MXS
> To compile this driver as a module, choose M here: the module
> will be called pwm-mxs.
>
> +config PWM_OMAP
> + tristate "OMAP pwm support"
> + depends on ARCH_OMAP
We should probably have depends on or selects OMAP_DM_TIMER here too.
> + help
> + Generic PWM framework driver for OMAP
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-omap
> +
> config PWM_PUV3
> tristate "PKUnity NetBook-0916 PWM support"
> depends on ARCH_PUV3
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index acfe482..f5d200d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
> obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> obj-$(CONFIG_PWM_LPC32XX)+= pwm-lpc32xx.o
> obj-$(CONFIG_PWM_MXS)+= pwm-mxs.o
> +obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
> obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> obj-$(CONFIG_PWM_PXA)+= pwm-pxa.o
> obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
> new file mode 100644
> index 000..e3dbce3
> --- /dev/null
> +++ b/drivers/pwm/pwm-omap.c
> @@ -0,0 +1,318 @@
> +/*
> + *Copyright (c) 2012 NeilBrown
> + *Heavily based on earlier code which is:
> + *Copyright (c) 2010 Grant Erickson
> + *
> + *Also based on pwm-samsung.c
> + *
> + *This program is free software; you can redistribute it and/or
> + *modify it under the terms of the GNU General Public License
> + *version 2 as published by the Free Software Foundation.
> + *
> + *Description:
> + * This file is the core OMAP2/3 support for the generic, Linux
I would drop the OMAP2/3 and just say OMAP here. Potentially this should
work for OMAP1-5.
> + * PWM driver / controller, using the OMAP's dual-mode timers.
> + *
> + *The 'id' number for the device encodes the number of the dm timer
> + *to use, and the polarity of the output.
> + *lsb is '1' of active-high, and '0' for active low
> + *remaining bit a timer number and need to be shifted down before use.
> + */
> +
> +#define pr_fmt(fmt) "pwm-omap: " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
This is going to be a problem for the single zImage work, because we
cannot include any plat headers in driver code any more. Therefore,
although it is not ideal, one way to handle this is pass function
pointers to the various dmtimer APIs that are needed via the platform
data. Painful I know ...
> +#define DM_TIMER_LOAD_MIN0xFFFE
> +
> +struct omap_chip {
> + struct platform_device *pdev;
> +
> + struct omap_dm_timer*dm_timer;
> + unsigned intpolarity;
> + const char *label;
> +
> + unsigned intduty_ns, period_ns;
> + struct pwm_chip chip;
> +};
> +
> +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
> +
> +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
> +
> +/**
> + * pwm_calc_value - determines the counter value for a clock rate and period.
> + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
> + *counter value for.
> + * @ns: The period, in nanoseconds, to computer the counter value for.
> + *
> + * Returns the PWM counter value for the specified clock rate and period.
> + */
> +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
> +{
> + const unsigned long nanoseconds_per_second = 10;
> + int cycles;
> + __u64 c;
> +
> + c = (__u64)clk_rate * ns;
> + do_div(c, nanoseconds_per_second);
> + cycles = c;
> +
> + return DM_TIMER_LOAD_MIN - cycles;
> +}
> +
> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct omap_chip *omap = to_omap_chip(chip);
> + int status = 0;
> +
> + /* Enable the counter--always--before attempting to write its
> + * registers and then set the timer to its minimum load value to
> + * ensure we get an overflow event right away once we start it.
> + */
> +
> + omap_dm_timer_enable(omap->dm_timer);
> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> + omap_dm_timer_start(omap->dm_timer);
> + omap_dm_timer_disable(omap->dm_timer);
Why not just use
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
>
>
> This patch is based on an earlier patch by Grant Erickson
> which provided pwm devices using the 'legacy' interface.
>
> This driver instead uses the new framework interface.
I'd prefer some kind of description about the driver here. Also the
subject should be something like:
pwm: Add OMAP support using dual-mode timers
or
pwm: omap: Add PWM support using dual-mode timers
I take this description to mean that OMAP doesn't have dedicated PWM
hardware? Otherwise it might be bad to call this pwm-omap.
Also please use all-caps when referring to PWM devices in prose. A few
other comments inline below.
> Cc: Grant Erickson
> Signed-off-by: NeilBrown
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ed81720..7df573a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -85,6 +85,15 @@ config PWM_MXS
> To compile this driver as a module, choose M here: the module
> will be called pwm-mxs.
>
> +config PWM_OMAP
> + tristate "OMAP pwm support"
"OMAP PWM support"
> diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
[...]
> + *The 'id' number for the device encodes the number of the dm timer
> + *to use, and the polarity of the output.
> + *lsb is '1' of active-high, and '0' for active low
> + *remaining bit a timer number and need to be shifted down before use.
I don't know if this is such a good idea. Usually you number platform
devices sequentially, while this would leave gaps in the numbering. I
know that adding platform data may sound a bit like overkill, but I
really think the added clarity and consistency is worth it.
> +#define pr_fmt(fmt) "pwm-omap: " fmt
You don't seem to be using any of the pr_*() logging functions, so this
isn't needed.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define DM_TIMER_LOAD_MIN0xFFFE
> +
> +struct omap_chip {
> + struct platform_device *pdev;
I don't see this field being used anywhere.
> + struct omap_dm_timer*dm_timer;
> + unsigned intpolarity;
The PWM subsystem already has enum pwm_polarity for this.
> + const char *label;
This isn't used anywhere either.
> +
> + unsigned intduty_ns, period_ns;
> + struct pwm_chip chip;
> +};
> +
> +#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
> +
> +#define pwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
This is never used.
> +
> +/**
> + * pwm_calc_value - determines the counter value for a clock rate and period.
Nit: You should either start the sentence with a capital or not
terminate it with a full stop.
> + * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
> + *counter value for.
> + * @ns: The period, in nanoseconds, to computer the counter value for.
"compute"
> + *
> + * Returns the PWM counter value for the specified clock rate and period.
> + */
> +static inline int pwm_calc_value(unsigned long clk_rate, int ns)
> +{
> + const unsigned long nanoseconds_per_second = 10;
Maybe use NSEC_PER_SEC instead?
> + int cycles;
> + __u64 c;
I think for in-kernel use, the custom is to stick with simply u64.
> +
> + c = (__u64)clk_rate * ns;
> + do_div(c, nanoseconds_per_second);
> + cycles = c;
> +
> + return DM_TIMER_LOAD_MIN - cycles;
Can't you just do "DM_TIMER_LOAD_MIN - c" and get rid of the cycles
variable altogether?
> +static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct omap_chip *omap = to_omap_chip(chip);
> + int status = 0;
> +
> + /* Enable the counter--always--before attempting to write its
> + * registers and then set the timer to its minimum load value to
> + * ensure we get an overflow event right away once we start it.
> + */
Block comments should be in the following format:
/*
* foo...
* bar...
*/
> +
> + omap_dm_timer_enable(omap->dm_timer);
> + omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
> + omap_dm_timer_start(omap->dm_timer);
> + omap_dm_timer_disable(omap->dm_timer);
So omap_dm_timer_disable() doesn't actually stop the timer? It just
disables the access to the registers?
> + return status;
"return 0;" and drop the status variable.
> +static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +int duty_ns, int period_ns)
> +{
> + struct omap_chip *omap = to_omap_chip(chip);
> + int status = 0;
This one can be dropped as well.
> + const bool enable = true;
> + const bool autoreload = true;
> + const bool toggle = true;
> + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
I understand that these extra variables are
[PATCH] OMAP: add pwm driver using dmtimers.
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Cc: Grant Erickson
Signed-off-by: NeilBrown
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate "OMAP pwm support"
+ depends on ARCH_OMAP
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate "PKUnity NetBook-0916 PWM support"
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..e3dbce3
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,318 @@
+/*
+ *Copyright (c) 2012 NeilBrown
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP2/3 support for the generic, Linux
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
+ */
+
+#define pr_fmt(fmt) "pwm-omap: " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define DM_TIMER_LOAD_MIN 0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
+
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
+ const char *label;
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#definepwm_dbg(_pwm, msg...) dev_dbg(&(_pwm)->pdev->dev, msg)
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
+ int cycles;
+ __u64 c;
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
+
+ /* Enable the counter--always--before attempting to write its
+* registers and then set the timer to its minimum load value to
+* ensure we get an overflow event right away once we start it.
+*/
+
+ omap_dm_timer_enable(omap->dm_timer);
+ omap_dm_timer_write_counter(omap->dm_timer, DM_TIMER_LOAD_MIN);
+ omap_dm_timer_start(omap->dm_timer);
+ omap_dm_timer_disable(omap->dm_timer);
+
+ return status;
+}
+
+static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+
+ omap_dm_timer_stop(omap->dm_timer);
+}
+
+static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
+ const bool enable = true;
+ const bool autoreload = true;
+ const bool toggle = true;
+ const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
+ int load_value, match_value;
+ unsigned long clk_rate;
+
+ dev_dbg(chip->dev,
+ "duty cycle: %d, period %d\n",
[PATCH] OMAP: add pwm driver using dmtimers.
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP pwm support
+ depends on ARCH_OMAP
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate PKUnity NetBook-0916 PWM support
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..e3dbce3
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,318 @@
+/*
+ *Copyright (c) 2012 NeilBrown ne...@suse.de
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson maratho...@gmail.com
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP2/3 support for the generic, Linux
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
+ */
+
+#define pr_fmt(fmt) pwm-omap: fmt
+
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+
+#include plat/dmtimer.h
+
+#define DM_TIMER_LOAD_MIN 0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
+
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
+ const char *label;
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#definepwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
+ int cycles;
+ __u64 c;
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
+
+ /* Enable the counter--always--before attempting to write its
+* registers and then set the timer to its minimum load value to
+* ensure we get an overflow event right away once we start it.
+*/
+
+ omap_dm_timer_enable(omap-dm_timer);
+ omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
+ omap_dm_timer_start(omap-dm_timer);
+ omap_dm_timer_disable(omap-dm_timer);
+
+ return status;
+}
+
+static void omap_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+
+ omap_dm_timer_stop(omap-dm_timer);
+}
+
+static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
+ const bool enable = true;
+ const bool autoreload = true;
+ const bool toggle = true;
+ const int trigger =
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
I'd prefer some kind of description about the driver here. Also the
subject should be something like:
pwm: Add OMAP support using dual-mode timers
or
pwm: omap: Add PWM support using dual-mode timers
I take this description to mean that OMAP doesn't have dedicated PWM
hardware? Otherwise it might be bad to call this pwm-omap.
Also please use all-caps when referring to PWM devices in prose. A few
other comments inline below.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP pwm support
OMAP PWM support
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
[...]
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
I don't know if this is such a good idea. Usually you number platform
devices sequentially, while this would leave gaps in the numbering. I
know that adding platform data may sound a bit like overkill, but I
really think the added clarity and consistency is worth it.
+#define pr_fmt(fmt) pwm-omap: fmt
You don't seem to be using any of the pr_*() logging functions, so this
isn't needed.
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+
+#include plat/dmtimer.h
+
+#define DM_TIMER_LOAD_MIN0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
I don't see this field being used anywhere.
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
The PWM subsystem already has enum pwm_polarity for this.
+ const char *label;
This isn't used anywhere either.
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#define pwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)
This is never used.
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and period.
Nit: You should either start the sentence with a capital or not
terminate it with a full stop.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
compute
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
Maybe use NSEC_PER_SEC instead?
+ int cycles;
+ __u64 c;
I think for in-kernel use, the custom is to stick with simply u64.
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
Can't you just do DM_TIMER_LOAD_MIN - c and get rid of the cycles
variable altogether?
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
+
+ /* Enable the counter--always--before attempting to write its
+ * registers and then set the timer to its minimum load value to
+ * ensure we get an overflow event right away once we start it.
+ */
Block comments should be in the following format:
/*
* foo...
* bar...
*/
+
+ omap_dm_timer_enable(omap-dm_timer);
+ omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
+ omap_dm_timer_start(omap-dm_timer);
+ omap_dm_timer_disable(omap-dm_timer);
So omap_dm_timer_disable() doesn't actually stop the timer? It just
disables the access to the registers?
+ return status;
return 0; and drop the status variable.
+static int omap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+int duty_ns, int period_ns)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
This one can be dropped as well.
+ const bool enable = true;
+ const bool autoreload = true;
+ const bool toggle = true;
+ const int trigger =
Re: [PATCH] OMAP: add pwm driver using dmtimers.
Hi Neil,
On 12/12/2012 02:24 AM, NeilBrown wrote:
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP pwm support
+ depends on ARCH_OMAP
We should probably have depends on or selects OMAP_DM_TIMER here too.
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate PKUnity NetBook-0916 PWM support
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX)+= pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS)+= pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA)+= pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..e3dbce3
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,318 @@
+/*
+ *Copyright (c) 2012 NeilBrown ne...@suse.de
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson maratho...@gmail.com
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP2/3 support for the generic, Linux
I would drop the OMAP2/3 and just say OMAP here. Potentially this should
work for OMAP1-5.
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
+ */
+
+#define pr_fmt(fmt) pwm-omap: fmt
+
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+
+#include plat/dmtimer.h
This is going to be a problem for the single zImage work, because we
cannot include any plat headers in driver code any more. Therefore,
although it is not ideal, one way to handle this is pass function
pointers to the various dmtimer APIs that are needed via the platform
data. Painful I know ...
+#define DM_TIMER_LOAD_MIN0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
+
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
+ const char *label;
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#define pwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
+ int cycles;
+ __u64 c;
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
+
+ /* Enable the counter--always--before attempting to write its
+ * registers and then set the timer to its minimum load value to
+ * ensure we get an overflow event right away once we start it.
+ */
+
+ omap_dm_timer_enable(omap-dm_timer);
+ omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
+ omap_dm_timer_start(omap-dm_timer);
+
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 05:31 AM, Thierry Reding wrote:
On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[snip]
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+struct omap_chip *omap = to_omap_chip(chip);
+int status = 0;
+
+/* Enable the counter--always--before attempting to write its
+ * registers and then set the timer to its minimum load value to
+ * ensure we get an overflow event right away once we start it.
+ */
Block comments should be in the following format:
/*
* foo...
* bar...
*/
+
+omap_dm_timer_enable(omap-dm_timer);
+omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
+omap_dm_timer_start(omap-dm_timer);
+omap_dm_timer_disable(omap-dm_timer);
So omap_dm_timer_disable() doesn't actually stop the timer? It just
disables the access to the registers?
I thought this looked odd too ;-)
So what is going on here is that omap_dm_timer_start() calls
omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the
last disable really just complements the first enable (ie. decrements
the use count), but the timer will not actually be disabled, because the
start has called an extra enable.
These four function calls can be replaced by one call to
omap_dm_timer_set_load_start() and I think that will be much clearer and
concise.
In general, it should not be necessary to call these
omap_dm_timer_enable/disable APIs directly. I am not sure what the
history is or if there is a use-case that really requires this. So in
the future may be I should make them static so they cannot be used
directly :-)
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding
thierry.red...@avionic-design.de wrote:
On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
I'd prefer some kind of description about the driver here.
I'm not really sure what more there is to say. There was a bit of text in a
comment at the top of the file which I've copied to the commit comment.
Also the
subject should be something like:
pwm: Add OMAP support using dual-mode timers
or
pwm: omap: Add PWM support using dual-mode timers
Done - I chose the second.
I take this description to mean that OMAP doesn't have dedicated PWM
hardware? Otherwise it might be bad to call this pwm-omap.
Correct. The timers can be used for a number of things which explicitly
includes PWM.
Also please use all-caps when referring to PWM devices in prose. A few
other comments inline below.
OK.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP pwm support
OMAP PWM support
Fixed.
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
[...]
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
I don't know if this is such a good idea. Usually you number platform
devices sequentially, while this would leave gaps in the numbering. I
know that adding platform data may sound a bit like overkill, but I
really think the added clarity and consistency is worth it.
I guess so. No other PWM driver seems to use platform data, and I needed so
little...
I'll see what I can do.
+#define pr_fmt(fmt) pwm-omap: fmt
You don't seem to be using any of the pr_*() logging functions, so this
isn't needed.
Gone now, thanks.
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+
+#include plat/dmtimer.h
+
+#define DM_TIMER_LOAD_MIN 0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
I don't see this field being used anywhere.
No. Gone.
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
The PWM subsystem already has enum pwm_polarity for this.
I'll use that then and as there is a pwm_set_polarity() interface, that
probably means that I don't need to configure the polarity via the platform
data? That would be a lot cleaner.
+ const char *label;
This isn't used anywhere either.
Gone.
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#definepwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)
This is never used.
:-) There is a theme here.
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and
period.
Nit: You should either start the sentence with a capital or not
terminate it with a full stop.
In this case the sentence really includes the function name which is
case-sensitive so cannot be capitalised ;-)
I'll rephrase a bit and find something to capitalise.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute
the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
compute
Yep.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
Maybe use NSEC_PER_SEC instead?
Good idea, thanks.
+ int cycles;
+ __u64 c;
I think for in-kernel use, the custom is to stick with simply u64.
It is, yes.
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
Can't you just do DM_TIMER_LOAD_MIN - c and get rid of the cycles
variable altogether?
Yep.
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Wed, 12 Dec 2012 10:20:34 -0600 Jon Hunter jon-hun...@ti.com wrote:
On 12/12/2012 05:31 AM, Thierry Reding wrote:
On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[snip]
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
+
+ /* Enable the counter--always--before attempting to write its
+ * registers and then set the timer to its minimum load value to
+ * ensure we get an overflow event right away once we start it.
+ */
Block comments should be in the following format:
/*
* foo...
* bar...
*/
+
+ omap_dm_timer_enable(omap-dm_timer);
+ omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
+ omap_dm_timer_start(omap-dm_timer);
+ omap_dm_timer_disable(omap-dm_timer);
So omap_dm_timer_disable() doesn't actually stop the timer? It just
disables the access to the registers?
I thought this looked odd too ;-)
So what is going on here is that omap_dm_timer_start() calls
omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the
last disable really just complements the first enable (ie. decrements
the use count), but the timer will not actually be disabled, because the
start has called an extra enable.
These four function calls can be replaced by one call to
omap_dm_timer_set_load_start() and I think that will be much clearer and
concise.
So it now reads:
/*
* Set the timer to its minimum load value to ensure we get an
* overflow event right away once we start it.
*/
omap_dm_timer_set_load_start(omap-dm_timer, true, DM_TIMER_LOAD_MIN);
Certainly more concise - thanks.
In general, it should not be necessary to call these
omap_dm_timer_enable/disable APIs directly. I am not sure what the
history is or if there is a use-case that really requires this. So in
the future may be I should make them static so they cannot be used
directly :-)
I've removed the other instance of these calls - in omap_pwm_config.
Thanks,
NeilBrown
signature.asc
Description: PGP signature
Re: [PATCH] OMAP: add pwm driver using dmtimers.
[Thierry: question for you near the end - thanks]
On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter jon-hun...@ti.com wrote:
Hi Neil,
On 12/12/2012 02:24 AM, NeilBrown wrote:
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP pwm support
+ depends on ARCH_OMAP
We should probably have depends on or selects OMAP_DM_TIMER here too.
Sounds sensible - fixed.
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate PKUnity NetBook-0916 PWM support
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..e3dbce3
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,318 @@
+/*
+ *Copyright (c) 2012 NeilBrown ne...@suse.de
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson maratho...@gmail.com
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP2/3 support for the generic, Linux
I would drop the OMAP2/3 and just say OMAP here. Potentially this should
work for OMAP1-5.
Done.
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
+ */
+
+#define pr_fmt(fmt) pwm-omap: fmt
+
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+
+#include plat/dmtimer.h
This is going to be a problem for the single zImage work, because we
cannot include any plat headers in driver code any more. Therefore,
although it is not ideal, one way to handle this is pass function
pointers to the various dmtimer APIs that are needed via the platform
data. Painful I know ...
But that doesn't work with devicetree does it?
Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
It only included other things from include/linux, so it should be safe.
+#define DM_TIMER_LOAD_MIN 0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
+
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
+ const char *label;
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#definepwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and
period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute
the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
+ int cycles;
+ __u64 c;
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown ne...@suse.de wrote: + omap_dm_timer_enable(omap-dm_timer); Do you need to call omap_dm_timer_enable here? _set_load and _set_match will enable the timer. So this should not be necessary. True. That is what you get for copying someone else's code and not understanding it fully. However omap_dm_timer_write_counter *doesn't* enable the timer, and explicitly checks that it is already runtime-enabled. Does that mean I don't need to call omap_dm_timer_write_counter here? Or does it mean that I do need the enable/disable pair? + omap_dm_timer_set_load(omap-dm_timer, autoreload, load_value); + omap_dm_timer_set_match(omap-dm_timer, enable, match_value); + + dev_dbg(chip-dev, + load value: %#08x (%d), + match value: %#08x (%d)\n, + load_value, load_value, + match_value, match_value); + + omap_dm_timer_set_pwm(omap-dm_timer, + !omap-polarity, + toggle, + trigger); + + /* Set the counter to generate an overflow event immediately. */ + + omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN); + + /* Now that we're done configuring the dual-mode timer, disable it + * again. We'll enable and start it later, when requested. + */ + + omap_dm_timer_disable(omap-dm_timer); Similarly the disable should not be needed here either. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, Dec 13, 2012 at 02:06:35PM +1100, NeilBrown wrote:
[Thierry: question for you near the end - thanks]
On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter jon-hun...@ti.com wrote:
Hi Neil,
On 12/12/2012 02:24 AM, NeilBrown wrote:
[...]
+{
+ struct omap_chip *omap = platform_get_drvdata(pdev);
+ int status = 0;
+
+ status = pwmchip_remove(omap-chip);
+ if (status 0)
+ goto done;
+
+ omap_dm_timer_free(omap-dm_timer);
Is it guaranteed that the timer will be disabled at this point?
Uhmm... it seems that pwm_put() doesn't call pwm_disable(), so I guess it
might not be disabled.
Thierry: should pwm_put do that, or do I need a 'free' function in my chip
ops to do that?
To be honest, I haven't decided yet. =) There have been discussions that
resulted in a request to run pwm_disable() from pwmchip_remove() on all
PWM devices a chip provides.
This isn't implemented yet and I'm not sure about all the side-effects.
I think for now the best way would be to implement .free() within this
driver, or even do an explicit pwm_disable() in the driver's .remove()
function to do this. When I've come to a decision I'll refactor all of
that in one patch across the whole subsystem.
Thierry
pgpKbq0nitgDD.pgp
Description: PGP signature
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On Thu, Dec 13, 2012 at 01:38:28PM +1100, NeilBrown wrote:
On Wed, 12 Dec 2012 12:31:45 +0100 Thierry Reding
thierry.red...@avionic-design.de wrote:
On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[...]
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
The PWM subsystem already has enum pwm_polarity for this.
I'll use that then and as there is a pwm_set_polarity() interface, that
probably means that I don't need to configure the polarity via the platform
data? That would be a lot cleaner.
I guess the answer to that question is: it depends. If the user can set
the polarity (via platform or other means), then yes, you don't have to
pass it in here. However there may be users that don't support setting
the polarity or there may even be situations where the PWM goes through
an additional inverter on the board and therefore doesn't need the
polarity inversed after all, even if the user driver requests it.
Generally though I think that it is up to the user drivers to take care
of this and call pwm_set_polarity() as appropriate, so yes, I don't
think you have to explicitly pass it via platform data at all.
+ if (omap-duty_ns == duty_ns
+ omap-period_ns == period_ns)
+ /* No change - don't cause any transients */
+ return 0;
Note to self: this might be a candidate to put in the core.
might be useful, though the core doesn't currently know the current values.
Yes, but that can be changed. PWM is still a very young subsystem and
I'm trying to be cautious not to add too much cruft to it unless it's
really worth it.
+ omap_dm_timer_set_pwm(omap-dm_timer,
+ !omap-polarity,
+ toggle,
+ trigger);
This doesn't either. Also you should be explicit about the polarity
parameter, since enum pwm_polarity is an enum and therefore negating it
isn't very nice (it should work though).
You could solve this by doing something like:
if (omap-polarity == PWM_POLARITY_NORMAL)
polarity = 1;
else
polarity = 0;
(omap-polarity == PWM_POLARITY_NORMAL)
would have the same effect.
Yes, that should work as well. However I'm not a friend of using such
expressions in a function call. But since you'll probably be reworking
this anyway because of the pwm_set_polarity() comments from above you
might just want to stick the proper value into omap-polarity in your
.set_polarity() implementation and not need the extra negation here.
+static int __devinit omap_pwm_probe(struct platform_device *pdev)
No more __devinit, please.
If you say so (having no idea what it did :-)
This was used to mark functions depending on whether HOTPLUG was enabled
or not. For instance functions marked __devinit could be discarded after
the init stage if HOTPLUG was disabled because it would be guaranteed to
not be called after the init stage. Recently however HOTPLUG was changed
to be always enabled because the gains were very small and most people
would get them wrong anyway.
+#if CONFIG_PM
+static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t
state)
+{
+ struct omap_chip *omap = platform_get_drvdata(pdev);
+ /* No one preserve these values during suspend so reset them
+ * Otherwise driver leaves PWM unconfigured if same values
+ * passed to pwm_config
+ */
+ omap-period_ns = 0;
+ omap-duty_ns = 0;
+
+ return 0;
+}
+#else
+#define omap_pwm_suspend NULL
+#endif
This doesn't look right. You should implement .resume() if you really
care, in which case the resume callback would have to reconfigure with
the cached values. In that case maybe you should switch to dev_pm_ops
and SIMPLE_DEV_PM_OPS() as well.
If you don't, just resetting these values will not make the PWM work
properly after resume either since it will have to be explicitly
reconfigured.
I just copied that from pwm-samsung.c
I think the point is to avoid the no transients short-circuit in
omap_pwm_config if the config is unchanged.
The assumption is that pwm_disable() will be called before suspend and
pwm_config()/pwm_enable() after resume. So there is no point actually
configuring anything in .resume() - it makes sense to wait until pwm_config()
is called (if ever). But we want to make sure that pwm_config actually does
something.
Okay, that makes sense. User drivers should actually be better suited to
reset PWM devices to their proper state on resume.
+MODULE_AUTHOR(Grant Erickson maratho...@gmail.com);
+MODULE_AUTHOR(NeilBrown ne...@suse.de);
Shouldn't this be Neil Brown? I noticed you use the concatenated form
in the email address as well, so maybe that's on purpose?
Yes, it is on purpose. With a surname like Brown, one likes finding ways
to be distinctive :-)
Hehe, alright then =).
Thierry
pgp5NpLT3SalE.pgp
