[PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support

[PATCHv8 RFC] 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@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>
---

Hi Thierry, Bill

In this patch series only this one has been changed. I'm sending this
patch for your comments.

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 v8 RFC:
- Remove ftm_readl/ftm_writel.
- Add pwm-fsl-ftm.h file.

Changes in v8:
- Fix some issues pointed by Thierry.
- Fix the _readl/_writel of sparse check.

Changes in v7:
- Add big-endian mode support.
- Add FTM mutex lock.
- Add period time check with the current running pwm(s).
- Recode the counter clock source selecting.
- Sort some header files alphabetically, etc.

[snip] v1~v6




 drivers/pwm/Kconfig       |  10 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-fsl-ftm.c | 426 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pwm/pwm-fsl-ftm.h | 101 +++++++++++
 4 files changed, 538 insertions(+)
 create mode 100644 drivers/pwm/pwm-fsl-ftm.c
 create mode 100644 drivers/pwm/pwm-fsl-ftm.h

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 3f66427..ec4bf78 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -71,6 +71,16 @@ config PWM_EP93XX
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-ep93xx.
 
+config PWM_FSL_FTM
+	tristate "Freescale FlexTimer Module (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 8b754e4..b335db1 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
 obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
 obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
+obj-$(CONFIG_PWM_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..39093e5
--- /dev/null
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -0,0 +1,426 @@
+/*
+ *  Freescale FlexTimer Module (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/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+#include "pwm-fsl-ftm.h"
+
+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)
+{
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+
+	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 = to_fsl_chip(chip);
+
+	clk_disable_unprepare(fpc->sys_clk);
+}
+
+static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
+					       enum fsl_pwm_clk index)
+{
+	unsigned long sys_rate, cnt_rate;
+	unsigned long long ratio;
+
+	sys_rate = clk_get_rate(fpc->sys_clk);
+	if (!sys_rate)
+		return -EINVAL;
+
+	cnt_rate = clk_get_rate(fpc->counter_clk);
+	if (!cnt_rate)
+		return -EINVAL;
+
+	switch (index) {
+	case FSL_PWM_CLK_SYS:
+		fpc->clk_ps = 1;
+		break;
+	case FSL_PWM_CLK_FIX:
+		ratio = 2 * cnt_rate - 1;
+		do_div(ratio, sys_rate);
+		fpc->clk_ps = ratio;
+		break;
+	case FSL_PWM_CLK_EXT:
+		ratio = 4 * cnt_rate - 1;
+		do_div(ratio, sys_rate);
+		fpc->clk_ps = ratio;
+		break;
+	}
+
+	return 0;
+}
+
+static inline unsigned long fsl_pwm_calculate_cycles(struct fsl_pwm_chip *fpc,
+						     unsigned long period_ns)
+{
+	unsigned long long c, c0;
+
+	c = clk_get_rate(fpc->counter_clk);
+	c = c * period_ns;
+	do_div(c, 1000000000UL);
+
+	do {
+		c0 = c;
+		do_div(c0, (1 << fpc->clk_ps));
+		if (c0 <= 0xFFFF)
+			return (unsigned long)c0;
+	} while (++fpc->clk_ps < 8);
+
+	return 0;
+}
+
+static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip *fpc,
+						     unsigned long period_ns,
+						     enum fsl_pwm_clk index)
+{
+	bool bg = fpc->big_endian;
+	int ret;
+
+	fpc->counter_clk_select = FTM_SC_CLK(bg, index);
+
+	ret = fsl_pwm_calculate_default_ps(fpc, index);
+	if (ret) {
+		dev_err(fpc->chip.dev, "failed to calculate default "
+				"prescaler: %d\n", ret);
+		return 0;
+	}
+
+	return fsl_pwm_calculate_cycles(fpc, period_ns);
+}
+
+static unsigned long fsl_pwm_calculate_period(struct fsl_pwm_chip *fpc,
+					      unsigned long period_ns)
+{
+	struct clk *cnt_clk[3];
+	enum fsl_pwm_clk m0, m1;
+	unsigned long fix_rate, ext_rate, cycles;
+
+	fpc->counter_clk = fpc->sys_clk;
+	cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns,
+			FSL_PWM_CLK_SYS);
+	if (cycles)
+		return cycles;
+
+	cnt_clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
+	if (IS_ERR(cnt_clk[FSL_PWM_CLK_FIX]))
+		return PTR_ERR(cnt_clk[FSL_PWM_CLK_FIX]);
+
+	cnt_clk[FSL_PWM_CLK_EXT] = devm_clk_get(fpc->chip.dev, "ftm_ext");
+	if (IS_ERR(cnt_clk[FSL_PWM_CLK_EXT]))
+		return PTR_ERR(cnt_clk[FSL_PWM_CLK_EXT]);
+
+	fpc->counter_clk_en = devm_clk_get(fpc->chip.dev, "ftm_cnt_clk_en");
+	if (IS_ERR(fpc->counter_clk_en))
+		return PTR_ERR(fpc->counter_clk_en);
+
+	fix_rate = clk_get_rate(cnt_clk[FSL_PWM_CLK_FIX]);
+	ext_rate = clk_get_rate(cnt_clk[FSL_PWM_CLK_EXT]);
+
+	if (fix_rate > ext_rate) {
+		m0 = FSL_PWM_CLK_FIX;
+		m1 = FSL_PWM_CLK_EXT;
+	} else {
+		m0 = FSL_PWM_CLK_EXT;
+		m1 = FSL_PWM_CLK_FIX;
+	}
+
+	fpc->counter_clk = cnt_clk[m0];
+	cycles = fsl_pwm_calculate_period_cycles(fpc, period_ns, m0);
+	if (cycles)
+		return cycles;
+
+	fpc->counter_clk = cnt_clk[m1];
+
+	return fsl_pwm_calculate_period_cycles(fpc, period_ns, m0);
+}
+
+static unsigned long fsl_pwm_calculate_duty(struct fsl_pwm_chip *fpc,
+					    unsigned long period_ns,
+					    unsigned long duty_ns)
+{
+	unsigned long long val, duty;
+
+	val = readl(fpc->base + FTM_MOD);
+	duty = duty_ns * (val + 1);
+	do_div(duty, period_ns);
+
+	return (unsigned long)duty;
+}
+
+static int fsl_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			  int duty_ns, int period_ns)
+{
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+	bool bg = fpc->big_endian;
+	u32 val, period, duty;
+
+	mutex_lock(&fpc->lock);
+
+	/*
+	 * The Freescale FTM controller supports only a single period for
+	 * all PWM channels, therefore incompatible changes need to be
+	 * refused.
+	 */
+	if (fpc->period_ns && fpc->period_ns != period_ns) {
+		dev_err(fpc->chip.dev,
+				"conflicting period requested for PWM %u\n",
+				pwm->hwpwm);
+		mutex_unlock(&fpc->lock);
+		return -EBUSY;
+	}
+
+	if (!fpc->period_ns && duty_ns) {
+		period = fsl_pwm_calculate_period(fpc, period_ns);
+		if (!period) {
+			dev_err(fpc->chip.dev, "failed to calculate period\n");
+			mutex_unlock(&fpc->lock);
+			return -EINVAL;
+		}
+
+		val = readl(fpc->base + FTM_SC);
+		val &= ~FTM_SC_PS_MASK(bg);
+		val |= fpc->clk_ps << FTM_SC_PS_SHIFT(bg);
+		writel(val, fpc->base + FTM_SC);
+		writel(FTM_SWAP32(bg, period - 1), fpc->base + FTM_MOD);
+
+		fpc->period_ns = period_ns;
+	}
+
+	mutex_unlock(&fpc->lock);
+
+	duty = fsl_pwm_calculate_duty(fpc, period_ns, duty_ns);
+
+	writel((FTM_CSC_MSB(bg) | FTM_CSC_ELSB(bg)),
+			fpc->base + FTM_CSC(pwm->hwpwm));
+	writel(FTM_SWAP32(bg, duty), 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)
+{
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+	bool bg = fpc->big_endian;
+	u32 val;
+
+	val = readl(fpc->base + FTM_POL);
+
+	if (polarity == PWM_POLARITY_INVERSED)
+		val |= FTM_POL_CHAN(bg, pwm->hwpwm);
+	else
+		val &= ~FTM_POL_CHAN(bg, pwm->hwpwm);
+
+	writel(val, fpc->base + FTM_POL);
+
+	return 0;
+}
+
+static int fsl_counter_clock_enable(struct fsl_pwm_chip *fpc)
+{
+	bool bg = fpc->big_endian;
+	u32 val;
+	int ret;
+
+	if (fpc->counter_clk_enable++)
+		return 0;
+
+	ret = clk_prepare_enable(fpc->counter_clk);
+	if (ret) {
+		fpc->counter_clk_enable--;
+		return ret;
+	}
+
+	ret = clk_prepare_enable(fpc->counter_clk_en);
+	if (ret) {
+		fpc->counter_clk_enable--;
+		return ret;
+	}
+
+	/* select counter clock source */
+	val = readl(fpc->base + FTM_SC);
+	val &= ~FTM_SC_CLK_MASK(bg);
+	val |= fpc->counter_clk_select;
+	writel(val, fpc->base + FTM_SC);
+
+	return 0;
+}
+
+static int fsl_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+	bool bg = fpc->big_endian;
+	u32 val;
+	int ret;
+
+	val = readl(fpc->base + FTM_OUTMASK);
+	val &= ~FTM_OUTMASK_CHAN(bg, pwm->hwpwm);
+	writel(val, fpc->base + FTM_OUTMASK);
+
+	mutex_lock(&fpc->lock);
+	ret = fsl_counter_clock_enable(fpc);
+	mutex_unlock(&fpc->lock);
+
+	return ret;
+}
+
+static inline void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc)
+{
+	bool bg = fpc->big_endian;
+	u32 val;
+
+	if (--fpc->counter_clk_enable)
+		return;
+
+	val = readl(fpc->base + FTM_SC);
+	val &= ~FTM_SC_CLK_MASK(bg);
+	writel(val, fpc->base + FTM_SC);
+
+	clk_disable_unprepare(fpc->counter_clk_en);
+	clk_disable_unprepare(fpc->counter_clk);
+}
+
+static void fsl_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
+	bool bg = fpc->big_endian;
+	u32 val;
+
+	val = readl(fpc->base + FTM_OUTMASK);
+	val |= FTM_OUTMASK_CHAN(bg, pwm->hwpwm);
+	writel(val, fpc->base + FTM_OUTMASK);
+
+	mutex_lock(&fpc->lock);
+
+	fsl_counter_clock_disable(fpc);
+
+	val = readl(fpc->base + FTM_OUTMASK);
+
+	if ((val & FTM_OUTMASK_CHAN_MASK(bg)) == FTM_OUTMASK_CHAN_MASK(bg)) {
+		fpc->period_ns = 0;
+		fpc->counter_clk_en = NULL;
+	}
+
+	mutex_unlock(&fpc->lock);
+}
+
+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_probe(struct platform_device *pdev)
+{
+	struct fsl_pwm_chip *fpc;
+	struct resource *res;
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	fpc = devm_kzalloc(&pdev->dev, sizeof(*fpc), GFP_KERNEL);
+	if (!fpc)
+		return -ENOMEM;
+
+	mutex_init(&fpc->lock);
+
+	fpc->chip.dev = &pdev->dev;
+
+	fpc->big_endian = of_property_read_bool(np, "big-endian");
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	fpc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(fpc->base))
+		return PTR_ERR(fpc->base);
+
+	fpc->sys_clk = devm_clk_get(&pdev->dev, "ftm_sys");
+	if (IS_ERR(fpc->sys_clk)) {
+		dev_err(&pdev->dev,
+				"failed to get \"ftm_sys\" clock\n");
+		return PTR_ERR(fpc->sys_clk);
+	}
+
+	ret = clk_prepare_enable(fpc->sys_clk);
+	if (ret)
+		return ret;
+
+	writel(0x0, fpc->base + FTM_CNTIN);
+	writel(0x0, fpc->base + FTM_OUTINIT);
+	writel(FTM_OUTMASK_CHAN_MASK(fpc->big_endian),
+		fpc->base + FTM_OUTMASK);
+	clk_disable_unprepare(fpc->sys_clk);
+
+	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 = 8;
+
+	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 = platform_get_drvdata(pdev);
+
+	mutex_destroy(&fpc->lock);
+
+	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",
+		.of_match_table = fsl_pwm_dt_ids,
+	},
+	.probe = fsl_pwm_probe,
+	.remove = fsl_pwm_remove,
+};
+module_platform_driver(fsl_pwm_driver);
+
+MODULE_DESCRIPTION("Freescale FlexTimer Module PWM Driver");
+MODULE_AUTHOR("Xiubo Li <Li.Xiubo@freescale.com>");
+MODULE_ALIAS("platform:fsl-ftm-pwm");
+MODULE_LICENSE("GPL");
diff --git a/drivers/pwm/pwm-fsl-ftm.h b/drivers/pwm/pwm-fsl-ftm.h
new file mode 100644
index 0000000..5fb79ae
--- /dev/null
+++ b/drivers/pwm/pwm-fsl-ftm.h
@@ -0,0 +1,101 @@
+/*
+ *  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.
+ */
+
+#ifndef __PWM_FSL_FTM_H
+#define __PWM_FSL_FTM_H
+
+#include <linux/clk.h>
+#include <linux/mutex.h>
+#include <linux/pwm.h>
+
+#define FTM_SC		0x00
+#define FTM_SC_CLK_SHIFT(b)	(b ? 27 : 3)
+#define FTM_SC_CLK_MASK(b)	(0x3 << FTM_SC_CLK_SHIFT(b))
+#define FTM_SC_CLK(b, c)	((c + 1) << FTM_SC_CLK_SHIFT(b))
+#define FTM_SC_PS_SHIFT(b)	(b ? 24 : 0)
+#define FTM_SC_PS_MASK(b)	(0x7 << FTM_SC_PS_SHIFT(b))
+
+#define FTM_CNT		0x04
+#define FTM_MOD		0x08
+
+#define FTM_CSC_BASE	0x0C
+#define FTM_CSC_MSB(b)	(b ? BIT(29) : BIT(5))
+#define FTM_CSC_MSA(b)	(b ? BIT(28) : BIT(4))
+#define FTM_CSC_ELSB(b)	(b ? BIT(27) : BIT(3))
+#define FTM_CSC_ELSA(b)	(b ? BIT(26) : BIT(2))
+#define FTM_CSC(_channel)	(FTM_CSC_BASE + ((_channel) * 8))
+
+#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 FTM_MODE_FTMEN(b)	(b ? BIT(24) : BIT(0))
+#define FTM_MODE_INIT(b)	(b ? BIT(26) : BIT(2))
+#define FTM_MODE_PWMSYNC(b)	(b ? BIT(27) : BIT(3))
+
+#define FTM_SYNC	0x58
+#define FTM_OUTINIT	0x5C
+
+#define FTM_OUTMASK	0x60
+#define FTM_OUTMASK_CHAN_MASK(b)	(b ? 0xFF00 : 0xFF)
+#define FTM_OUTMASK_CHAN(b, c)	(b ? BIT(24 + c) : BIT(c))
+
+#define FTM_COMBINE	0x64
+#define FTM_DEADTIME	0x68
+#define FTM_EXTTRIG	0x6C
+
+#define FTM_POL		0x70
+#define FTM_POL_CHAN(b, c)	(b ? BIT(24 + c) : BIT(c))
+
+#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_SWAP32(v) ((u32)(\
+	(((u32)(v) & (u32)0x000000ffUL) << 24) |\
+	(((u32)(v) & (u32)0x0000ff00UL) <<  8) |\
+	(((u32)(v) & (u32)0x00ff0000UL) >>  8) |\
+	(((u32)(v) & (u32)0xff000000UL) >> 24)))
+#define FTM_SWAP32(b, v)	(b ? __FTM_SWAP32(v) : v)
+
+enum fsl_pwm_clk {
+	FSL_PWM_CLK_SYS,
+	FSL_PWM_CLK_FIX,
+	FSL_PWM_CLK_EXT,
+};
+
+struct fsl_pwm_chip {
+	struct pwm_chip chip;
+
+	struct mutex lock;
+
+	struct clk *sys_clk;
+	struct clk *counter_clk;
+	struct clk *counter_clk_en;
+	unsigned int counter_clk_select;
+	unsigned int counter_clk_enable;
+	unsigned int clk_ps;
+
+	void __iomem *base;
+
+	int period_ns;
+	bool big_endian;
+};
+
+#endif /* __PWM_FSL_FTM_H */
-- 
1.8.4

