lkml.org 
[lkml]   [2012]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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-hunter@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 <marathon96@gmail.com>
    >>> Signed-off-by: NeilBrown <neilb@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 0000000..e3dbce3
    >>> --- /dev/null
    >>> +++ b/drivers/pwm/pwm-omap.c
    >>> @@ -0,0 +1,318 @@
    >>> +/*
    >>> + * Copyright (c) 2012 NeilBrown <neilb@suse.de>
    >>> + * Heavily based on earlier code which is:
    >>> + * Copyright (c) 2010 Grant Erickson <marathon96@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 0xFFFFFFFE
    >>> +
    >>> +struct omap_chip {
    >>> + struct platform_device *pdev;
    >>> +
    >>> + struct omap_dm_timer *dm_timer;
    >>> + unsigned int polarity;
    >>> + const char *label;
    >>> +
    >>> + unsigned int duty_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 = 1000000000;
    >>> + 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 omap_dm_timer_load_start() here instead of the above 4
    >> APIs?
    >
    > Because I didn't know about it. I do now :-)

    No problem.

    >>
    >>> +
    >>> + 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",
    >>> + duty_ns, period_ns);
    >>> +
    >>> + if (omap->duty_ns == duty_ns &&
    >>> + omap->period_ns == period_ns)
    >>> + /* No change - don't cause any transients */
    >>> + return 0;
    >>> +
    >>> + clk_rate = clk_get_rate(omap_dm_timer_get_fclk(omap->dm_timer));
    >>> +
    >>> + /* Calculate the appropriate load and match values based on the
    >>> + * specified period and duty cycle. The load value determines the
    >>> + * cycle time and the match value determines the duty cycle.
    >>> + */
    >>> +
    >>> + load_value = pwm_calc_value(clk_rate, period_ns);
    >>> + match_value = pwm_calc_value(clk_rate, period_ns - duty_ns);
    >>> +
    >>> + /* We MUST enable yet stop the associated dual-mode timer before
    >>> + * attempting to write its registers. Hopefully it is already
    >>> + * disabled, but call the (idempotent) pwm_disable just in case
    >>> + */
    >>> +
    >>> + pwm_disable(pwm);
    >>> +
    >>> + 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.
    >
    >>
    >>> + 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.
    >>
    >>> + omap->duty_ns = duty_ns;
    >>> + omap->period_ns = period_ns;
    >>> +
    >>> + return status;
    >>> +}
    >>> +
    >>> +
    >>> +static struct pwm_ops omap_pwm_ops = {
    >>> + .enable = omap_pwm_enable,
    >>> + .disable= omap_pwm_disable,
    >>> + .config = omap_pwm_config,
    >>> + .owner = THIS_MODULE,
    >>> +};
    >>> +
    >>> +/**
    >>> + * omap_pwm_probe - check for the PWM and bind it to the driver.
    >>> + * @pdev: A pointer to the platform device node associated with the
    >>> + * PWM instance to be probed for driver binding.
    >>> + *
    >>> + * Returns 0 if the PWM instance was successfully bound to the driver;
    >>> + * otherwise, < 0 on error.
    >>> + */
    >>> +static int __devinit omap_pwm_probe(struct platform_device *pdev)
    >>
    >> I believe that __devinit is no longer required.
    >>
    >>> +{
    >>> + struct device *dev = &pdev->dev;
    >>> + struct omap_chip *omap;
    >>> + int status = 0;
    >>> + unsigned int id = pdev->id;
    >>> + unsigned int timer = id >> 1; /* lsb is polarity */
    >>> +
    >>> + omap = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
    >>> +
    >>> + if (omap == NULL) {
    >>> + dev_err(dev, "Could not allocate memory.\n");
    >>> + status = -ENOMEM;
    >>> + goto done;
    >>> + }
    >>> +
    >>> + /* 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?

    I don't have strong objections, but I did not think Thierry liked the
    use of the id.

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

    Ugh, good point. Looks like for dev-tree we will need a way to request a
    timer by IO and this is missing today!

    >
    >>
    >>> +
    >>> + if (omap->dm_timer == NULL) {
    >>> + status = -EPROBE_DEFER;
    >>> + goto err_free;
    >>> + }
    >>> +
    >>> + /* Configure the source for the dual-mode timer backing this
    >>> + * generic PWM device. The clock source will ultimately determine
    >>> + * how small or large the PWM frequency can be.
    >>> + *
    >>> + * At some point, it's probably worth revisiting moving this to
    >>> + * the configure method and choosing either the slow- or
    >>> + * system-clock source as appropriate for the desired PWM period.
    >>> + */
    >>> +
    >>> + omap_dm_timer_set_source(omap->dm_timer, OMAP_TIMER_SRC_SYS_CLK);
    >>> +
    >>> + /* Cache away other miscellaneous driver-private data and state
    >>> + * information and add the driver-private data to the platform
    >>> + * device.
    >>> + */
    >>> +
    >>> + omap->chip.dev = dev;
    >>> + omap->chip.ops = &omap_pwm_ops;
    >>> + omap->chip.base = -1;
    >>> + omap->chip.npwm = 1;
    >>> + omap->polarity = id & 1;
    >>> +
    >>> + status = pwmchip_add(&omap->chip);
    >>> + if (status < 0) {
    >>> + dev_err(dev, "failed to register pwm\n");
    >>> + omap_dm_timer_free(omap->dm_timer);
    >>> + goto err_free;
    >>> + }
    >>> +
    >>> + platform_set_drvdata(pdev, omap);
    >>> +
    >>> + status = 0;
    >>> + goto done;
    >>> +
    >>> + err_free:
    >>> + kfree(omap);
    >>> + done:
    >>> + return status;
    >>> +}
    >>> +
    >>> +/**
    >>> + * omap_pwm_remove - unbind the specified PWM platform device from the driver.
    >>> + * @pdev: A pointer to the platform device node associated with the
    >>> + * PWM instance to be unbound/removed.
    >>> + *
    >>> + * Returns 0 if the PWM was successfully removed as a platform device;
    >>> + * otherwise, < 0 on error.
    >>> + */
    >>> +static int __devexit omap_pwm_remove(struct platform_device *pdev)
    >>
    >> I believe that __devexit is no longer required.
    >>
    >>> +{
    >>> + 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?
    >
    >
    >>
    >>> +
    >>> + kfree(omap);
    >>> +
    >>> + done:
    >>> + return status;
    >>> +}
    >>> +
    >>> +#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.

    > Maybe I should test and see - though as my backlight always blanks before
    > suspend that might not be straight forward...

    Yes it would be great if you can test too. I have been finding that
    certain features of the timer are not that well tested.

    >>
    >> Please note that I am not familiar with the PWM sub-system to know how
    >> suspend-resume is typically handled and if this is normal or not.
    >>
    >>> + return 0;
    >>> +}
    >>> +#else
    >>> +#define omap_pwm_suspend NULL
    >>> +#endif
    >>> +
    >>> +static struct platform_driver omap_pwm_driver = {
    >>> + .driver.name = "omap-pwm",
    >>> + .driver.owner = THIS_MODULE,
    >>> + .probe = omap_pwm_probe,
    >>> + .remove = __devexit_p(omap_pwm_remove),
    >>
    >> I believe that __devexit_p is no longer required.
    >>
    >> Otherwise it looks good to me. Thanks for sending!
    >
    > And thanks a lot for reading and reviewing!

    No problem.

    Cheers
    Jon


    \
     
     \ /
      Last update: 2012-12-13 18:21    [W:2.968 / U:0.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site