| Message ID | 1410277361-26848-1-git-send-email-alban.bedel@avionic-design.de |
|---|---|
| State | Superseded |
| Headers | show |
On Tuesday 09 September 2014 17:42:41 Alban Bedel wrote: > +config PWM_LPC32XX_MOTOR > + tristate "LPC32xx Motor PWM support" > + depends on ARCH_LPC32XX > + help > + Generic PWM framework driver for LPC32xx motor PWM. The LPC32xx SOC > + has one motor PWM controllers. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-lpc32xx-motor. > + Can you change the dependency to ARCH_LPC32XX || COMPILE_TEST and add explicit dependencies for the subsystems the driver depends on (pwm and clk, I guess)? That would give us better build-time coverage with allmodconfig. > + /* Write to limit register -> period */ > + __raw_writel(period, lpc32xx->base + MCLIM_REG_OFFSET(pwm)); > + > + /* Write to match register -> duty */ > + __raw_writel(period - duty, lpc32xx->base + MCMAT_REG_OFFSET(pwm)); Please don't use __raw_{writel,readl} in driver, and change that to use readl_relaxed()/writel_relaxed(). Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 09 Sep 2014 17:47:53 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 09 September 2014 17:42:41 Alban Bedel wrote: > > +config PWM_LPC32XX_MOTOR > > + tristate "LPC32xx Motor PWM support" > > + depends on ARCH_LPC32XX > > + help > > + Generic PWM framework driver for LPC32xx motor PWM. The LPC32xx SOC > > + has one motor PWM controllers. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-lpc32xx-motor. > > + > > Can you change the dependency to ARCH_LPC32XX || COMPILE_TEST and > add explicit dependencies for the subsystems the driver depends > on (pwm and clk, I guess)? That would give us better build-time > coverage with allmodconfig. I'll add COMPILE_TEST, PWM is already there because this is in an if PWM block. However I'm not sure for the clk, the LPC32xx use its own implementation of the clk functions, what should I add in this case? > > + /* Write to limit register -> period */ > > + __raw_writel(period, lpc32xx->base + MCLIM_REG_OFFSET(pwm)); > > + > > + /* Write to match register -> duty */ > > + __raw_writel(period - duty, lpc32xx->base + MCMAT_REG_OFFSET(pwm)); > > Please don't use __raw_{writel,readl} in driver, and change that to use > readl_relaxed()/writel_relaxed(). Will do. Alban
On Tue, Sep 09, 2014 at 04:42:41PM +0100, Alban Bedel wrote: > The LPC32xx motor PWMs have two output pin, A and B, with B = !A. > The driver can switch the polarity to allow use either output pin A > or output pin B. > > Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> > --- > V3: * Updated to current mainline API > * Fixed LPC32xx vs. LPC32XX > * Various coding style fix > V2: * Splitted the DTS to its own patch > --- > .../devicetree/bindings/pwm/lpc32xx-motor-pwm.txt | 24 +++ > drivers/pwm/Kconfig | 10 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-lpc32xx-motor.c | 210 +++++++++++++++++++++ > 4 files changed, 245 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt > create mode 100644 drivers/pwm/pwm-lpc32xx-motor.c > > diff --git a/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt > new file mode 100644 > index 0000000..decc27c > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt > @@ -0,0 +1,24 @@ > +LPC32xx Motor PWM controller > + > +The LPC32xx motor PWMs have two output pin, A and B, with B = !A. > +By default, output A should be used, if output B is used the PWM > +polarity should be inverted using the linux,polarity property. > + > +Required properties: > +- compatible: should be "nxp,lpc3220-motor-pwm" > +- reg: physical base address and length of the controller's registers > + > +Optional properties: > +- linux,polarity: Bit mask of the polarity to use for each output, > + a bit set to 0 indicate the default polarity, a bit set to 1 > + indicate an inverted polarity. In other word this set if output > + pin A or output pin B has the correct polarity. What exactly does linux have to do with the choice of pin? Why should this be "linux,polarity"? > + > +Examples: > + > +mpwm@400e8000 { > + compatible = "nxp,lpc3220-motor-pwm"; > + reg = <0x400E8000 0x78>; > + linux,polarity = <0x5>; /* Use outputs B0, A1 and B2 */ This doesn't match the description of there being two output pins. I take it the description above is somewhat misleading? > + #pwm-cells = <2>; The format of these cells should be described. Wouldn't it make more sense to describe the polarity in the pwm-specifier? It seems like a property of the connection rather than the PWM controller itself. [...] > + lpc32xx->clk = devm_clk_get(&pdev->dev, NULL); No clock was described in the binding. Is there only the one clock feeding the pwm? (rather than separate interface and pwm clocks). Please describe clocks in the binding. If the clock inputs are named, please use clock-names. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 09 September 2014 18:05:30 Alban Bedel wrote: > > I'll add COMPILE_TEST, PWM is already there because this is in an if > PWM block. However I'm not sure for the clk, the LPC32xx use its own > implementation of the clk functions, what should I add in this case? You can add a dependency on CLKDEV_LOOKUP, which is set by both LPC32xx and by COMMON_CLK. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 9 Sep 2014 17:05:48 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Sep 09, 2014 at 04:42:41PM +0100, Alban Bedel wrote: > > The LPC32xx motor PWMs have two output pin, A and B, with B = !A. > > The driver can switch the polarity to allow use either output pin A > > or output pin B. > > > > Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> > > --- > > V3: * Updated to current mainline API > > * Fixed LPC32xx vs. LPC32XX > > * Various coding style fix > > V2: * Splitted the DTS to its own patch > > --- > > .../devicetree/bindings/pwm/lpc32xx-motor-pwm.txt | 24 +++ > > drivers/pwm/Kconfig | 10 + > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-lpc32xx-motor.c | 210 +++++++++++++++++++++ > > 4 files changed, 245 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt > > create mode 100644 drivers/pwm/pwm-lpc32xx-motor.c > > > > diff --git a/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt > > new file mode 100644 > > index 0000000..decc27c > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt > > @@ -0,0 +1,24 @@ > > +LPC32xx Motor PWM controller > > + > > +The LPC32xx motor PWMs have two output pin, A and B, with B = !A. > > +By default, output A should be used, if output B is used the PWM > > +polarity should be inverted using the linux,polarity property. > > + > > +Required properties: > > +- compatible: should be "nxp,lpc3220-motor-pwm" > > +- reg: physical base address and length of the controller's registers > > + > > +Optional properties: > > +- linux,polarity: Bit mask of the polarity to use for each output, > > + a bit set to 0 indicate the default polarity, a bit set to 1 > > + indicate an inverted polarity. In other word this set if output > > + pin A or output pin B has the correct polarity. > > What exactly does linux have to do with the choice of pin? Why should > this be "linux,polarity"? Right, I'll remove this in favor of the standard 3 cells pwm specifiers. > > + > > +Examples: > > + > > +mpwm@400e8000 { > > + compatible = "nxp,lpc3220-motor-pwm"; > > + reg = <0x400E8000 0x78>; > > + linux,polarity = <0x5>; /* Use outputs B0, A1 and B2 */ > > This doesn't match the description of there being two output pins. I > take it the description above is somewhat misleading? > > > + #pwm-cells = <2>; > > The format of these cells should be described. > > Wouldn't it make more sense to describe the polarity in the > pwm-specifier? It seems like a property of the connection rather than > the PWM controller itself. Yes, will be done. > > > + lpc32xx->clk = devm_clk_get(&pdev->dev, NULL); > > No clock was described in the binding. > > Is there only the one clock feeding the pwm? (rather than separate > interface and pwm clocks). There is only one clock for this PWM block. > Please describe clocks in the binding. If the clock inputs are named, > please use clock-names. No clock is defined in the current LPC32xx DTS, what should I do in this case? Alban
On Wednesday 10 September 2014 10:42:20 Alban Bedel wrote: > > > Please describe clocks in the binding. If the clock inputs are named, > > please use clock-names. > > No clock is defined in the current LPC32xx DTS, what should I do in > this case? This is a bit tricky. I would recommend describing the clock inputs in the binding anyway, as "optional" properties, in case we ever get a full DT-aware clocksource driver for lpc32xx. I've just mentioned the topic to Roland yesterday, I would really like to see such a clocksource driver done for other reasons (multiplatform support), but introducing one with full DT support is hard to do without breaking existing dtb files. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt new file mode 100644 index 0000000..decc27c --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt @@ -0,0 +1,24 @@ +LPC32xx Motor PWM controller + +The LPC32xx motor PWMs have two output pin, A and B, with B = !A. +By default, output A should be used, if output B is used the PWM +polarity should be inverted using the linux,polarity property. + +Required properties: +- compatible: should be "nxp,lpc3220-motor-pwm" +- reg: physical base address and length of the controller's registers + +Optional properties: +- linux,polarity: Bit mask of the polarity to use for each output, + a bit set to 0 indicate the default polarity, a bit set to 1 + indicate an inverted polarity. In other word this set if output + pin A or output pin B has the correct polarity. + +Examples: + +mpwm@400e8000 { + compatible = "nxp,lpc3220-motor-pwm"; + reg = <0x400E8000 0x78>; + linux,polarity = <0x5>; /* Use outputs B0, A1 and B2 */ + #pwm-cells = <2>; +}; diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index b800783..1143348 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -147,6 +147,16 @@ config PWM_LPC32XX To compile this driver as a module, choose M here: the module will be called pwm-lpc32xx. +config PWM_LPC32XX_MOTOR + tristate "LPC32xx Motor PWM support" + depends on ARCH_LPC32XX + help + Generic PWM framework driver for LPC32xx motor PWM. The LPC32xx SOC + has one motor PWM controllers. + + To compile this driver as a module, choose M here: the module + will be called pwm-lpc32xx-motor. + config PWM_LPSS tristate "Intel LPSS PWM support" depends on ACPI diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index f8c577d..3eb5dd9 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -12,6 +12,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o +obj-$(CONFIG_PWM_LPC32XX_MOTOR) += pwm-lpc32xx-motor.o obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o obj-$(CONFIG_PWM_MXS) += pwm-mxs.o obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o diff --git a/drivers/pwm/pwm-lpc32xx-motor.c b/drivers/pwm/pwm-lpc32xx-motor.c new file mode 100644 index 0000000..01cac53 --- /dev/null +++ b/drivers/pwm/pwm-lpc32xx-motor.c @@ -0,0 +1,210 @@ +/* + * Copyright 2012 Alban Bedel <alban.bedel@avionic-design.de> + * + * Based on pwm-lpc32xx.c from Alexandre Pereira da Silva + * <aletes.xgr@gmail.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2. + * + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> + +struct lpc32xx_motor_pwm_chip { + struct pwm_chip chip; + struct clk *clk; + unsigned int pins; + void __iomem *base; +}; + +#define to_motor_pwm_chip(_chip) \ + container_of(_chip, struct lpc32xx_motor_pwm_chip, chip) + +/* Register mapping for MCPWM modules */ +#define LPC32XX_MCPWM_MCCON 0x00 +#define LPC32XX_MCPWM_MCCON_SET 0x04 +#define LPC32XX_MCPWM_MCCON_CLR 0x08 +#define LPC32XX_MCPWM_MCCAPCON 0x0C +#define LPC32XX_MCPWM_MCCAPCON_SET 0x10 +#define LPC32XX_MCPWM_MCCAPCON_CLR 0x14 +#define LPC32XX_MCPWM_MCLIM0 0x24 +#define LPC32XX_MCPWM_MCLIM1 0x28 +#define LPC32XX_MCPWM_MCLIM2 0x2C +#define LPC32XX_MCPWM_MCMAT0 0x30 +#define LPC32XX_MCPWM_MCMAT1 0x34 +#define LPC32XX_MCPWM_MCMAT2 0x38 +#define LPC32XX_MCPWM_MCINTEN_CLR 0x58 + +#define LPC32XX_MCPWM_COUNT 3 + +#define PWM_EN_MASK(pwm) BIT(0 + (pwm)->hwpwm * 8) +#define MCLIM_REG_OFFSET(pwm) (LPC32XX_MCPWM_MCLIM0 + (pwm)->hwpwm * 4) +#define MCMAT_REG_OFFSET(pwm) (LPC32XX_MCPWM_MCMAT0 + (pwm)->hwpwm * 4) + +static int lpc32xx_motor_pwm_config(struct pwm_chip *chip, + struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct lpc32xx_motor_pwm_chip *lpc32xx = to_motor_pwm_chip(chip); + u64 rate, period, duty; + int err = 0; + + /* The clock is needed to access the registers */ + err = clk_enable(lpc32xx->clk); + if (err) + return err; + + /* Calculate period */ + rate = clk_get_rate(lpc32xx->clk); + period = (u64)period_ns * rate; + duty = (u64)duty_ns * rate; + do_div(period, 1000000000); + do_div(duty, 1000000000); + + /* Write to limit register -> period */ + __raw_writel(period, lpc32xx->base + MCLIM_REG_OFFSET(pwm)); + + /* Write to match register -> duty */ + __raw_writel(period - duty, lpc32xx->base + MCMAT_REG_OFFSET(pwm)); + + /* Disable the clock now that we are done */ + clk_disable(lpc32xx->clk); + return 0; +} + +static int lpc32xx_motor_pwm_enable(struct pwm_chip *chip, + struct pwm_device *pwm) +{ + struct lpc32xx_motor_pwm_chip *lpc32xx = to_motor_pwm_chip(chip); + int err; + + err = clk_enable(lpc32xx->clk); + if (err) + return err; + + __raw_writel(PWM_EN_MASK(pwm), lpc32xx->base + LPC32XX_MCPWM_MCCON_SET); + + return 0; +} + +static void lpc32xx_motor_pwm_disable(struct pwm_chip *chip, + struct pwm_device *pwm) +{ + struct lpc32xx_motor_pwm_chip *lpc32xx = to_motor_pwm_chip(chip); + + __raw_writel(PWM_EN_MASK(pwm), lpc32xx->base + LPC32XX_MCPWM_MCCON_CLR); + + clk_disable(lpc32xx->clk); +} + +static const struct pwm_ops lpc32xx_motor_pwm_ops = { + .config = lpc32xx_motor_pwm_config, + .enable = lpc32xx_motor_pwm_enable, + .disable = lpc32xx_motor_pwm_disable, + .owner = THIS_MODULE, +}; + +static int lpc32xx_motor_pwm_probe(struct platform_device *pdev) +{ + struct lpc32xx_motor_pwm_chip *lpc32xx; + struct resource *res; + int ret; + + lpc32xx = devm_kzalloc(&pdev->dev, sizeof(*lpc32xx), GFP_KERNEL); + if (!lpc32xx) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -EINVAL; + + lpc32xx->base = devm_ioremap_resource(&pdev->dev, res); + if (!lpc32xx->base) + return -EADDRNOTAVAIL; + + lpc32xx->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(lpc32xx->clk)) + return PTR_ERR(lpc32xx->clk); + + /* Configure the pins polarity */ + ret = of_property_read_u32(pdev->dev.of_node, "linux,polarity", + &lpc32xx->pins); + if (!ret) { + u32 set = 0, clr = 0; + int i; + + for (i = 0; i < LPC32XX_MCPWM_COUNT; i++) + if (lpc32xx->pins & BIT(i)) + set |= BIT(2 + i * 8); + else + clr |= BIT(2 + i * 8); + + ret = clk_enable(lpc32xx->clk); + if (ret) + return ret; + + __raw_writel(set, lpc32xx->base + LPC32XX_MCPWM_MCCON_SET); + __raw_writel(clr, lpc32xx->base + LPC32XX_MCPWM_MCCON_CLR); + + clk_disable(lpc32xx->clk); + } + + lpc32xx->chip.dev = &pdev->dev; + lpc32xx->chip.ops = &lpc32xx_motor_pwm_ops; + lpc32xx->chip.npwm = LPC32XX_MCPWM_COUNT; + lpc32xx->chip.base = -1; + + ret = pwmchip_add(&lpc32xx->chip); + if (ret < 0) { + dev_err(&pdev->dev, "failed to add PWM chip, error %d\n", ret); + return ret; + } + + platform_set_drvdata(pdev, lpc32xx); + + return 0; +} + +static int lpc32xx_motor_pwm_remove(struct platform_device *pdev) +{ + struct lpc32xx_motor_pwm_chip *lpc32xx = platform_get_drvdata(pdev); + int i; + + for (i = 0; i < lpc32xx->chip.npwm; i++) + pwm_disable(&lpc32xx->chip.pwms[i]); + + return pwmchip_remove(&lpc32xx->chip); +} + +static const struct of_device_id lpc32xx_motor_pwm_dt_ids[] = { + { .compatible = "nxp,lpc3220-motor-pwm", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, lpc32xx_motor_pwm_dt_ids); + +static struct platform_driver lpc32xx_motor_pwm_driver = { + .driver = { + .name = "lpc32xx-motor-pwm", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(lpc32xx_motor_pwm_dt_ids), + }, + .probe = lpc32xx_motor_pwm_probe, + .remove = lpc32xx_motor_pwm_remove, +}; +module_platform_driver(lpc32xx_motor_pwm_driver); + +MODULE_ALIAS("platform:lpc32xx-motor-pwm"); +MODULE_AUTHOR("Alban Bedel <alban.bedel@avionic-design.de>"); +MODULE_DESCRIPTION("LPC32xx Motor PWM Driver"); +MODULE_LICENSE("GPL v2");
The LPC32xx motor PWMs have two output pin, A and B, with B = !A. The driver can switch the polarity to allow use either output pin A or output pin B. Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> --- V3: * Updated to current mainline API * Fixed LPC32xx vs. LPC32XX * Various coding style fix V2: * Splitted the DTS to its own patch --- .../devicetree/bindings/pwm/lpc32xx-motor-pwm.txt | 24 +++ drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-lpc32xx-motor.c | 210 +++++++++++++++++++++ 4 files changed, 245 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/lpc32xx-motor-pwm.txt create mode 100644 drivers/pwm/pwm-lpc32xx-motor.c