[PATCHv6 0/4] Add Freescale FTM PWM driver.

[PATCHv6 0/4] Add Freescale FTM PWM driver.

[Xiubo Li]

Hello Therry,

The series of patches for this version has been reviewed and acked already.
To see each patch for detail.


------------------
------------------

This patch series is the Freescale FTM PWM implementation. And there are 8 channels most supported by the FTM PWM. This implementation is only compatible with device tree definition.

This patch series is based on linux-next and has been tested on Vybrid VF610 Tower board using device tree.



Changes in v6:
- Nothing.

Changes in RFCv2 of PATVHv5:
- Remove "fsl,pwm-counter-clk".

Changes in v5:
- Remove active/idle pinctrl stuff.

Changes in v4:
- Check for the result and return an error for devm_kzalloc().
- Move pinmux setting from the SoC file to the board specific file.
- Revise the written mistake of 'ret |= FTMSC_CLKEXT;' --> 'reg |= FTMSC_CLKEXT;'.


Changes in v3:
- Remove "availabe" field.
- Remove "fsl,pwm-avaliable-chs" property.
- ...

Changes in v2:
- Remove PWM CPWM/EPWM feature and sysfs.
- Remove some redundant code.
- Revise some code for more readable.
- Remove "fsl,pwm-clk-ps", "fsl,pwm-number", "fsl,pwm-channels",etc.
- Add "fsl,pwm-avaliable-chs", "fsl,pwm-counter-clk", etc.
- Support 8 channels default in dtsi file.
- Add counter clock source selection.
- Rename some function name, macro name, etc.
- Use PWM's and OF's existing function interfaces.
- Split clk_unprepare_enable to clk_unprepare and clk_enable,etc.
- ...

Added in v1:
- Add Freescale FTM PWM driver support.
- Add Freescale FTM PWM node for VF610.
- Enable Enables FTM PWM device for Vybrid VF610 TOWER.
- Add device tree bindings for Freescale.





--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Xiubo Li]

The FTM PWM device can be found on Vybrid VF610 Tower and Layerscape LS-1 SoCs.

Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Alison Wang <b18965-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Jingchang Lu <b35083-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Reviewed-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/pwm/Kconfig       |  10 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-fsl-ftm.c | 402 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 413 insertions(+)
 create mode 100644 drivers/pwm/pwm-fsl-ftm.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 75840b5..8144fb0 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_FSL_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-fsl-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..f383784 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_FSL_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..2420ce0
