[PATCH v4 0/4] PWM: add and enable CSR SiRFSoC PWM driver

[PATCH v4 0/4] PWM: add and enable CSR SiRFSoC PWM driver

[Barry Song]

From: Barry Song <Baohua.Song@csr.com>

Barry Song (3):
  pwm: sirf: add dt-binding document
  ARM: dts: sirf: fix the pwm-cells and clocks for pwm
  ARM: prima2_defconfig: enable PWM and sirf PWM driver

Huayi Li (1):
  pwm: add CSR SiRFSoC PWM driver

 Documentation/devicetree/bindings/pwm/pwm-sirf.txt |   23 ++
 arch/arm/boot/dts/atlas6.dtsi                      |    5 +-
 arch/arm/boot/dts/prima2.dtsi                      |    6 +-
 arch/arm/configs/prima2_defconfig                  |    2 +
 drivers/pwm/Kconfig                                |    9 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-sirf.c                             |  329 ++++++++++++++++++++
 7 files changed, 371 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sirf.txt
 create mode 100644 drivers/pwm/pwm-sirf.c

-- 
1.7.5.4

[PATCH v4 1/4] pwm: add CSR SiRFSoC PWM driver

[Barry Song]

From: Huayi Li <Huayi.Li@csr.com>

PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output
duty cycle can be adjusted by setting the corresponding wait & hold registers.
There are 6 external channels (0 to 5) and 1 internal channel (6).
Supports a wide frequency range: the source clock divider can be from 2
up to 65536*2.

Signed-off-by: Huayi Li <Huayi.Li@csr.com>
Signed-off-by: Rongjun Ying <Rongjun.ying@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
 -v4: fix many issues from Thierry's feedbacks

 drivers/pwm/Kconfig    |    9 ++
 drivers/pwm/Makefile   |    1 +
 drivers/pwm/pwm-sirf.c |  329 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 339 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/pwm-sirf.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 22f2f28..f2165eb 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -176,6 +176,15 @@ config PWM_SAMSUNG
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-samsung.
 
+config PWM_SIRF
+	tristate "SiRF PWM support"
+	depends on (ARCH_SIRF || COMPILE_TEST) && COMMON_CLK && OF
+	help
+	  Generic PWM framework driver for SiRF SoC.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sirf.
+
 config PWM_SPEAR
 	tristate "STMicroelectronics SPEAr PWM support"
 	depends on PLAT_SPEAR
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index d8906ec..aa222eb 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
+obj-$(CONFIG_PWM_SIRF)		+= pwm-sirf.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
 obj-$(CONFIG_PWM_TIECAP)	+= pwm-tiecap.o
