| Message ID | 1309255368-9775-1-git-send-email-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
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
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)?
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
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
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
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
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
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
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
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