--- /dev/null
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -0,0 +1,402 @@
+/*
+ *  Freescale FTM PWM Driver
+ *
+ *  Copyright 2012-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 <dt-bindings/clock/vf610-clock.h>
+
+#define FTM_SC              0x00
+#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) * 8))
+#define FTMCnSC_MSB         BIT(5)
+#define FTMCnSC_MSA         BIT(4)
+#define FTMCnSC_ELSB        BIT(3)
+#define FTMCnSC_ELSA        BIT(2)
+
+#define FTM_CV_BASE         0x10
+#define FTM_CV(_channel) (FTM_CV_BASE + ((_channel) * 8))
+
+#define FTM_CNTIN           0x4C
+#define FTM_STATUS          0x50
+
+#define FTM_MODE            0x54
+#define FTMMODE_FTMEN       BIT(0)
+#define FTMMODE_INIT        BIT(2)
+#define FTMMODE_PWMSYNC     BIT(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_CNTIN_VAL       0x00
+#define FTM_MAX_CHANNEL     8
+
+struct fsl_pwm_chip {
+	struct pwm_chip chip;
+
+	struct clk *sys_clk;
+	struct clk *counter_clk;
+	unsigned int counter_clk_select;
+	unsigned int counter_clk_enable;
+	unsigned int clk_ps;
+
+	void __iomem *base;
+};
+
+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(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	int ret;
+	struct fsl_pwm_chip *fpc;
+
+	fpc = to_fsl_chip(chip);
+
+	ret = clk_prepare_enable(fpc->sys_clk);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct fsl_pwm_chip *fpc;
+
+	fpc = to_fsl_chip(chip);
+
+	clk_disable_unprepare(fpc->sys_clk);
+}
+
+static unsigned long fsl_rate_to_cycles(struct fsl_pwm_chip *fpc,
+				       unsigned long time_ns)
+{
+	unsigned long long c;
+	unsigned long ps = 1 << fpc->clk_ps;
+
+	c = clk_get_rate(fpc->counter_clk);
+	c = c * time_ns;
+	do_div(c, 1000000000UL);
+	do_div(c, ps);
+
+	return (unsigned long)c;
+}
+
+static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			  int duty_ns, int period_ns)
+{
+	unsigned long period_cycles, duty_cycles;
+	unsigned long cntin = FTM_CNTIN_VAL;
+	struct fsl_pwm_chip *fpc;
+
+	fpc = to_fsl_chip(chip);
+
+	period_cycles = fsl_rate_to_cycles(fpc, period_ns);
+	if (period_cycles > 0xFFFF) {
+		dev_err(chip->dev, "required PWM period cycles(%lu) overflow "
+				"16-bits counter!\n", period_cycles);
+		return -EINVAL;
+	}
+
+	duty_cycles = fsl_rate_to_cycles(fpc, duty_ns);
+	if (duty_cycles >= 0xFFFF) {
+		dev_err(chip->dev, "required PWM duty cycles(%lu) overflow "
+				"16-bits counter!\n", duty_cycles);
+		return -EINVAL;
+	}
+
+	writel(FTMCnSC_MSB | FTMCnSC_ELSB, 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);
+
+	writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
+	writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
+
+	return 0;
+}
+
+static int fsl_pwm_set_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);
+	if (polarity == PWM_POLARITY_INVERSED)
+		reg |= BIT(pwm->hwpwm);
+	else
+		reg &= ~BIT(pwm->hwpwm);
+	writel(reg, fpc->base + FTM_POL);
+
+	return 0;
+}
+
+static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
+{
+	int ret;
+	unsigned long reg;
+
+	if (fpc->counter_clk_enable++)
+		return 0;
+
+	ret = clk_prepare_enable(fpc->counter_clk);
+	if (ret)
+		return ret;
+
+	reg = readl(fpc->base + FTM_SC);
+	reg &= ~((FTMSC_CLK_MASK << FTMSC_CLK_OFFSET) |
+			(FTMSC_PS_MASK << FTMSC_PS_OFFSET));
+	/* select counter clock source */
+	switch (fpc->counter_clk_select) {
+	case VF610_CLK_FTM0:
+		reg |= FTMSC_CLKSYS;
+		break;
+	case VF610_CLK_FTM0_FIX_SEL:
+		reg |= FTMSC_CLKFIX;
+		break;
+	case VF610_CLK_FTM0_EXT_SEL:
+		reg |= FTMSC_CLKEXT;
+		break;
+	default:
+		break;
+	}
+	reg |= fpc->clk_ps;
+	writel(reg, fpc->base + FTM_SC);
+
+	return 0;
+}
+
+static int fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
+{
+	unsigned long reg;
+
+	if (--fpc->counter_clk_enable)
+		return 0;
+
+	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);
+
+	clk_disable_unprepare(fpc->counter_clk);
+
+	return 0;
+}
+
+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct fsl_pwm_chip *fpc;
+
+	fpc = to_fsl_chip(chip);
+
+	fsl_counter_clock_enable(fpc);
+
+	return 0;
+}
+
+static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct fsl_pwm_chip *fpc;
+
+	fpc = to_fsl_chip(chip);
+
+	fsl_counter_clock_disable(fpc);
+}
+
+static const struct pwm_ops fsl_pwm_ops = {
+	.request = fsl_pwm_request,
+	.free = fsl_pwm_free,
+	.config = fsl_pwm_config,
+	.set_polarity = fsl_pwm_set_polarity,
+	.enable = fsl_pwm_enable,
+	.disable = fsl_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc)
+{
+	unsigned long long sys_rate, counter_rate, ratio;
+
+	sys_rate = clk_get_rate(fpc->sys_clk);
+	if (!sys_rate)
+		return -EINVAL;
+
+	counter_rate = clk_get_rate(fpc->counter_clk);
+	if (!counter_rate) {
+		fpc->counter_clk = fpc->sys_clk;
+		fpc->counter_clk_select = VF610_CLK_FTM0;
+		dev_warn(fpc->chip.dev,
+				"the counter source clock is a dummy clock, "
+				"so select the system clock as default!\n");
+	}
+
+	switch (fpc->counter_clk_select) {
+	case VF610_CLK_FTM0_FIX_SEL:
+		ratio = 2 * counter_rate - 1;
+		do_div(ratio, sys_rate);
+		fpc->clk_ps = ratio;
+		break;
+	case VF610_CLK_FTM0_EXT_SEL:
+		ratio = 4 * counter_rate - 1;
+		do_div(ratio, sys_rate);
+		fpc->clk_ps = ratio;
+		break;
+	case VF610_CLK_FTM0:
+		fpc->clk_ps = 7;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc)
+{
+	int ret;
+	struct of_phandle_args clkspec;
+	struct device_node *np = fpc->chip.dev->of_node;
+
+	fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0");
+	if (IS_ERR(fpc->sys_clk)) {
+		ret = PTR_ERR(fpc->sys_clk);
+		dev_err(fpc->chip.dev,
+				"failed to get \"ftm0\" clock %d\n", ret);
+		return ret;
+	}
+
+	fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter");
+	if (IS_ERR(fpc->counter_clk)) {
+		ret = PTR_ERR(fpc->counter_clk);
+		dev_err(fpc->chip.dev,
+				"failed to get \"ftm0_counter\" clock %d\n",
+				ret);
+		return ret;
+	}
+
+	ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", 1,
+					&clkspec);
+	if (ret)
+		return ret;
+
+	fpc->counter_clk_select = clkspec.args[0];
+
+	return fsl_pwm_calculate_ps(fpc);
+}
+
+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)
+		return -ENOMEM;
+
+	fpc->chip.dev = &pdev->dev;
+	fpc->counter_clk_enable = 0;
+
+	ret = fsl_pwm_parse_clk_ps(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)) {
+		ret = PTR_ERR(fpc->base);
+		return ret;
+	}
+
+	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;
+	fpc->chip.npwm = FTM_MAX_CHANNEL;
+	ret = pwmchip_add(&fpc->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to add PWM chip %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);
+
+	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 <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
+MODULE_ALIAS("platform:fsl-ftm-pwm");
+MODULE_LICENSE("GPL");
-- 
1.8.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11761 bytes --]

On Tue, Nov 12, 2013 at 09:36:55AM +0800, Xiubo Li wrote:
> The FTM PWM device can be found on Vybrid VF610 Tower and Layerscape LS-1 SoCs.
> 
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> Signed-off-by: Alison Wang <b18965@freescale.com>
> Signed-off-by: Jingchang Lu <b35083@freescale.com>
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/pwm/Kconfig       |  10 ++
>  drivers/pwm/Makefile      |   1 +
>  drivers/pwm/pwm-fsl-ftm.c | 402 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 413 insertions(+)
>  create mode 100644 drivers/pwm/pwm-fsl-ftm.c

Sorry for taking so long. Overall this looks pretty good. A few minor
comments below.

> 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>

Can you sort these alphabetically, please?

> +#include <dt-bindings/clock/vf610-clock.h>

This can stay separate, and I would even separate it from the others by
a blank line.

> +#define FTM_SC              0x00
> +#define FTMSC_CLK_MASK      0x03

Perhaps FTM_SC_CLK_MASK for consistency with the register name? Same for
all others below.

> +#define FTMSC_CLK_OFFSET    0x03

I think the more common suffix would be _SHIFT. _OFFSET is more
typically used for registers, not bitfields.

> +#define FTMSC_CLKSYS        (0x01 << 3)
> +#define FTMSC_CLKFIX        (0x02 << 3)
> +#define FTMSC_CLKEXT        (0x03 << 3)

Perhaps "<< 3" should be "<< FTM_SC_CLK_SHIFT"? And the names perhaps
FTM_SC_CLK_{SYS,FIX,EXT}? That somehow "namespaces" all of them.

> +#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) * 8))
> +#define FTMCnSC_MSB         BIT(5)

Same here: FTMCnSC_MSB isn't immediately obvious to be a bit within the
FTM_CSC register. FTM_CSC_MSB is.

> +#define FTM_CNTIN_VAL       0x00

Do we really need this?