diff --git a/drivers/pwm/pwm-sirf.c b/drivers/pwm/pwm-sirf.c
new file mode 100644
index 0000000..b3ed87e
--- /dev/null
+++ b/drivers/pwm/pwm-sirf.c
@@ -0,0 +1,329 @@
+/*
+ * SIRF serial SoC PWM device core driver
+ *
+ * Copyright (c) 2014 Cambridge Silicon Radio Limited, a CSR plc group company.
+ *
+ * Licensed under GPLv2.
+ */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/pwm.h>
+#include <linux/of.h>
+#include <linux/io.h>
+
+#define SIRF_PWM_SELECT_PRECLK			0x0
+#define SIRF_PWM_OE				0x4
+#define SIRF_PWM_ENABLE_PRECLOCK		0x8
+#define SIRF_PWM_ENABLE_POSTCLOCK		0xC
+#define SIRF_PWM_GET_WAIT_OFFSET(n)		(0x10 + 0x8*n)
+#define SIRF_PWM_GET_HOLD_OFFSET(n)		(0x14 + 0x8*n)
+
+#define SIRF_PWM_TR_STEP(n)			(0x48 + 0x8*n)
+#define SIRF_PWM_STEP_HOLD(n)			(0x4c + 0x8*n)
+
+#define SRC_FIELD_SIZE				3
+#define BYPASS_MODE_BIT				21
+#define TRANS_MODE_SELECT_BIT			7
+
+struct sirf_pwm {
+	struct pwm_chip	chip;
+	struct mutex mutex;
+	void __iomem *base;
+	struct clk *pwmc_clk;
+	/*  select OSC(26MHz) as clock source to generate PWM signals */
+	struct clk *sigsrc_clk;
+	unsigned long sigsrc_clk_rate;
+};
+
+static inline struct sirf_pwm *to_sirf_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct sirf_pwm, chip);
+}
+
+static u32 sirf_pwm_ns_to_cycles(struct pwm_chip *chip, u32 time_ns)
+{
+	struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
+	u64 dividend;
+	u32 cycle;
+
+	dividend = (u64)spwm->sigsrc_clk_rate * time_ns + NSEC_PER_SEC / 2;
+	do_div(dividend, NSEC_PER_SEC);
+
+	cycle = dividend;
+
+	return cycle > 1 ? cycle : 1;
+}
+
+static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			int duty_ns, int period_ns)
+{
+	u32 period_cycles, high_cycles, low_cycles;
+	struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
+
+	/* use OSC to generate PWM signals */
+	period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns);
+	if (period_cycles == 1)
+		return -EINVAL;
+
+	high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns);
+	low_cycles = period_cycles - high_cycles;
+
+	mutex_lock(&spwm->mutex);
+
+	if (high_cycles == period_cycles) {
+		high_cycles--;
+		low_cycles = 1;
+	}
+
+	writel(high_cycles - 1,
+		spwm->base + SIRF_PWM_GET_WAIT_OFFSET(pwm->hwpwm));
+	writel(low_cycles - 1,
+		spwm->base + SIRF_PWM_GET_HOLD_OFFSET(pwm->hwpwm));
+
+	mutex_unlock(&spwm->mutex);
+
+	return 0;
+}
+
+static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
+	u32 val;
+
+	mutex_lock(&spwm->mutex);
+
+	/* disable preclock */
+	val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
+	val &= ~(1 << pwm->hwpwm);
+	writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
+
+	/* select preclock source must after disable preclk*/
+	val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK);
+	val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm));
+	val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm));
+	writel(val, spwm->base + SIRF_PWM_SELECT_PRECLK);
+
+	/* wait for some time */
+	usleep_range(100, 200);
+
+	/* enable preclock */
+	val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
+	val |= (1 << pwm->hwpwm);
+	writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
+
+	/* enable post clock*/
+	val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK);
+	val |= (1 << pwm->hwpwm);
+	writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK);
+
+	/* enable output */
+	val = readl(spwm->base + SIRF_PWM_OE);
+	val |= 1 << pwm->hwpwm;
+	val |= 1 << (pwm->hwpwm + TRANS_MODE_SELECT_BIT);
+
+	writel(val, spwm->base + SIRF_PWM_OE);
+
+	mutex_unlock(&spwm->mutex);
+
+	return 0;
+}
+
+static void sirf_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	u32 val;
+	struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
+
+	mutex_lock(&spwm->mutex);
+
+	/* disable output */
+	val = readl(spwm->base + SIRF_PWM_OE);
+	val &= ~(1 << pwm->hwpwm);
+	writel(val, spwm->base + SIRF_PWM_OE);
+
+	/* disable postclock */
+	val = readl(spwm->base + SIRF_PWM_ENABLE_POSTCLOCK);
+	val &= ~(1 << pwm->hwpwm);
+	writel(val, spwm->base + SIRF_PWM_ENABLE_POSTCLOCK);
+
+	/* disable preclock */
+	val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
+	val &= ~(1 << pwm->hwpwm);
+	writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
+
+	mutex_unlock(&spwm->mutex);
+}
+
+static const struct pwm_ops sirf_pwm_ops = {
+	.enable = sirf_pwm_enable,
+	.disable = sirf_pwm_disable,
+	.config = sirf_pwm_config,
+	.owner = THIS_MODULE,
+};
+
+static int sirf_pwm_probe(struct platform_device *pdev)
+{
+	struct sirf_pwm *spwm;
+	struct resource *mem_res;
+	int ret;
+
+	spwm = devm_kzalloc(&pdev->dev, sizeof(*spwm), GFP_KERNEL);
+	if (!spwm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, spwm);
+
+	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	spwm->base = devm_ioremap_resource(&pdev->dev, mem_res);
+	if (!spwm->base)
+		return -ENOMEM;
+
+	/*
+	 * clock for PWM controller
+	 */
+	spwm->pwmc_clk = devm_clk_get(&pdev->dev, "pwmc");
+	if (IS_ERR(spwm->pwmc_clk)) {
+		dev_err(&pdev->dev, "failed to get PWM controller clock\n");
+		return PTR_ERR(spwm->pwmc_clk);
+	}
+
+	ret = clk_prepare_enable(spwm->pwmc_clk);
+	if (ret)
+		return ret;
+
+	/*
+	 * clocks to generate PWM signals, we use OSC with 26MHz
+	 */
+	spwm->sigsrc_clk = devm_clk_get(&pdev->dev, "sigsrc0");
+	if (IS_ERR(spwm->sigsrc_clk)) {
+		dev_err(&pdev->dev, "failed to get PWM signal source clock0\n");
+		ret = PTR_ERR(spwm->sigsrc_clk);
+		goto err_src_clk;
+	}
+
+	ret = clk_prepare_enable(spwm->sigsrc_clk);
+	if (ret)
+		goto err_src_clk;
+
+	spwm->sigsrc_clk_rate = clk_get_rate(spwm->sigsrc_clk);
+
+	spwm->chip.dev = &pdev->dev;
+	spwm->chip.ops = &sirf_pwm_ops;
+	spwm->chip.base = 0;
+	spwm->chip.npwm = 7;
+
+	mutex_init(&spwm->mutex);
+
+	ret = pwmchip_add(&spwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register PWM\n");
+		goto err_pwmadd;
+	}
+
+	return 0;
+
+err_pwmadd:
+	clk_disable_unprepare(spwm->sigsrc_clk);
+err_src_clk:
+	clk_disable_unprepare(spwm->pwmc_clk);
+
+	return ret;
+}
+
+static int sirf_pwm_remove(struct platform_device *pdev)
+{
+	struct sirf_pwm *spwm = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(spwm->pwmc_clk);
+	clk_disable_unprepare(spwm->sigsrc_clk);
+
+	return pwmchip_remove(&spwm->chip);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int sirf_pwm_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct sirf_pwm *spwm = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(spwm->pwmc_clk);
+
+	return 0;
+}
+
+static void sirf_pwm_config_restore(struct sirf_pwm *spwm)
+{
+	struct pwm_device *pwm;
+	int i;
+
+	for (i = 0; i < spwm->chip.npwm; i++) {
+		pwm = &spwm->chip.pwms[i];
+		/*
+		 * while restoring from hibernation, state of pwm is enabled,
+		 * but PWM hardware is not re-enabled
+		 */
+		if (test_bit(PWMF_REQUESTED, &pwm->flags) &&
+			test_bit(PWMF_ENABLED, &pwm->flags))
+			sirf_pwm_enable(&spwm->chip, pwm);
+	}
+}
+
+static int sirf_pwm_resume(struct device *dev)
+{
+	struct sirf_pwm *spwm = dev_get_drvdata(dev);
+
+	clk_prepare_enable(spwm->pwmc_clk);
+
+	sirf_pwm_config_restore(spwm);
+
+	return 0;
+}
+
+static int sirf_pwm_restore(struct device *dev)
+{
+	struct sirf_pwm *spwm = dev_get_drvdata(dev);
+
+	/* back from hibernation, clock is already enabled */
+	sirf_pwm_config_restore(spwm);
+
+	return 0;
+}
+
+#else
+#define sirf_pwm_resume NULL
+#define sirf_pwm_suspend NULL
+#define sirf_pwm_restore NULL
+#endif
+
+static const struct dev_pm_ops sirf_pwm_pm_ops = {
+	.suspend = sirf_pwm_suspend,
+	.resume = sirf_pwm_resume,
+	.restore = sirf_pwm_restore,
+};
+
+static const struct of_device_id sirf_pwm_of_match[] = {
+	{ .compatible = "sirf,prima2-pwm", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sirf_pwm_of_match);
+
+static struct platform_driver sirf_pwm_driver = {
+	.driver = {
+		.name = "sirf-pwm",
+		.pm = &sirf_pwm_pm_ops,
+		.of_match_table = sirf_pwm_of_match,
+	},
+	.probe = sirf_pwm_probe,
+	.remove = sirf_pwm_remove,
+};
+
+module_platform_driver(sirf_pwm_driver);
+
+MODULE_DESCRIPTION("SIRF serial SoC PWM device core driver");
+MODULE_AUTHOR("RongJun Ying <Rongjun.Ying@csr.com>");
+MODULE_AUTHOR("Huayi Li <huayi.li@csr.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.7.5.4

[PATCH v4 1/4] pwm: add CSR SiRFSoC PWM driver

[Thierry Reding]

On Wed, Mar 05, 2014 at 05:58:26PM +0800, Barry Song wrote:
> From: Huayi Li <Huayi.Li@csr.com>
> 
> PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output
> duty cycle can be adjusted by setting the corresponding wait & hold registers.
> There are 6 external channels (0 to 5) and 1 internal channel (6).
> Supports a wide frequency range: the source clock divider can be from 2
> up to 65536*2.

This commit message is too wide. Please always wrap the commit message
at 72 characters. See this for reference[0].

[0]: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
[...]
> +config PWM_SIRF
> +	tristate "SiRF PWM support"
> +	depends on (ARCH_SIRF || COMPILE_TEST) && COMMON_CLK && OF

I'd prefer these to be split into more lines for readability, like so:

	depends on ARCH_SIRF || COMPILE_TEST
	depends on COMMON_CLK
	depends on OF

> diff --git a/drivers/pwm/pwm-sirf.c b/drivers/pwm/pwm-sirf.c
[...]
> + * Licensed under GPLv2.
> + */
> +#include <linux/kernel.h>

Could use a blank line between the above two, visually separating the
header comment from the list of include files.

> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/pwm.h>
> +#include <linux/of.h>
> +#include <linux/io.h>

Also please keep these sorted alphabetically.

> +struct sirf_pwm {
> +	struct pwm_chip	chip;

There's a tab between pwm_chip and chip here. It should be a space.

> +	struct mutex mutex;
> +	void __iomem *base;
> +	struct clk *pwmc_clk;
> +	/*  select OSC(26MHz) as clock source to generate PWM signals */
> +	struct clk *sigsrc_clk;

Maybe I'm missing something, but what in this driver is selecting the
OSC as clock source?

> +	unsigned long sigsrc_clk_rate;

Why do you cache this value? Can't you simply query the clock framework
for it when you need it?

> +static u32 sirf_pwm_ns_to_cycles(struct pwm_chip *chip, u32 time_ns)
> +{
> +	struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
> +	u64 dividend;
> +	u32 cycle;
> +
> +	dividend = (u64)spwm->sigsrc_clk_rate * time_ns + NSEC_PER_SEC / 2;
> +	do_div(dividend, NSEC_PER_SEC);
> +
> +	cycle = dividend;
> +
> +	return cycle > 1 ? cycle : 1;
> +}

I think you can do without the cycle variable here.

> +static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			int duty_ns, int period_ns)

Please always align the first argument on subsequent lines with the
first argument on the first line.

> +{
> +	u32 period_cycles, high_cycles, low_cycles;
> +	struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
> +
> +	/* use OSC to generate PWM signals */
> +	period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns);

Again, how is this OSC specific?

> +	if (period_cycles == 1)
> +		return -EINVAL;
> +
> +	high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns);
> +	low_cycles = period_cycles - high_cycles;
> +
> +	mutex_lock(&spwm->mutex);
> +
> +	if (high_cycles == period_cycles) {
> +		high_cycles--;
> +		low_cycles = 1;
> +	}

This perhaps could use a comment. Why is it that the hardware can't be
programmed to high_cycles == period_cycles?

Also since you're only accessing local variables here you can safely
move this block out of the lock.

> +static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
> +	u32 val;
> +
> +	mutex_lock(&spwm->mutex);
> +
> +	/* disable preclock */
> +	val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
> +	val &= ~(1 << pwm->hwpwm);
> +	writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
> +
> +	/* select preclock source must after disable preclk*/

Missing space between "preclk" and "*/".

> +	val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK);
> +	val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm));
> +	val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm));

Perhaps the source field is used to configure which clock is used? In
that case I suggest you reshuffle the comments a bit. For example I
think it would be clearer if you stated in the file's header comment
that the driver currently only works if the OSC is used as the signal
source clock and make a comment here that this selects OSC.

One thing that worries me slightly is that there's no way we can
determine from a struct clk what clock it actually is. So it's
impossible to derive from a struct clk that it is indeed the OSC and
therefore this driver has no means to be extensible in the future.

But I have no idea how to solve this properly. Perhaps this would
somehow need to be modeled within the clock framework?

> +static int sirf_pwm_probe(struct platform_device *pdev)
> +{
> +	struct sirf_pwm *spwm;
> +	struct resource *mem_res;
> +	int ret;
> +
> +	spwm = devm_kzalloc(&pdev->dev, sizeof(*spwm), GFP_KERNEL);
> +	if (!spwm)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, spwm);
> +
> +	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	spwm->base = devm_ioremap_resource(&pdev->dev, mem_res);
> +	if (!spwm->base)
> +		return -ENOMEM;

devm_ioremap_resource() returns an ERR_PTR() encoded error on failure,
so the proper way to check for this is:

	if (IS_ERR(spwm->base))
		return PTR_ERR(spwm->base);

> +	/*
> +	 * clock for PWM controller
> +	 */
> +	spwm->pwmc_clk = devm_clk_get(&pdev->dev, "pwmc");

