[PATCH v4 0/2] pwm: Add support for R-Car PWM Timer

[PATCH v4 0/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda]

This patch set is based on the linux-pwm.git / for-next branch.
(commit id = efb0de55b6a2ec15fc424e660601f22ae2fa487a)

Changes from v3:
 - Fix register size in patch 1.
 - Add "Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>" in patch 1
 - Remove an unnecessary definition in patch 2.
 - Use "ULL" to avoid overflow in patch 2.
 - Remove unnecessary casts in patch 2.

Changes from v2:
 - Add compatible string "renesas,pwm-rcar".
 - Remove compatible strings "renesas,pwm-r8a77xx" in rcar_pwm_of_table.
 - Fix build error.

Changes from v1:
 - Change compatible string to SoC-specific compatible values.
 - Fix #pwm-call value to 2 in the device tree document.
 - Fix "depends on" value in Kconfig.
 - Fix help explanation in Kconfig.
 - Remove an unnecessary member in rcar_pwm_chip.
 - Remove hardcoded number of channels and change chip.npwm value to 1.
 - Fix formulas for clock calculation to improve accuracy.

Yoshihiro Shimoda (2):
  pwm: Add device tree binding document for R-Car PWM Timer
  pwm: Add support for R-Car PWM Timer

 .../devicetree/bindings/pwm/renesas,pwm-rcar.txt   |  27 +++
 drivers/pwm/Kconfig                                |  11 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-rcar.c                             | 261 +++++++++++++++++++++
 4 files changed, 300 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
 create mode 100644 drivers/pwm/pwm-rcar.c

-- 
1.9.1

[PATCH v4 1/2] pwm: Add device tree binding document for R-Car PWM Timer

[Yoshihiro Shimoda]

Add binding document for Renesas PWM Timer on R-Car SoCs.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 .../devicetree/bindings/pwm/renesas,pwm-rcar.txt   | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt

diff --git a/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
new file mode 100644
index 0000000..ea0a27b
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
@@ -0,0 +1,27 @@
+* Renesas R-Car PWM Timer Controller
+
+Required Properties:
+- compatible: should be one of the following.
+ - "renesas,pwm-rcar": for generic R-Car compatible PWM Timer
+ - "renesas,pwm-r8a7778": for R-Car M1A
+ - "renesas,pwm-r8a7779": for R-Car H1
+ - "renesas,pwm-r8a7790": for R-Car H2
+ - "renesas,pwm-r8a7791": for R-Car M2-W
+ - "renesas,pwm-r8a7794": for R-Car E2
+- reg: base address and length of the registers block for the PWM.
+- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
+  the cells format.
+- clocks: clock phandle and specifier pair.
+- pinctrl-0: phandle, referring to a default pin configuration node.
+- pinctrl-names: Set to "default".
+
+Example: R8A7790 (R-Car H2) PWM Timer node
+
+	pwm0: pwm@e6e30000 {
+		compatible = "renesas,pwm-r8a7790", "renesas,pwm-rcar";
+		reg = <0 0xe6e30000 0 0x8>;
+		#pwm-cells = <2>;
+		clocks = <&mstp5_clks R8A7790_CLK_PWM>;
+		pinctrl-0 = <&pwm0_pins>;
+		pinctrl-names = "default";
+	};
-- 
1.9.1

[PATCH v4 2/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda]

This patch adds support for R-Car SoCs PWM Timer.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/Kconfig    |  11 +++
 drivers/pwm/Makefile   |   1 +
 drivers/pwm/pwm-rcar.c | 261 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/pwm/pwm-rcar.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b1541f4..3e58a68 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -249,6 +249,17 @@ config PWM_PXA
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-pxa.
 
+config PWM_RCAR
+	tristate "Renesas R-Car PWM support"
+	depends on ARCH_RCAR_GEN1 || ARCH_RCAR_GEN2 || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This driver exposes the PWM Timer controller found in Renesas
+	  R-Car chips through the PWM API.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-rcar.
+
 config PWM_RENESAS_TPU
 	tristate "Renesas TPU PWM support"
 	depends on ARCH_SHMOBILE || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index ec50eb5..79d3dc3 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PUV3)		+= pwm-puv3.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