> +#define FTM_MAX_CHANNEL     8

This is used only once, so I prefer to just assign .npwm = 8 in the
driver's .probe() implementation.

> +static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	int ret;
> +	struct fsl_pwm_chip *fpc;
> +
> +	fpc = to_fsl_chip(chip);

You could assign this when declaring the fpc variable to save two lines.

> +	ret = clk_prepare_enable(fpc->sys_clk);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

You can save another few lines by making this simply:

	return clk_prepare_enable(fpc->sys_clk);

> +static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct fsl_pwm_chip *fpc;
> +
> +	fpc = to_fsl_chip(chip);

Same comment here.

> +static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  int duty_ns, int period_ns)
> +{
> +	unsigned long period_cycles, duty_cycles;

I don't think there's a need for the _cycles suffix here.

> +	unsigned long cntin = FTM_CNTIN_VAL;

I don't understand why this is needed. FTM_CNTIN_VAL is defined as 0 and
you never change cntin subsequently, so why not just use the literal 0
instead?

> +	struct fsl_pwm_chip *fpc;
> +
> +	fpc = to_fsl_chip(chip);
> +
> +	period_cycles = fsl_rate_to_cycles(fpc, period_ns);
> +	if (period_cycles > 0xFFFF) {
> +		dev_err(chip->dev, "required PWM period cycles(%lu) overflow "
> +				"16-bits counter!\n", period_cycles);
> +		return -EINVAL;
> +	}
> +
> +	duty_cycles = fsl_rate_to_cycles(fpc, duty_ns);
> +	if (duty_cycles >= 0xFFFF) {
> +		dev_err(chip->dev, "required PWM duty cycles(%lu) overflow "
> +				"16-bits counter!\n", duty_cycles);
> +		return -EINVAL;
> +	}

I'm not sure the error messages are all that useful. A -EINVAL error
code should make it pretty clear what the problem is.

> +	writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm));
> +
> +	writel(0xF0, fpc->base + FTM_OUTMASK);
> +	writel(0x0F, fpc->base + FTM_OUTINIT);

The purpose of this eludes me. These seem to be global (not specific to
channel pwm->hwpwm) registers, so why are they updated whenever a single
channel is reconfigured?

> +	writel(FTM_CNTIN_VAL, fpc->base + FTM_CNTIN);

This could become simply:

	writel(0, fpc->base + FTM_CNTIN);

> +	writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> +	writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));

And these:

	writel(period - 1, fpc->base + FTM_MOD);
	writel(duty, fpc->base + FTM_CV(pwm->hwpwm));

Although now that I think about it, this seems broken. The period is set
in a global register, so I assume it is valid for all channels. What if
you want to use different periods for individual channels? The way I
read this the last one to be configured will win and change the period
to whatever it wants. Other channels won't even notice.

Is there a way to set the period per channel?

> +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
> +{
> +	int ret;
> +	unsigned long reg;
> +
> +	if (fpc->counter_clk_enable++)
> +		return 0;

Are you sure this is safe? I think you'll need to use either an atomic
or a mutex to lock this.

> +	ret = clk_prepare_enable(fpc->counter_clk);
> +	if (ret)
> +		return ret;

In case clk_prepare_enable() fails, the counter_clk_enable will need to
be decremented in order to track the state correctly, doesn't it?

> +
> +	reg = readl(fpc->base + FTM_SC);
> +	reg &= ~((FTMSC_CLK_MASK << FTMSC_CLK_OFFSET) |
> +			(FTMSC_PS_MASK << FTMSC_PS_OFFSET));
> +	/* select counter clock source */

There should be an empty line between the above two.

> +	switch (fpc->counter_clk_select) {
> +	case VF610_CLK_FTM0:
> +		reg |= FTMSC_CLKSYS;
> +		break;
> +	case VF610_CLK_FTM0_FIX_SEL:
> +		reg |= FTMSC_CLKFIX;
> +		break;
> +	case VF610_CLK_FTM0_EXT_SEL:
> +		reg |= FTMSC_CLKEXT;
> +		break;
> +	default:
> +		break;
> +	}
> +	reg |= fpc->clk_ps;

And another one above this line.

> +	writel(reg, fpc->base + FTM_SC);

I think with the proper locking in place what you should do is increment
counter_clk_enable only here. That makes avoids having to decrement the
count on error.

Similarly in fsl_counter_clock_disable() you can postpone decrementing
the count until the very end.

> +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct fsl_pwm_chip *fpc;
> +
> +	fpc = to_fsl_chip(chip);
> +
> +	fsl_counter_clock_enable(fpc);

This can fail. Should the error be propagated?

> +
> +	return 0;
> +}
> +
> +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct fsl_pwm_chip *fpc;
> +
> +	fpc = to_fsl_chip(chip);
> +
> +	fsl_counter_clock_disable(fpc);
> +}

Same here. Since you can't propagate the error, perhaps an error message
would be appropriate here?

Also for the locking above, perhaps a good solution would be to acquire
the lock around the calls to fsl_counter_clock_{enable,disable}() so
that they can safely assume that they are called with the lock held,
which will make their implementation a lot simpler.

So what you could do is this:

	static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
	{
		struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
		int ret;

		mutex_lock(&fpc->lock);
		ret = fsl_counter_clock_enable(fpc);
		mutex_unlock(&fpc->lock);

		return ret;
	}

And analogously for fsl_pwm_disable().