I think you could drop the pwmc_ prefix, but I don't feel too strongly
about that.

> +	if (IS_ERR(spwm->pwmc_clk)) {
> +		dev_err(&pdev->dev, "failed to get PWM controller clock\n");
> +		return PTR_ERR(spwm->pwmc_clk);
> +	}
> +
> +	ret = clk_prepare_enable(spwm->pwmc_clk);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * clocks to generate PWM signals, we use OSC with 26MHz
> +	 */

Only a single clock is requested here, so "clock to generate...". And
again there's a reference to OSC and 26 MHz here, but the code is
completely generic and it could in fact be any other clock.

> +	spwm->sigsrc_clk = devm_clk_get(&pdev->dev, "sigsrc0");
> +	if (IS_ERR(spwm->sigsrc_clk)) {
> +		dev_err(&pdev->dev, "failed to get PWM signal source clock0\n");
> +		ret = PTR_ERR(spwm->sigsrc_clk);
> +		goto err_src_clk;
> +	}

Why is this called "sigsrc0" if there's only one?

> +	spwm->sigsrc_clk_rate = clk_get_rate(spwm->sigsrc_clk);

I guess this could be no issue at all if sigsrc_clk indeed always is
OSC, but caching the rate of the source clock is bad because it gives
you no chance at all to adapt to clock rate changes later on (if that
even happens). So in order not to set a bad example, I'd rather see this
not cached, but queried when needed.