+obj-$(CONFIG_PWM_RCAR)		+= pwm-rcar.o
 obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
new file mode 100644
index 0000000..b359cd1
--- /dev/null
+++ b/drivers/pwm/pwm-rcar.c
@@ -0,0 +1,261 @@
+/*
+ * R-Car PWM Timer driver
+ *
+ * Copyright (C) 2015 Renesas Electronics Corporation
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+#define RCAR_PWM_MAX_DIVISION	24
+#define RCAR_PWM_MAX_CYCLE	1023
+
+#define RCAR_PWMCR		0x00
+#define RCAR_PWMCNT		0x04
+
+#define RCAR_PWMCR_CC0_MASK	0x000f0000
+#define RCAR_PWMCR_CC0_SHIFT	16
+#define RCAR_PWMCR_CCMD		BIT(15)
+#define RCAR_PWMCR_SYNC		BIT(11)
+#define RCAR_PWMCR_SS0		BIT(4)
+#define RCAR_PWMCR_EN0		BIT(0)
+
+#define RCAR_PWMCNT_CYC0_MASK	0x03ff0000
+#define RCAR_PWMCNT_CYC0_SHIFT	16
+#define RCAR_PWMCNT_PH0_MASK	0x000003ff
+#define RCAR_PWMCNT_PH0_SHIFT	0
+
+struct rcar_pwm_chip {
+	struct pwm_chip chip;
+	void __iomem *base;
+	struct clk *clk;
+};
+
+#define to_rcar_pwm_chip(chip)	container_of(chip, struct rcar_pwm_chip, chip)
+
+static void rcar_pwm_write(struct rcar_pwm_chip *rp, u32 data, u32 reg)
+{
+	iowrite32(data, rp->base + reg);
+}
+
+static u32 rcar_pwm_read(struct rcar_pwm_chip *rp, u32 reg)
+{
+	return ioread32(rp->base + reg);
+}
+
+static void rcar_pwm_bit_modify(struct rcar_pwm_chip *rp,
+				u32 mask, u32 data, u32 reg)
+{
+	u32 val = rcar_pwm_read(rp, reg);
+
+	val &= ~mask;
+	val |= data & mask;
+	rcar_pwm_write(rp, val, reg);
+}
+
+static int rcar_pwn_get_clock_division(struct rcar_pwm_chip *rp,
+				       int period_ns)
+{
+	int div;
+	unsigned long clk_rate = clk_get_rate(rp->clk);
+	unsigned long long max;	/* max cycle / nanoseconds */
+
+	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
+		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
+			(1 << div);
+		do_div(max, clk_rate);
+		if (period_ns < max)
+			break;
+	}
+
+	return div;
+}
+
+static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp, int div)
+{
+	u32 val = rcar_pwm_read(rp, RCAR_PWMCR);
+
+	if (div > RCAR_PWM_MAX_DIVISION)
+		return;
+
+	val &= ~(RCAR_PWMCR_CCMD | RCAR_PWMCR_CC0_MASK);
+	if (div & 1)
+		val |= RCAR_PWMCR_CCMD;
+	div >>= 1;
+	val |= div << RCAR_PWMCR_CC0_SHIFT;
+	rcar_pwm_write(rp, val, RCAR_PWMCR);
+}
+
+static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div,
+				 int duty_ns, int period_ns)
+{
+	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
+	unsigned long clk_rate = clk_get_rate(rp->clk);
+	u32 cyc, ph;
+
+	one_cycle = (unsigned long long)NSEC_PER_SEC * 100ULL * (1 << div);
+	do_div(one_cycle, clk_rate);
+
+	tmp = period_ns * 100ULL;
+	do_div(tmp, one_cycle);
+	cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
+
+	tmp = duty_ns * 100ULL;
+	do_div(tmp, one_cycle);
+	ph = tmp & RCAR_PWMCNT_PH0_MASK;
+
+	/* Avoid prohibited setting */
+	if (cyc && ph)
+		rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
+}
+
+static int rcar_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+
+	return clk_prepare_enable(rp->clk);
+}
+
+static void rcar_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+
+	clk_disable_unprepare(rp->clk);
+}
+
+static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			   int duty_ns, int period_ns)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+	int div;
+
+	div = rcar_pwn_get_clock_division(rp, period_ns);
+
+	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
+	rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
+	rcar_pwm_set_clock_control(rp, div);
+	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
+
+	return 0;
+}
+
+static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+	u32 pwmcnt;
+
+	/* Don't enable the PWM device if CYC0 or PH0 is 0 */
+	pwmcnt = rcar_pwm_read(rp, RCAR_PWMCNT);
+	if (!(pwmcnt & RCAR_PWMCNT_CYC0_MASK) ||
+	    !(pwmcnt & RCAR_PWMCNT_PH0_MASK))
+		return -EINVAL;
+
+	rcar_pwm_bit_modify(rp, RCAR_PWMCR_EN0, RCAR_PWMCR_EN0, RCAR_PWMCR);
+
+	return 0;
+}
+
+static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+
+	rcar_pwm_bit_modify(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
+}
+
+static const struct pwm_ops rcar_pwm_ops = {
+	.request	= rcar_pwm_request,
+	.free		= rcar_pwm_free,
+	.config		= rcar_pwm_config,
+	.enable		= rcar_pwm_enable,
+	.disable	= rcar_pwm_disable,
+	.owner		= THIS_MODULE,
+};
+
+static int rcar_pwm_probe(struct platform_device *pdev)
+{
+	struct rcar_pwm_chip *rcar_pwm;
+	struct resource *res;
+	int ret;
+
+	rcar_pwm = devm_kzalloc(&pdev->dev, sizeof(*rcar_pwm), GFP_KERNEL);
+	if (rcar_pwm == NULL)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	rcar_pwm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rcar_pwm->base))
+		return PTR_ERR(rcar_pwm->base);
+
+	rcar_pwm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(rcar_pwm->clk)) {
+		dev_err(&pdev->dev, "cannot get clock\n");
+		return PTR_ERR(rcar_pwm->clk);
+	}
+
+	platform_set_drvdata(pdev, rcar_pwm);
+
+	rcar_pwm->chip.dev = &pdev->dev;
+	rcar_pwm->chip.ops = &rcar_pwm_ops;
+	rcar_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
+	rcar_pwm->chip.base = -1;
+	rcar_pwm->chip.npwm = 1;
+
+	ret = pwmchip_add(&rcar_pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register PWM chip\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "R-Car PWM Timer registered\n");
+
+	pm_runtime_enable(&pdev->dev);
+
+	return 0;
+}
+
+static int rcar_pwm_remove(struct platform_device *pdev)
+{
+	struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&rcar_pwm->chip);
+	if (ret)
+		return ret;
+
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id rcar_pwm_of_table[] = {
+	{ .compatible = "renesas,pwm-rcar", },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, rcar_pwm_of_table);
+
+static struct platform_driver rcar_pwm_driver = {
+	.probe		= rcar_pwm_probe,
+	.remove		= rcar_pwm_remove,
+	.driver		= {
+		.name	= "pwm-rcar",
+		.of_match_table = of_match_ptr(rcar_pwm_of_table),
+	}
+};
+module_platform_driver(rcar_pwm_driver);
+
+MODULE_AUTHOR("Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>");
+MODULE_DESCRIPTION("Renesas PWM Timer Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:pwm-rcar");
-- 
1.9.1