> +static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc)
> +{
> +	unsigned long long sys_rate, counter_rate, ratio;
> +
> +	sys_rate = clk_get_rate(fpc->sys_clk);
> +	if (!sys_rate)
> +		return -EINVAL;
> +
> +	counter_rate = clk_get_rate(fpc->counter_clk);
> +	if (!counter_rate) {
> +		fpc->counter_clk = fpc->sys_clk;
> +		fpc->counter_clk_select = VF610_CLK_FTM0;
> +		dev_warn(fpc->chip.dev,
> +				"the counter source clock is a dummy clock, "
> +				"so select the system clock as default!\n");
> +	}
> +
> +	switch (fpc->counter_clk_select) {
> +	case VF610_CLK_FTM0_FIX_SEL:
> +		ratio = 2 * counter_rate - 1;
> +		do_div(ratio, sys_rate);
> +		fpc->clk_ps = ratio;
> +		break;
> +	case VF610_CLK_FTM0_EXT_SEL:
> +		ratio = 4 * counter_rate - 1;
> +		do_div(ratio, sys_rate);
> +		fpc->clk_ps = ratio;
> +		break;
> +	case VF610_CLK_FTM0:
> +		fpc->clk_ps = 7;

Even though it doesn't matter here, you should still add a break.
Otherwise if you ever modify the code in the default case, you don't
have to remember to add it in then.

> +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc)
> +{
> +	int ret;
> +	struct of_phandle_args clkspec;
> +	struct device_node *np = fpc->chip.dev->of_node;
> +
> +	fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0");
> +	if (IS_ERR(fpc->sys_clk)) {
> +		ret = PTR_ERR(fpc->sys_clk);
> +		dev_err(fpc->chip.dev,
> +				"failed to get \"ftm0\" clock %d\n", ret);
> +		return ret;
> +	}
> +
> +	fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter");
> +	if (IS_ERR(fpc->counter_clk)) {
> +		ret = PTR_ERR(fpc->counter_clk);
> +		dev_err(fpc->chip.dev,
> +				"failed to get \"ftm0_counter\" clock %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", 1,
> +					&clkspec);
> +	if (ret)
> +		return ret;
> +
> +	fpc->counter_clk_select = clkspec.args[0];

This isn't at all pretty. But given that once you have access to a
struct clk there's no way to identify it, I don't know of a better
alternative.

> +
> +	return fsl_pwm_calculate_ps(fpc);
> +}
> +
> +static int fsl_pwm_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;

There's no need to initialize this.

> +	struct fsl_pwm_chip *fpc;
> +	struct resource *res;
> +
> +	fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
> +	if (!fpc)
> +		return -ENOMEM;
> +
> +	fpc->chip.dev = &pdev->dev;
> +	fpc->counter_clk_enable = 0;

You use kzalloc(), so this is already be zeroed out.

> +	ret = fsl_pwm_parse_clk_ps(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)) {
> +		ret = PTR_ERR(fpc->base);
> +		return ret;

You can simply do "return PTR_ERR(fpc->base);" here.

> +	}
> +
> +	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;
> +	fpc->chip.npwm = FTM_MAX_CHANNEL;
> +	ret = pwmchip_add(&fpc->chip);

There should be a blank linke between the above two.

> +static int fsl_pwm_remove(struct platform_device *pdev)
> +{
> +	struct fsl_pwm_chip *fpc;
> +
> +	fpc = platform_get_drvdata(pdev);

These can go on one line.

> +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),

No need for of_match_ptr() since you depend on OF. And while at it, you
can drop the initialization of .owner as well, since the core takes care
of initializing that nowadays.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

On Thu, Nov 28, 2013 at 10:25:11PM +0100, Thierry Reding wrote:
> On Tue, Nov 12, 2013 at 09:36:55AM +0800, Xiubo Li wrote:
[...]
> > +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc)
> > +{
> > +	int ret;
> > +	struct of_phandle_args clkspec;
> > +	struct device_node *np = fpc->chip.dev->of_node;
> > +
> > +	fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0");
> > +	if (IS_ERR(fpc->sys_clk)) {
> > +		ret = PTR_ERR(fpc->sys_clk);
> > +		dev_err(fpc->chip.dev,
> > +				"failed to get \"ftm0\" clock %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter");
> > +	if (IS_ERR(fpc->counter_clk)) {
> > +		ret = PTR_ERR(fpc->counter_clk);
> > +		dev_err(fpc->chip.dev,
> > +				"failed to get \"ftm0_counter\" clock %d\n",
> > +				ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", 1,
> > +					&clkspec);
> > +	if (ret)
> > +		return ret;
> > +
> > +	fpc->counter_clk_select = clkspec.args[0];
> 
> This isn't at all pretty. But given that once you have access to a
> struct clk there's no way to identify it, I don't know of a better
> alternative.

Hi Mike,

I've seen this crop up a number of times now, to varying degrees of
gravity. In this particular case, the driver needs to know the type of a
clock because it needs to program this hardware differently depending on
which clock feeds the counter. Since there is no way to obtain any kind
of identifying information from a struct clk, drivers need to rely on
hacks like this and manually reach into the device tree to obtain that
information.

I'm aware of similar cases which have been solved by using a mux clock
that takes a set of parents and the .set_parent() operation is then used
to program registers appropriately. However in all those cases, the mux
clock (and the parents) have all been part of a single driver, so I am
not sure if the same would be applicable here.

Do you have any other suggestions on how this could possibly be solved?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Mark Rutland]

Hi,

On Thu, Nov 28, 2013 at 10:37:19PM +0000, Thierry Reding wrote:
> On Thu, Nov 28, 2013 at 10:25:11PM +0100, Thierry Reding wrote:
> > On Tue, Nov 12, 2013 at 09:36:55AM +0800, Xiubo Li wrote:
> [...]
> > > +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc)
> > > +{
> > > +	int ret;
> > > +	struct of_phandle_args clkspec;
> > > +	struct device_node *np = fpc->chip.dev->of_node;
> > > +
> > > +	fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0");
> > > +	if (IS_ERR(fpc->sys_clk)) {
> > > +		ret = PTR_ERR(fpc->sys_clk);
> > > +		dev_err(fpc->chip.dev,
> > > +				"failed to get \"ftm0\" clock %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter");
> > > +	if (IS_ERR(fpc->counter_clk)) {
> > > +		ret = PTR_ERR(fpc->counter_clk);
> > > +		dev_err(fpc->chip.dev,
> > > +				"failed to get \"ftm0_counter\" clock %d\n",
> > > +				ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells", 1,
> > > +					&clkspec);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	fpc->counter_clk_select = clkspec.args[0];
> > 
> > This isn't at all pretty. But given that once you have access to a
> > struct clk there's no way to identify it, I don't know of a better
> > alternative.
> 
> Hi Mike,
> 
> I've seen this crop up a number of times now, to varying degrees of
> gravity. In this particular case, the driver needs to know the type of a
> clock because it needs to program this hardware differently depending on
> which clock feeds the counter. Since there is no way to obtain any kind
> of identifying information from a struct clk, drivers need to rely on
> hacks like this and manually reach into the device tree to obtain that
> information.

Which property of the clock is the consumer concerned with in this case?

>From a quick look at the driver it looks like there are actually a
number of different input lines to the device that share the clock-name
"ftm0_counter", though they are actually separate and each has a
different divider. Have I got that right?

If that's the case, having a unique clock-names value for each of those
lines would be the solution I'd expect. Then you just have to list the
one(s) that are wired up, and the driver can figure out the appropriate
line to use either by requesting by name until it finds a match or
inspecting the clock-names property.

Is there some other property of the parent that we care about here?

> 
> I'm aware of similar cases which have been solved by using a mux clock
> that takes a set of parents and the .set_parent() operation is then used
> to program registers appropriately. However in all those cases, the mux
> clock (and the parents) have all been part of a single driver, so I am
> not sure if the same would be applicable here.
> 
> Do you have any other suggestions on how this could possibly be solved?

This also feels like the case I describe above. What am I missing? :)

Thanks,
Mark.

RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Li Xiubo]

> > > > +static int fsl_pwm_parse_clk_ps(struct fsl_pwm_chip *fpc)
> > > > +{
> > > > +	int ret;
> > > > +	struct of_phandle_args clkspec;
> > > > +	struct device_node *np = fpc->chip.dev->of_node;
> > > > +
> > > > +	fpc->sys_clk = devm_clk_get(fpc->chip.dev, "ftm0");
> > > > +	if (IS_ERR(fpc->sys_clk)) {
> > > > +		ret = PTR_ERR(fpc->sys_clk);
> > > > +		dev_err(fpc->chip.dev,
> > > > +				"failed to get \"ftm0\" clock %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	fpc->counter_clk = devm_clk_get(fpc->chip.dev, "ftm0_counter");
> > > > +	if (IS_ERR(fpc->counter_clk)) {
> > > > +		ret = PTR_ERR(fpc->counter_clk);
> > > > +		dev_err(fpc->chip.dev,
> > > > +				"failed to get \"ftm0_counter\" clock %d\n",
> > > > +				ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
> 1,
> > > > +					&clkspec);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	fpc->counter_clk_select = clkspec.args[0];
> > >
> > > This isn't at all pretty. But given that once you have access to a
> > > struct clk there's no way to identify it, I don't know of a better
> > > alternative.
> >
> > Hi Mike,
> >
> > I've seen this crop up a number of times now, to varying degrees of
> > gravity. In this particular case, the driver needs to know the type of a
> > clock because it needs to program this hardware differently depending on
> > which clock feeds the counter. Since there is no way to obtain any kind
> > of identifying information from a struct clk, drivers need to rely on
> > hacks like this and manually reach into the device tree to obtain that
> > information.
> 
> Which property of the clock is the consumer concerned with in this case?
> 

The "ftm0_counter" clock.


> From a quick look at the driver it looks like there are actually a
> number of different input lines to the device that share the clock-name
> "ftm0_counter", though they are actually separate and each has a
> different divider. Have I got that right?
> 

Yes mostly, there are three different input lines wired to the counter
clock, but they share only one divider. And between the three lines and
the divider there is one mux inside of the FTM IP block.
The three clock sources are:
"ftm0",
"ftm0-fix",
"ftm0-ext".
		 _____________________________________________________
		|									|
		|   +++++++++++		FTM Module				|
ftm0 -------|-->+         +							|
		|   +         +     +++++++++++++     ++++++++++++++	|
ftm0-fix ---|-->+   MUX   +---->+  divider  +---->+  counter   +	|
		|   +         +     +++++++++++++     ++++++++++++++	|
ftm0-ext ---|-->+         +							|
		|   +++++++++++							|
		|_____________________________________________________|



> If that's the case, having a unique clock-names value for each of those
> lines would be the solution I'd expect. Then you just have to list the
> one(s) that are wired up, and the driver can figure out the appropriate
> line to use either by requesting by name until it finds a match or
> inspecting the clock-names property.
> 

Hi Kumar,
In the list archives for mails of "[RFC][PATCHv5 4/4] Documentation: Add
device tree bindings for Freescale FTM PWM.",we have discussed about this.
Is there any different with your suggestions or ideas? 


> Is there some other property of the parent that we care about here?
> 

As I know, not yet.

--
Best Regards,
 

RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Li Xiubo]

Hi Thierry,

Thanks for your detail comments.

> > +	switch (fpc->counter_clk_select) {
> > +	case VF610_CLK_FTM0:
> > +		reg |= FTMSC_CLKSYS;
> > +		break;
> > +	case VF610_CLK_FTM0_FIX_SEL:
> > +		reg |= FTMSC_CLKFIX;
> > +		break;
> > +	case VF610_CLK_FTM0_EXT_SEL:
> > +		reg |= FTMSC_CLKEXT;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +	reg |= fpc->clk_ps;
> 
> And another one above this line.
> 
> > +	writel(reg, fpc->base + FTM_SC);
> 
> I think with the proper locking in place what you should do is increment
> counter_clk_enable only here. That makes avoids having to decrement the
> count on error.
> 
> Similarly in fsl_counter_clock_disable() you can postpone decrementing
> the count until the very end.
> 

As the other mails we have talked about this that there are 8 channels
supported, but they share the same counter clock source. So we need to
make sure that when one channel is calling fsl_counter_clock_disable()
it shouldn't disable the counter clock if any other channel is still
enabled. Similary in fsl_counter clock_enable().

This is why I set counter_clk_enable property here.


--
Best Regards,

RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Li Xiubo]

> > +#define FTM_CNTIN_VAL       0x00
> 
> Do we really need this?
> 

Maybe not, I think that the initial value maybe modified in the future.
And this can be more easy to ajust it. 


> > +	period_cycles = fsl_rate_to_cycles(fpc, period_ns);
> > +	if (period_cycles > 0xFFFF) {
> > +		dev_err(chip->dev, "required PWM period cycles(%lu) overflow "
> > +				"16-bits counter!\n", period_cycles);
> > +		return -EINVAL;
> > +	}
> > +
> > +	duty_cycles = fsl_rate_to_cycles(fpc, duty_ns);
> > +	if (duty_cycles >= 0xFFFF) {
> > +		dev_err(chip->dev, "required PWM duty cycles(%lu) overflow "
> > +				"16-bits counter!\n", duty_cycles);
> > +		return -EINVAL;
> > +	}
> 
> I'm not sure the error messages are all that useful. A -EINVAL error
> code should make it pretty clear what the problem is.
> 

Yes, these could be absent.

> > +	writel(FTMCnSC_MSB | FTMCnSC_ELSB, fpc->base + FTM_CSC(pwm->hwpwm));
> > +
> > +	writel(0xF0, fpc->base + FTM_OUTMASK);
> > +	writel(0x0F, fpc->base + FTM_OUTINIT);
> 
> The purpose of this eludes me. These seem to be global (not specific to
> channel pwm->hwpwm) registers, so why are they updated whenever a single
> channel is reconfigured?
> 

Well, certainly there has one way to update per channel's configuration 
about this. I will revise it then.


> > +	writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> > +	writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
> 
> And these:
> 
> 	writel(period - 1, fpc->base + FTM_MOD);
> 	writel(duty, fpc->base + FTM_CV(pwm->hwpwm));
> 
> Although now that I think about it, this seems broken. The period is set
> in a global register, so I assume it is valid for all channels. What if
> you want to use different periods for individual channels? The way I
> read this the last one to be configured will win and change the period
> to whatever it wants. Other channels won't even notice.
> 

That's right. And all the 8 channels share the same period settings.

> Is there a way to set the period per channel?
> 

Not yet. Only could we do is to set the duty value individually for each
channel. So here is a limitation for the cusumers that all the 8 channels'
period values should be the same.



> > +static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
> > +{
> > +	int ret;
> > +	unsigned long reg;
> > +
> > +	if (fpc->counter_clk_enable++)
> > +		return 0;
> 
> Are you sure this is safe? I think you'll need to use either an atomic
> or a mutex to lock this.
> 

Maybe a mutex lock is a good choice.


> > +	ret = clk_prepare_enable(fpc->counter_clk);
> > +	if (ret)
> > +		return ret;
> 
> In case clk_prepare_enable() fails, the counter_clk_enable will need to
> be decremented in order to track the state correctly, doesn't it?
> 

Yes, it should be.


> > +static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct fsl_pwm_chip *fpc;
> > +
> > +	fpc = to_fsl_chip(chip);
> > +
> > +	fsl_counter_clock_enable(fpc);
> 
> This can fail. Should the error be propagated?
>

That's better.
 
> > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device
> *pwm)
> > +{
> > +	struct fsl_pwm_chip *fpc;
> > +
> > +	fpc = to_fsl_chip(chip);
> > +
> > +	fsl_counter_clock_disable(fpc);
> > +}
> 
> Same here. Since you can't propagate the error, perhaps an error message
> would be appropriate here?
> 

Well, in fsl_counter_clock_disable(fpc); only '0' could be returned, maybe
let it a void type value to return is better.
Just like:
static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) {}


> Also for the locking above, perhaps a good solution would be to acquire
> the lock around the calls to fsl_counter_clock_{enable,disable}() so
> that they can safely assume that they are called with the lock held,
> which will make their implementation a lot simpler.
> 
> So what you could do is this:
> 
> 	static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device
> *pwm)
> 	{
> 		struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> 		int ret;
> 
> 		mutex_lock(&fpc->lock);
> 		ret = fsl_counter_clock_enable(fpc);
> 		mutex_unlock(&fpc->lock);
> 
> 		return ret;
> 	}
> 
> And analogously for fsl_pwm_disable().
>

I will think about this.

> 
> > +static int fsl_pwm_calculate_ps(struct fsl_pwm_chip *fpc)
> > +{
> > +	unsigned long long sys_rate, counter_rate, ratio;
> > +
> > +	sys_rate = clk_get_rate(fpc->sys_clk);
> > +	if (!sys_rate)
> > +		return -EINVAL;
> > +
> > +	counter_rate = clk_get_rate(fpc->counter_clk);
> > +	if (!counter_rate) {
> > +		fpc->counter_clk = fpc->sys_clk;
> > +		fpc->counter_clk_select = VF610_CLK_FTM0;
> > +		dev_warn(fpc->chip.dev,
> > +				"the counter source clock is a dummy clock, "
> > +				"so select the system clock as default!\n");
> > +	}
> > +
> > +	switch (fpc->counter_clk_select) {
> > +	case VF610_CLK_FTM0_FIX_SEL:
> > +		ratio = 2 * counter_rate - 1;
> > +		do_div(ratio, sys_rate);
> > +		fpc->clk_ps = ratio;
> > +		break;
> > +	case VF610_CLK_FTM0_EXT_SEL:
> > +		ratio = 4 * counter_rate - 1;
> > +		do_div(ratio, sys_rate);
> > +		fpc->clk_ps = ratio;
> > +		break;
> > +	case VF610_CLK_FTM0:
> > +		fpc->clk_ps = 7;
> 
> Even though it doesn't matter here, you should still add a break.
> Otherwise if you ever modify the code in the default case, you don't
> have to remember to add it in then.
> 
You are right, adding one break is much safer.

--
Best Rwgards,

Re: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

On Fri, Nov 29, 2013 at 06:42:06AM +0000, Li Xiubo wrote:
> > > +#define FTM_CNTIN_VAL       0x00
> > 
> > Do we really need this?
> > 
> 
> Maybe not, I think that the initial value maybe modified in the future.
> And this can be more easy to ajust it. 

Why would it need to be modified?

> > > +	writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> > > +	writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
> > 
> > And these:
> > 
> > 	writel(period - 1, fpc->base + FTM_MOD);
> > 	writel(duty, fpc->base + FTM_CV(pwm->hwpwm));
> > 
> > Although now that I think about it, this seems broken. The period is set
> > in a global register, so I assume it is valid for all channels. What if
> > you want to use different periods for individual channels? The way I
> > read this the last one to be configured will win and change the period
> > to whatever it wants. Other channels won't even notice.
> > 
> 
> That's right. And all the 8 channels share the same period settings.
> 
> > Is there a way to set the period per channel?
> > 
> 
> Not yet. Only could we do is to set the duty value individually for each
> channel. So here is a limitation for the cusumers that all the 8 channels'
> period values should be the same.

That will need to be handled some way. Perhaps the easiest would be to
check whether or not another channel is already running and check that
indeed the period as requested by the new channel matches that of any
running channels. If it doesn't match, then pwm_config() should fail.

When you do that you can also optimize this a bit by only setting the
frequency once. Whenever a new channel is configured it will have the
same period and therefore the register doesn't need to be reprogrammed.

> > > +static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device
> > *pwm)
> > > +{
> > > +	struct fsl_pwm_chip *fpc;
> > > +
> > > +	fpc = to_fsl_chip(chip);
> > > +
> > > +	fsl_counter_clock_disable(fpc);
> > > +}
> > 
> > Same here. Since you can't propagate the error, perhaps an error message
> > would be appropriate here?
> > 
> 
> Well, in fsl_counter_clock_disable(fpc); only '0' could be returned, maybe
> let it a void type value to return is better.
> Just like:
> static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) {}

Yes, that sounds good.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Li Xiubo]

> > > > +#define FTM_CNTIN_VAL       0x00
> > >
> > > Do we really need this?
> > >
> >
> > Maybe not, I think that the initial value maybe modified in the future.
> > And this can be more easy to ajust it.
> 
> Why would it need to be modified?
> 

Well, for the PWM function modifying it make no sense, so I'll remove this. 


> > > > +	writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> > > > +	writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
> > >
> > > And these:
> > >
> > > 	writel(period - 1, fpc->base + FTM_MOD);
> > > 	writel(duty, fpc->base + FTM_CV(pwm->hwpwm));
> > >
> > > Although now that I think about it, this seems broken. The period is
> > > set in a global register, so I assume it is valid for all channels.
> > > What if you want to use different periods for individual channels?
> > > The way I read this the last one to be configured will win and
> > > change the period to whatever it wants. Other channels won't even
> notice.
> > >
> >
> > That's right. And all the 8 channels share the same period settings.
> >
> > > Is there a way to set the period per channel?
> > >
> >
> > Not yet. Only could we do is to set the duty value individually for
> > each channel. So here is a limitation for the cusumers that all the 8
> channels'
> > period values should be the same.
> 
> That will need to be handled some way. Perhaps the easiest would be to
> check whether or not another channel is already running and check that
> indeed the period as requested by the new channel matches that of any
> running channels. If it doesn't match, then pwm_config() should fail.
> 
> When you do that you can also optimize this a bit by only setting the
> frequency once. Whenever a new channel is configured it will have the same
> period and therefore the register doesn't need to be reprogrammed.
> 

Yes, if it dosen't match it should fail, or nothing to do with the period
register if it's not the first channel to be configured.


> > > > +static void fsl_pwm_disable(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > *pwm)
> > > > +{
> > > > +	struct fsl_pwm_chip *fpc;
> > > > +
> > > > +	fpc = to_fsl_chip(chip);
> > > > +
> > > > +	fsl_counter_clock_disable(fpc);
> > > > +}
> > >
> > > Same here. Since you can't propagate the error, perhaps an error
> > > message would be appropriate here?
> > >
> >
> > Well, in fsl_counter_clock_disable(fpc); only '0' could be returned,
> > maybe let it a void type value to return is better.
> > Just like:
> > static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) 

RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

[Li.Xiubo]

> > +	struct fsl_pwm_chip *fpc;
> > +
> > +	fpc = to_fsl_chip(chip);
> > +
> > +	period_cycles = fsl_rate_to_cycles(fpc, period_ns);
> > +	if (period_cycles > 0xFFFF) {
> > +		dev_err(chip->dev, "required PWM period cycles(%lu) overflow "
> > +				"16-bits counter!\n", period_cycles);
> > +		return -EINVAL;
> > +	}
> > +
> > +	duty_cycles = fsl_rate_to_cycles(fpc, duty_ns);
> > +	if (duty_cycles >= 0xFFFF) {
> > +		dev_err(chip->dev, "required PWM duty cycles(%lu) overflow "
> > +				"16-bits counter!\n", duty_cycles);
> > +		return -EINVAL;
> > +	}
> 
> I'm not sure the error messages are all that useful. A -EINVAL error code
> should make it pretty clear what the problem is.
> 

Yes, it is.

But for the pwm leds, there hasn't any check about the return values, if
there is something wrong, we cannot get any information about this.

So I think this error messages are useful for this case.


--
Xiubo

RE: [PATCHv6 0/4] Add Freescale FTM PWM driver.

[Li Xiubo]

Hi Thierry,

Could you help review this series of patches please ? 

And could it be merged into the PWM next tree?

Thanks very much.

--


> Subject: [PATCHv6 0/4] Add Freescale FTM PWM driver.
> 
> Hello Thierry,
> 
> The series of patches for this version has been reviewed and acked
> already.
> To see each patch for detail.
> 
> 
> ------------------
> ------------------
> 
> This patch series is the Freescale FTM PWM implementation. And there are
> 8 channels most supported by the FTM PWM. This implementation is only
> compatible with device tree definition.
> 
> This patch series is based on linux-next and has been tested on Vybrid
> VF610 Tower board using device tree.
> 
> 
> 
> Changes in v6:
> - Nothing.
> 
> Changes in RFCv2 of PATVHv5:
> - Remove "fsl,pwm-counter-clk".
> 
> Changes in v5:
> - Remove active/idle pinctrl stuff.
> 
> Changes in v4:
> - Check for the result and return an error for devm_kzalloc().
> - Move pinmux setting from the SoC file to the board specific file.
> - Revise the written mistake of 'ret |= FTMSC_CLKEXT;' --> 'reg |=
> FTMSC_CLKEXT;'.
> 
> 
> Changes in v3:
> - Remove "availabe" field.
> - Remove "fsl,pwm-avaliable-chs" property.
> - ...
> 
> Changes in v2:
> - Remove PWM CPWM/EPWM feature and sysfs.
> - Remove some redundant code.
> - Revise some code for more readable.
> - Remove "fsl,pwm-clk-ps", "fsl,pwm-number", "fsl,pwm-channels",etc.
> - Add "fsl,pwm-avaliable-chs", "fsl,pwm-counter-clk", etc.
> - Support 8 channels default in dtsi file.
> - Add counter clock source selection.
> - Rename some function name, macro name, etc.
> - Use PWM's and OF's existing function interfaces.
> - Split clk_unprepare_enable to clk_unprepare and clk_enable,etc.
> - ...
> 
> Added in v1:
> - Add Freescale FTM PWM driver support.
> - Add Freescale FTM PWM node for VF610.
> - Enable Enables FTM PWM device for Vybrid VF610 TOWER.
> - Add device tree bindings for Freescale.
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv6 2/4] ARM: dts: Add Freescale FTM PWM node for VF610.