Re: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support

[Dmitry Torokhov]

Hi Xiubo,

On Fri, Jan 03, 2014 at 01:24:21PM +0800, Xiubo Li wrote:
> +
> +static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
> +					       enum fsl_pwm_clk index)
> +{

Why do you declare this (and other module-local) function as inline?
It is usually better let compiler decide if given function should be
inlined or not.

[...]

> +
> +static int fsl_pwm_remove(struct platform_device *pdev)
> +{
> +	struct fsl_pwm_chip *fpc = platform_get_drvdata(pdev);
> +
> +	mutex_destroy(&fpc->lock);
> +
> +	return pwmchip_remove(&fpc->chip);

fpc->lock will be used while pwmchip_remove() is running so you should
not be destroying it before calling pwmchip_remove(). It should probbaly
go into free() method or just drop it altogether.

Thanks.

-- 
Dmitry

RE: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

Hi Dmitry,

> Subject: Re: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support
> 
> Hi Xiubo,
> 
> On Fri, Jan 03, 2014 at 01:24:21PM +0800, Xiubo Li wrote:
> > +
> > +static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
> > +					       enum fsl_pwm_clk index)
> > +{
> 
> Why do you declare this (and other module-local) function as inline?
> It is usually better let compiler decide if given function should be
> inlined or not.
> 

Could the compiler know when the given function should be inlined by not declare it?
And when will be inlined ?

> [...]
> 
> > +
> > +static int fsl_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct fsl_pwm_chip *fpc = platform_get_drvdata(pdev);
> > +
> > +	mutex_destroy(&fpc->lock);
> > +
> > +	return pwmchip_remove(&fpc->chip);
> 
> fpc->lock will be used while pwmchip_remove() is running so you should
> not be destroying it before calling pwmchip_remove(). It should probbaly
> go into free() method or just drop it altogether.
> 

Yes, I think I should drop it here as Thierry has already pointed out.


Thanks very much.

--
Best Regards,
Xiubo

--
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: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support

[Dmitry Torokhov]

On Fri, Jan 03, 2014 at 09:16:06AM +0000, Li.Xiubo@freescale.com wrote:
> Hi Dmitry,
> 
> > Subject: Re: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support
> > 
> > Hi Xiubo,
> > 
> > On Fri, Jan 03, 2014 at 01:24:21PM +0800, Xiubo Li wrote:
> > > +
> > > +static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
> > > +					       enum fsl_pwm_clk index)
> > > +{
> > 
> > Why do you declare this (and other module-local) function as inline?
> > It is usually better let compiler decide if given function should be
> > inlined or not.
> > 
> 
> Could the compiler know when the given function should be inlined by
> not declare it?

Yes.

> And when will be inlined ?

When it makes sense to do so from compiler's point of view: i.e. it is
smallish or with single call site and there are enough registers, etc,
etc.

Thanks.

-- 
Dmitry

RE: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support

[Li.Xiubo]

> > > > +static inline int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
> > > > +					       enum fsl_pwm_clk index)
> > > > +{
> > >
> > > Why do you declare this (and other module-local) function as inline?
> > > It is usually better let compiler decide if given function should be
> > > inlined or not.
> > >
> >
> > Could the compiler know when the given function should be inlined by
> > not declare it?
> 
> Yes.
> 
> > And when will be inlined ?
> 
> When it makes sense to do so from compiler's point of view: i.e. it is
> smallish or with single call site and there are enough registers, etc,
> etc.
>

Well, the inline is not very useful here, I will revise this.


Thanks,

--
Best Regards,
Xiubo

Re: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support

[Bill Pringlemeir]

On  3 Jan 2014, Li.Xiubo@freescale.com 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>
> ---
>
> Hi Thierry, Bill

[snip]

> +static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip *fpc,
> +						     unsigned long period_ns,
> +						     enum fsl_pwm_clk index)
> +{
> +	bool bg = fpc->big_endian;
> +	int ret;
> +
> +	fpc->counter_clk_select = FTM_SC_CLK(bg, index);

Yes, this is the spirit of what I was suggesting.  The code is much less
efficient/bigger on the Vybrid with this run-time detection; but this is
more efficient/smaller than previous versions.  I think that 'bg' can be
a compiler '#define' base on the configured SOC-systems.  Ie, if the
kernel config only has 'Vybrid' or only 'LayerScape', then 'bg' can be a
hard coded value.  The compiler will produce much better code in these
cases.

Also, maybe 'distro' people may want to make a 'hand-held' (Debian) or a
'router' (OpenWRT) distribution and they would only pick either 'Vybrid'
or 'LayerScape'.  However, if someone wants an 'every ARM under the
sun', then the code still works.  So, I think that the code is better
setup for a subsequent patch set like this (or at least just a good).

Especially, the stuff on the I/O swapping in the 'readl()' and
'writel()' is no longer needed; I think you can use the same function
for both SOCs.

> +#define __FTM_SWAP32(v) ((u32)(\
> +	(((u32)(v) & (u32)0x000000ffUL) << 24) |\
> +	(((u32)(v) & (u32)0x0000ff00UL) <<  8) |\
> +	(((u32)(v) & (u32)0x00ff0000UL) >>  8) |\
> +	(((u32)(v) & (u32)0xff000000UL) >> 24)))
> +#define FTM_SWAP32(b, v)	(b ? __FTM_SWAP32(v) : v)