Re: [PATCH v4 2/2] pwm: Add support for R-Car PWM Timer

[Thierry Reding]

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

On Thu, May 21, 2015 at 07:57:26PM +0900, Yoshihiro Shimoda wrote:
[...]
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
[...]
> +#define to_rcar_pwm_chip(chip)	container_of(chip, struct rcar_pwm_chip, chip)

For consistency with other drivers I'd like this to be a static inline
function.

> +
> +static void rcar_pwm_write(struct rcar_pwm_chip *rp, u32 data, u32 reg)
> +{
> +	iowrite32(data, rp->base + reg);
> +}
> +
> +static u32 rcar_pwm_read(struct rcar_pwm_chip *rp, u32 reg)
> +{
> +	return ioread32(rp->base + reg);
> +}

ioread*() and iowrite*() are to be used for devices that can be on a
memory-mapped bus or an I/O mapped bus. I suspect that this particular
IP block doesn't fall into that category, so it should be using the
regular readl()/writel() instead.

> +static void rcar_pwm_bit_modify(struct rcar_pwm_chip *rp,
> +				u32 mask, u32 data, u32 reg)

You should try to fill up lines as much as you can: mask and data should
still fit on the first line.

> +{
> +	u32 val = rcar_pwm_read(rp, reg);
> +
> +	val &= ~mask;
> +	val |= data & mask;
> +	rcar_pwm_write(rp, val, reg);
> +}
> +
> +static int rcar_pwn_get_clock_division(struct rcar_pwm_chip *rp,

Typo: "pwn" -> "pwm"

> +				       int period_ns)
> +{
> +	int div;

Perhaps make this unsigned int?

> +	unsigned long clk_rate = clk_get_rate(rp->clk);
> +	unsigned long long max;	/* max cycle / nanoseconds */

I think you want to check for clk_rate == 0 here and return an error.
Otherwise the do_div() call below may try to divide by 0.

> +	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> +		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> +			(1 << div);
> +		do_div(max, clk_rate);
> +		if (period_ns < max)
> +			break;
> +	}
> +
> +	return div;
> +}
> +
> +static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp, int div)
> +{
> +	u32 val = rcar_pwm_read(rp, RCAR_PWMCR);
> +
> +	if (div > RCAR_PWM_MAX_DIVISION)
> +		return;

Shouldn't you return an error here (and propagate it later on) if an
invalid value is passed in? Or perhaps even avoid calling this function
with an invalid value in the first place? As it is, you're silently
ignoring cases where an invalid value is being passed in. That'll leave
anybody working with this driver completely puzzled when it doesn't
behave the way they expect it too. And it gives users no indication
about what went wrong.

> +
> +	val &= ~(RCAR_PWMCR_CCMD | RCAR_PWMCR_CC0_MASK);
> +	if (div & 1)
> +		val |= RCAR_PWMCR_CCMD;
> +	div >>= 1;
> +	val |= div << RCAR_PWMCR_CC0_SHIFT;
> +	rcar_pwm_write(rp, val, RCAR_PWMCR);
> +}
> +
> +static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div,
> +				 int duty_ns, int period_ns)
> +{
> +	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
> +	unsigned long clk_rate = clk_get_rate(rp->clk);
> +	u32 cyc, ph;
> +
> +	one_cycle = (unsigned long long)NSEC_PER_SEC * 100ULL * (1 << div);
> +	do_div(one_cycle, clk_rate);
> +
> +	tmp = period_ns * 100ULL;
> +	do_div(tmp, one_cycle);
> +	cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
> +
> +	tmp = duty_ns * 100ULL;
> +	do_div(tmp, one_cycle);
> +	ph = tmp & RCAR_PWMCNT_PH0_MASK;
> +
> +	/* Avoid prohibited setting */
> +	if (cyc && ph)
> +		rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);

