Hello,
This patch series is the freescale ftm pwm implementation.
There are 8 channels most supported by the ftm pwm,
and there are two pwm modes:
- Center-Aligned PWM (CPWM) mode
- Edge-Aligned PWM (EPWM) mode
This implementation is only compatible with device tree definition.
Please feel free to comment the dt bindinds.
This patch series is based on linux-next and has been tested on Vybrid VF610TWR board using device tree.
Best Regards,
Xiubo Li
Add freescale ftm pwm driver support. The ftm pwm device
can be found on Vybrid VF610 and Layerscape LS-1 SoCs.
Signed-off-by: Xiubo Li <[email protected]>
Signed-off-by: Jingchang Lu <[email protected]>
---
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-fsl-ftm.c | 586 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 597 insertions(+)
create mode 100644 drivers/pwm/pwm-fsl-ftm.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 75840b5..247b4c3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -62,6 +62,16 @@ config PWM_BFIN
To compile this driver as a module, choose M here: the module
will be called pwm-bfin.
+config PWM_FTM
+ tristate "Freescale FTM PWM support"
+ depends on OF
+ help
+ Generic FTM PWM framework driver for Freescale VF610 and
+ Layerscape LS-1 SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-ftm.
+
config PWM_IMX
tristate "i.MX PWM support"
depends on ARCH_MXC
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 77a8c18..0e7f6ae 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_FTM) += pwm-fsl-ftm.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-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
new file mode 100644
index 0000000..f10ed34
--- /dev/null
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -0,0 +1,586 @@
+/*
+ * Freescale FTM PWM Driver
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * 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.
+ */
+
+#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/io.h>
+#include <linux/pwm.h>
+#include <linux/of_address.h>
+#include <linux/pinctrl/consumer.h>
+
+#define FTM_SC 0x00
+#define FTMSC_CPWMS (0x1 << 5)
+#define FTMSC_CLK_MASK 0x03
+#define FTMSC_CLK_OFFSET 0x03
+#define FTMSC_CLKSYS (0x01 << 3)
+#define FTMSC_CLKFIX (0x02 << 3)
+#define FTMSC_CLKEXT (0x03 << 3)
+#define FTMSC_PS_MASK 0x07
+#define FTMSC_PS_OFFSET 0x00
+
+#define FTM_CNT 0x04
+#define FTM_MOD 0x08
+
+#define FTM_CSC_BASE 0x0C
+#define FTM_CSC(_CHANNEL) \
+ (FTM_CSC_BASE + (_CHANNEL * 0x08))
+#define FTMCnSC_MSB (0x01 << 5)
+#define FTMCnSC_MSA (0x01 << 4)
+#define FTMCnSC_ELSB (0x01 << 3)
+#define FTMCnSC_ELSA (0x01 << 2)
+#define FTM_PWMMODE (FTMCnSC_MSB)
+#define FTM_HIGH_TRUE (FTMCnSC_ELSB)
+#define FTM_LOW_TRUE (FTMCnSC_ELSA)
+
+#define FTM_CV_BASE 0x10
+#define FTM_CV(_CHANNEL) \
+ (FTM_CV_BASE + (_CHANNEL * 0x08))
+
+#define FTM_CNTIN 0x4C
+#define FTM_STATUS 0x50
+
+#define FTM_MODE 0x54
+#define FTMMODE_FTMEN (0x01 << 0)
+#define FTMMODE_INIT (0x01 << 2)
+#define FTMMODE_PWMSYNC (0x01 << 3)
+
+#define FTM_SYNC 0x58
+#define FTM_OUTINIT 0x5C
+#define FTM_OUTMASK 0x60
+#define FTM_COMBINE 0x64
+#define FTM_DEADTIME 0x68
+#define FTM_EXTTRIG 0x6C
+#define FTM_POL 0x70
+#define FTM_FMS 0x74
+#define FTM_FILTER 0x78
+#define FTM_FLTCTRL 0x7C
+#define FTM_QDCTRL 0x80
+#define FTM_CONF 0x84
+#define FTM_FLTPOL 0x88
+#define FTM_SYNCONF 0x8C
+#define FTM_INVCTRL 0x90
+#define FTM_SWOCTRL 0x94
+#define FTM_PWMLOAD 0x98
+
+#define FTM_MAX_CHANNEL 0x08
+#define FTM_CNTIN_VAL 0x00
+
+struct fsl_pwm {
+ unsigned long period_cycles;
+ unsigned long duty_cycles;
+};
+
+struct fsl_pwm_chip {
+ struct mutex lock;
+
+ struct platform_device *pdev;
+ struct pwm_chip chip;
+
+ unsigned int clk_ps;
+ struct clk *clk;
+
+ void __iomem *base;
+
+ unsigned int cpwm;
+ struct fsl_pwm fpwms[FTM_MAX_CHANNEL];
+
+ /* pinctrl handles */
+ struct pinctrl *pinctrl;
+};
+
+static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct fsl_pwm_chip, chip);
+}
+
+static int fsl_pwm_request_channel(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ int ret = 0;
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ ret = clk_prepare_enable(fpc->clk);
+ if (ret) {
+ dev_err(&fpc->pdev->dev,
+ "failed to clk_prepare_enable "
+ "ftm pwm module clock : %d\n",
+ ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void fsl_pwm_free_channel(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ clk_disable_unprepare(fpc->clk);
+}
+
+/* For center-aligned PWM:
+ * period = 2*(MOD - CNTIN)
+ * duty = 2*(CnV - CNTIN)
+ * For edge-aligend PWM:
+ * period = MOD - CNTIN + 1
+ * duty = CnV - CNTIN
+ */
+static int fsl_updata_config(struct fsl_pwm_chip *fpc,
+ struct pwm_device *pwm)
+{
+ int i;
+ unsigned long reg, cntin = FTM_CNTIN_VAL;
+ struct pwm_chip *chip = &fpc->chip;
+
+ reg = readl(fpc->base + FTM_SC);
+ reg &= ~(FTMSC_CPWMS);
+ reg |= (fpc->cpwm ? FTMSC_CPWMS : 0x00);
+ writel(reg, fpc->base + FTM_SC);
+
+ if (pwm && fpc->cpwm) {
+ writel(fpc->fpwms[pwm->hwpwm].period_cycles / 2 + cntin,
+ fpc->base + FTM_MOD);
+ writel(fpc->fpwms[pwm->hwpwm].duty_cycles / 2 + cntin,
+ fpc->base + FTM_CV(pwm->hwpwm));
+ } else if (pwm) {
+ writel(fpc->fpwms[pwm->hwpwm].period_cycles + cntin - 1,
+ fpc->base + FTM_MOD);
+ writel(fpc->fpwms[pwm->hwpwm].duty_cycles + cntin,
+ fpc->base + FTM_CV(pwm->hwpwm));
+ }
+
+ if (pwm)
+ return 0;
+
+ for (i = 0; i < chip->npwm; i++) {
+ if (!test_bit(PWMF_ENABLED, &chip->pwms[i].flags))
+ continue;
+ if (fpc->cpwm) {
+ writel(fpc->fpwms[i].period_cycles / 2 + cntin,
+ fpc->base + FTM_MOD);
+ writel(fpc->fpwms[i].duty_cycles / 2 + cntin,
+ fpc->base + FTM_CV(i));
+ } else {
+ writel(fpc->fpwms[i].period_cycles + cntin - 1,
+ fpc->base + FTM_MOD);
+ writel(fpc->fpwms[i].duty_cycles + cntin,
+ fpc->base + FTM_CV(i));
+ }
+ }
+
+ return 0;
+}
+
+static unsigned long
+fsl_rate_to_cycles(struct fsl_pwm_chip *fpc, int time_ns)
+{
+ unsigned long ps;
+ unsigned long long c;
+
+ ps = (0x01 << fpc->clk_ps);
+
+ /* The module clk is HZ/s, the time_ns parameter
+ * is based nanosecond, so here should divide
+ * 1000000000UL.
+ */
+ c = clk_get_rate(fpc->clk);
+ c = c * time_ns;
+ do_div(c, 1000000000UL);
+ do_div(c, ps);
+
+ return (unsigned long)c;
+}
+
+static int fsl_pwm_config_channel(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ int duty_ns,
+ int period_ns)
+{
+ unsigned long period_cycles, duty_cycles;
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
+ return -ESHUTDOWN;
+
+ period_cycles = fsl_rate_to_cycles(fpc, period_ns);
+ if (period_cycles > 0xFFFF) {
+ dev_warn(&fpc->pdev->dev,
+ "required PWM period cycles(%lu) "
+ "overflow 16-bits counter!\n",
+ period_cycles);
+ period_cycles = 0xFFFF;
+ }
+
+ duty_cycles = fsl_rate_to_cycles(fpc, duty_ns);
+ if (duty_cycles >= 0xFFFF) {
+ dev_warn(&fpc->pdev->dev,
+ "required PWM duty cycles(%lu) "
+ "overflow 16-bits counter!\n",
+ duty_cycles);
+ duty_cycles = 0xFFFF - 1;
+ }
+
+ fpc->fpwms[pwm->hwpwm].period_cycles = period_cycles;
+ fpc->fpwms[pwm->hwpwm].duty_cycles = duty_cycles;
+
+ writel(FTM_PWMMODE | FTM_HIGH_TRUE,
+ fpc->base + FTM_CSC(pwm->hwpwm));
+
+ writel(0xF0, fpc->base + FTM_OUTMASK);
+ writel(0x0F, fpc->base + FTM_OUTINIT);
+ writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN);
+
+ mutex_lock(&fpc->lock);
+ fsl_updata_config(fpc, pwm);
+ mutex_unlock(&fpc->lock);
+
+ return 0;
+}
+
+static int fsl_pwm_set_channel_polarity(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ unsigned long reg;
+ struct fsl_pwm_chip *fpc;
+
+ fpc = to_fsl_chip(chip);
+
+ reg = readl(fpc->base + FTM_POL);
+ reg &= ~(0x01 << pwm->hwpwm);
+ reg |= (polarity << pwm->hwpwm);
+ writel(reg, fpc->base + FTM_POL);
+
+ return 0;
+}
+
+static int is_any_other_channel_enabled(struct pwm_chip *chip,
+ unsigned int cur)
+{
+ int i;
+
+ for (i = 0; i < chip->npwm; i++) {
+ if (i == cur)
+ continue;
+ if (test_bit(PWMF_ENABLED, &chip->pwms[i].flags))
+ return 1;
+ }
+
+ return 0;
+}
+
+static int fsl_pwm_enable_channel(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ int ret;
+ unsigned long reg;
+ struct fsl_pwm_chip *fpc;
+ struct pinctrl_state *pins_state;
+ const char *statename;
+
+ fpc = to_fsl_chip(chip);
+
+ if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
+ return -ESHUTDOWN;
+
+ statename = kasprintf(GFP_KERNEL, "en%d", pwm->hwpwm);
+ pins_state = pinctrl_lookup_state(fpc->pinctrl,
+ statename);
+ /* enable pins to be muxed in and configured */
+ if (!IS_ERR(pins_state)) {
+ ret = pinctrl_select_state(fpc->pinctrl, pins_state);
+ if (ret)
+ dev_warn(&fpc->pdev->dev,
+ "could not set default pins\n");
+ } else
+ dev_warn(&fpc->pdev->dev,
+ "could not get default pinstate\n");
+
+ if (is_any_other_channel_enabled(chip, pwm->hwpwm))
+ return 0;
+
+ reg = readl(fpc->base + FTM_SC);
+ reg &= ~((FTMSC_CLK_MASK << FTMSC_CLK_OFFSET) |
+ (FTMSC_PS_MASK << FTMSC_PS_OFFSET));
+ /* select system clock source */
+ reg |= (FTMSC_CLKSYS | fpc->clk_ps);
+ writel(reg, fpc->base + FTM_SC);
+
+ return 0;
+}
+
+static void fsl_pwm_disable_channel(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ int ret;
+ unsigned long reg;
+ struct fsl_pwm_chip *fpc;
+ struct pinctrl_state *pins_state;
+ const char *statename;
+
+ fpc = to_fsl_chip(chip);
+
+ statename = kasprintf(GFP_KERNEL, "ds%d", pwm->hwpwm);
+ pins_state = pinctrl_lookup_state(fpc->pinctrl,
+ statename);
+ /* enable pins to be muxed in and configured */
+ if (!IS_ERR(pins_state)) {
+ ret = pinctrl_select_state(fpc->pinctrl, pins_state);
+ if (ret)
+ dev_warn(&fpc->pdev->dev,
+ "could not set default pins\n");
+ } else
+ dev_warn(&fpc->pdev->dev,
+ "could not get default pinstate\n");
+
+ if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
+ return;
+
+ if (is_any_other_channel_enabled(chip, pwm->hwpwm))
+ return;
+
+ writel(0xFF, fpc->base + FTM_OUTMASK);
+ reg = readl(fpc->base + FTM_SC);
+ reg &= ~(FTMSC_CLK_MASK << FTMSC_CLK_OFFSET);
+ writel(reg, fpc->base + FTM_SC);
+}
+
+static const struct pwm_ops fsl_pwm_ops = {
+ .request = fsl_pwm_request_channel,
+ .free = fsl_pwm_free_channel,
+ .config = fsl_pwm_config_channel,
+ .set_polarity = fsl_pwm_set_channel_polarity,
+ .enable = fsl_pwm_enable_channel,
+ .disable = fsl_pwm_disable_channel,
+ .owner = THIS_MODULE,
+};
+
+static ssize_t fsl_pwm_cpwm_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct fsl_pwm_chip *fpc;
+
+ fpc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", fpc->cpwm);
+}
+
+static ssize_t fsl_pwm_cpwm_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ int ret;
+ unsigned int val;
+ struct fsl_pwm_chip *fpc;
+
+ fpc = dev_get_drvdata(dev);
+
+ ret = kstrtouint(buf, 0, &val);
+ if (ret)
+ return ret;
+
+ mutex_lock(&fpc->lock);
+ if (!!(val) != !!(fpc->cpwm)) {
+ fpc->cpwm = !!val;
+ fsl_updata_config(fpc, NULL);
+ }
+ mutex_unlock(&fpc->lock);
+
+ return count;
+}
+
+static DEVICE_ATTR(cpwm, S_IRUGO | S_IWUSR | S_IWGRP,
+ fsl_pwm_cpwm_show, fsl_pwm_cpwm_store);
+
+static struct attribute *fsl_pwm_attrs[] = {
+ &dev_attr_cpwm.attr,
+ NULL
+};
+
+static const struct attribute_group fsl_pwm_attr_group = {
+ .attrs = fsl_pwm_attrs,
+};
+
+static int fsl_pwm_parse_dt(struct fsl_pwm_chip *fpc)
+{
+ int ret = 0;
+ u32 chs[FTM_MAX_CHANNEL];
+ struct device_node *np = fpc->pdev->dev.of_node;
+
+ ret = of_property_read_u32(np, "fsl,pwm-clk-ps",
+ &fpc->clk_ps);
+ if (ret < 0) {
+ dev_err(&fpc->pdev->dev,
+ "failed to get pwm "
+ "clk prescaler : %d\n",
+ ret);
+ return ret;
+ }
+ if (fpc->clk_ps > 7 || fpc->clk_ps < 0)
+ return -EINVAL;
+
+ ret = of_property_read_u32(np, "fsl,pwm-cpwm",
+ &fpc->cpwm);
+ if (ret < 0) {
+ dev_err(&fpc->pdev->dev,
+ "failed to get cpwm "
+ "status: %d\n",
+ ret);
+ return ret;
+ }
+
+ ret = of_property_read_u32(np, "fsl,pwm-number",
+ &fpc->chip.npwm);
+ if (ret < 0) {
+ dev_err(&fpc->pdev->dev,
+ "failed to get pwm number: %d\n",
+ ret);
+ return ret;
+ }
+ if (fpc->chip.npwm > FTM_MAX_CHANNEL
+ || fpc->chip.npwm <= 0)
+ return -EINVAL;
+
+ ret = of_property_read_u32_array(np, "fsl,pwm-channels",
+ chs, fpc->chip.npwm);
+ if (ret < 0) {
+ dev_err(&fpc->pdev->dev,
+ "failed to get pwm channel Ids: %d\n",
+ ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int fsl_pwm_probe(struct platform_device *pdev)
+{
+ int ret = 0;
+ struct fsl_pwm_chip *fpc;
+ struct resource *res;
+
+ fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
+ if (!fpc) {
+ dev_err(&pdev->dev,
+ "failed to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ mutex_init(&fpc->lock);
+
+ fpc->pdev = pdev;
+
+ ret = fsl_pwm_parse_dt(fpc);
+ if (ret < 0)
+ return ret;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ fpc->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(fpc->base)) {
+ dev_err(&pdev->dev,
+ "failed to ioremap() registers\n");
+ ret = PTR_ERR(fpc->base);
+ return ret;
+ }
+
+ fpc->chip.dev = &pdev->dev;
+ fpc->chip.ops = &fsl_pwm_ops;
+ fpc->chip.of_xlate = of_pwm_xlate_with_flags;
+ fpc->chip.of_pwm_n_cells = 3;
+ fpc->chip.base = -1;
+
+ ret = pwmchip_add(&fpc->chip);
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "failed to add ftm0 pwm chip %d\n",
+ ret);
+ return ret;
+ }
+
+ fpc->clk = devm_clk_get(&pdev->dev, "ftm0");
+ if (IS_ERR(fpc->clk)) {
+ ret = PTR_ERR(fpc->clk);
+ dev_err(&pdev->dev,
+ "failed to get ftm0's module clock %d\n",
+ ret);
+ return ret;
+ }
+
+ fpc->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (IS_ERR(fpc->pinctrl)) {
+ ret = PTR_ERR(fpc->pinctrl);
+ return ret;
+ }
+
+ ret = sysfs_create_group(&pdev->dev.kobj,
+ &fsl_pwm_attr_group);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to create sysfs "
+ "attributes, err: %d\n",
+ ret);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, fpc);
+
+ return 0;
+}
+
+static int fsl_pwm_remove(struct platform_device *pdev)
+{
+ struct fsl_pwm_chip *fpc;
+
+ fpc = platform_get_drvdata(pdev);
+ if (fpc == NULL)
+ return -ENODEV;
+
+ mutex_destroy(&fpc->lock);
+
+ sysfs_remove_group(&pdev->dev.kobj,
+ &fsl_pwm_attr_group);
+
+ return pwmchip_remove(&fpc->chip);
+}
+
+static const struct of_device_id fsl_pwm_dt_ids[] = {
+ { .compatible = "fsl,vf610-ftm-pwm", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, fsl_pwm_dt_ids);
+
+static struct platform_driver fsl_pwm_driver = {
+ .driver = {
+ .name = "fsl-ftm-pwm",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(fsl_pwm_dt_ids),
+ },
+ .probe = fsl_pwm_probe,
+ .remove = fsl_pwm_remove,
+};
+module_platform_driver(fsl_pwm_driver);
+
+MODULE_DESCRIPTION("Freescale FTM PWM Driver");
+MODULE_AUTHOR("Xiubo Li <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.8.0
Signed-off-by: Xiubo Li <[email protected]>
---
arch/arm/boot/dts/vf610.dtsi | 83 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 82 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 67d929c..b3a0b6e 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -140,6 +140,17 @@
clock-names = "pit";
};
+ pwm0: pwm@40038000 {
+ compatible = "fsl,vf610-ftm-pwm";
+ #pwm-cells = <3>;
+ reg = <0x40038000 0x1000>;
+ clocks = <&clks VF610_CLK_FTM0_EXT_FIX_EN>,
+ <&clks VF610_CLK_FTM0_EXT_SEL>,
+ <&clks VF610_CLK_FTM0>;
+ clock-names = "ftm0_ext_fix_en", "ftm0_ext_sel", "ftm0";
+ status = "disabled";
+ };
+
wdog@4003e000 {
compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
reg = <0x4003e000 0x1000>;
@@ -270,16 +281,86 @@
};
pwm0 {
- pinctrl_pwm0_1: pwm0grp_1 {
+ pinctrl_pwm0_ch0_en: pwm0grp_ch0_en {
fsl,pins = <
VF610_PAD_PTB0__FTM0_CH0 0x1582
+ >;
+ };
+ pinctrl_pwm0_ch0_ds: pwm0grp_ch0_ds {
+ fsl,pins = <
+ VF610_PAD_PTB0__FTM0_CH0 0x0000
+ >;
+ };
+ pinctrl_pwm0_ch1_en: pwm0grp_ch1_en {
+ fsl,pins = <
VF610_PAD_PTB1__FTM0_CH1 0x1582
+ >;
+ };
+ pinctrl_pwm0_ch1_ds: pwm0grp_ch1_ds {
+ fsl,pins = <
+ VF610_PAD_PTB1__FTM0_CH1 0x0000
+ >;
+ };
+ pinctrl_pwm0_ch2_en: pwm0grp_ch2_en {
+ fsl,pins = <
VF610_PAD_PTB2__FTM0_CH2 0x1582
+ >;
+ };
+ pinctrl_pwm0_ch2_ds: pwm0grp_ch2_ds {
+ fsl,pins = <
+ VF610_PAD_PTB2__FTM0_CH2 0x0000
+ >;
+ };
+ pinctrl_pwm0_ch3_en: pwm0grp_ch3_en {
+ fsl,pins = <
VF610_PAD_PTB3__FTM0_CH3 0x1582
+ >;
+ };
+ pinctrl_pwm0_ch3_ds: pwm0grp_ch3_ds {
+ fsl,pins = <
+ VF610_PAD_PTB3__FTM0_CH3 0x0000
+ >;
+ };
+ pinctrl_pwm0_ch4_en: pwm0grp_ch4_en {
+ fsl,pins = <
+ VF610_PAD_PTB4__FTM0_CH4 0x1582
+ >;
+ };
+ pinctrl_pwm0_ch4_ds: pwm0grp_ch4_ds {
+ fsl,pins = <
+ VF610_PAD_PTB4__FTM0_CH4 0x0000
+ >;
+ };
+ pinctrl_pwm0_ch5_en: pwm0grp_ch5_en {
+ fsl,pins = <
+ VF610_PAD_PTB5__FTM0_CH5 0x1582
+ >;
+ };
+ pinctrl_pwm0_ch5_ds: pwm0grp_ch5_ds {
+ fsl,pins = <
+ VF610_PAD_PTB5__FTM0_CH5 0x0000
+ >;
+ };
+ pinctrl_pwm0_ch6_en: pwm0grp_ch6_en {
+ fsl,pins = <
VF610_PAD_PTB6__FTM0_CH6 0x1582
+ >;
+ };
+ pinctrl_pwm0_ch6_ds: pwm0grp_ch6_ds {
+ fsl,pins = <
+ VF610_PAD_PTB6__FTM0_CH6 0x0000
+ >;
+ };
+ pinctrl_pwm0_ch7_en: pwm0grp_ch7_en {
+ fsl,pins = <
VF610_PAD_PTB7__FTM0_CH7 0x1582
>;
};
+ pinctrl_pwm0_ch7_ds: pwm0grp_ch7_ds {
+ fsl,pins = <
+ VF610_PAD_PTB7__FTM0_CH7 0x0000
+ >;
+ };
};
qspi0 {
--
1.8.0
Signed-off-by: Xiubo Li <[email protected]>
---
arch/arm/boot/dts/vf610-twr.dts | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index b3905f5..6f93d8d 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -50,6 +50,23 @@
status = "okay";
};
+&pwm0 {
+ pinctrl-names = "en0", "ds0", "en1", "ds1", "en2", "ds2", "en3", "ds3";
+ pinctrl-0 = <&pinctrl_pwm0_ch0_en>;
+ pinctrl-1 = <&pinctrl_pwm0_ch0_ds>;
+ pinctrl-2 = <&pinctrl_pwm0_ch1_en>;
+ pinctrl-3 = <&pinctrl_pwm0_ch1_ds>;
+ pinctrl-4 = <&pinctrl_pwm0_ch2_en>;
+ pinctrl-5 = <&pinctrl_pwm0_ch2_ds>;
+ pinctrl-6 = <&pinctrl_pwm0_ch3_en>;
+ pinctrl-7 = <&pinctrl_pwm0_ch3_ds>;
+ fsl,pwm-clk-ps = <7>;
+ fsl,pwm-cpwm = <0>;
+ fsl,pwm-number = <4>;
+ fsl,pwm-channels = <0 1 2 3>;
+ status = "okay";
+};
+
&uart1 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_uart1_1>;
--
1.8.0
Signed-off-by: Xiubo Li <[email protected]>
---
.../devicetree/bindings/pwm/fsl-ftm-pwm.txt | 52 ++++++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
new file mode 100644
index 0000000..698965b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
@@ -0,0 +1,52 @@
+Freescale FTM PWM controller
+
+Required properties:
+- compatible: should be "fsl,vf610-ftm-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
+ First cell specifies the per-chip channel index of the PWM to use, the
+ second cell is the period in nanoseconds and bit 0 in the third cell is
+ used to encode the polarity of PWM output. Set bit 0 of the third in PWM
+ specifier to 1 for inverse polarity & set to 0 for normal polarity.
+- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0 ~ 7).
+- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
+- fsl,pwm-number: the number of PWM devices, and is must equal to the number
+ of "fsl,pwm-channels".
+- fsl,pwm-channels: the channels' order which is be used for pwm in ftm0
+ module, and they must be one or some of 0 ~ 7, because the ftm0 only has
+ 8 channels can be used.
+- for very channel, the revlatived the pinctrl should be at least two state
+ {"enN", "dsN"}, which "en" means "enable", "ds" means "disable" and "N"
+ means the order of the channel.
+
+Example:
+
+pwm0: pwm@40038000 {
+ compatible = "fsl,vf610-ftm-pwm";
+ reg = <0x40038000 0x1000>;
+ #pwm-cells = <3>;
+ pinctrl-names = "en0", "ds0", "en3", "ds3";
+ pinctrl-0 = <&pinctrl_pwm0_ch0_en>;
+ pinctrl-1 = <&pinctrl_pwm0_ch0_ds>;
+ pinctrl-2 = <&pinctrl_pwm0_ch3_en>;
+ pinctrl-3 = <&pinctrl_pwm0_ch3_ds>;
+ fsl,pwm-clk-ps = <7>;
+ fsl,pwm-cpwm = <0>;
+ fsl,pwm-number = <2>;
+ fsl,pwm-channels = <0 3>;
+ ...
+ };
+
+leds {
+ compatible = "pwm-leds";
+ led {
+ label = "fsl_led";
+ pwms = <&pwm0 0 10000000 0>;
+ max-brightness = <127>;
+ };
+ backlight {
+ label = "fsl_backlight";
+ pwms = <&pwm0 3 10000000 1>;
+ max-brightness = <100>;
+ };
+};
--
1.8.0
On Wed, Aug 21, 2013 at 11:07:39AM +0800, Xiubo Li wrote:
> +
> +#define FTM_CSC_BASE 0x0C
> +#define FTM_CSC(_CHANNEL) \
> + (FTM_CSC_BASE + (_CHANNEL * 0x08))
I see this more and more in FSL code. This is unsafe! Consider what
happens when we call FTM_CSC(1 + 1). The result is certainly not what
you want.
> +
> +static int fsl_pwm_request_channel(struct pwm_chip *chip,
> + struct pwm_device *pwm)
> +{
> + int ret = 0;
No need to initialize this variable.
> + struct fsl_pwm_chip *fpc;
> +
> + fpc = to_fsl_chip(chip);
> +
> + ret = clk_prepare_enable(fpc->clk);
> + if (ret) {
> + dev_err(&fpc->pdev->dev,
> + "failed to clk_prepare_enable "
> + "ftm pwm module clock : %d\n",
> + ret);
Just return ret. We do not need a message for each failed function in
the kernel.
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int fsl_updata_config(struct fsl_pwm_chip *fpc,
> + struct pwm_device *pwm)
> +{
> + int i;
> + unsigned long reg, cntin = FTM_CNTIN_VAL;
> + struct pwm_chip *chip = &fpc->chip;
> +
> + reg = readl(fpc->base + FTM_SC);
> + reg &= ~(FTMSC_CPWMS);
> + reg |= (fpc->cpwm ? FTMSC_CPWMS : 0x00);
> + writel(reg, fpc->base + FTM_SC);
> +
> + if (pwm && fpc->cpwm) {
> + writel(fpc->fpwms[pwm->hwpwm].period_cycles / 2 + cntin,
> + fpc->base + FTM_MOD);
> + writel(fpc->fpwms[pwm->hwpwm].duty_cycles / 2 + cntin,
> + fpc->base + FTM_CV(pwm->hwpwm));
> + } else if (pwm) {
> + writel(fpc->fpwms[pwm->hwpwm].period_cycles + cntin - 1,
> + fpc->base + FTM_MOD);
> + writel(fpc->fpwms[pwm->hwpwm].duty_cycles + cntin,
> + fpc->base + FTM_CV(pwm->hwpwm));
> + }
> +
> + if (pwm)
> + return 0;
> +
> + for (i = 0; i < chip->npwm; i++) {
> + if (!test_bit(PWMF_ENABLED, &chip->pwms[i].flags))
> + continue;
> + if (fpc->cpwm) {
> + writel(fpc->fpwms[i].period_cycles / 2 + cntin,
> + fpc->base + FTM_MOD);
> + writel(fpc->fpwms[i].duty_cycles / 2 + cntin,
> + fpc->base + FTM_CV(i));
> + } else {
> + writel(fpc->fpwms[i].period_cycles + cntin - 1,
> + fpc->base + FTM_MOD);
> + writel(fpc->fpwms[i].duty_cycles + cntin,
> + fpc->base + FTM_CV(i));
> + }
> + }
> +
The behaviour of this function is completely different for pwm == NULL
and pwm != NULL. This indicates that it should really be two functions.
This probably makes the intention of this code much clearer.
> +static int fsl_pwm_config_channel(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + int duty_ns,
> + int period_ns)
> +{
> + unsigned long period_cycles, duty_cycles;
> + struct fsl_pwm_chip *fpc;
> +
> + fpc = to_fsl_chip(chip);
> +
> + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> + return -ESHUTDOWN;
> +
> + period_cycles = fsl_rate_to_cycles(fpc, period_ns);
> + if (period_cycles > 0xFFFF) {
> + dev_warn(&fpc->pdev->dev,
> + "required PWM period cycles(%lu) "
> + "overflow 16-bits counter!\n",
> + period_cycles);
> + period_cycles = 0xFFFF;
If you can't fulfill the requirements you have to return an error. It's
the caller that needs to know the failure. The caller can't read read
the syslog.
> +static int fsl_pwm_enable_channel(struct pwm_chip *chip,
> + struct pwm_device *pwm)
> +{
> + int ret;
> + unsigned long reg;
> + struct fsl_pwm_chip *fpc;
> + struct pinctrl_state *pins_state;
> + const char *statename;
> +
> + fpc = to_fsl_chip(chip);
> +
> + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> + return -ESHUTDOWN;
> +
> + statename = kasprintf(GFP_KERNEL, "en%d", pwm->hwpwm);
> + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> + statename);
> + /* enable pins to be muxed in and configured */
> + if (!IS_ERR(pins_state)) {
> + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> + if (ret)
> + dev_warn(&fpc->pdev->dev,
> + "could not set default pins\n");
Why do you need to manipulate the pinctrl to en/disable a channel?
> +static ssize_t fsl_pwm_cpwm_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + int ret;
> + unsigned int val;
> + struct fsl_pwm_chip *fpc;
> +
> + fpc = dev_get_drvdata(dev);
> +
> + ret = kstrtouint(buf, 0, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&fpc->lock);
> + if (!!(val) != !!(fpc->cpwm)) {
> + fpc->cpwm = !!val;
> + fsl_updata_config(fpc, NULL);
> + }
> + mutex_unlock(&fpc->lock);
> +
> + return count;
> +}
What is this cpwm thingy?
> +
> +static DEVICE_ATTR(cpwm, S_IRUGO | S_IWUSR | S_IWGRP,
> + fsl_pwm_cpwm_show, fsl_pwm_cpwm_store);
> +
> +static struct attribute *fsl_pwm_attrs[] = {
> + &dev_attr_cpwm.attr,
> + NULL
> +};
> +
> +static int fsl_pwm_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + struct fsl_pwm_chip *fpc;
> + struct resource *res;
> +
> + fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
> + if (!fpc) {
> + dev_err(&pdev->dev,
> + "failed to allocate memory\n");
Drop this message. You'll never see it.
> + return -ENOMEM;
> + }
> +
> + mutex_init(&fpc->lock);
> +
> + fpc->pdev = pdev;
> +
> + ret = fsl_pwm_parse_dt(fpc);
> + if (ret < 0)
> + return ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + fpc->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(fpc->base)) {
> + dev_err(&pdev->dev,
> + "failed to ioremap() registers\n");
> + ret = PTR_ERR(fpc->base);
> + return ret;
> + }
> +
> + fpc->chip.dev = &pdev->dev;
> + fpc->chip.ops = &fsl_pwm_ops;
> + fpc->chip.of_xlate = of_pwm_xlate_with_flags;
> + fpc->chip.of_pwm_n_cells = 3;
> + fpc->chip.base = -1;
> +
> + ret = pwmchip_add(&fpc->chip);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "failed to add ftm0 pwm chip %d\n",
> + ret);
> + return ret;
> + }
You can only add the PWM when you grabbed all resources, not before
this.
> +
> + fpc->clk = devm_clk_get(&pdev->dev, "ftm0");
> + if (IS_ERR(fpc->clk)) {
> + ret = PTR_ERR(fpc->clk);
> + dev_err(&pdev->dev,
> + "failed to get ftm0's module clock %d\n",
> + ret);
> + return ret;
> + }
> +
> + fpc->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(fpc->pinctrl)) {
> + ret = PTR_ERR(fpc->pinctrl);
> + return ret;
> + }
> +
> + ret = sysfs_create_group(&pdev->dev.kobj,
> + &fsl_pwm_attr_group);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to create sysfs "
> + "attributes, err: %d\n",
> + ret);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, fpc);
> +
> + return 0;
> +}
> +
> +static int fsl_pwm_remove(struct platform_device *pdev)
> +{
> + struct fsl_pwm_chip *fpc;
> +
> + fpc = platform_get_drvdata(pdev);
> + if (fpc == NULL)
> + return -ENODEV;
This won't happen.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
TO Sascha,
Thanks very much for your comments.
> Subject: Re: [PATCH 1/4] pwm: add freescale ftm pwm driver support
>
> On Wed, Aug 21, 2013 at 11:07:39AM +0800, Xiubo Li wrote:
> > +
> > +#define FTM_CSC_BASE 0x0C
> > +#define FTM_CSC(_CHANNEL) \
> > + (FTM_CSC_BASE + (_CHANNEL * 0x08))
>
> I see this more and more in FSL code. This is unsafe! Consider what
> happens when we call FTM_CSC(1 + 1). The result is certainly not what you
> want.
>
Oh yes, I'll fix it in v2.
> > +
> > +static int fsl_pwm_request_channel(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
> > +{
> > + int ret = 0;
>
> No need to initialize this variable.
>
Yeah, I'll fix it in v2.
> > + ret = clk_prepare_enable(fpc->clk);
> > + if (ret) {
> > + dev_err(&fpc->pdev->dev,
> > + "failed to clk_prepare_enable "
> > + "ftm pwm module clock : %d\n",
> > + ret);
>
> Just return ret. We do not need a message for each failed function in the
> kernel.
>
OK, I will remove it in v2.
> > +static int fsl_updata_config(struct fsl_pwm_chip *fpc,
> > + struct pwm_device *pwm)
> > +{
> > + int i;
> > + unsigned long reg, cntin = FTM_CNTIN_VAL;
> > + struct pwm_chip *chip = &fpc->chip;
> > +
> > + reg = readl(fpc->base + FTM_SC);
> > + reg &= ~(FTMSC_CPWMS);
> > + reg |= (fpc->cpwm ? FTMSC_CPWMS : 0x00);
> > + writel(reg, fpc->base + FTM_SC);
> > +
> > + if (pwm && fpc->cpwm) {
> > + writel(fpc->fpwms[pwm->hwpwm].period_cycles / 2 + cntin,
> > + fpc->base + FTM_MOD);
> > + writel(fpc->fpwms[pwm->hwpwm].duty_cycles / 2 + cntin,
> > + fpc->base + FTM_CV(pwm->hwpwm));
> > + } else if (pwm) {
> > + writel(fpc->fpwms[pwm->hwpwm].period_cycles + cntin - 1,
> > + fpc->base + FTM_MOD);
> > + writel(fpc->fpwms[pwm->hwpwm].duty_cycles + cntin,
> > + fpc->base + FTM_CV(pwm->hwpwm));
> > + }
> > +
> > + if (pwm)
> > + return 0;
> > +
> > + for (i = 0; i < chip->npwm; i++) {
> > + if (!test_bit(PWMF_ENABLED, &chip->pwms[i].flags))
> > + continue;
> > + if (fpc->cpwm) {
> > + writel(fpc->fpwms[i].period_cycles / 2 + cntin,
> > + fpc->base + FTM_MOD);
> > + writel(fpc->fpwms[i].duty_cycles / 2 + cntin,
> > + fpc->base + FTM_CV(i));
> > + } else {
> > + writel(fpc->fpwms[i].period_cycles + cntin - 1,
> > + fpc->base + FTM_MOD);
> > + writel(fpc->fpwms[i].duty_cycles + cntin,
> > + fpc->base + FTM_CV(i));
> > + }
> > + }
> > +
>
> The behaviour of this function is completely different for pwm == NULL
> and pwm != NULL. This indicates that it should really be two functions.
> This probably makes the intention of this code much clearer.
>
OK, I will split it into two functions in v2.
> > + period_cycles = fsl_rate_to_cycles(fpc, period_ns);
> > + if (period_cycles > 0xFFFF) {
> > + dev_warn(&fpc->pdev->dev,
> > + "required PWM period cycles(%lu) "
> > + "overflow 16-bits counter!\n",
> > + period_cycles);
> > + period_cycles = 0xFFFF;
>
> If you can't fulfill the requirements you have to return an error. It's
> the caller that needs to know the failure. The caller can't read read the
> syslog.
>
OK, I will fix it in v2.
> > +static int fsl_pwm_enable_channel(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
> > +{
> > + int ret;
> > + unsigned long reg;
> > + struct fsl_pwm_chip *fpc;
> > + struct pinctrl_state *pins_state;
> > + const char *statename;
> > +
> > + fpc = to_fsl_chip(chip);
> > +
> > + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> > + return -ESHUTDOWN;
> > +
> > + statename = kasprintf(GFP_KERNEL, "en%d", pwm->hwpwm);
> > + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> > + statename);
> > + /* enable pins to be muxed in and configured */
> > + if (!IS_ERR(pins_state)) {
> > + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> > + if (ret)
> > + dev_warn(&fpc->pdev->dev,
> > + "could not set default pins\n");
>
> Why do you need to manipulate the pinctrl to en/disable a channel?
>
This is because in Vybrid VF610 TOWER board, there are 4 leds, and each led's one point(diode's positive pole) is connected to 3.3V,
and the other point is connected to pwm's one channel. When the 4 pinctrls are configured as enable at the same time,
the 4 pinctrls is low valtage, and the 4 leds will be lighted up as default, then when you enable/disable one led will effects others.
These pinctrls are belong to pwm, and I don't think led or other customer could control them directly.
So, here I authorize the 4 pinctrls to each channel controls.
> > +static ssize_t fsl_pwm_cpwm_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf,
> > + size_t count)
> > +{
> > + int ret;
> > + unsigned int val;
> > + struct fsl_pwm_chip *fpc;
> > +
> > + fpc = dev_get_drvdata(dev);
> > +
> > + ret = kstrtouint(buf, 0, &val);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&fpc->lock);
> > + if (!!(val) != !!(fpc->cpwm)) {
> > + fpc->cpwm = !!val;
> > + fsl_updata_config(fpc, NULL);
> > + }
> > + mutex_unlock(&fpc->lock);
> > +
> > + return count;
> > +}
>
> What is this cpwm thingy?
Up-down counting mode:
CNTIN(a register) defines the starting value of the count and MOD(a register) defines the final value of the
count. The value of CNTIN is loaded into the FTM counter, and the counter increments
until the value of MOD is reached, at which point the counter is decremented until it
returns to the value of CNTIN and the up-down counting restarts.
> > +static int fsl_pwm_probe(struct platform_device *pdev) {
> > + int ret = 0;
> > + struct fsl_pwm_chip *fpc;
> > + struct resource *res;
> > +
> > + fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
> > + if (!fpc) {
> > + dev_err(&pdev->dev,
> > + "failed to allocate memory\n");
>
> Drop this message. You'll never see it.
OK, I will drop it in v2.
> > + fpc->chip.dev = &pdev->dev;
> > + fpc->chip.ops = &fsl_pwm_ops;
> > + fpc->chip.of_xlate = of_pwm_xlate_with_flags;
> > + fpc->chip.of_pwm_n_cells = 3;
> > + fpc->chip.base = -1;
> > +
> > + ret = pwmchip_add(&fpc->chip);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev,
> > + "failed to add ftm0 pwm chip %d\n",
> > + ret);
> > + return ret;
> > + }
>
> You can only add the PWM when you grabbed all resources, not before this.
>
Yeah, I will adjust it in v2.
> > +
> > +static int fsl_pwm_remove(struct platform_device *pdev) {
> > + struct fsl_pwm_chip *fpc;
> > +
> > + fpc = platform_get_drvdata(pdev);
> > + if (fpc == NULL)
> > + return -ENODEV;
>
> This won't happen.
OK, I will drop it in v2.
Thanks very much.
---
Best Regards,
Xiubo Li
On Wed, Aug 21, 2013 at 09:24:56AM +0000, Xiubo Li-B47053 wrote:
> TO Sascha,
>
> > > +
> > > + fpc = to_fsl_chip(chip);
> > > +
> > > + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> > > + return -ESHUTDOWN;
> > > +
> > > + statename = kasprintf(GFP_KERNEL, "en%d", pwm->hwpwm);
> > > + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> > > + statename);
> > > + /* enable pins to be muxed in and configured */
> > > + if (!IS_ERR(pins_state)) {
> > > + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> > > + if (ret)
> > > + dev_warn(&fpc->pdev->dev,
> > > + "could not set default pins\n");
> >
> > Why do you need to manipulate the pinctrl to en/disable a channel?
> >
>
> This is because in Vybrid VF610 TOWER board, there are 4 leds, and each led's one point(diode's positive pole) is connected to 3.3V,
> and the other point is connected to pwm's one channel. When the 4 pinctrls are configured as enable at the same time,
> the 4 pinctrls is low valtage, and the 4 leds will be lighted up as default, then when you enable/disable one led will effects others.
>
I think the inactive state of a PWM is pretty much undefined by the PWM
framework and left to the drivers.
I stumbled upon this aswell. It would be good to think about the
inactive state and how the PWM framework could help us here getting
things right.
There are several things to consider. For a noninverted PWM the inactive
state should probably logic 0. For an inverted PWM it should probably be
logic 1. I guess several PWM devices have an undefined inactive state,
most of the PWM devices probably can control the inactive state by
setting the duty cycle to 100% / 0% without actually disabling the PWM.
Using the pinctrl is one way to control the inactive state and probaby
the only one before initialization. It might be good to wire this up in
the core instead of the individual PWM drivers.
These are just the thoughts which first came to my mind.
Thierry, any more input about this?
> > > + fpc = dev_get_drvdata(dev);
> > > +
> > > + ret = kstrtouint(buf, 0, &val);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mutex_lock(&fpc->lock);
> > > + if (!!(val) != !!(fpc->cpwm)) {
> > > + fpc->cpwm = !!val;
> > > + fsl_updata_config(fpc, NULL);
> > > + }
> > > + mutex_unlock(&fpc->lock);
> > > +
> > > + return count;
> > > +}
> >
> > What is this cpwm thingy?
>
> Up-down counting mode:
> CNTIN(a register) defines the starting value of the count and MOD(a register) defines the final value of the
> count. The value of CNTIN is loaded into the FTM counter, and the counter increments
> until the value of MOD is reached, at which point the counter is decremented until it
> returns to the value of CNTIN and the up-down counting restarts.
The current PWM framework only cares about period times and duty cycles.
Why would I want to care about this?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
TO Sascha,
Thanks very much for your quick reply.
> > > > + fpc = to_fsl_chip(chip);
> > > > +
> > > > + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> > > > + return -ESHUTDOWN;
> > > > +
> > > > + statename = kasprintf(GFP_KERNEL, "en%d", pwm->hwpwm);
> > > > + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> > > > + statename);
> > > > + /* enable pins to be muxed in and configured */
> > > > + if (!IS_ERR(pins_state)) {
> > > > + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> > > > + if (ret)
> > > > + dev_warn(&fpc->pdev->dev,
> > > > + "could not set default pins\n");
> > >
> > > Why do you need to manipulate the pinctrl to en/disable a channel?
> > >
> >
> > This is because in Vybrid VF610 TOWER board, there are 4 leds, and
> > each led's one point(diode's positive pole) is connected to 3.3V, and
> > the other point is connected to pwm's one channel. When the 4 pinctrls
> are configured as enable at the same time, the 4 pinctrls is low valtage,
> and the 4 leds will be lighted up as default, then when you
> enable/disable one led will effects others.
> >
>
> I think the inactive state of a PWM is pretty much undefined by the PWM
> framework and left to the drivers.
>
> I stumbled upon this aswell. It would be good to think about the inactive
> state and how the PWM framework could help us here getting things right.
>
> There are several things to consider. For a noninverted PWM the inactive
> state should probably logic 0. For an inverted PWM it should probably be
> logic 1. I guess several PWM devices have an undefined inactive state,
> most of the PWM devices probably can control the inactive state by
> setting the duty cycle to 100% / 0% without actually disabling the PWM.
>
> Using the pinctrl is one way to control the inactive state and probaby
> the only one before initialization. It might be good to wire this up in
> the core instead of the individual PWM drivers.
>
> These are just the thoughts which first came to my mind.
>
That's a very good idea, and I have also thought about it before.
But from the power dissipation:
If so, upon my board is up, the pwm core should be alive even I won't use it.
I think the pwm core should be in idle mode when not using it,
when any of it's channels is requested and enabled, the pwm core will keep alive.
What do you think about it ?
> Thierry, any more input about this?
>
>
> > > > + fpc = dev_get_drvdata(dev);
> > > > +
> > > > + ret = kstrtouint(buf, 0, &val);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + mutex_lock(&fpc->lock);
> > > > + if (!!(val) != !!(fpc->cpwm)) {
> > > > + fpc->cpwm = !!val;
> > > > + fsl_updata_config(fpc, NULL);
> > > > + }
> > > > + mutex_unlock(&fpc->lock);
> > > > +
> > > > + return count;
> > > > +}
> > >
> > > What is this cpwm thingy?
> >
> > Up-down counting mode:
> > CNTIN(a register) defines the starting value of the count and MOD(a
> > register) defines the final value of the count. The value of CNTIN is
> > loaded into the FTM counter, and the counter increments until the
> > value of MOD is reached, at which point the counter is decremented
> until it returns to the value of CNTIN and the up-down counting restarts.
>
> The current PWM framework only cares about period times and duty cycles.
> Why would I want to care about this?
I will think it over, If this hasn't any help here, I'll drop it in v2.
Thanks very much.
--
Best Regards,
Xiubo Li
Hi Xiubo,
Please see my comments inline.
On Wednesday 21 of August 2013 11:07:42 Xiubo Li wrote:
> Signed-off-by: Xiubo Li <[email protected]>
> ---
> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt | 52
> ++++++++++++++++++++++ 1 file changed, 52 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt new file mode
> 100644
> index 0000000..698965b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> @@ -0,0 +1,52 @@
> +Freescale FTM PWM controller
> +
> +Required properties:
> +- compatible: should be "fsl,vf610-ftm-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: Should be 3. Number of cells being used to specify PWM
> property.
> + First cell specifies the per-chip channel index of the PWM
> to use, the
> + second cell is the period in nanoseconds and bit 0 in
> the third cell is
> + used to encode the polarity of PWM output. Set bit
> 0 of the third in PWM
> + specifier to 1 for inverse polarity & set to 0
> for normal polarity.
If the meaning of flags cell is the same as in generic, default PWM
specifier format, then it should be noted here and generic PWM binding
documentation mentioned.
> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> divide-by 2^n(n = 0 ~ 7).
Is it a hardware-specific property?
> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM)
> mode.
Could you explain meaning of this property?
> +- fsl,pwm-number: the number of PWM devices, and is must equal to the
> number + of "fsl,pwm-channels".
This is redundant, because you can simply count how many entries have been
specified in fsl,pwm-channels.
> +- fsl,pwm-channels: the channels' order which is be used for pwm in
> ftm0 + module, and they must be one or some of 0 ~ 7, because the ftm0
> only has + 8 channels can be used.
Could you explain meaning of this property more precisely? I'm interested
especially how is this related to the PWM IP block and boards.
> +- for very channel, the revlatived the pinctrl should be at least two
^ typo?
> state + {"enN", "dsN"}, which "en" means "enable", "ds" means
> "disable" and "N" + means the order of the channel.
I'd suggest a more readable naming convention, for example chN-active and
chN-idle. These words seem to be more common across existing bindings.
Best regards,
Tomasz
Hi Tomasz,
Thanks for your comments.
> > +- #pwm-cells: Should be 3. Number of cells being used to specify PWM
> > property.
> > + First cell specifies the per-chip channel index of the PWM
> > to use, the
> > + second cell is the period in nanoseconds and bit 0 in
> > the third cell is
> > + used to encode the polarity of PWM output. Set bit
> > 0 of the third in PWM
> > + specifier to 1 for inverse polarity & set to 0
> > for normal polarity.
>
> If the meaning of flags cell is the same as in generic, default PWM
> specifier format, then it should be noted here and generic PWM binding
> documentation mentioned.
>
OK, How about the following ?
- #pwm-cells: Should be 3. See pwm.txt in this directory for a
description of the cells format.
I will replace it in v2.
> > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> > divide-by 2^n(n = 0 ~ 7).
>
> Is it a hardware-specific property?
Yes, I will revise it in v2.
>
> > +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM)
> > mode.
>
> Could you explain meaning of this property?
>
Well, this feature will be removed from the pwm core in v2.
> > +- fsl,pwm-number: the number of PWM devices, and is must equal to the
> > number + of "fsl,pwm-channels".
>
> This is redundant, because you can simply count how many entries have
> been specified in fsl,pwm-channels.
>
Yes, I will revise it in v2.
And this would be renamed to " fsl,pwm-channel-number", which can be more
Readable and understood.
> > +- fsl,pwm-channels: the channels' order which is be used for pwm in
> > ftm0 + module, and they must be one or some of 0 ~ 7, because the
> > ftm0 only has + 8 channels can be used.
>
> Could you explain meaning of this property more precisely? I'm interested
> especially how is this related to the PWM IP block and boards.
>
Yes.
There are 8 channels most. While the pinctrls of 4th and 5th channels could be
used by uart's Rx and Tx, then these 2 channels won't be used for pwm output,
so there will be 6 channels available by the pwm.
Thus, the pwm chip will register only 6 pwms(6 channels) most("fsl,pwm-channel-orders
= {0 1 2 3 6 7}").And also the "fsl,pwm-channel-number" will be 6.
If hasn't any other problems, I will revise It in v2.
And this will be renamed to "fsl,pwm-channel-orders", which can be more readable
and understood.
> > +- for very channel, the revlatived the pinctrl should be at least two
> ^ typo?
>
> > state + {"enN", "dsN"}, which "en" means "enable", "ds" means
> > "disable" and "N" + means the order of the channel.
>
> I'd suggest a more readable naming convention, for example chN-active and
> chN-idle. These words seem to be more common across existing bindings.
>
That's a good idea, I will think it over and revise it in v2.
Thanks very much.
--
Best Regards,
Xiubo
On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
> Hi Tomasz,
>
> Thanks for your comments.
>
>
> > Could you explain meaning of this property more precisely? I'm interested
> > especially how is this related to the PWM IP block and boards.
> >
>
> Yes.
> There are 8 channels most. While the pinctrls of 4th and 5th channels could be
> used by uart's Rx and Tx, then these 2 channels won't be used for pwm output,
> so there will be 6 channels available by the pwm.
> Thus, the pwm chip will register only 6 pwms(6 channels) most("fsl,pwm-channel-orders
> = {0 1 2 3 6 7}").And also the "fsl,pwm-channel-number" will be 6.
If the chip has eight PWMs I would register all of them. If some of them
are not routed out by the pinmux then just nothing happens if you use
them. In a sane devicetree they won't be referenced anyway when they are
not routed out of the SoC.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Sascha,
> > > Could you explain meaning of this property more precisely? I'm
> > > interested especially how is this related to the PWM IP block and
> boards.
> > >
> >
> > Yes.
> > There are 8 channels most. While the pinctrls of 4th and 5th channels
> > could be used by uart's Rx and Tx, then these 2 channels won't be used
> > for pwm output, so there will be 6 channels available by the pwm.
> > Thus, the pwm chip will register only 6 pwms(6 channels)
> > most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the "fsl,pwm-
> > channel-number" will be 6.
>
> If the chip has eight PWMs I would register all of them. If some of them
> are not routed out by the pinmux then just nothing happens if you use
> them. In a sane devicetree they won't be referenced anyway when they are
> not routed out of the SoC.
>
Yes, that's perfect well.
I will do it in v2.
Thanks very much.
--
Best Regards,
Xiubo
On Thursday 22 of August 2013 02:55:42 Xiubo Li-B47053 wrote:
> Hi Tomasz,
>
> Thanks for your comments.
>
> > > +- #pwm-cells: Should be 3. Number of cells being used to specify
> > > PWM
> > > property.
> > > + First cell specifies the per-chip channel index of the PWM
> > > to use, the
> > > + second cell is the period in nanoseconds and bit 0 in
> > > the third cell is
> > > + used to encode the polarity of PWM output. Set bit
> > > 0 of the third in PWM
> > > + specifier to 1 for inverse polarity & set to 0
> > > for normal polarity.
> >
> > If the meaning of flags cell is the same as in generic, default PWM
> > specifier format, then it should be noted here and generic PWM binding
> > documentation mentioned.
>
> OK, How about the following ?
> - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> description of the cells format.
I meant just the last cell, which stores flags, but actually this might be
a good idea, but with slightly extended description. Something among those
lines:
- #pwm-cells: Should be 3. The default three cell format specified by
generic PWM bindings are used. Refer to the documentation of generic PWM
bindings for more information about the meaning of cells.
> I will replace it in v2.
>
> > > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> > > divide-by 2^n(n = 0 ~ 7).
> >
> > Is it a hardware-specific property?
>
> Yes, I will revise it in v2.
I'd like to hear a bit more elaborate description of this property. What
are the factors that decide what value should be used here?
> > > +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM)
> > > mode.
> >
> > Could you explain meaning of this property?
>
> Well, this feature will be removed from the pwm core in v2.
OK.
> > > +- fsl,pwm-number: the number of PWM devices, and is must equal to
> > > the
> > > number + of "fsl,pwm-channels".
> >
> > This is redundant, because you can simply count how many entries have
> > been specified in fsl,pwm-channels.
>
> Yes, I will revise it in v2.
> And this would be renamed to " fsl,pwm-channel-number", which can be
> more Readable and understood.
I meant that you don't need to specify how many entries other property has
in another property, because device tree provides information about sizes
of all properties. So, in parsing code, you would be able to simply get
the size of "fsl,pwm-channels" property in bytes, divide that by
sizeof(u32) and get the numer of cells specified.
Also see below.
> > > +- fsl,pwm-channels: the channels' order which is be used for pwm in
> > > ftm0 + module, and they must be one or some of 0 ~ 7, because the
> > > ftm0 only has + 8 channels can be used.
> >
> > Could you explain meaning of this property more precisely? I'm
> > interested especially how is this related to the PWM IP block and
> > boards.
> Yes.
> There are 8 channels most. While the pinctrls of 4th and 5th channels
> could be used by uart's Rx and Tx, then these 2 channels won't be used
> for pwm output, so there will be 6 channels available by the pwm.
> Thus, the pwm chip will register only 6 pwms(6 channels)
> most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the
> "fsl,pwm-channel-number" will be 6.
>
> If hasn't any other problems, I will revise It in v2.
> And this will be renamed to "fsl,pwm-channel-orders", which can be more
> readable and understood.
As Sascha Hauer already suggested, you could get rid of both this and
"fsl,pwm-channel-number" properties and simply register all the channels.
This way you will have a deterministic 1:1 mapping of real hardware
channels to channels specified in device tree, which is definitely the way
to go.
Now as a safety measure, you could simply move the specification of
pinctrl states from SoC family or SoC level dtsi file to board level dts,
where only pinctrl states of channels used by a particular board would be
specified, so nothing could break operation of other devices that share
pin muxes with remaining channels.
> > > +- for very channel, the revlatived the pinctrl should be at least
> > > two
> > >
> > ^ typo?
> > >
> > > state + {"enN", "dsN"}, which "en" means "enable", "ds" means
> > > "disable" and "N" + means the order of the channel.
> >
> > I'd suggest a more readable naming convention, for example chN-active
> > and chN-idle. These words seem to be more common across existing
> > bindings.
> That's a good idea, I will think it over and revise it in v2.
OK. Looking forward to v2 then. Thanks.
Best regards,
Tomasz
Hi Tomasz,
> > > If the meaning of flags cell is the same as in generic, default PWM
> > > specifier format, then it should be noted here and generic PWM
> > > binding documentation mentioned.
> >
> > OK, How about the following ?
> > - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> > description of the cells format.
>
> I meant just the last cell, which stores flags, but actually this might
> be a good idea, but with slightly extended description. Something among
> those
> lines:
>
> - #pwm-cells: Should be 3. The default three cell format specified by
> generic PWM bindings are used. Refer to the documentation of generic PWM
> bindings for more information about the meaning of cells.
>
That's perfect.
> > > > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> > > > divide-by 2^n(n = 0 ~ 7).
> > >
> > > Is it a hardware-specific property?
> >
> > Yes, I will revise it in v2.
>
> I'd like to hear a bit more elaborate description of this property. What
> are the factors that decide what value should be used here?
>
Sorry, "the ftm0 pwm clock's prescaler" maybe also confuse you, it should be
"the ftm pwm counter clock input prescaler".
The ftm's counter clock can be selected as :
System clock,
Fixed frequency clock,
External clock.
And the ftm module clock has only system clock source.
The fixed frequency clock is an alternative clock source for the FTM counter that allows
the selection of a clock other than the system clock or an external clock. This clock input
is defined by chip integration. Due to FTM hardware implementation limitations, the
frequency of the fixed frequency clock must not exceed 1/2 of the system clock frequency.
The external clock passes through a synchronizer clocked by the system clock to assure
that counter transitions are properly aligned to system clock transitions.Therefore, to
meet Nyquist criteria considering also jitter, the frequency of the external clock source
must not exceed 1/4 of the system clock frequency.
So, if the fixed frequency clock or external clock equal or exceed the system clock...
> > > > +- fsl,pwm-number: the number of PWM devices, and is must equal to
> > > > the
> > > > number + of "fsl,pwm-channels".
> > >
> > > This is redundant, because you can simply count how many entries
> > > have been specified in fsl,pwm-channels.
> >
> > Yes, I will revise it in v2.
> > And this would be renamed to " fsl,pwm-channel-number", which can be
> > more Readable and understood.
>
> I meant that you don't need to specify how many entries other property
> has in another property, because device tree provides information about
> sizes of all properties. So, in parsing code, you would be able to simply
> get the size of "fsl,pwm-channels" property in bytes, divide that by
> sizeof(u32) and get the numer of cells specified.
>
OK, I will revise it in v2.
> > > > +- fsl,pwm-channels: the channels' order which is be used for pwm
> > > > +in
> > > > ftm0 + module, and they must be one or some of 0 ~ 7, because the
> > > > ftm0 only has + 8 channels can be used.
> > >
> > > Could you explain meaning of this property more precisely? I'm
> > > interested especially how is this related to the PWM IP block and
> > > boards.
> > Yes.
> > There are 8 channels most. While the pinctrls of 4th and 5th channels
> > could be used by uart's Rx and Tx, then these 2 channels won't be used
> > for pwm output, so there will be 6 channels available by the pwm.
> > Thus, the pwm chip will register only 6 pwms(6 channels)
> > most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the
> > "fsl,pwm-channel-number" will be 6.
> >
> > If hasn't any other problems, I will revise It in v2.
> > And this will be renamed to "fsl,pwm-channel-orders", which can be
> > more readable and understood.
>
> As Sascha Hauer already suggested, you could get rid of both this and
> "fsl,pwm-channel-number" properties and simply register all the channels.
> This way you will have a deterministic 1:1 mapping of real hardware
> channels to channels specified in device tree, which is definitely the
> way to go.
>
> Now as a safety measure, you could simply move the specification of
> pinctrl states from SoC family or SoC level dtsi file to board level dts,
> where only pinctrl states of channels used by a particular board would be
> specified, so nothing could break operation of other devices that share
> pin muxes with remaining channels.
Yes, I was also considering it, but not very be clear yet.
Thanks very much for your and Sascha's help.
I will try to implement it in v2.
Thanks very much.
--
Best regards,
Xiubo
On Thursday 22 of August 2013 09:52:42 Xiubo Li-B47053 wrote:
> Hi Tomasz,
>
> > > > If the meaning of flags cell is the same as in generic, default PWM
> > > > specifier format, then it should be noted here and generic PWM
> > > > binding documentation mentioned.
> > >
> > > OK, How about the following ?
> > > - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> > >
> > > description of the cells format.
> >
> > I meant just the last cell, which stores flags, but actually this might
> > be a good idea, but with slightly extended description. Something among
> > those
> >
> > lines:
> > - #pwm-cells: Should be 3. The default three cell format specified by
> >
> > generic PWM bindings are used. Refer to the documentation of generic
> > PWM
> > bindings for more information about the meaning of cells.
>
> That's perfect.
OK.
> > > > > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler,
> > > > > divide-by 2^n(n = 0 ~ 7).
> > > >
> > > > Is it a hardware-specific property?
> > >
> > > Yes, I will revise it in v2.
> >
> > I'd like to hear a bit more elaborate description of this property.
> > What
> > are the factors that decide what value should be used here?
>
> Sorry, "the ftm0 pwm clock's prescaler" maybe also confuse you, it should
> be "the ftm pwm counter clock input prescaler".
>
> The ftm's counter clock can be selected as :
> System clock,
> Fixed frequency clock,
> External clock.
>
> And the ftm module clock has only system clock source.
>
> The fixed frequency clock is an alternative clock source for the FTM
> counter that allows the selection of a clock other than the system clock
> or an external clock. This clock input is defined by chip integration.
> Due to FTM hardware implementation limitations, the frequency of the
> fixed frequency clock must not exceed 1/2 of the system clock frequency.
>
> The external clock passes through a synchronizer clocked by the system
> clock to assure that counter transitions are properly aligned to system
> clock transitions.Therefore, to meet Nyquist criteria considering also
> jitter, the frequency of the external clock source must not exceed 1/4
> of the system clock frequency.
>
> So, if the fixed frequency clock or external clock equal or exceed the
> system clock...
Can't the driver check the frequency of fixed or external clock and
calculate optimal divisor value so that it is less than 1/4 of system clock
frequency?
> > > > > +- fsl,pwm-number: the number of PWM devices, and is must equal
> > > > > to
> > > > > the
> > > > > number + of "fsl,pwm-channels".
> > > >
> > > > This is redundant, because you can simply count how many entries
> > > > have been specified in fsl,pwm-channels.
> > >
> > > Yes, I will revise it in v2.
> > > And this would be renamed to " fsl,pwm-channel-number", which can be
> > > more Readable and understood.
> >
> > I meant that you don't need to specify how many entries other property
> > has in another property, because device tree provides information about
> > sizes of all properties. So, in parsing code, you would be able to
> > simply get the size of "fsl,pwm-channels" property in bytes, divide
> > that by sizeof(u32) and get the numer of cells specified.
>
> OK, I will revise it in v2.
As I noted below, it won't be needed anymore. I just commented on this as a
side note.
> > > > > +- fsl,pwm-channels: the channels' order which is be used for pwm
> > > > > +in
> > > > > ftm0 + module, and they must be one or some of 0 ~ 7, because
> > > > > the
> > > > > ftm0 only has + 8 channels can be used.
> > > >
> > > > Could you explain meaning of this property more precisely? I'm
> > > > interested especially how is this related to the PWM IP block and
> > > > boards.
> > >
> > > Yes.
> > > There are 8 channels most. While the pinctrls of 4th and 5th channels
> > > could be used by uart's Rx and Tx, then these 2 channels won't be
> > > used
> > > for pwm output, so there will be 6 channels available by the pwm.
> > > Thus, the pwm chip will register only 6 pwms(6 channels)
> > > most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the
> > > "fsl,pwm-channel-number" will be 6.
> > >
> > > If hasn't any other problems, I will revise It in v2.
> > > And this will be renamed to "fsl,pwm-channel-orders", which can be
> > > more readable and understood.
> >
> > As Sascha Hauer already suggested, you could get rid of both this and
> > "fsl,pwm-channel-number" properties and simply register all the
> > channels. This way you will have a deterministic 1:1 mapping of real
> > hardware channels to channels specified in device tree, which is
> > definitely the way to go.
> >
> > Now as a safety measure, you could simply move the specification of
> > pinctrl states from SoC family or SoC level dtsi file to board level
> > dts, where only pinctrl states of channels used by a particular board
> > would be specified, so nothing could break operation of other devices
> > that share pin muxes with remaining channels.
>
> Yes, I was also considering it, but not very be clear yet.
> Thanks very much for your and Sascha's help.
> I will try to implement it in v2.
OK, good.
Best regards,
Tomasz
On Thu, Aug 22, 2013 at 08:26:10AM +0200, Sascha Hauer wrote:
> On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
> > Hi Tomasz,
> >
> > Thanks for your comments.
> >
> >
> > > Could you explain meaning of this property more precisely? I'm interested
> > > especially how is this related to the PWM IP block and boards.
> > >
> >
> > Yes.
> > There are 8 channels most. While the pinctrls of 4th and 5th channels could be
> > used by uart's Rx and Tx, then these 2 channels won't be used for pwm output,
> > so there will be 6 channels available by the pwm.
> > Thus, the pwm chip will register only 6 pwms(6 channels) most("fsl,pwm-channel-orders
> > = {0 1 2 3 6 7}").And also the "fsl,pwm-channel-number" will be 6.
>
> If the chip has eight PWMs I would register all of them. If some of them
> are not routed out by the pinmux then just nothing happens if you use
> them. In a sane devicetree they won't be referenced anyway when they are
> not routed out of the SoC.
In that case, shouldn't this be hooked up to the pinctrl subsystem as
well? As I understand the above, the logical thing would be for each PWM
channel's .request() operation to configure the pinmuxing appropriately.
And if it can't be configured as necessary then .request() should return
an error (or propagate the error from the pinctrl subsystem).
Thierry
On Wed, Aug 21, 2013 at 11:50:49AM +0200, Sascha Hauer wrote:
> On Wed, Aug 21, 2013 at 09:24:56AM +0000, Xiubo Li-B47053 wrote:
> > TO Sascha,
> >
> > > > +
> > > > + fpc = to_fsl_chip(chip);
> > > > +
> > > > + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> > > > + return -ESHUTDOWN;
> > > > +
> > > > + statename = kasprintf(GFP_KERNEL, "en%d", pwm->hwpwm);
> > > > + pins_state = pinctrl_lookup_state(fpc->pinctrl,
> > > > + statename);
> > > > + /* enable pins to be muxed in and configured */
> > > > + if (!IS_ERR(pins_state)) {
> > > > + ret = pinctrl_select_state(fpc->pinctrl, pins_state);
> > > > + if (ret)
> > > > + dev_warn(&fpc->pdev->dev,
> > > > + "could not set default pins\n");
> > >
> > > Why do you need to manipulate the pinctrl to en/disable a channel?
> > >
> >
> > This is because in Vybrid VF610 TOWER board, there are 4 leds, and each led's one point(diode's positive pole) is connected to 3.3V,
> > and the other point is connected to pwm's one channel. When the 4 pinctrls are configured as enable at the same time,
> > the 4 pinctrls is low valtage, and the 4 leds will be lighted up as default, then when you enable/disable one led will effects others.
> >
>
> I think the inactive state of a PWM is pretty much undefined by the PWM
> framework and left to the drivers.
>
> I stumbled upon this aswell. It would be good to think about the
> inactive state and how the PWM framework could help us here getting
> things right.
I'm not sure if imposing what the inactive state should be is a good
thing to do. For one, it might not be possible to set that particular
state in all of the PWM drivers. Similarly some drivers may know of a
more optimal state to put the pin into.
> There are several things to consider. For a noninverted PWM the inactive
> state should probably logic 0. For an inverted PWM it should probably be
> logic 1. I guess several PWM devices have an undefined inactive state,
> most of the PWM devices probably can control the inactive state by
> setting the duty cycle to 100% / 0% without actually disabling the PWM.
That sounds like a reasonable set of rules. The above should also be
equivalent to the "no power" state. I think we could possibly write down
those rules (even though I think they are obvious enough), but enforcing
one specific state doesn't sound right to me.
> Using the pinctrl is one way to control the inactive state and probaby
> the only one before initialization. It might be good to wire this up in
> the core instead of the individual PWM drivers.
I don't really see how the PWM core can make an educated decision about
this. The proper inactive state for a PWM can only be known by it's
corresponding driver. Each driver's .disable() operation should take
care of putting the PWM into the inactive state. That is, if it can be
done without too much effort. If putting the PWM into the inactive state
involves pinmuxing and such, it's probably better to defer that to the
.free() operation.
Thierry
On Thu, Aug 22, 2013 at 10:25:30AM +0200, Tomasz Figa wrote:
> On Thursday 22 of August 2013 02:55:42 Xiubo Li-B47053 wrote:
> > Hi Tomasz,
> >
> > Thanks for your comments.
> >
> > > > +- #pwm-cells: Should be 3. Number of cells being used to specify
> > > > PWM
> > > > property.
> > > > + First cell specifies the per-chip channel index of the PWM
> > > > to use, the
> > > > + second cell is the period in nanoseconds and bit 0 in
> > > > the third cell is
> > > > + used to encode the polarity of PWM output. Set bit
> > > > 0 of the third in PWM
> > > > + specifier to 1 for inverse polarity & set to 0
> > > > for normal polarity.
> > >
> > > If the meaning of flags cell is the same as in generic, default PWM
> > > specifier format, then it should be noted here and generic PWM binding
> > > documentation mentioned.
> >
> > OK, How about the following ?
> > - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> > description of the cells format.
>
> I meant just the last cell, which stores flags, but actually this might be
> a good idea, but with slightly extended description. Something among those
> lines:
>
> - #pwm-cells: Should be 3. The default three cell format specified by
> generic PWM bindings are used. Refer to the documentation of generic PWM
> bindings for more information about the meaning of cells.
Actually I prefer the second proposal, that is:
> > - #pwm-cells: Should be 3. See pwm.txt in this directory for a
> > description of the cells format.
We agreed on that wording in another thread and I'd prefer to be
consistent across bindings.
Thierry
On Wed, Aug 21, 2013 at 11:07:39AM +0800, Xiubo Li wrote:
> Add freescale ftm pwm driver support. The ftm pwm device
I think the correct capitalizations are: "Freescale", "FTM" and "PWM".
There are other occurrences in the rest of the driver that I haven't
explicitly pointed out.
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 75840b5..247b4c3 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -62,6 +62,16 @@ config PWM_BFIN
> To compile this driver as a module, choose M here: the module
> will be called pwm-bfin.
>
> +config PWM_FTM
Perhaps name this PWM_FSL_FTM to match the driver name?
> + tristate "Freescale FTM PWM support"
> + depends on OF
> + help
> + Generic FTM PWM framework driver for Freescale VF610 and
> + Layerscape LS-1 SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-ftm.
And fix this up to be "pwm-fsl-ftm".
> diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
[...]
> +#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/io.h>
> +#include <linux/pwm.h>
> +#include <linux/of_address.h>
> +#include <linux/pinctrl/consumer.h>
> +
> +#define FTM_SC 0x00
> +#define FTMSC_CPWMS (0x1 << 5)
> +#define FTMSC_CLK_MASK 0x03
> +#define FTMSC_CLK_OFFSET 0x03
> +#define FTMSC_CLKSYS (0x01 << 3)
> +#define FTMSC_CLKFIX (0x02 << 3)
> +#define FTMSC_CLKEXT (0x03 << 3)
> +#define FTMSC_PS_MASK 0x07
> +#define FTMSC_PS_OFFSET 0x00
> +
> +#define FTM_CNT 0x04
> +#define FTM_MOD 0x08
> +
> +#define FTM_CSC_BASE 0x0C
> +#define FTM_CSC(_CHANNEL) \
> + (FTM_CSC_BASE + (_CHANNEL * 0x08))
I prefer lowercase variables in macros:
#define FTM_CSC(channel) \
(FTM_CSC_BASE + (channel * 8))
> +#define FTMCnSC_MSB (0x01 << 5)
> +#define FTMCnSC_MSA (0x01 << 4)
> +#define FTMCnSC_ELSB (0x01 << 3)
> +#define FTMCnSC_ELSA (0x01 << 2)
> +#define FTM_PWMMODE (FTMCnSC_MSB)
> +#define FTM_HIGH_TRUE (FTMCnSC_ELSB)
> +#define FTM_LOW_TRUE (FTMCnSC_ELSA)
What's the reason for redefining these? Can't you just use the FTMCnSC_*
defines directly?
> +#define FTM_CV(_CHANNEL) \
> + (FTM_CV_BASE + (_CHANNEL * 0x08))
Same comment as for FTM_CSC above.
> +#define FTM_MAX_CHANNEL 0x08
There should be no need for this. The only place where you use this is
when assigning it to pwm_chip.npwm, so you may as well use the literal
there.
> +struct fsl_pwm {
> + unsigned long period_cycles;
> + unsigned long duty_cycles;
> +};
> +
> +struct fsl_pwm_chip {
> + struct mutex lock;
> +
> + struct platform_device *pdev;
You never use this platform_device. And you have to assign &pdev->dev to
the pwm_chip.dev anyway, so why not just use that consistently and drop
the pdev field?
> + struct pwm_chip chip;
> +
> + unsigned int clk_ps;
> + struct clk *clk;
> +
> + void __iomem *base;
> +
> + unsigned int cpwm;
> + struct fsl_pwm fpwms[FTM_MAX_CHANNEL];
Please don't do this. Use pwm_set_chip_data()/pwm_get_chip_data() to
associate per-channel specific data.
> + /* pinctrl handles */
Nit: it's only a single handle.
> + struct pinctrl *pinctrl;
You also use tabs and spaces inconsistently here. I suggest you use a
single space between the data type and the field name, that way it's
much easier to stay consistent.
> +static int fsl_pwm_request_channel(struct pwm_chip *chip,
> + struct pwm_device *pwm)
The arguments on the trailing line aren't properly aligned. They should
be aligned with the arguments on the first line.
> +{
> + int ret = 0;
> + struct fsl_pwm_chip *fpc;
> +
> + fpc = to_fsl_chip(chip);
> +
> + ret = clk_prepare_enable(fpc->clk);
This should probably be just clk_prepare(). Or is there some reason why
you can't delay clk_enable() to the .enable() operation?
> +static int fsl_updata_config(struct fsl_pwm_chip *fpc,
> + struct pwm_device *pwm)
Parameter alignment again. Please also check all other functions.
> +{
> + int i;
This should be unsigned int.
> +static unsigned long
> +fsl_rate_to_cycles(struct fsl_pwm_chip *fpc, int time_ns)
The above two lines are 78 characters when joined, so there's no need to
split them.
Perhaps time_ns should be "unsigned long"?
> +{
> + unsigned long ps;
> + unsigned long long c;
> +
> + ps = (0x01 << fpc->clk_ps);
This is fairly short, so you could do it along with the variable
declaration. Also there's no need for the parentheses or the hexadecimal
prefix (0x01 == 1):
unsigned long ps = 1 << fpc->clk_ps;
> + /* The module clk is HZ/s, the time_ns parameter
> + * is based nanosecond, so here should divide
> + * 1000000000UL.
> + */
Block comments should be:
/*
* ...
* ...
*/
Also HZ/s isn't a valid unit for a clock frequency. And time_ns also has
the _ns suffix to designate the unit, so as a whole that comment doesn't
add any real information and can just as well be dropped.
> +static int fsl_pwm_config_channel(struct pwm_chip *chip,
I think you can safely drop the _channel suffix from the PWM operations.
> + struct pwm_device *pwm,
> + int duty_ns,
> + int period_ns)
> +{
> + unsigned long period_cycles, duty_cycles;
> + struct fsl_pwm_chip *fpc;
> +
> + fpc = to_fsl_chip(chip);
> +
> + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> + return -ESHUTDOWN;
Erm... how do you think this could ever happen? Users need to request a
PWM to obtain a struct pwm_device, in which case PWMF_REQUESTED will
always be set. There are a few other occurrences throughout the rest of
the driver that I haven't pointed out explicitly.
> + period_cycles = fsl_rate_to_cycles(fpc, period_ns);
> + if (period_cycles > 0xFFFF) {
> + dev_warn(&fpc->pdev->dev,
> + "required PWM period cycles(%lu) "
> + "overflow 16-bits counter!\n",
> + period_cycles);
> + period_cycles = 0xFFFF;
> + }
> +
> + duty_cycles = fsl_rate_to_cycles(fpc, duty_ns);
> + if (duty_cycles >= 0xFFFF) {
> + dev_warn(&fpc->pdev->dev,
> + "required PWM duty cycles(%lu) "
> + "overflow 16-bits counter!\n",
> + duty_cycles);
> + duty_cycles = 0xFFFF - 1;
> + }
> +
> + fpc->fpwms[pwm->hwpwm].period_cycles = period_cycles;
> + fpc->fpwms[pwm->hwpwm].duty_cycles = duty_cycles;
You use these for the sole purpose of passing them into the
fsl_updata_config() function, so I wonder why you can't just pass them
as parameters and get rid of struct fsl_pwm as a bonus?
> +
> + writel(FTM_PWMMODE | FTM_HIGH_TRUE,
> + fpc->base + FTM_CSC(pwm->hwpwm));
> +
> + writel(0xF0, fpc->base + FTM_OUTMASK);
> + writel(0x0F, fpc->base + FTM_OUTINIT);
> + writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN);
> +
> + mutex_lock(&fpc->lock);
> + fsl_updata_config(fpc, pwm);
And now that I think about it: perhaps this was supposed to be called
fsl_update_config() instead of fsl_updat*a*_config()?
> + mutex_unlock(&fpc->lock);
> +
> + return 0;
> +}
> +
> +static int fsl_pwm_set_channel_polarity(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + enum pwm_polarity polarity)
> +{
> + unsigned long reg;
> + struct fsl_pwm_chip *fpc;
> +
> + fpc = to_fsl_chip(chip);
> +
> + reg = readl(fpc->base + FTM_POL);
> + reg &= ~(0x01 << pwm->hwpwm);
This would be slightly more canonical as:
reg &= ~BIT(pwm->hwpwm);
> + reg |= (polarity << pwm->hwpwm);
And perhaps here it would be better to be explicit. I think it's
unlikely that enum pwm_polarity will even gain a third entry, but better
be safe than sorry:
if (polarity == PWM_POLARITY_INVERSED)
reg |= BIT(pwm->hwpwm);
else
reg &= ~BIT(pwm->hwpwm);
> +static int is_any_other_channel_enabled(struct pwm_chip *chip,
> + unsigned int cur)
> +{
> + int i;
> +
> + for (i = 0; i < chip->npwm; i++) {
> + if (i == cur)
> + continue;
> + if (test_bit(PWMF_ENABLED, &chip->pwms[i].flags))
> + return 1;
> + }
> +
> + return 0;
> +}
This can probably be better done using a reference/enable count. Instead
of having to iterate over all PWM channels of the chip and checking
their flags, just keep track of how many times .enable() and .disable()
are called.
To make it easier you can probably wrap that into two functions, such as
fsl_clock_enable() and fsl_clock_disable().
> +static int fsl_pwm_enable_channel(struct pwm_chip *chip,
> + struct pwm_device *pwm)
> +{
[...]
> + if (is_any_other_channel_enabled(chip, pwm->hwpwm))
> + return 0;
This is where you'd call fsl_clock_enable()...
> + reg = readl(fpc->base + FTM_SC);
> + reg &= ~((FTMSC_CLK_MASK << FTMSC_CLK_OFFSET) |
> + (FTMSC_PS_MASK << FTMSC_PS_OFFSET));
> + /* select system clock source */
> + reg |= (FTMSC_CLKSYS | fpc->clk_ps);
> + writel(reg, fpc->base + FTM_SC);
... and run this code in fsl_clock_enable() if the enable count is 0 to
select the system clock when the first PWM is enabled.
> +static void fsl_pwm_disable_channel(struct pwm_chip *chip,
> + struct pwm_device *pwm)
> +{
[...]
> + if (is_any_other_channel_enabled(chip, pwm->hwpwm))
> + return;
Then call fsl_clock_disable() here...
> + writel(0xFF, fpc->base + FTM_OUTMASK);
> + reg = readl(fpc->base + FTM_SC);
> + reg &= ~(FTMSC_CLK_MASK << FTMSC_CLK_OFFSET);
> + writel(reg, fpc->base + FTM_SC);
... and run this code in fsl_clock_disable() if the enable count reaches
0 so that the clock is disabled when no PWM channels remain on.
> +static int fsl_pwm_parse_dt(struct fsl_pwm_chip *fpc)
> +{
[...]
> + int ret = 0;
> + u32 chs[FTM_MAX_CHANNEL];
> + struct device_node *np = fpc->pdev->dev.of_node;
> +
> + ret = of_property_read_u32(np, "fsl,pwm-clk-ps",
> + &fpc->clk_ps);
> + if (ret < 0) {
> + dev_err(&fpc->pdev->dev,
> + "failed to get pwm "
> + "clk prescaler : %d\n",
> + ret);
Perhaps it's more useful to mention the missing property explicitly in
the error message:
dev_err(fpc->chip.dev,
"failed to parse \"fsl,pwm-clk-ps\" property: %d\n",
ret);
> + return ret;
> + }
> + if (fpc->clk_ps > 7 || fpc->clk_ps < 0)
clk_ps is unsigned and therefore can't be < 0. And a blank line would be
useful between the previous line ("}") and this line.
> +static int fsl_pwm_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + struct fsl_pwm_chip *fpc;
> + struct resource *res;
> +
> + fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
> + if (!fpc) {
> + dev_err(&pdev->dev,
> + "failed to allocate memory\n");
I don't think that's very useful either. If this should indeed ever
fail, then the driver core will complain about the probe failing and
include the error code. Since it's the only location where you return
that error code you know immediately what went wrong.
> + return -ENOMEM;
> + }
> +
> + mutex_init(&fpc->lock);
> +
> + fpc->pdev = pdev;
> +
> + ret = fsl_pwm_parse_dt(fpc);
> + if (ret < 0)
> + return ret;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + fpc->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(fpc->base)) {
> + dev_err(&pdev->dev,
> + "failed to ioremap() registers\n");
No need for this error message. devm_ioremap_resource() prints out
errors already on failure.
> + fpc->chip.dev = &pdev->dev;
> + fpc->chip.ops = &fsl_pwm_ops;
> + fpc->chip.of_xlate = of_pwm_xlate_with_flags;
> + fpc->chip.of_pwm_n_cells = 3;
> + fpc->chip.base = -1;
> +
> + ret = pwmchip_add(&fpc->chip);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "failed to add ftm0 pwm chip %d\n",
> + ret);
dev_err() will already include the device name in the error message, so
I don't think you need the "ftm0" here. Also make sure to use the
correct capitalization for "PWM". And there is no need to split this
over so many lines, it all fits on a single line just fine. There are
other occurrences of this, so please double-check.
Thierry
On Wed, Aug 21, 2013 at 11:07:41AM +0800, Xiubo Li wrote:
> Signed-off-by: Xiubo Li <[email protected]>
> ---
> arch/arm/boot/dts/vf610-twr.dts | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
Same comments as for patch 2/4.
Thierry
On Wed, Aug 21, 2013 at 11:07:42AM +0800, Xiubo Li wrote:
> Signed-off-by: Xiubo Li <[email protected]>
> ---
> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt | 52 ++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
It used to be that device tree bindings documentation was squashed into
the same commit as the driver. I see some point in splitting this up
given that this will eventually go into a completely separate tree, but
I just want to make sure this is now the official way of splitting
patches so I can tell that to people in good conscience.
> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> new file mode 100644
> index 0000000..698965b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> @@ -0,0 +1,52 @@
> +Freescale FTM PWM controller
> +
> +Required properties:
> +- compatible: should be "fsl,vf610-ftm-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
> + First cell specifies the per-chip channel index of the PWM to use, the
> + second cell is the period in nanoseconds and bit 0 in the third cell is
> + used to encode the polarity of PWM output. Set bit 0 of the third in PWM
> + specifier to 1 for inverse polarity & set to 0 for normal polarity.
> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0 ~ 7).
> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
> +- fsl,pwm-number: the number of PWM devices, and is must equal to the number
> + of "fsl,pwm-channels".
> +- fsl,pwm-channels: the channels' order which is be used for pwm in ftm0
> + module, and they must be one or some of 0 ~ 7, because the ftm0 only has
> + 8 channels can be used.
> +- for very channel, the revlatived the pinctrl should be at least two state
> + {"enN", "dsN"}, which "en" means "enable", "ds" means "disable" and "N"
> + means the order of the channel.
This is missing a description of the clocks and clock-names properties.
> +
> +Example:
> +
> +pwm0: pwm@40038000 {
> + compatible = "fsl,vf610-ftm-pwm";
> + reg = <0x40038000 0x1000>;
> + #pwm-cells = <3>;
> + pinctrl-names = "en0", "ds0", "en3", "ds3";
> + pinctrl-0 = <&pinctrl_pwm0_ch0_en>;
> + pinctrl-1 = <&pinctrl_pwm0_ch0_ds>;
> + pinctrl-2 = <&pinctrl_pwm0_ch3_en>;
> + pinctrl-3 = <&pinctrl_pwm0_ch3_ds>;
> + fsl,pwm-clk-ps = <7>;
> + fsl,pwm-cpwm = <0>;
> + fsl,pwm-number = <2>;
> + fsl,pwm-channels = <0 3>;
> + ...
And this mixes tabs and spaces for indentation.
> + };
> +
> +leds {
> + compatible = "pwm-leds";
> + led {
> + label = "fsl_led";
> + pwms = <&pwm0 0 10000000 0>;
> + max-brightness = <127>;
> + };
> + backlight {
> + label = "fsl_backlight";
> + pwms = <&pwm0 3 10000000 1>;
> + max-brightness = <100>;
> + };
I don't think I like this at all. This example suggests to use the
pwm-leds driver for backlight control. Why not use pwm-backlight
instead?
Thierry
On Wed, Aug 21, 2013 at 11:07:40AM +0800, Xiubo Li wrote:
> Signed-off-by: Xiubo Li <[email protected]>
> ---
> arch/arm/boot/dts/vf610.dtsi | 83 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 82 insertions(+), 1 deletion(-)
Please also pay attention to the correct capitalization in the subject
here. "FTM" and "PWM". Furthermore this could probably use more than a
single line as patch description.
Thierry
On 08/23/2013 01:36 AM, Thierry Reding wrote:
> On Thu, Aug 22, 2013 at 08:26:10AM +0200, Sascha Hauer wrote:
>> On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
>>> Hi Tomasz,
>>>
>>> Thanks for your comments.
>>>
>>>
>>>> Could you explain meaning of this property more precisely?
>>>> I'm interested especially how is this related to the PWM IP
>>>> block and boards.
>>>>
>>>
>>> Yes. There are 8 channels most. While the pinctrls of 4th and
>>> 5th channels could be used by uart's Rx and Tx, then these 2
>>> channels won't be used for pwm output, so there will be 6
>>> channels available by the pwm. Thus, the pwm chip will register
>>> only 6 pwms(6 channels) most("fsl,pwm-channel-orders = {0 1 2 3
>>> 6 7}").And also the "fsl,pwm-channel-number" will be 6.
>>
>> If the chip has eight PWMs I would register all of them. If some
>> of them are not routed out by the pinmux then just nothing
>> happens if you use them. In a sane devicetree they won't be
>> referenced anyway when they are not routed out of the SoC.
>
> In that case, shouldn't this be hooked up to the pinctrl subsystem
> as well? As I understand the above, the logical thing would be for
> each PWM channel's .request() operation to configure the pinmuxing
> appropriately. And if it can't be configured as necessary then
> .request() should return an error (or propagate the error from the
> pinctrl subsystem).
I think the pin-muxing should be static, i.e. set up when the PWM
device as a whole probe()s, rather than being twiddled at request/free
time. Certainly the pinmux support in the device core is now set up to
acquire the default state right before probe(). I don't see a need to
do anything custom here.
On 08/23/2013 03:10 AM, Thierry Reding wrote:
> On Wed, Aug 21, 2013 at 11:07:42AM +0800, Xiubo Li wrote:
>> Signed-off-by: Xiubo Li <[email protected]> ---
>> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt | 52
>> ++++++++++++++++++++++ 1 file changed, 52 insertions(+) create
>> mode 100644
>> Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>> diff --git
>> a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>> b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>> +Required properties: +- compatible: should be
>> "fsl,vf610-ftm-pwm" +- reg: physical base address and length of
>> the controller's registers +- #pwm-cells: Should be 3. Number of
>> cells being used to specify PWM property. + First cell specifies
>> the per-chip channel index of the PWM to use, the + second cell
>> is the period in nanoseconds and bit 0 in the third cell is +
>> used to encode the polarity of PWM output. Set bit 0 of the third
>> in PWM + specifier to 1 for inverse polarity & set to 0 for
>> normal polarity.
Lines 2..n of a property description should be indented so it's easier
to see where each entry in the list starts.
>> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by
>> 2^n(n = 0 ~ 7). +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
>> +- fsl,pwm-number: the number of PWM devices, and is must equal
>> to the number + of "fsl,pwm-channels".
Isn't that value a static facet of the HW, and hence it can be
determined solely from the compatible value?
>> +- fsl,pwm-channels: the channels' order which is be used for pwm
>> in ftm0 + module, and they must be one or some of 0 ~ 7, because
>> the ftm0 only has + 8 channels can be used.
Why is there a need to re-order the channels? Why not simply reference
the actual physical channel IDs in client nodes?
>> +- for very channel, the revlatived the pinctrl should be at
>> least two state + {"enN", "dsN"}, which "en" means "enable",
>> "ds" means "disable" and "N" + means the order of the channel.
revlatived??
Why is there a need for pinctrl interaction at all?
Is the "N" in these property names the index into fsl,pwm-channels, or
the physical channel number in the controller HW, or something else?
"dis" would be better than "ds", although "enable-chN", "disable-chN"
would be even better.
Note that it's not possible to enable multiple pinctrl states at once,
so what happens when channel 0 is enabled, yet channel 1 is disabled,
and you want to enable "en0" and "ds1" at the same time?
Hi Stephen,
> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
>
> On 08/23/2013 01:36 AM, Thierry Reding wrote:
> > On Thu, Aug 22, 2013 at 08:26:10AM +0200, Sascha Hauer wrote:
> >> On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
> >>> Hi Tomasz,
> >>>
> >>> Thanks for your comments.
> >>>
> >>>
> >>>> Could you explain meaning of this property more precisely?
> >>>> I'm interested especially how is this related to the PWM IP block
> >>>> and boards.
> >>>>
> >>>
> >>> Yes. There are 8 channels most. While the pinctrls of 4th and 5th
> >>> channels could be used by uart's Rx and Tx, then these 2 channels
> >>> won't be used for pwm output, so there will be 6 channels available
> >>> by the pwm. Thus, the pwm chip will register only 6 pwms(6 channels)
> >>> most("fsl,pwm-channel-orders = {0 1 2 3
> >>> 6 7}").And also the "fsl,pwm-channel-number" will be 6.
> >>
> >> If the chip has eight PWMs I would register all of them. If some of
> >> them are not routed out by the pinmux then just nothing happens if
> >> you use them. In a sane devicetree they won't be referenced anyway
> >> when they are not routed out of the SoC.
> >
> > In that case, shouldn't this be hooked up to the pinctrl subsystem as
> > well? As I understand the above, the logical thing would be for each
> > PWM channel's .request() operation to configure the pinmuxing
> > appropriately. And if it can't be configured as necessary then
> > .request() should return an error (or propagate the error from the
> > pinctrl subsystem).
>
> I think the pin-muxing should be static, i.e. set up when the PWM device
> as a whole probe()s, rather than being twiddled at request/free time.
> Certainly the pinmux support in the device core is now set up to acquire
> the default state right before probe(). I don't see a need to do anything
> custom here.
As we have tolk about this before in [PATCH 1/4]:
"
> > Why do you need to manipulate the pinctrl to en/disable a channel?
> >
>
> This is because in Vybrid VF610 TOWER board, there are 4 leds, and each
> led's one point(diode's positive pole) is connected to 3.3V, and the
> other point is connected to pwm's one channel. When the 4 pinctrls are
> configured as enable at the same time, the 4 pinctrls is low valtage, and
> the 4 leds will be lighted up as default, then when you enable/disable
> one led will effects others.
>
> These pinctrls are belong to pwm, and I don't think led or other customer
> could control them directly.
> So, here I authorize the 4 pinctrls to each channel controls.
>
"
For the reason above, I have to control the pinctrls separately.
If all the pinctrls set as default state, the 8 pinctrls must be controlled together.
And the 4 leds will all be lighted up as default and will influence each other.
Thanks very much.
--
Best Regards.
Xiubo
Hi Thierry,
> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
>
> On Thu, Aug 22, 2013 at 08:26:10AM +0200, Sascha Hauer wrote:
> > On Thu, Aug 22, 2013 at 02:55:42AM +0000, Xiubo Li-B47053 wrote:
> > > Hi Tomasz,
> > >
> > > Thanks for your comments.
> > >
> > >
> > > > Could you explain meaning of this property more precisely? I'm
> > > > interested especially how is this related to the PWM IP block and
> boards.
> > > >
> > >
> > > Yes.
> > > There are 8 channels most. While the pinctrls of 4th and 5th
> > > channels could be used by uart's Rx and Tx, then these 2 channels
> > > won't be used for pwm output, so there will be 6 channels available
> by the pwm.
> > > Thus, the pwm chip will register only 6 pwms(6 channels)
> > > most("fsl,pwm-channel-orders = {0 1 2 3 6 7}").And also the "fsl,pwm-
> channel-number" will be 6.
> >
> > If the chip has eight PWMs I would register all of them. If some of
> > them are not routed out by the pinmux then just nothing happens if you
> > use them. In a sane devicetree they won't be referenced anyway when
> > they are not routed out of the SoC.
>
> In that case, shouldn't this be hooked up to the pinctrl subsystem as
> well? As I understand the above, the logical thing would be for each PWM
> channel's .request() operation to configure the pinmuxing appropriately.
> And if it can't be configured as necessary then .request() should return
> an error (or propagate the error from the pinctrl subsystem).
>
That's maybe better, if so, the pinctrl configuration must be split into two steps:
1, get the channel pinctrl "active" and "idle" states by callig pinctrl_lookup_state() in .request().
2, select the proper state in .enable()/.disable().
Thanks very much.
--
Best Regards.
Xiubo
Hi Thierry,
> Subject: Re: [PATCH 2/4] ARM: dts: Add Freescale ftm pwm node for VF610.
>
> On Wed, Aug 21, 2013 at 11:07:40AM +0800, Xiubo Li wrote:
> > Signed-off-by: Xiubo Li <[email protected]>
> > ---
> > arch/arm/boot/dts/vf610.dtsi | 83
> > +++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 82 insertions(+), 1 deletion(-)
>
> Please also pay attention to the correct capitalization in the subject
> here. "FTM" and "PWM". Furthermore this could probably use more than a
> single line as patch description.
>
Yes, I will revise it in v2.
Thanks very much.
--
Best Regards.
Xiubo
Hi Thierry,
> Subject: Re: [PATCH 3/4] ARM: dts: Enables ftm pwm device for Vybrid
> VF610 TOWER board.
>
> On Wed, Aug 21, 2013 at 11:07:41AM +0800, Xiubo Li wrote:
> > Signed-off-by: Xiubo Li <[email protected]>
> > ---
> > arch/arm/boot/dts/vf610-twr.dts | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
>
> Same comments as for patch 2/4.
>
Yes, I will revise it then.
Thanks very much.
--
Best Regards.
Xiubo
Hi Thierry,
See the comments below.
> I think the correct capitalizations are: "Freescale", "FTM" and "PWM".
> There are other occurrences in the rest of the driver that I haven't
> explicitly pointed out.
>
Yes, that's much better.
> > +config PWM_FTM
>
> Perhaps name this PWM_FSL_FTM to match the driver name?
>
OK.
> And fix this up to be "pwm-fsl-ftm".
Yes, I will.
> > +#define FTM_CSC_BASE 0x0C
> > +#define FTM_CSC(_CHANNEL) \
> > + (FTM_CSC_BASE + (_CHANNEL * 0x08))
>
> I prefer lowercase variables in macros:
>
> #define FTM_CSC(channel) \
> (FTM_CSC_BASE + (channel * 8))
>
Yes, That's better.
> > +#define FTM_PWMMODE (FTMCnSC_MSB)
> > +#define FTM_HIGH_TRUE (FTMCnSC_ELSB)
> > +#define FTM_LOW_TRUE (FTMCnSC_ELSA)
>
> What's the reason for redefining these? Can't you just use the FTMCnSC_*
> defines directly?
>
Just for more readable and comprehension.
I will revise it by not losing above two key elements.
> > +#define FTM_CV(_CHANNEL) \
> > + (FTM_CV_BASE + (_CHANNEL * 0x08))
>
> Same comment as for FTM_CSC above.
>
Yes.
> > +#define FTM_MAX_CHANNEL 0x08
>
> There should be no need for this. The only place where you use this is
> when assigning it to pwm_chip.npwm, so you may as well use the literal
> there.
>
I have recoded the driver, and the macro is used more than one time now.
> > + struct platform_device *pdev;
>
> You never use this platform_device. And you have to assign &pdev->dev to
> the pwm_chip.dev anyway, so why not just use that consistently and drop
> the pdev field?
>
Yes, I have droped the pdev field now.
> > + unsigned int cpwm;
> > + struct fsl_pwm fpwms[FTM_MAX_CHANNEL];
>
> Please don't do this. Use pwm_set_chip_data()/pwm_get_chip_data() to
> associate per-channel specific data.
>
I have replaced them now.
> > + /* pinctrl handles */
>
> Nit: it's only a single handle.
>
Yes.
> > + struct pinctrl *pinctrl;
>
> You also use tabs and spaces inconsistently here. I suggest you use a
> single space between the data type and the field name, that way it's much
> easier to stay consistent.
>
Now I have revised all about this.
> > +static int fsl_pwm_request_channel(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
>
> The arguments on the trailing line aren't properly aligned. They should
> be aligned with the arguments on the first line.
>
The same as the above comment.
> > + ret = clk_prepare_enable(fpc->clk);
>
> This should probably be just clk_prepare(). Or is there some reason why
> you can't delay clk_enable() to the .enable() operation?
>
Firstly, we should be clear that the fpc->clk is chip's work clock.
If so, after the .request() is called and before .enable() is called, the custumer will call .config(),
in which will read/write the pwm chip registers, if the module clock is still disabled, then the system will hang up.
> > +static int fsl_updata_config(struct fsl_pwm_chip *fpc,
> > + struct pwm_device *pwm)
>
> Parameter alignment again. Please also check all other functions.
>
Yes, I have revise all about this.
> > +{
> > + int i;
>
> This should be unsigned int.
>
I will revise it.
> > +static unsigned long
> > +fsl_rate_to_cycles(struct fsl_pwm_chip *fpc, int time_ns)
>
> The above two lines are 78 characters when joined, so there's no need to
> split them.
>
OK, I have revise all about this.
> Perhaps time_ns should be "unsigned long"?
>
Shouldn't this be same with "int duty_ns" and "int period_ns", which are defined by
struct pwm_ops {
...
int (*config)(struct pwm_chip *chip,
struct pwm_device *pwm,
int duty_ns, int period_ns);
...
} ?
> > + ps = (0x01 << fpc->clk_ps);
>
> This is fairly short, so you could do it along with the variable
> declaration. Also there's no need for the parentheses or the hexadecimal
> prefix (0x01 == 1):
>
> unsigned long ps = 1 << fpc->clk_ps;
>
OK, I will revise it then.
> > + /* The module clk is HZ/s, the time_ns parameter
> > + * is based nanosecond, so here should divide
> > + * 1000000000UL.
> > + */
>
> Block comments should be:
>
> /*
> * ...
> * ...
> */
>
> Also HZ/s isn't a valid unit for a clock frequency. And time_ns also has
> the _ns suffix to designate the unit, so as a whole that comment doesn't
> add any real information and can just as well be dropped.
>
I will revise it.
> > +static int fsl_pwm_config_channel(struct pwm_chip *chip,
>
> I think you can safely drop the _channel suffix from the PWM operations.
>
By adding _channel suffix just for more comprehensave about the pwm's muti-channel operation.
If this is redundant here, I will drop it.
> > + fpc = to_fsl_chip(chip);
> > +
> > + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> > + return -ESHUTDOWN;
>
> Erm... how do you think this could ever happen? Users need to request a
> PWM to obtain a struct pwm_device, in which case PWMF_REQUESTED will
> always be set. There are a few other occurrences throughout the rest of
> the driver that I haven't pointed out explicitly.
>
Does the following case is exist ?
The customer in one thread has .free(pwm_1), while in another thread,
which maybe had slept in for some reason, will call .config/.enable/.disable?
If so, as I have explained before, if the pwm_1 has been freed, the module clock maybe
disabled too, so if the .config is call the system will hang up.
> > + fpc->fpwms[pwm->hwpwm].period_cycles = period_cycles;
> > + fpc->fpwms[pwm->hwpwm].duty_cycles = duty_cycles;
>
> You use these for the sole purpose of passing them into the
> fsl_updata_config() function, so I wonder why you can't just pass them as
> parameters and get rid of struct fsl_pwm as a bonus?
>
Before I think the fpc has all the information the fsl_update_config() function needed.
Now, I have dropt the fsl_update_config() function.
> > + fsl_updata_config(fpc, pwm);
>
> And now that I think about it: perhaps this was supposed to be called
> fsl_update_config() instead of fsl_updat*a*_config()?
>
Well, a written mistake.
> > + reg &= ~(0x01 << pwm->hwpwm);
>
> This would be slightly more canonical as:
>
> reg &= ~BIT(pwm->hwpwm);
>
Yes, looks much better.
> > + reg |= (polarity << pwm->hwpwm);
>
> And perhaps here it would be better to be explicit. I think it's unlikely
> that enum pwm_polarity will even gain a third entry, but better be safe
> than sorry:
>
> if (polarity == PWM_POLARITY_INVERSED)
> reg |= BIT(pwm->hwpwm);
> else
> reg &= ~BIT(pwm->hwpwm);
>
I will think it over.
While only the polarity's bit0 is used for polarity's statement. If other bit won't has any other
meaning and purpose in the feature, I will replace it derictly.
> > +static int is_any_other_channel_enabled(struct pwm_chip *chip,
> > + unsigned int cur)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < chip->npwm; i++) {
> > + if (i == cur)
> > + continue;
> > + if (test_bit(PWMF_ENABLED, &chip->pwms[i].flags))
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
>
> This can probably be better done using a reference/enable count. Instead
> of having to iterate over all PWM channels of the chip and checking their
> flags, just keep track of how many times .enable() and .disable() are
> called.
>
> To make it easier you can probably wrap that into two functions, such as
> fsl_clock_enable() and fsl_clock_disable().
>
> > +static int fsl_pwm_enable_channel(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
> > +{
> [...]
> > + if (is_any_other_channel_enabled(chip, pwm->hwpwm))
> > + return 0;
>
> This is where you'd call fsl_clock_enable()...
>
> > + reg = readl(fpc->base + FTM_SC);
> > + reg &= ~((FTMSC_CLK_MASK << FTMSC_CLK_OFFSET) |
> > + (FTMSC_PS_MASK << FTMSC_PS_OFFSET));
> > + /* select system clock source */
> > + reg |= (FTMSC_CLKSYS | fpc->clk_ps);
> > + writel(reg, fpc->base + FTM_SC);
>
> ... and run this code in fsl_clock_enable() if the enable count is 0 to
> select the system clock when the first PWM is enabled.
>
> > +static void fsl_pwm_disable_channel(struct pwm_chip *chip,
> > + struct pwm_device *pwm)
> > +{
> [...]
> > + if (is_any_other_channel_enabled(chip, pwm->hwpwm))
> > + return;
>
> Then call fsl_clock_disable() here...
>
> > + writel(0xFF, fpc->base + FTM_OUTMASK);
> > + reg = readl(fpc->base + FTM_SC);
> > + reg &= ~(FTMSC_CLK_MASK << FTMSC_CLK_OFFSET);
> > + writel(reg, fpc->base + FTM_SC);
>
> ... and run this code in fsl_clock_disable() if the enable count reaches
> 0 so that the clock is disabled when no PWM channels remain on.
>
I will think it over and recode about this.
> > +static int fsl_pwm_parse_dt(struct fsl_pwm_chip *fpc) {
> [...]
> > + int ret = 0;
> > + u32 chs[FTM_MAX_CHANNEL];
> > + struct device_node *np = fpc->pdev->dev.of_node;
> > +
> > + ret = of_property_read_u32(np, "fsl,pwm-clk-ps",
> > + &fpc->clk_ps);
> > + if (ret < 0) {
> > + dev_err(&fpc->pdev->dev,
> > + "failed to get pwm "
> > + "clk prescaler : %d\n",
> > + ret);
>
> Perhaps it's more useful to mention the missing property explicitly in
> the error message:
>
> dev_err(fpc->chip.dev,
> "failed to parse \"fsl,pwm-clk-ps\" property: %d\n",
> ret);
>
Whil I think the following is better in code.
dev_err(fpc->chip.dev,
"failed to parse <fsl,pwm-clk-ps> property: %d\n",
ret);
>
> > + return ret;
> > + }
> > + if (fpc->clk_ps > 7 || fpc->clk_ps < 0)
>
> clk_ps is unsigned and therefore can't be < 0. And a blank line would be
> useful between the previous line ("}") and this line.
>
I will revise it.
> > +static int fsl_pwm_probe(struct platform_device *pdev) {
> > + int ret = 0;
> > + struct fsl_pwm_chip *fpc;
> > + struct resource *res;
> > +
> > + fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
> > + if (!fpc) {
> > + dev_err(&pdev->dev,
> > + "failed to allocate memory\n");
>
> I don't think that's very useful either. If this should indeed ever fail,
> then the driver core will complain about the probe failing and include
> the error code. Since it's the only location where you return that error
> code you know immediately what went wrong.
>
Yes, for the devm_kzlloc() have print out the fail reason, this will be redundant.
I have revised all about this.
> > + return -ENOMEM;
> > + }
> > +
> > + mutex_init(&fpc->lock);
> > +
> > + fpc->pdev = pdev;
> > +
> > + ret = fsl_pwm_parse_dt(fpc);
> > + if (ret < 0)
> > + return ret;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + fpc->base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(fpc->base)) {
> > + dev_err(&pdev->dev,
> > + "failed to ioremap() registers\n");
>
> No need for this error message. devm_ioremap_resource() prints out errors
> already on failure.
>
The same comment as the last one.
> > + fpc->chip.dev = &pdev->dev;
> > + fpc->chip.ops = &fsl_pwm_ops;
> > + fpc->chip.of_xlate = of_pwm_xlate_with_flags;
> > + fpc->chip.of_pwm_n_cells = 3;
> > + fpc->chip.base = -1;
> > +
> > + ret = pwmchip_add(&fpc->chip);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev,
> > + "failed to add ftm0 pwm chip %d\n",
> > + ret);
>
> dev_err() will already include the device name in the error message, so I
> don't think you need the "ftm0" here. Also make sure to use the correct
> capitalization for "PWM". And there is no need to split this over so many
> lines, it all fits on a single line just fine. There are other
> occurrences of this, so please double-check.
>
Yes.
I have revise all about this.
Thanks very much.
--
Best Regards.
Xiubo
On 08/25/2013 11:35 PM, Xiubo Li-B47053 wrote:
>> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
...
>>> Why do you need to manipulate the pinctrl to en/disable a channel?
>>
>> This is because in Vybrid VF610 TOWER board, there are 4 leds, and each
>> led's one point(diode's positive pole) is connected to 3.3V, and the
>> other point is connected to pwm's one channel. When the 4 pinctrls are
>> configured as enable at the same time, the 4 pinctrls is low valtage, and
>> the 4 leds will be lighted up as default, then when you enable/disable
>> one led will effects others.
>>
>> These pinctrls are belong to pwm, and I don't think led or other customer
>> could control them directly.
>> So, here I authorize the 4 pinctrls to each channel controls.
>>
> "
> For the reason above, I have to control the pinctrls separately.
>
> If all the pinctrls set as default state, the 8 pinctrls must be controlled together.
> And the 4 leds will all be lighted up as default and will influence each other.
Sorry, that still doesn't make much sense. Either way though, having
separate pinctrl setup for a single device isn't going to work. You'll
either need to have all combinations of 4 (8?) PWMs represented as
pinctrl states(!), or register separate PWM devices so that they get
independant pinctrl states.
Hi Stephen,
> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
>
> On 08/25/2013 11:35 PM, Xiubo Li-B47053 wrote:
> >> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> ...
> >>> Why do you need to manipulate the pinctrl to en/disable a channel?
> >>
> >> This is because in Vybrid VF610 TOWER board, there are 4 leds, and
> >> each led's one point(diode's positive pole) is connected to 3.3V, and
> >> the other point is connected to pwm's one channel. When the 4
> >> pinctrls are configured as enable at the same time, the 4 pinctrls is
> >> low valtage, and the 4 leds will be lighted up as default, then when
> >> you enable/disable one led will effects others.
> >>
> >> These pinctrls are belong to pwm, and I don't think led or other
> >> customer could control them directly.
> >> So, here I authorize the 4 pinctrls to each channel controls.
> >>
> > "
> > For the reason above, I have to control the pinctrls separately.
> >
> > If all the pinctrls set as default state, the 8 pinctrls must be
> controlled together.
> > And the 4 leds will all be lighted up as default and will influence
> each other.
>
> Sorry, that still doesn't make much sense. Either way though, having
> separate pinctrl setup for a single device isn't going to work. You'll
> either need to have all combinations of 4 (8?) PWMs represented as
> pinctrl states(!), or register separate PWM devices so that they get
> independant pinctrl states.
>
Well, I have ever thought about registering separate PWM device for each channel.
But, if so, how should I control the pinctrl of each PWM(actually one channel of FTM PWM) ?
I must select "default" state(the "default" state here, the pinctrl must be setup as "dsN" or "chN-idle" state we discussed before) when probing, and when customers .request-->.config-->.enable the PWM I also must select an "active" state to config the pinctrl...
Thus, this is still not static. I think this isn't much different from the current.
Also if having all combinations of 8 PWMs represented as pinctrl states, how could I deal with the problem about LEDs ?
Thanks very much,
--
Best Regards,
Xiubo
On 08/26/2013 09:48 PM, Xiubo Li-B47053 wrote:
> Hi Stephen,
>
>
>> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
>> Freescale FTM PWM
>>
>> On 08/25/2013 11:35 PM, Xiubo Li-B47053 wrote:
>>>> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
>> ...
>>>>> Why do you need to manipulate the pinctrl to en/disable a channel?
>>>>
>>>> This is because in Vybrid VF610 TOWER board, there are 4 leds, and
>>>> each led's one point(diode's positive pole) is connected to 3.3V, and
>>>> the other point is connected to pwm's one channel. When the 4
>>>> pinctrls are configured as enable at the same time, the 4 pinctrls is
>>>> low valtage, and the 4 leds will be lighted up as default, then when
>>>> you enable/disable one led will effects others.
>>>>
>>>> These pinctrls are belong to pwm, and I don't think led or other
>>>> customer could control them directly.
>>>> So, here I authorize the 4 pinctrls to each channel controls.
>>>>
>>> "
>>> For the reason above, I have to control the pinctrls separately.
>>>
>>> If all the pinctrls set as default state, the 8 pinctrls must be
>> controlled together.
>>> And the 4 leds will all be lighted up as default and will influence
>> each other.
>>
>> Sorry, that still doesn't make much sense. ...
What may help here is to show the exact content you expect the various
en/dis states to have, so that we can see exactly which pinctrl options
get set when and why.
On Mon, Aug 26, 2013 at 07:32:23AM +0000, Xiubo Li-B47053 wrote:
> > > +#define FTM_CSC_BASE 0x0C
> > > +#define FTM_CSC(_CHANNEL) \
> > > + (FTM_CSC_BASE + (_CHANNEL * 0x08))
> >
> > I prefer lowercase variables in macros:
> >
> > #define FTM_CSC(channel) \
> > (FTM_CSC_BASE + (channel * 8))
> >
> Yes, That's better.
Actually it should even be:
#define FTM_CSC(channel) \
(FTM_CSC_BASE + ((channel) * 8))
Just in case channel ends up being an expression.
> > > + ret = clk_prepare_enable(fpc->clk);
> >
> > This should probably be just clk_prepare(). Or is there some reason why
> > you can't delay clk_enable() to the .enable() operation?
> >
>
> Firstly, we should be clear that the fpc->clk is chip's work clock.
> If so, after the .request() is called and before .enable() is called, the custumer will call .config(),
> in which will read/write the pwm chip registers, if the module clock is still disabled, then the system will hang up.
Okay. In that case perhaps the better thing to do is call clk_prepare()
during driver probe and only clk_enable() here.
> > Perhaps time_ns should be "unsigned long"?
> >
>
> Shouldn't this be same with "int duty_ns" and "int period_ns", which are defined by
> struct pwm_ops {
> ...
> int (*config)(struct pwm_chip *chip,
> struct pwm_device *pwm,
> int duty_ns, int period_ns);
> ...
> } ?
Well, the plan is to eventually make duty_ns and period_ns unsigned int
or unsigned long because negative values don't make any sense for them.
With that in mind I think it makes sense to use the proper type here
now.
> > > +static int fsl_pwm_config_channel(struct pwm_chip *chip,
> >
> > I think you can safely drop the _channel suffix from the PWM operations.
> >
>
> By adding _channel suffix just for more comprehensave about the pwm's muti-channel operation.
> If this is redundant here, I will drop it.
The operations are implicitly per-channel operations. So yes, the
_channel suffix is redundant here.
> > > + fpc = to_fsl_chip(chip);
> > > +
> > > + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> > > + return -ESHUTDOWN;
> >
> > Erm... how do you think this could ever happen? Users need to request a
> > PWM to obtain a struct pwm_device, in which case PWMF_REQUESTED will
> > always be set. There are a few other occurrences throughout the rest of
> > the driver that I haven't pointed out explicitly.
> >
>
> Does the following case is exist ?
> The customer in one thread has .free(pwm_1), while in another thread,
> which maybe had slept in for some reason, will call .config/.enable/.disable?
>
> If so, as I have explained before, if the pwm_1 has been freed, the module clock maybe
> disabled too, so if the .config is call the system will hang up.
While the above could possibly happen, there's no way the core could
prevent it. And your explicit test couldn't either. So what usually
happens is that a driver requests a PWM device and then has exclusive
access to it. Any other driver that wants to use the same PWM device
can't because it will get an -EBUSY return.
So in your hypothetical case above, if one driver does stuff like that
with a PWM device then that's a driver bug, not something the PWM core
should be required to handle.
> > > +static int fsl_pwm_parse_dt(struct fsl_pwm_chip *fpc) {
> > [...]
> > > + int ret = 0;
> > > + u32 chs[FTM_MAX_CHANNEL];
> > > + struct device_node *np = fpc->pdev->dev.of_node;
> > > +
> > > + ret = of_property_read_u32(np, "fsl,pwm-clk-ps",
> > > + &fpc->clk_ps);
> > > + if (ret < 0) {
> > > + dev_err(&fpc->pdev->dev,
> > > + "failed to get pwm "
> > > + "clk prescaler : %d\n",
> > > + ret);
> >
> > Perhaps it's more useful to mention the missing property explicitly in
> > the error message:
> >
> > dev_err(fpc->chip.dev,
> > "failed to parse \"fsl,pwm-clk-ps\" property: %d\n",
> > ret);
> >
>
> Whil I think the following is better in code.
>
> dev_err(fpc->chip.dev,
> "failed to parse <fsl,pwm-clk-ps> property: %d\n",
> ret);
Why? You're quoting which property failed to parse so you should use the
correct character for quoting, which is either the apostrophe (') or the
quotation mark (").
Thierry
> Actually it should even be:
>
> #define FTM_CSC(channel) \
> (FTM_CSC_BASE + ((channel) * 8))
>
Well, yes, It should be, as Sascha has comment about this before, I have
already revise it.
> > Firstly, we should be clear that the fpc->clk is chip's work clock.
> > If so, after the .request() is called and before .enable() is called,
> > the custumer will call .config(), in which will read/write the pwm chip
> registers, if the module clock is still disabled, then the system will
> hang up.
>
> Okay. In that case perhaps the better thing to do is call clk_prepare()
> during driver probe and only clk_enable() here.
>
Yes, it is.
> > > Perhaps time_ns should be "unsigned long"?
> > >
> >
> > Shouldn't this be same with "int duty_ns" and "int period_ns", which
> > are defined by struct pwm_ops { ...
> > int (*config)(struct pwm_chip *chip,
> > struct pwm_device *pwm,
> > int duty_ns, int period_ns); ...
> > } ?
>
> Well, the plan is to eventually make duty_ns and period_ns unsigned int
> or unsigned long because negative values don't make any sense for them.
> With that in mind I think it makes sense to use the proper type here now.
>
Okey, I will think it over again and revise it.
> > > > +static int fsl_pwm_config_channel(struct pwm_chip *chip,
> > >
> > > I think you can safely drop the _channel suffix from the PWM
> operations.
> > >
> >
> > By adding _channel suffix just for more comprehensave about the pwm's
> muti-channel operation.
> > If this is redundant here, I will drop it.
>
> The operations are implicitly per-channel operations. So yes, the
> _channel suffix is redundant here.
>
Agree, I will drop it.
> > > > + fpc = to_fsl_chip(chip);
> > > > +
> > > > + if (WARN_ON(!test_bit(PWMF_REQUESTED, &pwm->flags)))
> > > > + return -ESHUTDOWN;
> > >
> > > Erm... how do you think this could ever happen? Users need to
> > > request a PWM to obtain a struct pwm_device, in which case
> > > PWMF_REQUESTED will always be set. There are a few other occurrences
> > > throughout the rest of the driver that I haven't pointed out
> explicitly.
> > >
> >
> > Does the following case is exist ?
> > The customer in one thread has .free(pwm_1), while in another thread,
> > which maybe had slept in for some reason, will
> call .config/.enable/.disable?
> >
> > If so, as I have explained before, if the pwm_1 has been freed, the
> > module clock maybe disabled too, so if the .config is call the system
> will hang up.
>
> While the above could possibly happen, there's no way the core could
> prevent it. And your explicit test couldn't either. So what usually
> happens is that a driver requests a PWM device and then has exclusive
> access to it. Any other driver that wants to use the same PWM device
> can't because it will get an -EBUSY return.
>
> So in your hypothetical case above, if one driver does stuff like that
> with a PWM device then that's a driver bug, not something the PWM core
> should be required to handle.
>
Okey, I will drop this.
> > While I think the following is better in code.
> >
> > dev_err(fpc->chip.dev,
> > "failed to parse <fsl,pwm-clk-ps> property: %d\n",
> > ret);
>
> Why? You're quoting which property failed to parse so you should use the
> correct character for quoting, which is either the apostrophe (') or the
> quotation mark (").
>
For my first impression, I just thought that maybe a little better.
Okey, I will adopt this in the feature.
--
Xiubo
Should have at least something w/regards to a commit message.
On Aug 20, 2013, at 10:07 PM, Xiubo Li wrote:
> Signed-off-by: Xiubo Li <[email protected]>
> ---
> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt | 52 ++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>
> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> new file mode 100644
> index 0000000..698965b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> @@ -0,0 +1,52 @@
> +Freescale FTM PWM controller
> +
> +Required properties:
> +- compatible: should be "fsl,vf610-ftm-pwm"
> +- reg: physical base address and length of the controller's registers
> +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
> + First cell specifies the per-chip channel index of the PWM to use, the
> + second cell is the period in nanoseconds and bit 0 in the third cell is
> + used to encode the polarity of PWM output. Set bit 0 of the third in PWM
> + specifier to 1 for inverse polarity & set to 0 for normal polarity.
> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0 ~ 7).
> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
Should describe this in more detail, what does the value actually mean for what modes there are?
> +- fsl,pwm-number: the number of PWM devices, and is must equal to the number
> + of "fsl,pwm-channels".
If they must be equal why do we need both?
> +- fsl,pwm-channels: the channels' order which is be used for pwm in ftm0
> + module, and they must be one or some of 0 ~ 7, because the ftm0 only has
> + 8 channels can be used.
> +- for very channel, the revlatived the pinctrl should be at least two state
revlatived?
> + {"enN", "dsN"}, which "en" means "enable", "ds" means "disable" and "N"
> + means the order of the channel.
> +
> +Example:
> +
> +pwm0: pwm@40038000 {
> + compatible = "fsl,vf610-ftm-pwm";
> + reg = <0x40038000 0x1000>;
> + #pwm-cells = <3>;
> + pinctrl-names = "en0", "ds0", "en3", "ds3";
> + pinctrl-0 = <&pinctrl_pwm0_ch0_en>;
> + pinctrl-1 = <&pinctrl_pwm0_ch0_ds>;
> + pinctrl-2 = <&pinctrl_pwm0_ch3_en>;
> + pinctrl-3 = <&pinctrl_pwm0_ch3_ds>;
> + fsl,pwm-clk-ps = <7>;
> + fsl,pwm-cpwm = <0>;
> + fsl,pwm-number = <2>;
> + fsl,pwm-channels = <0 3>;
> + ...
> + };
> +
> +leds {
> + compatible = "pwm-leds";
> + led {
> + label = "fsl_led";
> + pwms = <&pwm0 0 10000000 0>;
> + max-brightness = <127>;
> + };
> + backlight {
> + label = "fsl_backlight";
> + pwms = <&pwm0 3 10000000 1>;
> + max-brightness = <100>;
> + };
> +};
> --
> 1.8.0
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 08/30/2013 01:19 PM, Kumar Gala wrote:
> Should have at least something w/regards to a commit message.
>
> On Aug 20, 2013, at 10:07 PM, Xiubo Li wrote:
>
>> Signed-off-by: Xiubo Li <[email protected]>
>> ---
>> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt | 52 ++++++++++++++++++++++
>> 1 file changed, 52 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>> new file mode 100644
>> index 0000000..698965b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
>> @@ -0,0 +1,52 @@
>> +Freescale FTM PWM controller
>> +
>> +Required properties:
>> +- compatible: should be "fsl,vf610-ftm-pwm"
>> +- reg: physical base address and length of the controller's registers
>> +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
>> + First cell specifies the per-chip channel index of the PWM to use, the
>> + second cell is the period in nanoseconds and bit 0 in the third cell is
>> + used to encode the polarity of PWM output. Set bit 0 of the third in PWM
>> + specifier to 1 for inverse polarity & set to 0 for normal polarity.
>> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0 ~ 7).
>> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
>
> Should describe this in more detail, what does the value actually mean for what modes there are?
Assuming "CPWM" is clearly explained in the HW documentation for this
chip (I have no idea if that's actually the case), then is it still
necessary to explain what this means in *detail*? Perhaps simply "see
section XXX in the TRM" or "see register XXX, bit YYY in the HW
documentation" would be enough?
> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
>
> Should have at least something w/regards to a commit message.
>
I have sent a V2 patch and have added it.
> > + used to encode the polarity of PWM output. Set bit 0 of the third
> > +in PWM
> > + specifier to 1 for inverse polarity & set to 0 for normal polarity.
> > +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0
> ~ 7).
> > +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
>
> Should describe this in more detail, what does the value actually mean
> for what modes there are?
>
It's maybe a very useful feature for FTM core, but for PWM is not.
This isn't needed any more for PWM, and in V2 patch I have removed CPWM mode.
> > +- fsl,pwm-number: the number of PWM devices, and is must equal to the
> > +number
> > + of "fsl,pwm-channels".
>
> If they must be equal why do we need both?
>
I have replaced both of them too in V2.
> > +- fsl,pwm-channels: the channels' order which is be used for pwm in
> > +ftm0
> > + module, and they must be one or some of 0 ~ 7, because the ftm0
> > +only has
> > + 8 channels can be used.
> > +- for very channel, the revlatived the pinctrl should be at least two
> > +state
>
> revlatived?
>
This too.
--
Best Regards.
Xiubo
> Subject: Re: [PATCH 4/4] Documentation: Add device tree bindings for
> Freescale FTM PWM
>
> On 08/30/2013 01:19 PM, Kumar Gala wrote:
> > Should have at least something w/regards to a commit message.
> >
> > On Aug 20, 2013, at 10:07 PM, Xiubo Li wrote:
> >
> >> Signed-off-by: Xiubo Li <[email protected]>
> >> ---
> >> .../devicetree/bindings/pwm/fsl-ftm-pwm.txt | 52
> ++++++++++++++++++++++
> >> 1 file changed, 52 insertions(+)
> >> create mode 100644
> >> Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> >> b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> >> new file mode 100644
> >> index 0000000..698965b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pwm/fsl-ftm-pwm.txt
> >> @@ -0,0 +1,52 @@
> >> +Freescale FTM PWM controller
> >> +
> >> +Required properties:
> >> +- compatible: should be "fsl,vf610-ftm-pwm"
> >> +- reg: physical base address and length of the controller's
> >> +registers
> >> +- #pwm-cells: Should be 3. Number of cells being used to specify PWM
> property.
> >> + First cell specifies the per-chip channel index of the PWM to use,
> >> +the
> >> + second cell is the period in nanoseconds and bit 0 in the third
> >> +cell is
> >> + used to encode the polarity of PWM output. Set bit 0 of the third
> >> +in PWM
> >> + specifier to 1 for inverse polarity & set to 0 for normal polarity.
> >> +- fsl,pwm-clk-ps: the ftm0 pwm clock's prescaler, divide-by 2^n(n = 0
> ~ 7).
> >> +- fsl,pwm-cpwm: Center-Aligned PWM (CPWM) mode.
> >
> > Should describe this in more detail, what does the value actually mean
> for what modes there are?
>
> Assuming "CPWM" is clearly explained in the HW documentation for this
> chip (I have no idea if that's actually the case), then is it still
> necessary to explain what this means in *detail*? Perhaps simply "see
> section XXX in the TRM" or "see register XXX, bit YYY in the HW
> documentation" would be enough?
>
If to clearly explain the 'CPWM' mode, there maybe need much more words, I think just simply explain it, and then for more detail information "see section XXX in the TRM" or "see register XXX, bit YYY in HW documentation".
Thanks.
--
Best Regards,
Xiubo