Message-ID: <50CA0B4D.2000302@ti.com>
Date:	Thu, 13 Dec 2012 11:07:25 -0600
From:	Jon Hunter <jon-hunter@...com>
To:	NeilBrown <neilb@...e.de>
CC:	Thierry Reding <thierry.reding@...onic-design.de>,
	Grant Erickson <marathon96@...il.com>,
	<linux-omap@...r.kernel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: 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-hunter@...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@...il.com>
>>> Signed-off-by: NeilBrown <neilb@...e.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@...e.de>
>>> + *    Heavily based on earlier code which is:
>>> + *    Copyright (c) 2010 Grant Erickson <marathon96@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists