Patchwork [RFC] implement a generic PWM framework - once again

login
register
mail settings
Submitter Sascha Hauer
Date June 28, 2011, 10:02 a.m.
Message ID <1309255368-9775-1-git-send-email-s.hauer@pengutronix.de>
Download mbox
Permalink /patch/102344/
State New
Headers show

Pull-request

git://git.pengutronix.de/git/imx/linux-2.6.git pwm

Comments

Sascha Hauer - June 28, 2011, 10:02 a.m.
Hi All,

The following implements a generic pwm framework and adds a user
for it. I already posted this series back in January. Based on
the comments I received I added some details about the motivation
for adding such a framework and not using the led or hwmon framework
to patch 1/2.
I also added some documentation to Documentation/pwm.txt.

This patch does not change the user API for PWMs, in particular
it does not enforce any sleep/nonsleep context to the PWM users.
The patch merely puts the status quo into a core wrapper to be able
to register multiple PWM drivers in the system. Improvements to
the API can still be made later once we have at least a place
in the kernel to collect the existing PWM drivers.

Sascha

The following changes since commit b0af8dfdd67699e25083478c63eedef2e72ebd85:

  Linux 3.0-rc5 (2011-06-27 19:12:22 -0700)

are available in the git repository at:
  git://git.pengutronix.de/git/imx/linux-2.6.git pwm

Sascha Hauer (2):
      PWM: add pwm framework support
      pwm: Add a i.MX23/28 pwm driver

 Documentation/pwm.txt |   56 ++++++++++
 MAINTAINERS           |    5 +
 drivers/Kconfig       |    2 +
 drivers/Makefile      |    1 +
 drivers/pwm/Kconfig   |   12 ++
 drivers/pwm/Makefile  |    1 +
 drivers/pwm/core.c    |  246 +++++++++++++++++++++++++++++++++++++++++++
 drivers/pwm/mxs-pwm.c |  275 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h   |   38 +++++++
 9 files changed, 636 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/pwm.txt
 create mode 100644 drivers/pwm/Kconfig
 create mode 100644 drivers/pwm/Makefile
 create mode 100644 drivers/pwm/core.c
 create mode 100644 drivers/pwm/mxs-pwm.c
Lothar Waßmann - June 28, 2011, 10:28 a.m.
Hi,

just some nitpicks...

Sascha Hauer writes:
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/pwm/mxs-pwm.c |  275 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 275 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/pwm/mxs-pwm.c
> 
> diff --git a/drivers/pwm/mxs-pwm.c b/drivers/pwm/mxs-pwm.c
> new file mode 100644
> index 0000000..052cb0c
> --- /dev/null
> +++ b/drivers/pwm/mxs-pwm.c
> @@ -0,0 +1,275 @@
> +/*
> + * Copyright (C) 2011 Pengutronix
> + * Sascha Hauer <s.hauer@pengutronix.de>
> + *
> + * simple driver for PWM (Pulse Width Modulator) controller
> + *
> + * 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.
> + *
> + * Derived from pxa PWM driver by eric miao <eric.miao@marvell.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/pwm.h>
> +#include <mach/hardware.h>
> +#include <mach/mxs.h>
> +#include <mach/mx23.h>
> +#include <mach/mx28.h>
> +#include <asm/div64.h>
> +
> +struct mxs_pwm_device {
> +	struct device		*dev;
> +
> +	struct clk	*clk;
> +
> +	int		enabled;
> +	void __iomem	*mmio_base;
> +
> +	unsigned int	pwm_id;
> +
> +	u32		val_active;
> +	u32		val_period;
> +	int		period_us;
> +	struct pwm_chip chip;
> +};
> +
> +/* common register space */
> +static void __iomem *pwm_base_common;
> +#define REG_PWM_CTRL	0x0
> +#define PWM_SFTRST	(1 << 31)
> +#define PWM_CLKGATE	(1 << 30)
> +#define PWM_ENABLE(p)	(1 << (p))
> +
> +/* per pwm register space */
> +#define REG_ACTIVE	0x0
> +#define REG_PERIOD	0x10
> +
> +#define PERIOD_PERIOD(p)	((p) & 0xffff)
> +#define PERIOD_ACTIVE_HIGH	(3 << 16)
> +#define PERIOD_INACTIVE_LOW	(2 << 18)
> +#define PERIOD_CDIV(div)	(((div) & 0x7) << 20)
> +
[...]
> +static int __devinit mxs_pwm_probe(struct platform_device *pdev)
> +{
> +	struct mxs_pwm_device *pwm;
> +	struct resource *r;
> +	int ret = 0;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(struct mxs_pwm_device), GFP_KERNEL);
> +	if (pwm == NULL) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	pwm->clk = clk_get(&pdev->dev, NULL);
> +
> +	if (IS_ERR(pwm->clk)) {
Why an empty line here, but not in other similar places?

> +		ret = PTR_ERR(pwm->clk);
> +		goto err_free;
> +	}
> +
> +	pwm->chip.ops = &mxs_pwm_ops;
> +
> +	pwm->enabled = 0;
is already 0 due to kzalloc().

