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(&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(&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) != -
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. + * @clk_ra
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 = (__u64)clk_
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. Wi
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(&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, 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 *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 = to_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 su