So if a period and duty-cycle are passed in that yield the prohibited
setting the operation will simply be silently ignored?

> +}
> +
> +static int rcar_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> +
> +	return clk_prepare_enable(rp->clk);
> +}
> +
> +static void rcar_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> +
> +	clk_disable_unprepare(rp->clk);
> +}
> +
> +static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   int duty_ns, int period_ns)
> +{
> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> +	int div;
> +
> +	div = rcar_pwn_get_clock_division(rp, period_ns);

The above three lines can be collapsed into a single one.

> +
> +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> +	rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> +	rcar_pwm_set_clock_control(rp, div);
> +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> +
> +	return 0;
> +}
> +
> +static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> +	u32 pwmcnt;
> +
> +	/* Don't enable the PWM device if CYC0 or PH0 is 0 */
> +	pwmcnt = rcar_pwm_read(rp, RCAR_PWMCNT);
> +	if (!(pwmcnt & RCAR_PWMCNT_CYC0_MASK) ||
> +	    !(pwmcnt & RCAR_PWMCNT_PH0_MASK))
> +		return -EINVAL;
> +
> +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_EN0, RCAR_PWMCR_EN0, RCAR_PWMCR);
> +
> +	return 0;
> +}
> +
> +static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> +
> +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> +}
> +
> +static const struct pwm_ops rcar_pwm_ops = {
> +	.request	= rcar_pwm_request,
> +	.free		= rcar_pwm_free,
> +	.config		= rcar_pwm_config,
> +	.enable		= rcar_pwm_enable,
> +	.disable	= rcar_pwm_disable,
> +	.owner		= THIS_MODULE,
> +};

