Add support for PWM chips present on SPEAr platforms. These PWM
chips support 4 channel output with programmable duty cycle and
frequency.
More details on these PWM chips can be obtained from relevant
chapter of reference manual, present at following[1] location.
1. http://www.st.com/internet/mcu/product/251211.jsp
Cc: Thierry Reding <[email protected]>
Signed-off-by: Shiraz Hashim <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
Reviewed-by: Vipin Kumar <[email protected]>
---
Changes:-
V2 --> V3:
* remove "disabled" line from pwm dt binding documentation
* remove un-necessary check on pwm chip (for NULL) in remove.
V1 --> V2:
* make proper reference to pwm and pwm chip
* take care to capitalize PWM at appropriate places
* fix compatible string to the SoC where pwm chip was introduced
* Rename the documentation file to the name of driver
* Fix cosmetic changes like names, function name alignment, paragraph
formating, comments placement and formating, etc.
* Group and associate the bit field definitions to their registers
* Fix kerneldoc for structure definition
* Use chip to name pwm device and pwm for the channel instance
* Remove init section qualifiers
* Remove ifdefs around device tree from code and add dependency on CONFIG_OF
* prepare/unprepare clock once in probe/remove and just enable/disable
at rest of the places.
* Use _relaxed for readl/writel.
* Fix pwm disable part in remove
.../devicetree/bindings/pwm/spear-pwm.txt | 18 ++
drivers/pwm/Kconfig | 11 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-spear.c | 280 ++++++++++++++++++++
4 files changed, 310 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/spear-pwm.txt
create mode 100644 drivers/pwm/pwm-spear.c
diff --git a/Documentation/devicetree/bindings/pwm/spear-pwm.txt b/Documentation/devicetree/bindings/pwm/spear-pwm.txt
new file mode 100644
index 0000000..3ac779d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/spear-pwm.txt
@@ -0,0 +1,18 @@
+== ST SPEAr SoC PWM controller ==
+
+Required properties:
+- compatible: should be one of:
+ - "st,spear320-pwm"
+ - "st,spear1340-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: number of cells used to specify PWM which is fixed to 2 on
+ SPEAr. The first cell specifies the per-chip index of the PWM to use and
+ the second cell is the period in nanoseconds.
+
+Example:
+
+ pwm: pwm@a8000000 {
+ compatible ="st,spear320-pwm";
+ reg = <0xa8000000 0x1000>;
+ #pwm-cells = <2>;
+ };
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..6e556c7 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -112,6 +112,17 @@ config PWM_SAMSUNG
To compile this driver as a module, choose M here: the module
will be called pwm-samsung.
+config PWM_SPEAR
+ tristate "STMicroelectronics SPEAr PWM support"
+ depends on PLAT_SPEAR
+ depends on OF
+ help
+ Generic PWM framework driver for the PWM controller on ST
+ SPEAr SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-spear.
+
config PWM_TEGRA
tristate "NVIDIA Tegra PWM support"
depends on ARCH_TEGRA
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..6512786 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
+obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
obj-$(CONFIG_PWM_TWL6030) += pwm-twl6030.o
diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
new file mode 100644
index 0000000..bb63508
--- /dev/null
+++ b/drivers/pwm/pwm-spear.c
@@ -0,0 +1,280 @@
+/*
+ * ST Microelectronics SPEAr Pulse Width Modulator driver
+ *
+ * Copyright (C) 2012 ST Microelectronics
+ * Shiraz Hashim <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define NUM_PWM 4
+
+/* PWM registers and bits definitions */
+#define PWMCR 0x00 /* Control Register */
+#define PWMCR_PWM_ENABLE 0x1
+#define PWMCR_PRESCALE_SHIFT 2
+#define PWMCR_MIN_PRESCALE 0x00
+#define PWMCR_MAX_PRESCALE 0x3FFF
+
+#define PWMDCR 0x04 /* Duty Cycle Register */
+#define PWMDCR_MIN_DUTY 0x0001
+#define PWMDCR_MAX_DUTY 0xFFFF
+
+#define PWMPCR 0x08 /* Period Register */
+#define PWMPCR_MIN_PERIOD 0x0001
+#define PWMPCR_MAX_PERIOD 0xFFFF
+
+/* Following only available on 13xx SoCs */
+#define PWMMCR 0x3C /* Master Control Register */
+#define PWMMCR_PWM_ENABLE 0x1
+
+/**
+ * struct spear_pwm_chip - struct representing pwm chip
+ *
+ * @mmio_base: base address of pwm chip
+ * @clk: pointer to clk structure of pwm chip
+ * @chip: linux pwm chip representation
+ * @dev: pointer to device structure of pwm chip
+ */
+struct spear_pwm_chip {
+ void __iomem *mmio_base;
+ struct clk *clk;
+ struct pwm_chip chip;
+ struct device *dev;
+};
+
+static inline struct spear_pwm_chip *to_spear_pwm_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct spear_pwm_chip, chip);
+}
+
+static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int num,
+ unsigned long offset)
+{
+ return readl_relaxed(chip->mmio_base + (num << 4) + offset);
+}
+
+static inline void spear_pwm_writel(struct spear_pwm_chip *chip,
+ unsigned int num, unsigned long offset,
+ unsigned long val)
+{
+ writel_relaxed(val, chip->mmio_base + (num << 4) + offset);
+}
+
+int spear_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns,
+ int period_ns)
+{
+ struct spear_pwm_chip *pc = to_spear_pwm_chip(chip);
+ u64 val, div, clk_rate;
+ unsigned long prescale = PWMCR_MIN_PRESCALE, pv, dc;
+ int ret;
+
+ /*
+ * Find pv, dc and prescale to suit duty_ns and period_ns. This is done
+ * according to formulas described below:
+ *
+ * period_ns = 10^9 * (PRESCALE + 1) * PV / PWM_CLK_RATE
+ * duty_ns = 10^9 * (PRESCALE + 1) * DC / PWM_CLK_RATE
+ *
+ * PV = (PWM_CLK_RATE * period_ns) / (10^9 * (PRESCALE + 1))
+ * DC = (PWM_CLK_RATE * duty_ns) / (10^9 * (PRESCALE + 1))
+ */
+ clk_rate = clk_get_rate(pc->clk);
+ while (1) {
+ div = 1000000000;
+ div *= 1 + prescale;
+ val = clk_rate * period_ns;
+ pv = div64_u64(val, div);
+ val = clk_rate * duty_ns;
+ dc = div64_u64(val, div);
+
+ /* if duty_ns and period_ns are not achievable then return */
+ if (pv < PWMPCR_MIN_PERIOD || dc < PWMDCR_MIN_DUTY)
+ return -EINVAL;
+
+ /*
+ * if pv and dc have crossed their upper limit, then increase
+ * prescale and recalculate pv and dc.
+ */
+ if (pv > PWMPCR_MAX_PERIOD || dc > PWMDCR_MAX_DUTY) {
+ if (++prescale > PWMCR_MAX_PRESCALE)
+ return -EINVAL;
+ continue;
+ }
+ break;
+ }
+
+ /*
+ * NOTE: the clock to PWM has to be enabled first before writing to the
+ * registers.
+ */
+ ret = clk_enable(pc->clk);
+ if (ret)
+ return ret;
+
+ spear_pwm_writel(pc, pwm->hwpwm, PWMCR,
+ prescale << PWMCR_PRESCALE_SHIFT);
+ spear_pwm_writel(pc, pwm->hwpwm, PWMDCR, dc);
+ spear_pwm_writel(pc, pwm->hwpwm, PWMPCR, pv);
+ clk_disable(pc->clk);
+
+ return 0;
+}
+
+static int spear_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct spear_pwm_chip *pc = to_spear_pwm_chip(chip);
+ int rc = 0;
+ u32 val;
+
+ rc = clk_enable(pc->clk);
+ if (rc < 0)
+ return rc;
+
+ val = spear_pwm_readl(pc, pwm->hwpwm, PWMCR);
+ val |= PWMCR_PWM_ENABLE;
+ spear_pwm_writel(pc, pwm->hwpwm, PWMCR, val);
+
+ return 0;
+}
+
+static void spear_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct spear_pwm_chip *pc = to_spear_pwm_chip(chip);
+ u32 val;
+
+ val = spear_pwm_readl(pc, pwm->hwpwm, PWMCR);
+ val &= ~PWMCR_PWM_ENABLE;
+ spear_pwm_writel(pc, pwm->hwpwm, PWMCR, val);
+
+ clk_disable(pc->clk);
+}
+
+static const struct pwm_ops spear_pwm_ops = {
+ .config = spear_pwm_config,
+ .enable = spear_pwm_enable,
+ .disable = spear_pwm_disable,
+ .owner = THIS_MODULE,
+};
+
+static int spear_pwm_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct spear_pwm_chip *pc;
+ struct resource *r;
+ int ret;
+ u32 val;
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!r) {
+ dev_err(&pdev->dev, "no memory resources defined\n");
+ return -ENODEV;
+ }
+
+ pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
+ if (!pc) {
+ dev_err(&pdev->dev, "failed to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ pc->dev = &pdev->dev;
+
+ pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
+ if (!pc->mmio_base)
+ return -EADDRNOTAVAIL;
+
+ platform_set_drvdata(pdev, pc);
+
+ pc->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pc->clk))
+ return PTR_ERR(pc->clk);
+
+ pc->chip.dev = &pdev->dev;
+ pc->chip.ops = &spear_pwm_ops;
+ pc->chip.base = -1;
+ pc->chip.npwm = NUM_PWM;
+
+ ret = pwmchip_add(&pc->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(pc->clk);
+ if (ret < 0)
+ return pwmchip_remove(&pc->chip);
+
+ if (np && of_device_is_compatible(np, "st,spear1340-pwm")) {
+ /*
+ * Following enables PWM chip, channels would still be
+ * enabled individually through their control register
+ */
+ val = readl_relaxed(pc->mmio_base + PWMMCR);
+ val |= PWMMCR_PWM_ENABLE;
+ writel_relaxed(val, pc->mmio_base + PWMMCR);
+
+ }
+
+ /* only disable the clk and leave it prepared */
+ clk_disable(pc->clk);
+
+ return 0;
+}
+
+static int spear_pwm_remove(struct platform_device *pdev)
+{
+ struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
+ int i;
+
+ for (i = 0; i < NUM_PWM; i++) {
+ struct pwm_device *pwm = &pc->chip.pwms[i];
+
+ if (test_bit(PWMF_ENABLED, &pwm->flags)) {
+ spear_pwm_writel(pc, i, PWMCR, 0);
+ clk_disable(pc->clk);
+ }
+ }
+
+ /* clk was prepared in probe, hence unprepare it here */
+ clk_unprepare(pc->clk);
+ return pwmchip_remove(&pc->chip);
+}
+
+static struct of_device_id spear_pwm_of_match[] = {
+ { .compatible = "st,spear320-pwm" },
+ { .compatible = "st,spear1340-pwm" },
+ { }
+};
+
+MODULE_DEVICE_TABLE(of, spear_pwm_of_match);
+
+static struct platform_driver spear_pwm_driver = {
+ .driver = {
+ .name = "spear-pwm",
+ .of_match_table = spear_pwm_of_match,
+ },
+ .probe = spear_pwm_probe,
+ .remove = spear_pwm_remove,
+};
+
+module_platform_driver(spear_pwm_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Shiraz Hashim <[email protected]>");
+MODULE_AUTHOR("Viresh Kumar <[email protected]>");
+MODULE_ALIAS("platform:spear-pwm");
--
1.7.10
Every time you read a code, you figure out new things about it.
Sorry for these comments Now :(
On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <[email protected]> wrote:
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index acfe482..6512786 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> +obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
I have gone through this on every version of this patch, but couldn't figure
out that you were trying to add it in alphabetical order, but you couldn't.
> diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
> +static int spear_pwm_probe(struct platform_device *pdev)
> +{
> + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> + if (!pc) {
> + dev_err(&pdev->dev, "failed to allocate memory\n");
> + return -ENOMEM;
> + }
> +
> + pc->dev = &pdev->dev;
If you are going to send another version, then please move this
> + pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> + if (!pc->mmio_base)
> + return -EADDRNOTAVAIL;
> +
> + platform_set_drvdata(pdev, pc);
and this
> + pc->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pc->clk))
> + return PTR_ERR(pc->clk);
to this place :)
So, that we don't do these for failure cases.
> + pc->chip.dev = &pdev->dev;
> + pc->chip.ops = &spear_pwm_ops;
> + pc->chip.base = -1;
> + pc->chip.npwm = NUM_PWM;
> +
> + if (np && of_device_is_compatible(np, "st,spear1340-pwm")) {
I have noticed it earlier, but don't know why didn't i gave a comment here?
you don't need to check for np here. It can't be NULL as you depend on
CONFIG_OF.
> + /*
> + * Following enables PWM chip, channels would still be
> + * enabled individually through their control register
> + */
> + val = readl_relaxed(pc->mmio_base + PWMMCR);
> + val |= PWMMCR_PWM_ENABLE;
> + writel_relaxed(val, pc->mmio_base + PWMMCR);
> +
> + }
> +
> + /* only disable the clk and leave it prepared */
> + clk_disable(pc->clk);
> +
> + return 0;
> +}
> +
> +static int spear_pwm_remove(struct platform_device *pdev)
> +{
> + struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> + int i;
> +
> + for (i = 0; i < NUM_PWM; i++) {
> + struct pwm_device *pwm = &pc->chip.pwms[i];
> +
> + if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> + spear_pwm_writel(pc, i, PWMCR, 0);
> + clk_disable(pc->clk);
> + }
> + }
> +
> + /* clk was prepared in probe, hence unprepare it here */
> + clk_unprepare(pc->clk);
I believe you need to remove the chip first and then do above to
avoid any race conditions, that might occur.
> + return pwmchip_remove(&pc->chip);
> +}
> +
After all this please add my:
Acked-by: Viresh Kumar <[email protected]>
Sorry Shiraz for so late comments, i can understand your frustration :(
--
viresh
Hi Viresh,
On Mon, Oct 22, 2012 at 09:39:21AM +0530, viresh kumar wrote:
> Every time you read a code, you figure out new things about it.
> Sorry for these comments Now :(
No problem, it is important to fix now than catch them later.
> On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <[email protected]> wrote:
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index acfe482..6512786 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
> > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
> > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
> > obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
> > +obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
>
> I have gone through this on every version of this patch, but couldn't figure
> out that you were trying to add it in alphabetical order, but you couldn't.
>
Would fix this.
> > diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c
>
> > +static int spear_pwm_probe(struct platform_device *pdev)
> > +{
> > + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
> > + if (!pc) {
> > + dev_err(&pdev->dev, "failed to allocate memory\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pc->dev = &pdev->dev;
>
> If you are going to send another version, then please move this
>
> > + pc->mmio_base = devm_request_and_ioremap(&pdev->dev, r);
> > + if (!pc->mmio_base)
> > + return -EADDRNOTAVAIL;
> > +
> > + platform_set_drvdata(pdev, pc);
>
> and this
>
> > + pc->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(pc->clk))
> > + return PTR_ERR(pc->clk);
>
> to this place :)
> So, that we don't do these for failure cases.
>
Okay.
> > + pc->chip.dev = &pdev->dev;
> > + pc->chip.ops = &spear_pwm_ops;
> > + pc->chip.base = -1;
> > + pc->chip.npwm = NUM_PWM;
> > +
>
> > + if (np && of_device_is_compatible(np, "st,spear1340-pwm")) {
>
> I have noticed it earlier, but don't know why didn't i gave a comment here?
> you don't need to check for np here. It can't be NULL as you depend on
> CONFIG_OF.
Okay, would remove this.
> > + /*
> > + * Following enables PWM chip, channels would still be
> > + * enabled individually through their control register
> > + */
> > + val = readl_relaxed(pc->mmio_base + PWMMCR);
> > + val |= PWMMCR_PWM_ENABLE;
> > + writel_relaxed(val, pc->mmio_base + PWMMCR);
> > +
> > + }
> > +
> > + /* only disable the clk and leave it prepared */
> > + clk_disable(pc->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int spear_pwm_remove(struct platform_device *pdev)
> > +{
> > + struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> > + int i;
> > +
> > + for (i = 0; i < NUM_PWM; i++) {
> > + struct pwm_device *pwm = &pc->chip.pwms[i];
> > +
> > + if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> > + spear_pwm_writel(pc, i, PWMCR, 0);
> > + clk_disable(pc->clk);
> > + }
> > + }
> > +
> > + /* clk was prepared in probe, hence unprepare it here */
> > + clk_unprepare(pc->clk);
>
> I believe you need to remove the chip first and then do above to
> avoid any race conditions, that might occur.
>
I am afraid, I would loose all chips and their related information
(PWMF_ENABLED) then.
> > + return pwmchip_remove(&pc->chip);
> > +}
> > +
>
> After all this please add my:
> Acked-by: Viresh Kumar <[email protected]>
I had already added your sob as you were the original author,
should I add a separate acked-by also ?
> Sorry Shiraz for so late comments, i can understand your
> frustration :(
No issues, review is higienic :)
--
regards
Shiraz
On 22 October 2012 11:36, Shiraz Hashim <[email protected]> wrote:
> On Mon, Oct 22, 2012 at 09:39:21AM +0530, viresh kumar wrote:
>> On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <[email protected]> wrote:
>> > +static int spear_pwm_remove(struct platform_device *pdev)
>> > +{
>> > + struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
>> > + int i;
>> > +
>> > + for (i = 0; i < NUM_PWM; i++) {
>> > + struct pwm_device *pwm = &pc->chip.pwms[i];
>> > +
>> > + if (test_bit(PWMF_ENABLED, &pwm->flags)) {
>> > + spear_pwm_writel(pc, i, PWMCR, 0);
>> > + clk_disable(pc->clk);
>> > + }
>> > + }
>> > +
>> > + /* clk was prepared in probe, hence unprepare it here */
>> > + clk_unprepare(pc->clk);
>>
>> I believe you need to remove the chip first and then do above to
>> avoid any race conditions, that might occur.
>
> I am afraid, I would loose all chips and their related information
> (PWMF_ENABLED) then.
I have just checked core's code, and yes you are correct.
Now i have another doubt :)
Why shouldn't you do this instead:
for (i = 0; i < NUM_PWM; i++)
pwm_diable(&pc->chip.pwms[i]);
And, why should we put above code in pwmchip_remove() instead, so that
pwm drivers don't need to do all this?
@Thierry: Your inputs are required here :)
>> > + return pwmchip_remove(&pc->chip);
>> > +}
>> > +
>>
>> After all this please add my:
>> Acked-by: Viresh Kumar <[email protected]>
>
> I had already added your sob as you were the original author,
> should I add a separate acked-by also ?
Yes, it should be fine. I have seen that earlier.
Because you weren't the original author, you kept my SOB.
This Acked-by will guarantee that original author is okay with
the changes you have made. :)
--
viresh
On Mon, Oct 22, 2012 at 11:51:11AM +0530, Viresh Kumar wrote:
> On 22 October 2012 11:36, Shiraz Hashim <[email protected]> wrote:
> > On Mon, Oct 22, 2012 at 09:39:21AM +0530, viresh kumar wrote:
> >> On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <[email protected]> wrote:
>
> >> > +static int spear_pwm_remove(struct platform_device *pdev)
> >> > +{
> >> > + struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> >> > + int i;
> >> > +
> >> > + for (i = 0; i < NUM_PWM; i++) {
> >> > + struct pwm_device *pwm = &pc->chip.pwms[i];
> >> > +
> >> > + if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> >> > + spear_pwm_writel(pc, i, PWMCR, 0);
> >> > + clk_disable(pc->clk);
> >> > + }
> >> > + }
> >> > +
> >> > + /* clk was prepared in probe, hence unprepare it here */
> >> > + clk_unprepare(pc->clk);
> >>
> >> I believe you need to remove the chip first and then do above to
> >> avoid any race conditions, that might occur.
> >
> > I am afraid, I would loose all chips and their related information
> > (PWMF_ENABLED) then.
>
> I have just checked core's code, and yes you are correct.
> Now i have another doubt :)
>
> Why shouldn't you do this instead:
>
> for (i = 0; i < NUM_PWM; i++)
> pwm_diable(&pc->chip.pwms[i]);
>
> And, why should we put above code in pwmchip_remove() instead, so that
> pwm drivers don't need to do all this?
>
> @Thierry: Your inputs are required here :)
We could probably do that in the core. I've had some discussions about
this with Lars-Peter (Cc'ed) who also had doubts about how this is
currently handled.
The problem is that the core driver code ignores errors from the
driver's .remove() callback, so actually returning the error of
pwmchip_remove() here isn't terribly useful. I had actually assumed
(without checking the code) that the device wouldn't be removed if an
error was returned, but that isn't true.
IIRC Lars-Peter suggested that we do reference counting on PWM devices
so that they could stay around after the module is unloaded but return
errors (-ENODEV?) on all operations to make sure users are aware of them
disappearing.
What you're proposing is different, however. If we put that code in the
core it will mean that once the module is unloaded, all PWM devices will
be disabled. There is currently code in the core that prevents the chip
from being removed if one or more PWM devices are busy. But as explained
above, with the current core code this return value isn't useful at all.
This needs to be addressed, but I'm not quite sure how yet. Obviously it
cannot be solved in the core, because the PWM devices may be provided by
real hotpluggable devices, so just preventing the driver from being
removed won't help.
Thierry
On 22 October 2012 13:25, Thierry Reding
<[email protected]> wrote:
> We could probably do that in the core. I've had some discussions about
> this with Lars-Peter (Cc'ed) who also had doubts about how this is
> currently handled.
>
> What you're proposing is different, however. If we put that code in the
> core it will mean that once the module is unloaded, all PWM devices will
> be disabled. There is currently code in the core that prevents the chip
> from being removed if one or more PWM devices are busy. But as explained
> above, with the current core code this return value isn't useful at all.
This is what many drivers in pwm framework are doing currently too..
They disable
pwm and its clock and then do chip remove.
Sorry, i didn't get the conclusion completely :(
Should we keep code suggested by me in core or spear's driver?
--
viresh
On Mon, Oct 22, 2012 at 01:55:29PM +0530, Viresh Kumar wrote:
> On 22 October 2012 13:25, Thierry Reding
> <[email protected]> wrote:
> > We could probably do that in the core. I've had some discussions about
> > this with Lars-Peter (Cc'ed) who also had doubts about how this is
> > currently handled.
> >
> > What you're proposing is different, however. If we put that code in the
> > core it will mean that once the module is unloaded, all PWM devices will
> > be disabled. There is currently code in the core that prevents the chip
> > from being removed if one or more PWM devices are busy. But as explained
> > above, with the current core code this return value isn't useful at all.
>
> This is what many drivers in pwm framework are doing currently too..
> They disable
> pwm and its clock and then do chip remove.
>
> Sorry, i didn't get the conclusion completely :(
> Should we keep code suggested by me in core or spear's driver?
I think for now we can keep it in the SPEAr driver. I'll make sure to
refactor it out into the core once I have a good plan on how to solve
this issue properly.
Thierry
On 10/22/2012 09:55 AM, Thierry Reding wrote:
> On Mon, Oct 22, 2012 at 11:51:11AM +0530, Viresh Kumar wrote:
>> On 22 October 2012 11:36, Shiraz Hashim <[email protected]> wrote:
>>> On Mon, Oct 22, 2012 at 09:39:21AM +0530, viresh kumar wrote:
>>>> On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <[email protected]> wrote:
>>
>>>>> +static int spear_pwm_remove(struct platform_device *pdev)
>>>>> +{
>>>>> + struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
>>>>> + int i;
>>>>> +
>>>>> + for (i = 0; i < NUM_PWM; i++) {
>>>>> + struct pwm_device *pwm = &pc->chip.pwms[i];
>>>>> +
>>>>> + if (test_bit(PWMF_ENABLED, &pwm->flags)) {
>>>>> + spear_pwm_writel(pc, i, PWMCR, 0);
>>>>> + clk_disable(pc->clk);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + /* clk was prepared in probe, hence unprepare it here */
>>>>> + clk_unprepare(pc->clk);
>>>>
>>>> I believe you need to remove the chip first and then do above to
>>>> avoid any race conditions, that might occur.
>>>
>>> I am afraid, I would loose all chips and their related information
>>> (PWMF_ENABLED) then.
>>
>> I have just checked core's code, and yes you are correct.
>> Now i have another doubt :)
>>
>> Why shouldn't you do this instead:
>>
>> for (i = 0; i < NUM_PWM; i++)
>> pwm_diable(&pc->chip.pwms[i]);
>>
>> And, why should we put above code in pwmchip_remove() instead, so that
>> pwm drivers don't need to do all this?
>>
>> @Thierry: Your inputs are required here :)
>
> We could probably do that in the core. I've had some discussions about
> this with Lars-Peter (Cc'ed) who also had doubts about how this is
> currently handled.
>
> The problem is that the core driver code ignores errors from the
> driver's .remove() callback, so actually returning the error of
> pwmchip_remove() here isn't terribly useful. I had actually assumed
> (without checking the code) that the device wouldn't be removed if an
> error was returned, but that isn't true.
>
> IIRC Lars-Peter suggested that we do reference counting on PWM devices
> so that they could stay around after the module is unloaded but return
> errors (-ENODEV?) on all operations to make sure users are aware of them
> disappearing.
>
> What you're proposing is different, however. If we put that code in the
> core it will mean that once the module is unloaded, all PWM devices will
> be disabled. There is currently code in the core that prevents the chip
> from being removed if one or more PWM devices are busy. But as explained
> above, with the current core code this return value isn't useful at all.
>
> This needs to be addressed, but I'm not quite sure how yet. Obviously it
> cannot be solved in the core, because the PWM devices may be provided by
> real hotpluggable devices, so just preventing the driver from being
> removed won't help.
In my opinion it would make sense to put this into the PWM core. Even if the
device is still physically connected, e.g. because it is a on-SoC device, it
should be stopped if the device is removed. You do not want the PWM device
to continue to provide it's service (which is the PWM signal) after the
device has been removed. This means this is something that needs to be
implemented by every PWM driver.
Btw. you still won't be able to remove the module while one or more devices
are busy. You'll just be able to either hotunplug the device or unbind it
via sysfs. Once the device is no longer bound you can then remove the module
as well. Or at least should be able.
On Mon, Oct 22, 2012 at 02:20:26PM +0200, Lars-Peter Clausen wrote:
> On 10/22/2012 09:55 AM, Thierry Reding wrote:
> > On Mon, Oct 22, 2012 at 11:51:11AM +0530, Viresh Kumar wrote:
> >> On 22 October 2012 11:36, Shiraz Hashim <[email protected]> wrote:
> >>> On Mon, Oct 22, 2012 at 09:39:21AM +0530, viresh kumar wrote:
> >>>> On Mon, Oct 22, 2012 at 9:21 AM, Shiraz Hashim <[email protected]> wrote:
> >>
> >>>>> +static int spear_pwm_remove(struct platform_device *pdev)
> >>>>> +{
> >>>>> + struct spear_pwm_chip *pc = platform_get_drvdata(pdev);
> >>>>> + int i;
> >>>>> +
> >>>>> + for (i = 0; i < NUM_PWM; i++) {
> >>>>> + struct pwm_device *pwm = &pc->chip.pwms[i];
> >>>>> +
> >>>>> + if (test_bit(PWMF_ENABLED, &pwm->flags)) {
> >>>>> + spear_pwm_writel(pc, i, PWMCR, 0);
> >>>>> + clk_disable(pc->clk);
> >>>>> + }
> >>>>> + }
> >>>>> +
> >>>>> + /* clk was prepared in probe, hence unprepare it here */
> >>>>> + clk_unprepare(pc->clk);
> >>>>
> >>>> I believe you need to remove the chip first and then do above to
> >>>> avoid any race conditions, that might occur.
> >>>
> >>> I am afraid, I would loose all chips and their related information
> >>> (PWMF_ENABLED) then.
> >>
> >> I have just checked core's code, and yes you are correct.
> >> Now i have another doubt :)
> >>
> >> Why shouldn't you do this instead:
> >>
> >> for (i = 0; i < NUM_PWM; i++)
> >> pwm_diable(&pc->chip.pwms[i]);
> >>
> >> And, why should we put above code in pwmchip_remove() instead, so that
> >> pwm drivers don't need to do all this?
> >>
> >> @Thierry: Your inputs are required here :)
> >
> > We could probably do that in the core. I've had some discussions about
> > this with Lars-Peter (Cc'ed) who also had doubts about how this is
> > currently handled.
> >
> > The problem is that the core driver code ignores errors from the
> > driver's .remove() callback, so actually returning the error of
> > pwmchip_remove() here isn't terribly useful. I had actually assumed
> > (without checking the code) that the device wouldn't be removed if an
> > error was returned, but that isn't true.
> >
> > IIRC Lars-Peter suggested that we do reference counting on PWM devices
> > so that they could stay around after the module is unloaded but return
> > errors (-ENODEV?) on all operations to make sure users are aware of them
> > disappearing.
> >
> > What you're proposing is different, however. If we put that code in the
> > core it will mean that once the module is unloaded, all PWM devices will
> > be disabled. There is currently code in the core that prevents the chip
> > from being removed if one or more PWM devices are busy. But as explained
> > above, with the current core code this return value isn't useful at all.
> >
> > This needs to be addressed, but I'm not quite sure how yet. Obviously it
> > cannot be solved in the core, because the PWM devices may be provided by
> > real hotpluggable devices, so just preventing the driver from being
> > removed won't help.
>
> In my opinion it would make sense to put this into the PWM core. Even if the
> device is still physically connected, e.g. because it is a on-SoC device, it
> should be stopped if the device is removed. You do not want the PWM device
> to continue to provide it's service (which is the PWM signal) after the
> device has been removed. This means this is something that needs to be
> implemented by every PWM driver.
Alright. Let's keep it in the driver for now and I'll remove it once I
get around to implementing the functionality in the core.
Thierry