> +	spwm->chip.dev = &pdev->dev;
> +	spwm->chip.ops = &sirf_pwm_ops;
> +	spwm->chip.base = 0;

I think you should want to set this to -1.

> +		dev_err(&pdev->dev, "failed to register PWM\n");

"PWM chip", please.

> +static int sirf_pwm_remove(struct platform_device *pdev)
> +{
> +	struct sirf_pwm *spwm = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(spwm->pwmc_clk);
> +	clk_disable_unprepare(spwm->sigsrc_clk);
> +
> +	return pwmchip_remove(&spwm->chip);
> +}

Don't you want to remove the chip first, before disabling all the
clocks? pwmchip_remove() may end up accessing registers that need the
clock enabled.

> +static int sirf_pwm_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct sirf_pwm *spwm = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(spwm->pwmc_clk);
> +
> +	return 0;
> +}

Why doesn't this disable (and unprepare) the source clock?

> +static void sirf_pwm_config_restore(struct sirf_pwm *spwm)
> +{
> +	struct pwm_device *pwm;
> +	int i;
> +
> +	for (i = 0; i < spwm->chip.npwm; i++) {
> +		pwm = &spwm->chip.pwms[i];
> +		/*
> +		 * while restoring from hibernation, state of pwm is enabled,

pwm -> PWM, please.

> +static int sirf_pwm_restore(struct device *dev)
> +{
> +	struct sirf_pwm *spwm = dev_get_drvdata(dev);
> +
> +	/* back from hibernation, clock is already enabled */
> +	sirf_pwm_config_restore(spwm);
> +
> +	return 0;
> +}
> +
> +#else

Gratuitous blank line above this one.

> +static struct platform_driver sirf_pwm_driver = {
> +	.driver = {
> +		.name = "sirf-pwm",
> +		.pm = &sirf_pwm_pm_ops,
> +		.of_match_table = sirf_pwm_of_match,
> +	},
> +	.probe = sirf_pwm_probe,
> +	.remove = sirf_pwm_remove,
> +};
> +
> +module_platform_driver(sirf_pwm_driver);

Another gratuitous blank line above this one.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140318/4da5760d/attachment-0001.sig>

[PATCH v4 1/4] pwm: add CSR SiRFSoC PWM driver

[Barry Song]

2014-03-19 5:03 GMT+08:00 Thierry Reding <thierry.reding@gmail.com>:
> On Wed, Mar 05, 2014 at 05:58:26PM +0800, Barry Song wrote:
>> From: Huayi Li <Huayi.Li@csr.com>
>>
>> PWM controller of CSR SiRFSoC can generate 7 independent outputs. Each output
>> duty cycle can be adjusted by setting the corresponding wait & hold registers.
>> There are 6 external channels (0 to 5) and 1 internal channel (6).
>> Supports a wide frequency range: the source clock divider can be from 2
>> up to 65536*2.
>
> This commit message is too wide. Please always wrap the commit message
> at 72 characters. See this for reference[0].
>
> [0]: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> [...]
>> +config PWM_SIRF
>> +     tristate "SiRF PWM support"
>> +     depends on (ARCH_SIRF || COMPILE_TEST) && COMMON_CLK && OF
>
> I'd prefer these to be split into more lines for readability, like so:
>
>         depends on ARCH_SIRF || COMPILE_TEST
>         depends on COMMON_CLK
>         depends on OF

ok

>
>> diff --git a/drivers/pwm/pwm-sirf.c b/drivers/pwm/pwm-sirf.c
> [...]
>> + * Licensed under GPLv2.
>> + */
>> +#include <linux/kernel.h>
>
> Could use a blank line between the above two, visually separating the
> header comment from the list of include files.

ok.

>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/pwm.h>
>> +#include <linux/of.h>
>> +#include <linux/io.h>
>
> Also please keep these sorted alphabetically.
>
>> +struct sirf_pwm {
>> +     struct pwm_chip chip;
>
> There's a tab between pwm_chip and chip here. It should be a space.

ok

>
>> +     struct mutex mutex;
>> +     void __iomem *base;
>> +     struct clk *pwmc_clk;
>> +     /*  select OSC(26MHz) as clock source to generate PWM signals */
>> +     struct clk *sigsrc_clk;
>
> Maybe I'm missing something, but what in this driver is selecting the
> OSC as clock source?

it can select one of rtc(32KHz), osc(26MHz) and 3 plls in several
hundreds Hz. 26MHz is a fixed one which can generate a wide range of
pwm waves.
and the PWM also supports bypass, which can bypass the above five to
clock out directly.

>
>> +     unsigned long sigsrc_clk_rate;
>
> Why do you cache this value? Can't you simply query the clock framework
> for it when you need it?

since the freq is fixed, we cache it now. but we can drop the cache.

>
>> +static u32 sirf_pwm_ns_to_cycles(struct pwm_chip *chip, u32 time_ns)
>> +{
>> +     struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
>> +     u64 dividend;
>> +     u32 cycle;
>> +
>> +     dividend = (u64)spwm->sigsrc_clk_rate * time_ns + NSEC_PER_SEC / 2;
>> +     do_div(dividend, NSEC_PER_SEC);
>> +
>> +     cycle = dividend;
>> +
>> +     return cycle > 1 ? cycle : 1;
>> +}
>
> I think you can do without the cycle variable here.
right

>
>> +static int sirf_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +                     int duty_ns, int period_ns)
>
> Please always align the first argument on subsequent lines with the
> first argument on the first line.
ok

>
>> +{
>> +     u32 period_cycles, high_cycles, low_cycles;
>> +     struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
>> +
>> +     /* use OSC to generate PWM signals */
>> +     period_cycles = sirf_pwm_ns_to_cycles(chip, period_ns);
>
> Again, how is this OSC specific?

as above

>
>> +     if (period_cycles == 1)
>> +             return -EINVAL;
>> +
>> +     high_cycles = sirf_pwm_ns_to_cycles(chip, duty_ns);
>> +     low_cycles = period_cycles - high_cycles;
>> +
>> +     mutex_lock(&spwm->mutex);
>> +
>> +     if (high_cycles == period_cycles) {
>> +             high_cycles--;
>> +             low_cycles = 1;
>> +     }
>
> This perhaps could use a comment. Why is it that the hardware can't be
> programmed to high_cycles == period_cycles?

high=period means that is not a wave but a fixed high voltage level.

>
> Also since you're only accessing local variables here you can safely
> move this block out of the lock.
>
>> +static int sirf_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     struct sirf_pwm *spwm = to_sirf_pwm_chip(chip);
>> +     u32 val;
>> +
>> +     mutex_lock(&spwm->mutex);
>> +
>> +     /* disable preclock */
>> +     val = readl(spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
>> +     val &= ~(1 << pwm->hwpwm);
>> +     writel(val, spwm->base + SIRF_PWM_ENABLE_PRECLOCK);
>> +
>> +     /* select preclock source must after disable preclk*/
>
> Missing space between "preclk" and "*/".

will fix.

>
>> +     val = readl(spwm->base + SIRF_PWM_SELECT_PRECLK);
>> +     val &= ~(0x1 << (BYPASS_MODE_BIT + pwm->hwpwm));
>> +     val &= ~(0x7 << (SRC_FIELD_SIZE * pwm->hwpwm));
>
> Perhaps the source field is used to configure which clock is used? In
> that case I suggest you reshuffle the comments a bit. For example I
> think it would be clearer if you stated in the file's header comment
> that the driver currently only works if the OSC is used as the signal
> source clock and make a comment here that this selects OSC.

well. that is the plan for the 1st step.

>
> One thing that worries me slightly is that there's no way we can
> determine from a struct clk what clock it actually is. So it's
> impossible to derive from a struct clk that it is indeed the OSC and
> therefore this driver has no means to be extensible in the future.

we might extend, for example, using 32KHz as source, we can get a very
low frequency wave.

>
> But I have no idea how to solve this properly. Perhaps this would
> somehow need to be modeled within the clock framework?

since this pwm can also bypass, bypass is actually a clock out not a
pwm out. so hooking into clock framework might be good to do in the
next step

>
>> +static int sirf_pwm_probe(struct platform_device *pdev)
>> +{
>> +     struct sirf_pwm *spwm;
>> +     struct resource *mem_res;
>> +     int ret;
>> +
>> +     spwm = devm_kzalloc(&pdev->dev, sizeof(*spwm), GFP_KERNEL);
>> +     if (!spwm)
>> +             return -ENOMEM;
>> +
>> +     platform_set_drvdata(pdev, spwm);
>> +
>> +     mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     spwm->base = devm_ioremap_resource(&pdev->dev, mem_res);
>> +     if (!spwm->base)
>> +             return -ENOMEM;
>
> devm_ioremap_resource() returns an ERR_PTR() encoded error on failure,
> so the proper way to check for this is:
>
>         if (IS_ERR(spwm->base))
>                 return PTR_ERR(spwm->base);

real.

>
>> +     /*
>> +      * clock for PWM controller
>> +      */
>> +     spwm->pwmc_clk = devm_clk_get(&pdev->dev, "pwmc");
>
> I think you could drop the pwmc_ prefix, but I don't feel too strongly
> about that.

if there is no other clock as signal source, it is good. now there is
another one, that is why this clock get prefix.

>
>> +     if (IS_ERR(spwm->pwmc_clk)) {
>> +             dev_err(&pdev->dev, "failed to get PWM controller clock\n");
>> +             return PTR_ERR(spwm->pwmc_clk);
>> +     }
>> +
>> +     ret = clk_prepare_enable(spwm->pwmc_clk);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /*
>> +      * clocks to generate PWM signals, we use OSC with 26MHz
>> +      */
>
> Only a single clock is requested here, so "clock to generate...". And
> again there's a reference to OSC and 26 MHz here, but the code is
> completely generic and it could in fact be any other clock.

right.

>
>> +     spwm->sigsrc_clk = devm_clk_get(&pdev->dev, "sigsrc0");
>> +     if (IS_ERR(spwm->sigsrc_clk)) {
>> +             dev_err(&pdev->dev, "failed to get PWM signal source clock0\n");
>> +             ret = PTR_ERR(spwm->sigsrc_clk);
>> +             goto err_src_clk;
>> +     }
>
> Why is this called "sigsrc0" if there's only one?

there are five, OSC is the first one in the HW index.

>
>> +     spwm->sigsrc_clk_rate = clk_get_rate(spwm->sigsrc_clk);
>
> I guess this could be no issue at all if sigsrc_clk indeed always is
> OSC, but caching the rate of the source clock is bad because it gives
> you no chance at all to adapt to clock rate changes later on (if that
> even happens). So in order not to set a bad example, I'd rather see this
> not cached, but queried when needed.

ok. understood.

>
>> +     spwm->chip.dev = &pdev->dev;
>> +     spwm->chip.ops = &sirf_pwm_ops;
>> +     spwm->chip.base = 0;
>
> I think you should want to set this to -1.

ok

>
>> +             dev_err(&pdev->dev, "failed to register PWM\n");
>
> "PWM chip", please.
>
ok

>> +static int sirf_pwm_remove(struct platform_device *pdev)
>> +{
>> +     struct sirf_pwm *spwm = platform_get_drvdata(pdev);
>> +
>> +     clk_disable_unprepare(spwm->pwmc_clk);
>> +     clk_disable_unprepare(spwm->sigsrc_clk);
>> +
>> +     return pwmchip_remove(&spwm->chip);
>> +}
>
> Don't you want to remove the chip first, before disabling all the
> clocks? pwmchip_remove() may end up accessing registers that need the
> clock enabled.

i will check this issue. it seems generally we disable hw before
removing SW to avoid SW get more events like interrupts.

>
>> +static int sirf_pwm_suspend(struct device *dev)
>> +{
>> +     struct platform_device *pdev = to_platform_device(dev);
>> +     struct sirf_pwm *spwm = platform_get_drvdata(pdev);
>> +
>> +     clk_disable_unprepare(spwm->pwmc_clk);
>> +
>> +     return 0;
>> +}
>
> Why doesn't this disable (and unprepare) the source clock?

we can have.
>
>> +static void sirf_pwm_config_restore(struct sirf_pwm *spwm)
>> +{
>> +     struct pwm_device *pwm;
>> +     int i;
>> +
>> +     for (i = 0; i < spwm->chip.npwm; i++) {
>> +             pwm = &spwm->chip.pwms[i];
>> +             /*
>> +              * while restoring from hibernation, state of pwm is enabled,
>
> pwm -> PWM, please.

ok

>
>> +static int sirf_pwm_restore(struct device *dev)
>> +{
>> +     struct sirf_pwm *spwm = dev_get_drvdata(dev);
>> +
>> +     /* back from hibernation, clock is already enabled */
>> +     sirf_pwm_config_restore(spwm);
>> +
>> +     return 0;
>> +}
>> +
>> +#else
>
> Gratuitous blank line above this one.

ok

>
>> +static struct platform_driver sirf_pwm_driver = {
>> +     .driver = {
>> +             .name = "sirf-pwm",
>> +             .pm = &sirf_pwm_pm_ops,
>> +             .of_match_table = sirf_pwm_of_match,
>> +     },
>> +     .probe = sirf_pwm_probe,
>> +     .remove = sirf_pwm_remove,
>> +};
>> +
>> +module_platform_driver(sirf_pwm_driver);
>
> Another gratuitous blank line above this one.
>
ok

> Thierry

-barry