No need for this padding. Single spaces around = are good enough.

> +
> +static int rcar_pwm_probe(struct platform_device *pdev)
> +{
> +	struct rcar_pwm_chip *rcar_pwm;
> +	struct resource *res;
> +	int ret;
> +
> +	rcar_pwm = devm_kzalloc(&pdev->dev, sizeof(*rcar_pwm), GFP_KERNEL);
> +	if (rcar_pwm == NULL)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	rcar_pwm->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(rcar_pwm->base))
> +		return PTR_ERR(rcar_pwm->base);
> +
> +	rcar_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(rcar_pwm->clk)) {
> +		dev_err(&pdev->dev, "cannot get clock\n");
> +		return PTR_ERR(rcar_pwm->clk);
> +	}
> +
> +	platform_set_drvdata(pdev, rcar_pwm);
> +
> +	rcar_pwm->chip.dev = &pdev->dev;
> +	rcar_pwm->chip.ops = &rcar_pwm_ops;
> +	rcar_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> +	rcar_pwm->chip.base = -1;
> +	rcar_pwm->chip.npwm = 1;
> +
> +	ret = pwmchip_add(&rcar_pwm->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register PWM chip\n");
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "R-Car PWM Timer registered\n");

No need to brag about success. Expect that things will go well and let
users know when they don't. Output error messages, not success messages.

> +	pm_runtime_enable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static int rcar_pwm_remove(struct platform_device *pdev)
> +{
> +	struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&rcar_pwm->chip);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_disable(&pdev->dev);

Perhaps you'd still like to disable runtime PM even if the chip can't be
removed?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rcar_pwm_of_table[] = {
> +	{ .compatible = "renesas,pwm-rcar", },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, rcar_pwm_of_table);

No blank line between the above two.

Thierry

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

RE: [PATCH v4 2/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda]

Hi Thierry,

Thank you for the review!

> Sent: Friday, June 12, 2015 9:06 PM
> 
> On Thu, May 21, 2015 at 07:57:26PM +0900, Yoshihiro Shimoda wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> [...]
> > +#define to_rcar_pwm_chip(chip)	container_of(chip, struct rcar_pwm_chip, chip)
> 
> For consistency with other drivers I'd like this to be a static inline
> function.

I got it. I will modify this.

> > +
> > +static void rcar_pwm_write(struct rcar_pwm_chip *rp, u32 data, u32 reg)
> > +{
> > +	iowrite32(data, rp->base + reg);
> > +}
> > +
> > +static u32 rcar_pwm_read(struct rcar_pwm_chip *rp, u32 reg)
> > +{
> > +	return ioread32(rp->base + reg);
> > +}
> 
> ioread*() and iowrite*() are to be used for devices that can be on a
> memory-mapped bus or an I/O mapped bus. I suspect that this particular
> IP block doesn't fall into that category, so it should be using the
> regular readl()/writel() instead.

I will use readl()/writel().

> > +static void rcar_pwm_bit_modify(struct rcar_pwm_chip *rp,
> > +				u32 mask, u32 data, u32 reg)
> 
> You should try to fill up lines as much as you can: mask and data should
> still fit on the first line.

I will fix it.