[Xiubo Li]

This adds devicetree node for VF610, and there are 8 channels supported
by default.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boot/dts/vf610.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 67d929c..ec12397 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -140,6 +140,16 @@
 				clock-names = "pit";
 			};
 
+			pwm0: pwm@40038000 {
+				compatible = "fsl,vf610-ftm-pwm";
+				#pwm-cells = <3>;
+				reg = <0x40038000 0x1000>;
+				clock-names = "ftm0", "ftm0_counter";
+				clocks = <&clks VF610_CLK_FTM0>,
+					<&clks VF610_CLK_FTM0>;
+				status = "disabled";
+			};
+
 			wdog@4003e000 {
 				compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
 				reg = <0x4003e000 0x1000>;
-- 
1.8.4

Re: [PATCHv6 2/4] ARM: dts: Add Freescale FTM PWM node for VF610.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

On Tue, Nov 12, 2013 at 09:36:56AM +0800, Xiubo Li wrote:
> This adds devicetree node for VF610, and there are 8 channels supported
> by default.

By default? From looking at the driver code it seemed that the device
always supports 8 channels.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCHv6 3/4] ARM: dts: Enables FTM PWM device for Vybrid VF610 TOWER board.

[Xiubo Li]

Selecting system clock as the counter source clock by default.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boot/dts/vf610-twr.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index 82d352f..3130f85 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -83,6 +83,12 @@
 	status = "okay";
 };
 