I think that there are macros that you could use here.  For instance,
'#include <linux/swab.h>' (powerpc and arm) has some assembler macros
that are quite fast for swapping.  If the kernel config has ARCH >= 6
for ARM, then the very fast 'rev' instruction is used.  If not, then a
generic version is used as you have coded.  The PowerPC (another
possible future ARCH for QorIQ/Layerscape SOC?) always has inline
assembler macros.

So,

+ #include <linux/swab.h>
...
+ #define FTM_SWAP32(b, v)	(b ? __swab32(v) : v)

might be better.

Suggested-by: Bill Pringlemeir <bpringlemeir@nbsps.com>

Thanks,
Bill Pringlemeir.

RE: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support

[Li.Xiubo]

> > +static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip
> *fpc,
> > +						     unsigned long period_ns,
> > +						     enum fsl_pwm_clk index)
> > +{
> > +	bool bg = fpc->big_endian;
> > +	int ret;
> > +
> > +	fpc->counter_clk_select = FTM_SC_CLK(bg, index);
> 
> Yes, this is the spirit of what I was suggesting.  The code is much less
> efficient/bigger on the Vybrid with this run-time detection; but this is
> more efficient/smaller than previous versions.  I think that 'bg' can be
> a compiler '#define' base on the configured SOC-systems.  Ie, if the
> kernel config only has 'Vybrid' or only 'LayerScape', then 'bg' can be a
> hard coded value.  The compiler will produce much better code in these
> cases.
> 
> Also, maybe 'distro' people may want to make a 'hand-held' (Debian) or a
> 'router' (OpenWRT) distribution and they would only pick either 'Vybrid'
> or 'LayerScape'.  However, if someone wants an 'every ARM under the
> sun', then the code still works.  So, I think that the code is better
> setup for a subsequent patch set like this (or at least just a good).
> 
> Especially, the stuff on the I/O swapping in the 'readl()' and
> 'writel()' is no longer needed; I think you can use the same function
> for both SOCs.
> 
> > +#define __FTM_SWAP32(v) ((u32)(\
> > +	(((u32)(v) & (u32)0x000000ffUL) << 24) |\
> > +	(((u32)(v) & (u32)0x0000ff00UL) <<  8) |\
> > +	(((u32)(v) & (u32)0x00ff0000UL) >>  8) |\
> > +	(((u32)(v) & (u32)0xff000000UL) >> 24)))
> > +#define FTM_SWAP32(b, v)	(b ? __FTM_SWAP32(v) : v)
> 
> I think that there are macros that you could use here.  For instance,
> '#include <linux/swab.h>' (powerpc and arm) has some assembler macros
> that are quite fast for swapping.  If the kernel config has ARCH >= 6
> for ARM, then the very fast 'rev' instruction is used.  If not, then a
> generic version is used as you have coded.  The PowerPC (another
> possible future ARCH for QorIQ/Layerscape SOC?) always has inline
> assembler macros.
> 
> So,
> 
> + #include <linux/swab.h>
> ...
> + #define FTM_SWAP32(b, v)	(b ? __swab32(v) : v)
> 
> might be better.
> 

Yes.

I have removed the big-endian support temporarily, and will send followed
patches about this.

 
Thanks,

--
Best Regards,
Xiubo