Remove the non-standard EP93xx pwm driver in drivers/misc and add
a new driver for the PWM chips on the EP93xx platforms based on the
PWM framework.
These PWM chips each support 1 PWM channel with programmable duty
cycle, frequency, and polarity inversion.
Signed-off-by: H Hartley Sweeten <[email protected]>
Cc: Ryan Mallon <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/misc/Kconfig | 13 ---
drivers/misc/Makefile | 1 -
drivers/misc/ep93xx_pwm.c | 286 ----------------------------------------------
drivers/pwm/Kconfig | 9 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-ep93xx.c | 218 +++++++++++++++++++++++++++++++++++
6 files changed, 228 insertions(+), 300 deletions(-)
delete mode 100644 drivers/misc/ep93xx_pwm.c
create mode 100644 drivers/pwm/pwm-ep93xx.c
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 8dacd4c..c43c66a 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -381,19 +381,6 @@ config HMC6352
This driver provides support for the Honeywell HMC6352 compass,
providing configuration and heading data via sysfs.
-config EP93XX_PWM
- tristate "EP93xx PWM support"
- depends on ARCH_EP93XX
- help
- This option enables device driver support for the PWM channels
- on the Cirrus EP93xx processors. The EP9307 chip only has one
- PWM channel all the others have two, the second channel is an
- alternate function of the EGPIO14 pin. A sysfs interface is
- provided to control the PWM channels.
-
- To compile this driver as a module, choose M here: the module will
- be called ep93xx_pwm.
-
config DS1682
tristate "Dallas DS1682 Total Elapsed Time Recorder with Alarm"
depends on I2C
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c235d5b..ecccd00 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -33,7 +33,6 @@ obj-$(CONFIG_APDS9802ALS) += apds9802als.o
obj-$(CONFIG_ISL29003) += isl29003.o
obj-$(CONFIG_ISL29020) += isl29020.o
obj-$(CONFIG_SENSORS_TSL2550) += tsl2550.o
-obj-$(CONFIG_EP93XX_PWM) += ep93xx_pwm.o
obj-$(CONFIG_DS1682) += ds1682.o
obj-$(CONFIG_TI_DAC7512) += ti_dac7512.o
obj-$(CONFIG_C2PORT) += c2port/
diff --git a/drivers/misc/ep93xx_pwm.c b/drivers/misc/ep93xx_pwm.c
deleted file mode 100644
index cdb67a9..0000000
--- a/drivers/misc/ep93xx_pwm.c
+++ /dev/null
@@ -1,286 +0,0 @@
-/*
- * Simple PWM driver for EP93XX
- *
- * (c) Copyright 2009 Matthieu Crapet <[email protected]>
- * (c) Copyright 2009 H Hartley Sweeten <[email protected]>
- *
- * 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; either version
- * 2 of the License, or (at your option) any later version.
- *
- * EP9307 has only one channel:
- * - PWMOUT
- *
- * EP9301/02/12/15 have two channels:
- * - PWMOUT
- * - PWMOUT1 (alternate function for EGPIO14)
- */
-
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-#include <linux/clk.h>
-#include <linux/err.h>
-#include <linux/io.h>
-
-#include <mach/platform.h>
-
-#define EP93XX_PWMx_TERM_COUNT 0x00
-#define EP93XX_PWMx_DUTY_CYCLE 0x04
-#define EP93XX_PWMx_ENABLE 0x08
-#define EP93XX_PWMx_INVERT 0x0C
-
-#define EP93XX_PWM_MAX_COUNT 0xFFFF
-
-struct ep93xx_pwm {
- void __iomem *mmio_base;
- struct clk *clk;
- u32 duty_percent;
-};
-
-/*
- * /sys/devices/platform/ep93xx-pwm.N
- * /min_freq read-only minimum pwm output frequency
- * /max_req read-only maximum pwm output frequency
- * /freq read-write pwm output frequency (0 = disable output)
- * /duty_percent read-write pwm duty cycle percent (1..99)
- * /invert read-write invert pwm output
- */
-
-static ssize_t ep93xx_pwm_get_min_freq(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
- unsigned long rate = clk_get_rate(pwm->clk);
-
- return sprintf(buf, "%ld\n", rate / (EP93XX_PWM_MAX_COUNT + 1));
-}
-
-static ssize_t ep93xx_pwm_get_max_freq(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
- unsigned long rate = clk_get_rate(pwm->clk);
-
- return sprintf(buf, "%ld\n", rate / 2);
-}
-
-static ssize_t ep93xx_pwm_get_freq(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
-
- if (readl(pwm->mmio_base + EP93XX_PWMx_ENABLE) & 0x1) {
- unsigned long rate = clk_get_rate(pwm->clk);
- u16 term = readl(pwm->mmio_base + EP93XX_PWMx_TERM_COUNT);
-
- return sprintf(buf, "%ld\n", rate / (term + 1));
- } else {
- return sprintf(buf, "disabled\n");
- }
-}
-
-static ssize_t ep93xx_pwm_set_freq(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
- long val;
- int err;
-
- err = kstrtol(buf, 10, &val);
- if (err)
- return -EINVAL;
-
- if (val == 0) {
- writel(0x0, pwm->mmio_base + EP93XX_PWMx_ENABLE);
- } else if (val <= (clk_get_rate(pwm->clk) / 2)) {
- u32 term, duty;
-
- val = (clk_get_rate(pwm->clk) / val) - 1;
- if (val > EP93XX_PWM_MAX_COUNT)
- val = EP93XX_PWM_MAX_COUNT;
- if (val < 1)
- val = 1;
-
- term = readl(pwm->mmio_base + EP93XX_PWMx_TERM_COUNT);
- duty = ((val + 1) * pwm->duty_percent / 100) - 1;
-
- /* If pwm is running, order is important */
- if (val > term) {
- writel(val, pwm->mmio_base + EP93XX_PWMx_TERM_COUNT);
- writel(duty, pwm->mmio_base + EP93XX_PWMx_DUTY_CYCLE);
- } else {
- writel(duty, pwm->mmio_base + EP93XX_PWMx_DUTY_CYCLE);
- writel(val, pwm->mmio_base + EP93XX_PWMx_TERM_COUNT);
- }
-
- if (!readl(pwm->mmio_base + EP93XX_PWMx_ENABLE) & 0x1)
- writel(0x1, pwm->mmio_base + EP93XX_PWMx_ENABLE);
- } else {
- return -EINVAL;
- }
-
- return count;
-}
-
-static ssize_t ep93xx_pwm_get_duty_percent(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
-
- return sprintf(buf, "%d\n", pwm->duty_percent);
-}
-
-static ssize_t ep93xx_pwm_set_duty_percent(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
- long val;
- int err;
-
- err = kstrtol(buf, 10, &val);
- if (err)
- return -EINVAL;
-
- if (val > 0 && val < 100) {
- u32 term = readl(pwm->mmio_base + EP93XX_PWMx_TERM_COUNT);
- u32 duty = ((term + 1) * val / 100) - 1;
-
- writel(duty, pwm->mmio_base + EP93XX_PWMx_DUTY_CYCLE);
- pwm->duty_percent = val;
- return count;
- }
-
- return -EINVAL;
-}
-
-static ssize_t ep93xx_pwm_get_invert(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
- int inverted = readl(pwm->mmio_base + EP93XX_PWMx_INVERT) & 0x1;
-
- return sprintf(buf, "%d\n", inverted);
-}
-
-static ssize_t ep93xx_pwm_set_invert(struct device *dev,
- struct device_attribute *attr, const char *buf, size_t count)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
- long val;
- int err;
-
- err = kstrtol(buf, 10, &val);
- if (err)
- return -EINVAL;
-
- if (val == 0)
- writel(0x0, pwm->mmio_base + EP93XX_PWMx_INVERT);
- else if (val == 1)
- writel(0x1, pwm->mmio_base + EP93XX_PWMx_INVERT);
- else
- return -EINVAL;
-
- return count;
-}
-
-static DEVICE_ATTR(min_freq, S_IRUGO, ep93xx_pwm_get_min_freq, NULL);
-static DEVICE_ATTR(max_freq, S_IRUGO, ep93xx_pwm_get_max_freq, NULL);
-static DEVICE_ATTR(freq, S_IWUSR | S_IRUGO,
- ep93xx_pwm_get_freq, ep93xx_pwm_set_freq);
-static DEVICE_ATTR(duty_percent, S_IWUSR | S_IRUGO,
- ep93xx_pwm_get_duty_percent, ep93xx_pwm_set_duty_percent);
-static DEVICE_ATTR(invert, S_IWUSR | S_IRUGO,
- ep93xx_pwm_get_invert, ep93xx_pwm_set_invert);
-
-static struct attribute *ep93xx_pwm_attrs[] = {
- &dev_attr_min_freq.attr,
- &dev_attr_max_freq.attr,
- &dev_attr_freq.attr,
- &dev_attr_duty_percent.attr,
- &dev_attr_invert.attr,
- NULL
-};
-
-static const struct attribute_group ep93xx_pwm_sysfs_files = {
- .attrs = ep93xx_pwm_attrs,
-};
-
-static int ep93xx_pwm_probe(struct platform_device *pdev)
-{
- struct ep93xx_pwm *pwm;
- struct resource *res;
- int ret;
-
- pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
- if (!pwm)
- return -ENOMEM;
-
- pwm->clk = devm_clk_get(&pdev->dev, "pwm_clk");
- if (IS_ERR(pwm->clk))
- return PTR_ERR(pwm->clk);
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- pwm->mmio_base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(pwm->mmio_base))
- return PTR_ERR(pwm->mmio_base);
-
- ret = ep93xx_pwm_acquire_gpio(pdev);
- if (ret)
- return ret;
-
- ret = sysfs_create_group(&pdev->dev.kobj, &ep93xx_pwm_sysfs_files);
- if (ret) {
- ep93xx_pwm_release_gpio(pdev);
- return ret;
- }
-
- pwm->duty_percent = 50;
-
- /* disable pwm at startup. Avoids zero value. */
- writel(0x0, pwm->mmio_base + EP93XX_PWMx_ENABLE);
- writel(EP93XX_PWM_MAX_COUNT, pwm->mmio_base + EP93XX_PWMx_TERM_COUNT);
- writel(EP93XX_PWM_MAX_COUNT/2, pwm->mmio_base + EP93XX_PWMx_DUTY_CYCLE);
-
- clk_enable(pwm->clk);
-
- platform_set_drvdata(pdev, pwm);
- return 0;
-}
-
-static int ep93xx_pwm_remove(struct platform_device *pdev)
-{
- struct ep93xx_pwm *pwm = platform_get_drvdata(pdev);
-
- writel(0x0, pwm->mmio_base + EP93XX_PWMx_ENABLE);
- clk_disable(pwm->clk);
- sysfs_remove_group(&pdev->dev.kobj, &ep93xx_pwm_sysfs_files);
- ep93xx_pwm_release_gpio(pdev);
-
- return 0;
-}
-
-static struct platform_driver ep93xx_pwm_driver = {
- .driver = {
- .name = "ep93xx-pwm",
- .owner = THIS_MODULE,
- },
- .probe = ep93xx_pwm_probe,
- .remove = ep93xx_pwm_remove,
-};
-module_platform_driver(ep93xx_pwm_driver);
-
-MODULE_AUTHOR("Matthieu Crapet <[email protected]>, "
- "H Hartley Sweeten <[email protected]>");
-MODULE_DESCRIPTION("EP93xx PWM driver");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:ep93xx-pwm");
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 75840b5..eece329 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -62,6 +62,15 @@ config PWM_BFIN
To compile this driver as a module, choose M here: the module
will be called pwm-bfin.
+config PWM_EP93XX
+ tristate "Cirrus Logic EP93xx PWM support"
+ depends on ARCH_EP93XX
+ help
+ Generic PWM framework driver for Cirrus Logic EP93xx.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-ep93xx.
+
config PWM_IMX
tristate "i.MX PWM support"
depends on ARCH_MXC
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 77a8c18..8b754e4 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_PWM_SYSFS) += sysfs.o
obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o
obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o
+obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o
obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
new file mode 100644
index 0000000..43e9b60
--- /dev/null
+++ b/drivers/pwm/pwm-ep93xx.c
@@ -0,0 +1,218 @@
+/*
+ * PWM framework driver for Cirrus Logic EP93xx
+ *
+ * Copyright (C) 2013 H Hartley Sweeten <[email protected]>
+ *
+ * EP9301/02 have only one channel:
+ * platform device ep93xx-pwm.1 - PWMOUT1 (EGPIO14)
+ *
+ * EP9307 has only one channel:
+ * platform device ep93xx-pwm.0 - PWMOUT
+ *
+ * EP9312/15 have two channels:
+ * platform device ep93xx-pwm.0 - PWMOUT
+ * platform device ep93xx-pwm.1 - PWMOUT1 (EGPIO14)
+ *
+ * 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; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/pwm.h>
+
+#include <asm/div64.h>
+
+#include <mach/platform.h> /* for ep93xx_pwm_{acquire,release}_gpio() */
+
+#define EP93XX_PWMx_TERM_COUNT 0x00
+#define EP93XX_PWMx_DUTY_CYCLE 0x04
+#define EP93XX_PWMx_ENABLE 0x08
+#define EP93XX_PWMx_INVERT 0x0c
+
+struct ep93xx_pwm {
+ void __iomem *base;
+ struct clk *clk;
+ struct pwm_chip chip;
+};
+
+static inline struct ep93xx_pwm *to_ep93xx_pwm(struct pwm_chip *chip)
+{
+ return container_of(chip, struct ep93xx_pwm, chip);
+}
+
+static int ep93xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct platform_device *pdev = to_platform_device(chip->dev);
+
+ return ep93xx_pwm_acquire_gpio(pdev);
+}
+
+static void ep93xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct platform_device *pdev = to_platform_device(chip->dev);
+
+ ep93xx_pwm_release_gpio(pdev);
+}
+
+static int ep93xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
+ void __iomem *base = ep93xx_pwm->base;
+ unsigned long long c;
+ unsigned long period_cycles;
+ unsigned long duty_cycles;
+ unsigned long term;
+ int ret = 0;
+
+ /*
+ * The clock needs to be enabled to access the PWM registers.
+ * Configuration can be changed at any time.
+ */
+ if (!test_bit(PWMF_ENABLED, &pwm->flags))
+ clk_enable(ep93xx_pwm->clk);
+
+ c = clk_get_rate(ep93xx_pwm->clk);
+ c *= period_ns;
+ do_div(c, 1000000000);
+ period_cycles = c;
+
+ c = period_cycles;
+ c *= duty_ns;
+ do_div(c, period_ns);
+ duty_cycles = c;
+
+ if (period_cycles < 0x10000 && duty_cycles < 0x10000) {
+ term = readw(base + EP93XX_PWMx_TERM_COUNT);
+
+ /* Order is important if PWM is running */
+ if (period_cycles > term) {
+ writew(period_cycles, base + EP93XX_PWMx_TERM_COUNT);
+ writew(duty_cycles, base + EP93XX_PWMx_DUTY_CYCLE);
+ } else {
+ writew(duty_cycles, base + EP93XX_PWMx_DUTY_CYCLE);
+ writew(period_cycles, base + EP93XX_PWMx_TERM_COUNT);
+ }
+ } else {
+ ret = -EINVAL;
+ }
+
+ if (!test_bit(PWMF_ENABLED, &pwm->flags))
+ clk_disable(ep93xx_pwm->clk);
+
+ return ret;
+}
+
+static int ep93xx_pwm_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
+
+ /*
+ * The clock needs to be enabled to access the PWM registers.
+ * Polarity can only be changed when the PWM is disabled.
+ */
+ clk_enable(ep93xx_pwm->clk);
+ writew(polarity, ep93xx_pwm->base + EP93XX_PWMx_INVERT);
+ clk_disable(ep93xx_pwm->clk);
+
+ return 0;
+}
+
+static int ep93xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
+
+ clk_enable(ep93xx_pwm->clk);
+ writew(0x1, ep93xx_pwm->base + EP93XX_PWMx_ENABLE);
+
+ return 0;
+}
+
+static void ep93xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
+
+ writew(0x0, ep93xx_pwm->base + EP93XX_PWMx_ENABLE);
+ clk_disable(ep93xx_pwm->clk);
+}
+
+static struct pwm_ops ep93xx_pwm_ops = {
+ .request = ep93xx_pwm_request,
+ .free = ep93xx_pwm_free,
+ .config = ep93xx_pwm_config,
+ .set_polarity = ep93xx_pwm_polarity,
+ .enable = ep93xx_pwm_enable,
+ .disable = ep93xx_pwm_disable,
+ .owner = THIS_MODULE,
+};
+
+static int ep93xx_pwm_probe(struct platform_device *pdev)
+{
+ struct ep93xx_pwm *ep93xx_pwm;
+ struct resource *res;
+ int ret;
+
+ ep93xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*ep93xx_pwm), GFP_KERNEL);
+ if (!ep93xx_pwm)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ ep93xx_pwm->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(ep93xx_pwm->base))
+ return PTR_ERR(ep93xx_pwm->base);
+
+ ep93xx_pwm->clk = devm_clk_get(&pdev->dev, "pwm_clk");
+ if (IS_ERR(ep93xx_pwm->clk))
+ return PTR_ERR(ep93xx_pwm->clk);
+
+ ep93xx_pwm->chip.dev = &pdev->dev;
+ ep93xx_pwm->chip.ops = &ep93xx_pwm_ops;
+ ep93xx_pwm->chip.base = -1;
+ ep93xx_pwm->chip.npwm = 1;
+
+ ret = pwmchip_add(&ep93xx_pwm->chip);
+ if (ret < 0)
+ return ret;
+
+ platform_set_drvdata(pdev, ep93xx_pwm);
+ return 0;
+}
+
+static int ep93xx_pwm_remove(struct platform_device *pdev)
+{
+ struct ep93xx_pwm *ep93xx_pwm;
+
+ ep93xx_pwm = platform_get_drvdata(pdev);
+ if (!ep93xx_pwm)
+ return -ENODEV;
+
+ return pwmchip_remove(&ep93xx_pwm->chip);
+}
+
+static struct platform_driver ep93xx_pwm_driver = {
+ .driver = {
+ .name = "ep93xx-pwm",
+ .owner = THIS_MODULE,
+ },
+ .probe = ep93xx_pwm_probe,
+ .remove = ep93xx_pwm_remove,
+};
+module_platform_driver(ep93xx_pwm_driver);
+
+MODULE_DESCRIPTION("Cirrus Logic EP93xx PWM driver");
+MODULE_AUTHOR("H Hartley Sweeten <[email protected]>");
+MODULE_ALIAS("platform:ep93xx-pwm");
+MODULE_LICENSE("GPL");
--
1.8.3.2
On Mon, Oct 14, 2013 at 02:57:48PM -0700, H Hartley Sweeten wrote:
> Remove the non-standard EP93xx pwm driver in drivers/misc and add
pwm -> PWM
> a new driver for the PWM chips on the EP93xx platforms based on the
> PWM framework.
>
> These PWM chips each support 1 PWM channel with programmable duty
Perhaps "chips" -> "controllers"?
> cycle, frequency, and polarity inversion.
>
> Signed-off-by: H Hartley Sweeten <[email protected]>
> Cc: Ryan Mallon <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
>
> diff --git a/drivers/misc/ep93xx_pwm.c b/drivers/misc/ep93xx_pwm.c
[...]
> - * (c) Copyright 2009 Matthieu Crapet <[email protected]>
> - * (c) Copyright 2009 H Hartley Sweeten <[email protected]>
[...]
> -MODULE_AUTHOR("Matthieu Crapet <[email protected]>, "
> - "H Hartley Sweeten <[email protected]>");
[...]
> diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
[...]
> + * Copyright (C) 2013 H Hartley Sweeten <[email protected]>
[...]
> +MODULE_AUTHOR("H Hartley Sweeten <[email protected]>");
Why are you removing Matthieu from the list of authors and copyright
here? From a brief look it seems like this new driver is still based on
code from the old driver and not a complete rewrite.
> +#include <mach/platform.h> /* for ep93xx_pwm_{acquire,release}_gpio() */
I'm not sure how well that will play together with multiplatform support
but perhaps that's not an issue for ep93xx?
> +static int ep93xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct platform_device *pdev = to_platform_device(chip->dev);
> +
> + return ep93xx_pwm_acquire_gpio(pdev);
> +}
> +
> +static void ep93xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct platform_device *pdev = to_platform_device(chip->dev);
> +
> + ep93xx_pwm_release_gpio(pdev);
> +}
This looks like it would belong in the domain of pinctrl, but I suspect
that ep93xx doesn't support that.
> +static int ep93xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> +{
> + struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
> + void __iomem *base = ep93xx_pwm->base;
> + unsigned long long c;
> + unsigned long period_cycles;
> + unsigned long duty_cycles;
> + unsigned long term;
> + int ret = 0;
> +
> + /*
> + * The clock needs to be enabled to access the PWM registers.
> + * Configuration can be changed at any time.
> + */
> + if (!test_bit(PWMF_ENABLED, &pwm->flags))
> + clk_enable(ep93xx_pwm->clk);
clk_enable() can fail, so you should check the return value and
propagate errors.
> +static int ep93xx_pwm_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> + enum pwm_polarity polarity)
> +{
> + struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
> +
> + /*
> + * The clock needs to be enabled to access the PWM registers.
> + * Polarity can only be changed when the PWM is disabled.
> + */
Nit: the closing */ is wrongly aligned.
> + clk_enable(ep93xx_pwm->clk);
Needs a check of the return value.
> + writew(polarity, ep93xx_pwm->base + EP93XX_PWMx_INVERT);
I'd prefer if this did some explicit conversion from the PWM framework
value to the driver-specific value, even if they happen to be the same
in this case.
> +static int ep93xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
> +
> + clk_enable(ep93xx_pwm->clk);
Also needs to check the return value.
> +static struct pwm_ops ep93xx_pwm_ops = {
static const, please.
> +static int ep93xx_pwm_remove(struct platform_device *pdev)
> +{
> + struct ep93xx_pwm *ep93xx_pwm;
> +
> + ep93xx_pwm = platform_get_drvdata(pdev);
> + if (!ep93xx_pwm)
> + return -ENODEV;
No need for this check. It will never happen.
> +
> + return pwmchip_remove(&ep93xx_pwm->chip);
> +}
> +
> +static struct platform_driver ep93xx_pwm_driver = {
> + .driver = {
> + .name = "ep93xx-pwm",
> + .owner = THIS_MODULE,
This is no longer required because the core sets it to the proper value.
> + },
> + .probe = ep93xx_pwm_probe,
> + .remove = ep93xx_pwm_remove,
> +};
Oh, and I didn't mention it before, but please get rid of all the
needless tabs for alignment. It's completely useless and doesn't help
with readability at all in my opinion.
Thierry
On Tuesday, October 15, 2013 3:40 AM, Thierry Reding wrote:
> On Mon, Oct 14, 2013 at 02:57:48PM -0700, H Hartley Sweeten wrote:
>> Remove the non-standard EP93xx pwm driver in drivers/misc and add
>
> pwm -> PWM
OK
>> a new driver for the PWM chips on the EP93xx platforms based on the
>> PWM framework.
>>
>> These PWM chips each support 1 PWM channel with programmable duty
>
> Perhaps "chips" -> "controllers"?
OK
>> cycle, frequency, and polarity inversion.
>>
>> Signed-off-by: H Hartley Sweeten <[email protected]>
>> Cc: Ryan Mallon <[email protected]>
>> Cc: Thierry Reding <[email protected]>
>> Cc: Arnd Bergmann <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>>
>> diff --git a/drivers/misc/ep93xx_pwm.c b/drivers/misc/ep93xx_pwm.c
> [...]
>> - * (c) Copyright 2009 Matthieu Crapet <[email protected]>
>> - * (c) Copyright 2009 H Hartley Sweeten <[email protected]>
> [...]
>> -MODULE_AUTHOR("Matthieu Crapet <[email protected]>, "
>> - "H Hartley Sweeten <[email protected]>");
> [...]
>
>> diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
> [...]
>> + * Copyright (C) 2013 H Hartley Sweeten <[email protected]>
> [...]
>> +MODULE_AUTHOR("H Hartley Sweeten <[email protected]>");
>
> Why are you removing Matthieu from the list of authors and copyright
> here? From a brief look it seems like this new driver is still based on
> code from the old driver and not a complete rewrite.
My bad. It is based on the misc driver but I forgot to put Matthieu in as
one of the original authors when I wrote it.
I'll fix that.
>> +#include <mach/platform.h> /* for ep93xx_pwm_{acquire,release}_gpio() */
>
> I'm not sure how well that will play together with multiplatform support
> but perhaps that's not an issue for ep93xx?
For multiplatform it would probably be a problem. But I don't think anyone
would be including ep93xx in a multiplatform kernel. If the problem comes up
I'll figure out some way to deal with it, probably with a pinctrl driver.
>> +static int ep93xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct platform_device *pdev = to_platform_device(chip->dev);
>> +
>> + return ep93xx_pwm_acquire_gpio(pdev);
>> +}
>> +
>> +static void ep93xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct platform_device *pdev = to_platform_device(chip->dev);
>> +
>> + ep93xx_pwm_release_gpio(pdev);
>> +}
>
> This looks like it would belong in the domain of pinctrl, but I suspect
> that ep93xx doesn't support that.
It should be but I have not worked out how to support EP93xx GPIOs with a
pinctrl driver yet. The GPIOs are pretty limited on this platform compared to
the other pinctrl users.
>> +static int ep93xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> + int duty_ns, int period_ns)
>> +{
>> + struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
>> + void __iomem *base = ep93xx_pwm->base;
>> + unsigned long long c;
>> + unsigned long period_cycles;
>> + unsigned long duty_cycles;
>> + unsigned long term;
>> + int ret = 0;
>> +
>> + /*
>> + * The clock needs to be enabled to access the PWM registers.
>> + * Configuration can be changed at any time.
>> + */
>> + if (!test_bit(PWMF_ENABLED, &pwm->flags))
>> + clk_enable(ep93xx_pwm->clk);
>
> clk_enable() can fail, so you should check the return value and
> propagate errors.
I overlooked that. This will be fixed in the next version.
>> +static int ep93xx_pwm_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
>> + enum pwm_polarity polarity)
>> +{
>> + struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
>> +
>> + /*
>> + * The clock needs to be enabled to access the PWM registers.
>> + * Polarity can only be changed when the PWM is disabled.
>> + */
>
> Nit: the closing */ is wrongly aligned.
OK
>> + clk_enable(ep93xx_pwm->clk);
>
> Needs a check of the return value.
OK
>> + writew(polarity, ep93xx_pwm->base + EP93XX_PWMx_INVERT);
>
> I'd prefer if this did some explicit conversion from the PWM framework
> value to the driver-specific value, even if they happen to be the same
> in this case.
OK
>> +static int ep93xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct ep93xx_pwm *ep93xx_pwm = to_ep93xx_pwm(chip);
>> +
>> + clk_enable(ep93xx_pwm->clk);
>
> Also needs to check the return value.
OK
>> +static struct pwm_ops ep93xx_pwm_ops = {
>
> static const, please.
OK
>> +static int ep93xx_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct ep93xx_pwm *ep93xx_pwm;
>> +
>> + ep93xx_pwm = platform_get_drvdata(pdev);
>> + if (!ep93xx_pwm)
>> + return -ENODEV;
>
> No need for this check. It will never happen.
OK
>> +
>> + return pwmchip_remove(&ep93xx_pwm->chip);
>> +}
>> +
>> +static struct platform_driver ep93xx_pwm_driver = {
>> + .driver = {
>> + .name = "ep93xx-pwm",
>> + .owner = THIS_MODULE,
>
> This is no longer required because the core sets it to the proper value.
OK
>> + },
>> + .probe = ep93xx_pwm_probe,
>> + .remove = ep93xx_pwm_remove,
>> +};
>
> Oh, and I didn't mention it before, but please get rid of all the
> needless tabs for alignment. It's completely useless and doesn't help
> with readability at all in my opinion.
Opinions differ.. But I'll remove the tabs.
Thanks for the review,
Hartley