+&pwm0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm0_1>;
+	status = "okay";
+};
+
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1_1>;
-- 
1.8.4

[PATCHv6 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.

[Xiubo Li]

This adds the Document for Freescale FTM PWM driver under
Documentation/devicetree/bindings/pwm/.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Acked-by: Kumar Gala <galak@codeaurora.org>
---
 .../devicetree/bindings/pwm/pwm-fsl-ftm.txt        | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
new file mode 100644
index 0000000..5c1cd2b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
@@ -0,0 +1,29 @@
+Freescale FlexTimer Module(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. See pwm.txt in this directory for a description of
+  the cells format.
+- clock-names : Should includes the following module clock source entries:
+    "ftm0" (module clock),
+    "ftm0_counter" (counter clock),
+- clocks : Must contain a clock specifier for each entry in clock-names,
+  See clock/clock-bindings.txt for details of the property values.
+- pinctrl-names: must contain a "default" entry.
+- pinctrl-NNN: One property must exist for each entry in pinctrl-names.
+  See pinctrl/pinctrl-bindings.txt for details of the property values.
+
+
+Example:
+
+pwm0: pwm@40038000 {
+		compatible = "fsl,vf610-ftm-pwm";
+		reg = <0x40038000 0x1000>;
+		#pwm-cells = <3>;
+		clock-names = "ftm0", "ftm0_counter;
+		clocks = <&clks VF610_CLK_FTM0>,
+			<&clks VF610_CLK_FTM0_FIX_SEL>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_pwm0_1>;
+};
-- 
1.8.4

Re: [PATCHv6 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Tue, Nov 12, 2013 at 09:36:58AM +0800, Xiubo Li wrote:
> This adds the Document for Freescale FTM PWM driver under

s/Document/binding documentation/?

> Documentation/devicetree/bindings/pwm/.
> 
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Acked-by: Kumar Gala <galak@codeaurora.org>
> ---
>  .../devicetree/bindings/pwm/pwm-fsl-ftm.txt        | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> new file mode 100644
> index 0000000..5c1cd2b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-fsl-ftm.txt
> @@ -0,0 +1,29 @@
> +Freescale FlexTimer Module(FTM) PWM controller

I think it's customary to have a space before the opening parenthesis.

> +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. See pwm.txt in this directory for a description of
> +  the cells format.
> +- clock-names : Should includes the following module clock source entries:

s/includes/include/

> +    "ftm0" (module clock),
> +    "ftm0_counter" (counter clock),
> +- clocks : Must contain a clock specifier for each entry in clock-names,
> +  See clock/clock-bindings.txt for details of the property values.

Note that the order is significant here, at least from the way the
driver currently implements this. You can probably make this more
generic in the driver by using of_property_match_string() on the
clock-names property to find the index of "ftm0_counter" and pass that
index to the of_parse_phandle_with_args() function to cope with the
situation where the device tree has the clocks in a different order.

> +Example:
> +
> +pwm0: pwm@40038000 {
> +		compatible = "fsl,vf610-ftm-pwm";
> +		reg = <0x40038000 0x1000>;
> +		#pwm-cells = <3>;
> +		clock-names = "ftm0", "ftm0_counter;

There's a missing " at the end here.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCHv6 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.

[Li Xiubo]

> 
> > +    "ftm0" (module clock),
> > +    "ftm0_counter" (counter clock),
> > +- clocks : Must contain a clock specifier for each entry in
> > +clock-names,
> > +  See clock/clock-bindings.txt for details of the property values.
> 
> Note that the order is significant here, at least from the way the driver
> currently implements this. You can probably make this more generic in the
> driver by using of_property_match_string() on the clock-names property to
> find the index of "ftm0_counter" and pass that index to the
> of_parse_phandle_with_args() function to cope with the situation where the
> device tree has the clocks in a different order.
> 

Yes, that's much better.

RE: [PATCHv6 4/4] Documentation: Add device tree bindings for Freescale FTM PWM.

[Li Xiubo]

> > +    "ftm0" (module clock),
> > +    "ftm0_counter" (counter clock),
> > +- clocks : Must contain a clock specifier for each entry in
> > +clock-names,
> > +  See clock/clock-bindings.txt for details of the property values.
> 
> Note that the order is significant here, at least from the way the driver
> currently implements this. You can probably make this more generic in the
> driver by using of_property_match_string() on the clock-names property to
> find the index of "ftm0_counter" and pass that index to the
> of_parse_phandle_with_args() function to cope with the situation where the
> device tree has the clocks in a different order.
> 

Yes, this's much better. As this is still under discussing, I will improve
about this after that.

And I will also revise other as you have piontet out.

--
Best Regards,


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html