> > +{
> > +	u32 val = rcar_pwm_read(rp, reg);
> > +
> > +	val &= ~mask;
> > +	val |= data & mask;
> > +	rcar_pwm_write(rp, val, reg);
> > +}
> > +
> > +static int rcar_pwn_get_clock_division(struct rcar_pwm_chip *rp,
> 
> Typo: "pwn" -> "pwm"

Oops, I will fix it.

> > +				       int period_ns)
> > +{
> > +	int div;
> 
> Perhaps make this unsigned int?

I will use unsigned int.

> > +	unsigned long clk_rate = clk_get_rate(rp->clk);
> > +	unsigned long long max;	/* max cycle / nanoseconds */
> 
> I think you want to check for clk_rate == 0 here and return an error.
> Otherwise the do_div() call below may try to divide by 0.

I will add a code to aboid dividing by 0.

> > +	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> > +		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> > +			(1 << div);
> > +		do_div(max, clk_rate);
> > +		if (period_ns < max)
> > +			break;
> > +	}
> > +
> > +	return div;
> > +}
> > +
> > +static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp, int div)
> > +{
> > +	u32 val = rcar_pwm_read(rp, RCAR_PWMCR);
> > +
> > +	if (div > RCAR_PWM_MAX_DIVISION)
> > +		return;
> 
> Shouldn't you return an error here (and propagate it later on) if an
> invalid value is passed in? Or perhaps even avoid calling this function
> with an invalid value in the first place? As it is, you're silently
> ignoring cases where an invalid value is being passed in. That'll leave
> anybody working with this driver completely puzzled when it doesn't
> behave the way they expect it too. And it gives users no indication
> about what went wrong.

I will add a code to return an error here.

> > +
> > +	val &= ~(RCAR_PWMCR_CCMD | RCAR_PWMCR_CC0_MASK);
> > +	if (div & 1)
> > +		val |= RCAR_PWMCR_CCMD;
> > +	div >>= 1;
> > +	val |= div << RCAR_PWMCR_CC0_SHIFT;
> > +	rcar_pwm_write(rp, val, RCAR_PWMCR);
> > +}
> > +
> > +static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div,
> > +				 int duty_ns, int period_ns)
> > +{
> > +	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
> > +	unsigned long clk_rate = clk_get_rate(rp->clk);
> > +	u32 cyc, ph;
> > +
> > +	one_cycle = (unsigned long long)NSEC_PER_SEC * 100ULL * (1 << div);
> > +	do_div(one_cycle, clk_rate);
> > +
> > +	tmp = period_ns * 100ULL;
> > +	do_div(tmp, one_cycle);
> > +	cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
> > +
> > +	tmp = duty_ns * 100ULL;
> > +	do_div(tmp, one_cycle);
> > +	ph = tmp & RCAR_PWMCNT_PH0_MASK;
> > +
> > +	/* Avoid prohibited setting */
> > +	if (cyc && ph)
> > +		rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> 
> So if a period and duty-cycle are passed in that yield the prohibited
> setting the operation will simply be silently ignored?

Yes, to update values of pwm->duty_cycle and ->period by pwm_config(),
this code will be silently ignored.

> > +}
> > +
> > +static int rcar_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> > +
> > +	return clk_prepare_enable(rp->clk);
> > +}
> > +
> > +static void rcar_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> > +
> > +	clk_disable_unprepare(rp->clk);
> > +}
> > +
> > +static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			   int duty_ns, int period_ns)
> > +{
> > +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> > +	int div;
> > +
> > +	div = rcar_pwn_get_clock_division(rp, period_ns);
> 
> The above three lines can be collapsed into a single one.

I will fix this.

> > +
> > +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> > +	rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
> > +	rcar_pwm_set_clock_control(rp, div);
> > +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> > +	u32 pwmcnt;
> > +
> > +	/* Don't enable the PWM device if CYC0 or PH0 is 0 */
> > +	pwmcnt = rcar_pwm_read(rp, RCAR_PWMCNT);
> > +	if (!(pwmcnt & RCAR_PWMCNT_CYC0_MASK) ||
> > +	    !(pwmcnt & RCAR_PWMCNT_PH0_MASK))
> > +		return -EINVAL;
> > +
> > +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_EN0, RCAR_PWMCR_EN0, RCAR_PWMCR);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> > +
> > +	rcar_pwm_bit_modify(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
> > +}
> > +
> > +static const struct pwm_ops rcar_pwm_ops = {
> > +	.request	= rcar_pwm_request,
> > +	.free		= rcar_pwm_free,
> > +	.config		= rcar_pwm_config,
> > +	.enable		= rcar_pwm_enable,
> > +	.disable	= rcar_pwm_disable,
> > +	.owner		= THIS_MODULE,
> > +};
> 
> No need for this padding. Single spaces around = are good enough.