> +
> +	pwm->chip.pwm_id = pdev->id;
> +	pwm->chip.owner = THIS_MODULE;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (r == NULL) {
> +		dev_err(&pdev->dev, "no memory resource defined\n");
> +		ret = -ENODEV;
> +		goto err_free_clk;
> +	}
> +
> +	r = request_mem_region(r->start, resource_size(r), pdev->name);
> +	if (r == NULL) {
> +		dev_err(&pdev->dev, "failed to request memory resource\n");
> +		ret = -EBUSY;
> +		goto err_free_clk;
> +	}
> +
> +	pwm->mmio_base = ioremap(r->start, resource_size(r));
> +	if (pwm->mmio_base == NULL) {
> +		dev_err(&pdev->dev, "failed to ioremap() registers\n");
> +		ret = -ENODEV;
-ENOMEM?.

> +	if (ret)
> +		goto err_free_mem;
> +
> +	platform_set_drvdata(pdev, pwm);
> +	return 0;
> +
> +err_free_mem:
> +	release_mem_region(r->start, resource_size(r));
> +err_free_clk:
> +	clk_put(pwm->clk);
> +err_free:
> +	kfree(pwm);
> +	return ret;
> +}
> +
> +static int __devexit mxs_pwm_remove(struct platform_device *pdev)
> +{
> +	struct mxs_pwm_device *pwm;
> +	struct resource *r;
> +	int ret;
> +
> +	pwm = platform_get_drvdata(pdev);
> +
> +	ret = pwmchip_remove(&pwm->chip);
> +	if (ret)
> +		return ret;
> +
> +	iounmap(pwm->mmio_base);
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(r->start, resource_size(r));
> +
> +	clk_put(pwm->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mxs_pwm_driver = {
> +	.driver		= {
> +		.name	= "mxs-pwm",
> +	},
> +	.probe		= mxs_pwm_probe,
> +	.remove		= __devexit_p(mxs_pwm_remove),
> +};
> +
> +static int __init mxs_pwm_init(void)
> +{
> +	if (cpu_is_mx28())
> +		pwm_base_common = MX28_IO_ADDRESS(MX28_PWM_BASE_ADDR);
> +	else
> +		pwm_base_common = MX23_IO_ADDRESS(MX23_PWM_BASE_ADDR);
> +
> +	__mxs_clrl(PWM_SFTRST | PWM_CLKGATE, pwm_base_common + REG_PWM_CTRL);
> +
Shouldn't CLKGATE be set again in the remove() function?



Lothar Waßmann
Dmitry Eremin-Solenikov - June 28, 2011, 12:23 p.m.
Hi,

Sascha Hauer wrote:

> Hi All,
>
> The following implements a generic pwm framework and adds a user
> for it. I already posted this series back in January. Based on
> the comments I received I added some details about the motivation
> for adding such a framework and not using the led or hwmon framework
> to patch 1/2.
> I also added some documentation to Documentation/pwm.txt.
>
> This patch does not change the user API for PWMs, in particular
> it does not enforce any sleep/nonsleep context to the PWM users.
> The patch merely puts the status quo into a core wrapper to be able
> to register multiple PWM drivers in the system. Improvements to
> the API can still be made later once we have at least a place
> in the kernel to collect the existing PWM drivers.

Just a small question: on PXA (just for example) PWMs are registered
from arch/arm via arch_initcall. Will your subsystem work for such early
registration (of course after moving PXA driver to drivers/pwm)?
Arnd Bergmann - June 28, 2011, 12:27 p.m.
On Tuesday 28 June 2011, Sascha Hauer wrote:
> This patch adds framework support for PWM (pulse width modulation) devices.
> 
> The is a barebone PWM API already in the kernel under include/linux/pwm.h,
> but it does not allow for multiple drivers as each of them implements the
> pwm_*() functions.

Hi Sascha,

This looks very nice.
I have only trivial comments, except for the suggestion to use idr.
 
> +PWM core
> +M:	Sascha Hauer <s.hauer@pengutronix.de>
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained

I would add

F:	drivers/pwm/

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> new file mode 100644
> index 0000000..93c1052
> --- /dev/null
> +++ b/drivers/pwm/Kconfig
> @@ -0,0 +1,12 @@
> +menuconfig PWM
> +	bool "PWM Support"
> +	help
> +	  This enables PWM support through the generic PWM framework.
> +	  You only need to enable this, if you also want to enable
> +	  one or more of the PWM drivers below.

Remove the comma.

> +
> +/*
> + * The next pwm id to assign. We do not bother to fill gaps which
> + * occur during dynamic registering/deregistering of pwms, but
> + * instead assign a uniq id to each new pwm.
                       unique

> + */
> +static int next_pwm_id;

How about using idr? It provides you a fast lookup and handles giving
out unique numbers.

> +
> +/**
> + * pwmchip_reserve() - reserve range of pwms to use with platform code only
> + * @npwms: number of pwms to reserve
> + * Context: platform init
> + *
> + * Maybe called only once. It reserves the first pwm_ids for platform use so

I assume you mean "May only be called once".

> +/**
> + * struct pwm_ops - PWM operations
> + * @request: optional hook for requesting a PWM
> + * @free: optional hook for freeing a PWM
> + * @config: configure duty cycles and period length for this PWM
> + * @enable: enable PWM output toggling
> + * @disable: disable PWM output toggling
> + */
> +struct pwm_ops {
> +	int			(*request)(struct pwm_chip *chip);
> +	void			(*free)(struct pwm_chip *chip);
> +	int			(*config)(struct pwm_chip *chip, int duty_ns,
> +						int period_ns);
> +	int			(*enable)(struct pwm_chip *chip);
> +	void			(*disable)(struct pwm_chip *chip);
> +};
>
> +/**
> + * struct pwm_chip - abstract a PWM
> + * @label: for diagnostics
> + * @owner: helps prevent removal of modules exporting active PWMs
> + * @ops: The callbacks for this PWM
> + */
> +struct pwm_chip {
> +	int			pwm_id;
> +	const char		*label;
> +	struct module		*owner;
> +	struct pwm_ops		*ops;
> +};

I think the "owner" field should be in the pwm_ops, not in the pwm_chip,
because the pwm_ops is likely to be statically allocated, while the
pwm_chip is probably runtime allocated and cannot be preinitialized.

Should a pwm_chip contain a "struct device" so you can refer to it in
sysfs and add attributes?

	Arnd
Arnd Bergmann - June 28, 2011, 3:22 p.m.
Hi Sascha,

On Tuesday 28 June 2011, Sascha Hauer wrote:
> +
> +/* common register space */
> +static void __iomem *pwm_base_common;
> +#define REG_PWM_CTRL   0x0
> +#define PWM_SFTRST     (1 << 31)
> +#define PWM_CLKGATE    (1 << 30)
> +#define PWM_ENABLE(p)  (1 << (p))
> +

The driver looks pretty good overall, but the global pwm_base_common register
is rather ugly. I think this should really be passed down through resources
from  the platform device in one way or another.

	Arnd
Sascha Hauer - June 28, 2011, 4:18 p.m.
On Tue, Jun 28, 2011 at 02:27:04PM +0200, Arnd Bergmann wrote:
> On Tuesday 28 June 2011, Sascha Hauer wrote:
> > This patch adds framework support for PWM (pulse width modulation) devices.
> > 
> > The is a barebone PWM API already in the kernel under include/linux/pwm.h,
> > but it does not allow for multiple drivers as each of them implements the
> > pwm_*() functions.
> 
> Hi Sascha,
> 
> This looks very nice.
> I have only trivial comments, except for the suggestion to use idr.
>  
> > +PWM core
> > +M:	Sascha Hauer <s.hauer@pengutronix.de>
> > +L:	linux-kernel@vger.kernel.org
> > +S:	Maintained
> 
> I would add
> 
> F:	drivers/pwm/
> 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > new file mode 100644
> > index 0000000..93c1052
> > --- /dev/null
> > +++ b/drivers/pwm/Kconfig
> > @@ -0,0 +1,12 @@
> > +menuconfig PWM
> > +	bool "PWM Support"
> > +	help
> > +	  This enables PWM support through the generic PWM framework.
> > +	  You only need to enable this, if you also want to enable
> > +	  one or more of the PWM drivers below.
> 
> Remove the comma.
> 
> > +
> > +/*
> > + * The next pwm id to assign. We do not bother to fill gaps which
> > + * occur during dynamic registering/deregistering of pwms, but
> > + * instead assign a uniq id to each new pwm.
>                        unique
> 
> > + */
> > +static int next_pwm_id;
> 
> How about using idr? It provides you a fast lookup and handles giving
> out unique numbers.

Sounds like a good idea, but is idr compatible with the pwmchip_reserve
function below? idr gives no guarantee that the first id is zero, but I
need exactly this to make the ids for the internal PWMs known at compile
time.

(In the longer term it probably makes sense to implement a
pwm_get(struct device *, const char *id) similar to the clk api, I just
wanted to stay compatible to the current PWM API for now)

Sascha
Arnd Bergmann - June 28, 2011, 5:03 p.m.
On Tuesday 28 June 2011, Sascha Hauer wrote:
> > How about using idr? It provides you a fast lookup and handles giving
> > out unique numbers.
> 
> Sounds like a good idea, but is idr compatible with the pwmchip_reserve
> function below? idr gives no guarantee that the first id is zero, but I
> need exactly this to make the ids for the internal PWMs known at compile
> time.

You can certainly use ida_get_new_above to guarantee that implicit
number allocation doesn't interfere with the reserved numbers. Whether
that guarantees that you get the reserved numbers when you ask for them,
I don't know.

A possible way to deal with that would be to allocate an array of pointers
for the reserved space but use IDR for the rest, but at that point the
complexity is probably higher than what you have now ;-)

Another question is whether the dynamic allocation even makes sense. How
does a driver that wants to use a PWM find out what number to ask for
when it's not reserved for the platform to start with. Maybe the
answer is to just use an array or tree for the lookup and refuse to
register multiple PWMs to the same number.

	Arnd
Matthias Kaehlcke - June 28, 2011, 7:40 p.m.
El Tue, Jun 28, 2011 at 12:02:47PM +0200 Sascha Hauer ha dit:

> This patch adds framework support for PWM (pulse width modulation) devices.
[...]
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> new file mode 100644
> index 0000000..79dc051
> --- /dev/null
> +++ b/drivers/pwm/core.c
[...]
> +/**
> + * pwmchip_add() - register a new pwm
> + * @chip: the pwm
> + *
> + * register a new pwm. pwm->pwm_id must be initialized. if pwm_id < 0 then
> + * a dynamically assigned id will be used, otherwise the id specified,
> + */
> +int pwmchip_add(struct pwm_chip *chip)
> +{
> +	struct pwm_device *pwm;
> +	int ret = 0;
> +
> +	pwm = kzalloc(sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->chip = chip;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	if (chip->pwm_id >= 0 && find_pwm(chip->pwm_id)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	if (chip->pwm_id < 0)
> +		chip->pwm_id = next_pwm_id++;
> +
> +	list_add_tail(&pwm->node, &pwm_list);
> +out:
> +	mutex_unlock(&pwm_lock);
> +
> +	kfree(pwm);

as already pointed out by kurt, pwm should only be freed in case of an
error

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pwmchip_add);
> +
> +/**
> + * pwmchip_remove() - remove a pwm
> + * @chip: the pwm
> + *
> + * remove a pwm. This function may return busy if the pwm is still requested.
> + */
> +int pwmchip_remove(struct pwm_chip *chip)
> +{
> +	struct pwm_device *pwm;
> +	int ret = 0;
> +
> +	mutex_lock(&pwm_lock);
> +
> +	pwm = find_pwm(chip->pwm_id);
> +	if (!pwm) {
> +		ret = -ENOENT;
> +		goto out;
> +	}
> +
> +	if (test_bit(FLAG_REQUESTED, &pwm->flags)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	list_del(&pwm->node);
  +	kfree(pwm);
> +out:
> +	mutex_unlock(&pwm_lock);
> +
> +	return ret;
> +}

[...]

best regards
Sascha Hauer - June 29, 2011, 8:50 a.m.
On Tue, Jun 28, 2011 at 07:03:05PM +0200, Arnd Bergmann wrote:
> On Tuesday 28 June 2011, Sascha Hauer wrote:
> > > How about using idr? It provides you a fast lookup and handles giving
> > > out unique numbers.
> > 
> > Sounds like a good idea, but is idr compatible with the pwmchip_reserve
> > function below? idr gives no guarantee that the first id is zero, but I
> > need exactly this to make the ids for the internal PWMs known at compile
> > time.
> 
> You can certainly use ida_get_new_above to guarantee that implicit
> number allocation doesn't interfere with the reserved numbers. Whether
> that guarantees that you get the reserved numbers when you ask for them,
> I don't know.
> 
> A possible way to deal with that would be to allocate an array of pointers
> for the reserved space but use IDR for the rest, but at that point the
> complexity is probably higher than what you have now ;-)
> 
> Another question is whether the dynamic allocation even makes sense. How
> does a driver that wants to use a PWM find out what number to ask for
> when it's not reserved for the platform to start with. Maybe the
> answer is to just use an array or tree for the lookup and refuse to
> register multiple PWMs to the same number.

I tend to remove the dynamic allocation support for now. Currently we
have two types of pwm drivers in the tree. The SoC internal PWMs can
cope with a fixed id so that platforms can pass the id to the pwm
function driver (backlight, led).
The other type is some mfd drivers which implement the pwm API themselves
right now. These are currently incompatible with the pwm framework anyway.
If a board wants to connect the mfd pwm to its backlight, the current
pwm_request based on an integer id is not suitable anymore. We need
a (struct device *dev, char *id) based pwm_request function then.
So my plan is to change the pwm API once we have all in tree drivers
in drivers/pwm.

Sascha
Arnd Bergmann - June 29, 2011, 11 a.m.
On Wednesday 29 June 2011, Sascha Hauer wrote:
> The other type is some mfd drivers which implement the pwm API themselves
> right now. These are currently incompatible with the pwm framework anyway.
> If a board wants to connect the mfd pwm to its backlight, the current
> pwm_request based on an integer id is not suitable anymore. We need
> a (struct device *dev, char *id) based pwm_request function then.
> So my plan is to change the pwm API once we have all in tree drivers
> in drivers/pwm.

Ok, sounds good. I don't much like the idea of using a string identifier
for this, but let's discuss that when we get there. Using fixed numbers
for the first version seems totally reasonable, as it keeps the existing
API.

	Arnd