I got it. I will fix it.

> > +
> > +static int rcar_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct rcar_pwm_chip *rcar_pwm;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	rcar_pwm = devm_kzalloc(&pdev->dev, sizeof(*rcar_pwm), GFP_KERNEL);
> > +	if (rcar_pwm == NULL)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	rcar_pwm->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(rcar_pwm->base))
> > +		return PTR_ERR(rcar_pwm->base);
> > +
> > +	rcar_pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(rcar_pwm->clk)) {
> > +		dev_err(&pdev->dev, "cannot get clock\n");
> > +		return PTR_ERR(rcar_pwm->clk);
> > +	}
> > +
> > +	platform_set_drvdata(pdev, rcar_pwm);
> > +
> > +	rcar_pwm->chip.dev = &pdev->dev;
> > +	rcar_pwm->chip.ops = &rcar_pwm_ops;
> > +	rcar_pwm->chip.of_xlate = of_pwm_xlate_with_flags;
> > +	rcar_pwm->chip.base = -1;
> > +	rcar_pwm->chip.npwm = 1;
> > +
> > +	ret = pwmchip_add(&rcar_pwm->chip);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed to register PWM chip\n");
> > +		return ret;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "R-Car PWM Timer registered\n");
> 
> No need to brag about success. Expect that things will go well and let
> users know when they don't. Output error messages, not success messages.

I got it. I will remove this message.

> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rcar_pwm_remove(struct platform_device *pdev)
> > +{
> > +	struct rcar_pwm_chip *rcar_pwm = platform_get_drvdata(pdev);
> > +	int ret;
> > +
> > +	ret = pwmchip_remove(&rcar_pwm->chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pm_runtime_disable(&pdev->dev);
> 
> Perhaps you'd still like to disable runtime PM even if the chip can't be
> removed?

Thank you for the point. I will fix this.

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id rcar_pwm_of_table[] = {
> > +	{ .compatible = "renesas,pwm-rcar", },
> > +	{ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, rcar_pwm_of_table);
> 
> No blank line between the above two.

I will remove the blank line.

Best regards,
Yoshihiro Shimoda

> Thierry

Re: [PATCH v4 2/2] pwm: Add support for R-Car PWM Timer

[Thierry Reding]

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

On Mon, Jun 15, 2015 at 05:48:00AM +0000, Yoshihiro Shimoda wrote:
[...]
> > > +static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div,
> > > +				 int duty_ns, int period_ns)
> > > +{
> > > +	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
> > > +	unsigned long clk_rate = clk_get_rate(rp->clk);
> > > +	u32 cyc, ph;
> > > +
> > > +	one_cycle = (unsigned long long)NSEC_PER_SEC * 100ULL * (1 << div);
> > > +	do_div(one_cycle, clk_rate);
> > > +
> > > +	tmp = period_ns * 100ULL;
> > > +	do_div(tmp, one_cycle);
> > > +	cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
> > > +
> > > +	tmp = duty_ns * 100ULL;
> > > +	do_div(tmp, one_cycle);
> > > +	ph = tmp & RCAR_PWMCNT_PH0_MASK;
> > > +
> > > +	/* Avoid prohibited setting */
> > > +	if (cyc && ph)
> > > +		rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> > 
> > So if a period and duty-cycle are passed in that yield the prohibited
> > setting the operation will simply be silently ignored?
> 
> Yes, to update values of pwm->duty_cycle and ->period by pwm_config(),
> this code will be silently ignored.

That's not right. If someone passes in invalid values you should report
an error.

Also, are you sure the check is correct? The current check:

	if (cyc && ph)

is the same as

	if (cyc != 0 && ph != 0)

which means that you will never be able to set a zero duty cycle. Is
that really what you want here?

Thierry

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

RE: [PATCH v4 2/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda]

Hi Thierry,

> Sent: Monday, June 29, 2015 6:12 PM
> 
> On Mon, Jun 15, 2015 at 05:48:00AM +0000, Yoshihiro Shimoda wrote:
> [...]
> > > > +static void rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div,
> > > > +				 int duty_ns, int period_ns)
> > > > +{
> > > > +	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
> > > > +	unsigned long clk_rate = clk_get_rate(rp->clk);
> > > > +	u32 cyc, ph;
> > > > +
> > > > +	one_cycle = (unsigned long long)NSEC_PER_SEC * 100ULL * (1 << div);
> > > > +	do_div(one_cycle, clk_rate);
> > > > +
> > > > +	tmp = period_ns * 100ULL;
> > > > +	do_div(tmp, one_cycle);
> > > > +	cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
> > > > +
> > > > +	tmp = duty_ns * 100ULL;
> > > > +	do_div(tmp, one_cycle);
> > > > +	ph = tmp & RCAR_PWMCNT_PH0_MASK;
> > > > +
> > > > +	/* Avoid prohibited setting */
> > > > +	if (cyc && ph)
> > > > +		rcar_pwm_write(rp, cyc | ph, RCAR_PWMCNT);
> > >
> > > So if a period and duty-cycle are passed in that yield the prohibited
> > > setting the operation will simply be silently ignored?
> >
> > Yes, to update values of pwm->duty_cycle and ->period by pwm_config(),
> > this code will be silently ignored.
> 
> That's not right. If someone passes in invalid values you should report
> an error.

Thank you for the comment.
After I sent the previous email, I was also confusing this behavior.
So, I fixed it in v5 patch.
(In other words, if the PWM is enabled and a user input invalid values,
 this driver will report an error.)

I'm sorry, I should have explained that in this email thread.

> Also, are you sure the check is correct? The current check:
> 
> 	if (cyc && ph)
> 
> is the same as
> 
> 	if (cyc != 0 && ph != 0)
> 
> which means that you will never be able to set a zero duty cycle. Is
> that really what you want here?

Yes, I really want to avoid to set a zero duty cycle because
the PWM doesn't accept it.
(According to the datasheet, it said "Setting H'000 is prohibited.")

Best regards,
Yoshihiro Shimoda

> Thierry

RE: [PATCH v4 0/2] pwm: Add support for R-Car PWM Timer

[Yoshihiro Shimoda]

Hi Thierry,

Would you apply this patch set to your repository?
I checked that I could apply this patch set correctly
on the latest linux-pwm.git / for-next branch.
(commit id = cccb94543c8299e0bc7564cc6f8b26e0f15bafde)

Best regards,
Yoshihiro Shimoda

> Sent: Thursday, May 21, 2015 7:57 PM
> 
> This patch set is based on the linux-pwm.git / for-next branch.
> (commit id = efb0de55b6a2ec15fc424e660601f22ae2fa487a)
> 
> Changes from v3:
>  - Fix register size in patch 1.
>  - Add "Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>" in patch 1
>  - Remove an unnecessary definition in patch 2.
>  - Use "ULL" to avoid overflow in patch 2.
>  - Remove unnecessary casts in patch 2.
> 
> Changes from v2:
>  - Add compatible string "renesas,pwm-rcar".
>  - Remove compatible strings "renesas,pwm-r8a77xx" in rcar_pwm_of_table.
>  - Fix build error.
> 
> Changes from v1:
>  - Change compatible string to SoC-specific compatible values.
>  - Fix #pwm-call value to 2 in the device tree document.
>  - Fix "depends on" value in Kconfig.
>  - Fix help explanation in Kconfig.
>  - Remove an unnecessary member in rcar_pwm_chip.
>  - Remove hardcoded number of channels and change chip.npwm value to 1.
>  - Fix formulas for clock calculation to improve accuracy.
> 
> Yoshihiro Shimoda (2):
>   pwm: Add device tree binding document for R-Car PWM Timer
>   pwm: Add support for R-Car PWM Timer
> 
>  .../devicetree/bindings/pwm/renesas,pwm-rcar.txt   |  27 +++
>  drivers/pwm/Kconfig                                |  11 +
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-rcar.c                             | 261 +++++++++++++++++++++
>  4 files changed, 300 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/renesas,pwm-rcar.txt
>  create mode 100644 drivers/pwm/pwm-rcar.c
> 
> --
> 1.9.1