[PATCH 0/3] Add Rockchip PWM driver

[PATCH 0/3] Add Rockchip PWM driver

[Beniamino Galvani]

This series adds support for the PWM controller found on Rockchip
RK29, RK30 and RK31 SoCs. It is based on the documentation publicly
available [0] and kernel sources from Rockchip [1].

The last patch, which adds device tree nodes in the dts, depends on
the clocks introduced by Heiko in his recent submission of a clk
driver for Rockchip RK3188.

[0] http://dl.radxa.com/rock/docs/hw/ds/Rockchip%20RK30xx%20TRM%20V2.0.pdf
[1] https://github.com/rkchrome/kernel

Beniamino Galvani (3):
  pwm: add Rockchip SoC PWM support
  pwm: rockchip: document device tree bindings
  ARM: dts: rk3xxx: add PWM nodes

 .../devicetree/bindings/pwm/pwm-rockchip.txt       |   17 ++
 arch/arm/boot/dts/rk3188.dtsi                      |   40 +++++
 arch/arm/boot/dts/rk3xxx.dtsi                      |   32 ++++
 drivers/pwm/Kconfig                                |    8 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-rockchip.c                         |  180 ++++++++++++++++++++
 6 files changed, 278 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
 create mode 100644 drivers/pwm/pwm-rockchip.c

-- 
1.7.10.4

[PATCH 1/3] pwm: add Rockchip SoC PWM support

[Beniamino Galvani]

This commit adds a driver for the PWM controller found on Rockchip
RK29, RK30 and RK31 SoCs.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 drivers/pwm/Kconfig        |    8 ++
 drivers/pwm/Makefile       |    1 +
 drivers/pwm/pwm-rockchip.c |  180 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+)
 create mode 100644 drivers/pwm/pwm-rockchip.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 5b34ff2..2e92245 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -197,6 +197,14 @@ config PWM_RENESAS_TPU
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-renesas-tpu.
 
+config PWM_ROCKCHIP
+	tristate "Rockchip PWM support"
+	depends on ARCH_ROCKCHIP
+	depends on OF
+	help
+	  Generic PWM framework driver for the PWM controller found on
+	  Rockchip SoCs.
+
 config PWM_SAMSUNG
 	tristate "Samsung PWM support"
 	depends on PLAT_SAMSUNG
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index e57d2c3..2e347f7 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 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_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_TEGRA)		+= pwm-tegra.o
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
new file mode 100644
index 0000000..9b51828
--- /dev/null
+++ b/drivers/pwm/pwm-rockchip.c
@@ -0,0 +1,180 @@
+/*
+ * PWM driver for Rockchip SoCs
+ *
+ * Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#define PWM_CNTR		0x00		/* Counter register */
+#define PWM_HRC			0x04		/* High reference register */
+#define PWM_LRC			0x08		/* Low reference register */
+#define PWM_CTRL		0x0c		/* Control register */
+#define PWM_CTRL_TIMER_EN	(1 << 0)
+#define PWM_CTRL_OUTPUT_EN	(1 << 3)
+
+#define PRESCALER		2
+#define NSECS_PER_SEC		1000000000
+
+struct rockchip_pwm_chip {
+	struct pwm_chip	chip;
+	struct clk	*clk;
+	void __iomem	*base;
+};
+
+static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
+{
+	return container_of(c, struct rockchip_pwm_chip, chip);
+}
+
+static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			       int duty_ns, int period_ns)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	unsigned long clk_rate, period, duty;
+	u64 div;
+	int ret;
+
+	clk_rate = clk_get_rate(pc->clk);
+
+	/*
+	 * Since period and duty cycle registers have a width of 32
+	 * bits, every possible input period can be obtained using the
+	 * default prescaler value for all practical clock rate values.
+	 */
+	div = clk_rate;
+	div *= period_ns;
+	do_div(div, PRESCALER * NSECS_PER_SEC);
+	period = div;
+
+	div = clk_rate;
+	div *= duty_ns;
+	do_div(div, PRESCALER * NSECS_PER_SEC);
+	duty = div;
+
+	ret = clk_enable(pc->clk);
+	if (ret)
+		return ret;
+
+	writel(period, pc->base + PWM_LRC);
+	writel(duty, pc->base + PWM_HRC);
+	writel(0, pc->base + PWM_CNTR);
+
+	clk_disable(pc->clk);
+
+	return 0;
+}
+
+static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	int ret;
+	u32 val;
+
+	ret = clk_enable(pc->clk);
+	if (ret)
+		return ret;
+
+	val = readl_relaxed(pc->base + PWM_CTRL);
+	val |= PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
+	writel_relaxed(val, pc->base + PWM_CTRL);
+
+	return 0;
+}
+
+static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 val;
+
+	val = readl_relaxed(pc->base + PWM_CTRL);
+	val &= ~(PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN);
+	writel_relaxed(val, pc->base + PWM_CTRL);
+
+	clk_disable(pc->clk);
+}
+
+static struct pwm_ops rockchip_pwm_ops = {
+	.config	= rockchip_pwm_config,
+	.enable = rockchip_pwm_enable,
+	.disable = rockchip_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int rockchip_pwm_probe(struct platform_device *pdev)
+{
+	struct rockchip_pwm_chip *pc;
+	struct resource *r;
+	int ret;
+
+	pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL);
+	if (!pc)
+		return -ENOMEM;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pc->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(pc->base))
+		return PTR_ERR(pc->base);
+
+	pc->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(pc->clk))
+		return PTR_ERR(pc->clk);
+
+	ret = clk_prepare(pc->clk);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, pc);
+
+	pc->chip.dev = &pdev->dev;
+	pc->chip.ops = &rockchip_pwm_ops;
+	pc->chip.base = -1;
+	pc->chip.npwm = 1;
+
+	ret = pwmchip_add(&pc->chip);
+	if (ret < 0) {
+		clk_unprepare(pc->clk);
+		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+	}
+
+	return ret;
+}
+
+static int rockchip_pwm_remove(struct platform_device *pdev)
+{
+	struct rockchip_pwm_chip *pc = platform_get_drvdata(pdev);
+
+	clk_unprepare(pc->clk);
+
+	return pwmchip_remove(&pc->chip);
+}
+
+static const struct of_device_id rockchip_pwm_dt_ids[] = {
+	{ .compatible = "rockchip,rk2928-pwm" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rockchip_pwm_id_ids);
+
+static struct platform_driver rockchip_pwm_driver = {
+	.driver = {
+		.name = "rockchip-pwm",
+		.owner = THIS_MODULE,
+		.of_match_table = rockchip_pwm_dt_ids,
+	},
+	.probe	= rockchip_pwm_probe,
+	.remove = rockchip_pwm_remove,
+};
+module_platform_driver(rockchip_pwm_driver);
+
+MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>");
+MODULE_DESCRIPTION("Rockchip PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.10.4

[PATCH 2/3] pwm: rockchip: document device tree bindings

[Beniamino Galvani]

This adds binding documentation for Rockchip SoC PWM driver.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 .../devicetree/bindings/pwm/pwm-rockchip.txt          |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-rockchip.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
new file mode 100644
index 0000000..35deba4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
@@ -0,0 +1,17 @@
+Rockchip PWM controller
+
+Required properties:
+ - compatible: should be "rockchip,rk2928-pwm"
+ - reg: physical base address and length of the controller's registers
+ - clocks: phandle and clock specifier of the PWM reference clock
+ - #pwm-cells: should be 2. See pwm.txt in this directory for a
+   description of the cell format
+
+Example:
+
+	pwm0: pwm@20030000 {
+		compatible = "rockchip,rk2928-pwm";
+		reg = <0x20030000 0x10>;
+		clocks = <&cru PCLK_PWM01>;
+		#pwm-cells = <2>;
+	};
-- 
1.7.10.4

[PATCH 3/3] ARM: dts: rk3xxx: add PWM nodes

[Beniamino Galvani]

This adds PWM nodes to the Rockchip device trees.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 arch/arm/boot/dts/rk3188.dtsi |   40 ++++++++++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/rk3xxx.dtsi |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index f1d8651..7dd8f6c 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -125,6 +125,22 @@
 			clock-names = "biu", "ciu";
 		};
 
+		pwm0: pwm@20030000 {
+			clocks = <&cru PCLK_PWM01>;
+		};
+
+		pwm1: pwm@20030010 {
+			clocks = <&cru PCLK_PWM01>;
+		};
+
+		pwm2: pwm@20050020 {
+			clocks = <&cru PCLK_PWM23>;
+		};
+
+		pwm3: pwm@20050030 {
+			clocks = <&cru PCLK_PWM23>;
+		};
+
 		cru: cru@20000000 {
 			compatible = "rockchip,rk3188-cru";
 			reg = <0x20000000 0x1000>,
@@ -331,6 +347,30 @@
 							<RK_GPIO3 20 RK_FUNC_1 &pcfg_pull_none>;
 				};
 			};
+
+			pwm0 {
+				pwm0_pins: pwm0-pins {
+					rockchip,pins = <RK_GPIO3 27 RK_FUNC_1 &pcfg_pull_none>;
+				};
+			};
+
+			pwm1 {
+				pwm1_pins: pwm1-pins {
+					rockchip,pins = <RK_GPIO3 28 RK_FUNC_1 &pcfg_pull_none>;
+				};
+			};
+
+			pwm2 {
+				pwm2_pins: pwm2-pins {
+					rockchip,pins = <RK_GPIO3 29 RK_FUNC_1 &pcfg_pull_none>;
+				};
+			};
+
+			pwm3 {
+				pwm3_pins: pwm3-pins {
+					rockchip,pins = <RK_GPIO3 30 RK_FUNC_1 &pcfg_pull_none>;
+				};
+			};
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/rk3xxx.dtsi b/arch/arm/boot/dts/rk3xxx.dtsi
index 26e5a96..c46d552 100644
--- a/arch/arm/boot/dts/rk3xxx.dtsi
+++ b/arch/arm/boot/dts/rk3xxx.dtsi
@@ -130,5 +130,37 @@
 
 			status = "disabled";
 		};
+
+		pwm0: pwm@20030000 {
+			compatible = "rockchip,rk2928-pwm";
+			reg = <0x20030000 0x10>;
+			#pwm-cells = <2>;
+			clocks = <&clk_gates7 10>;
+			status = "disabled";
+		};
+
+		pwm1: pwm@20030010 {
+			compatible = "rockchip,rk2928-pwm";
+			reg = <0x20030010 0x10>;
+			#pwm-cells = <2>;
+			clocks = <&clk_gates7 10>;
+			status = "disabled";
+		};
+
+		pwm2: pwm@20050020 {
+			compatible = "rockchip,rk2928-pwm";
+			reg = <0x20050020 0x10>;
+			#pwm-cells = <2>;
+			clocks = <&clk_gates7 11>;
+			status = "disabled";
+		};
+
+		pwm3: pwm@20050030 {
+			compatible = "rockchip,rk2928-pwm";
+			reg = <0x20050030 0x10>;
+			#pwm-cells = <2>;
+			clocks = <&clk_gates7 11>;
+			status = "disabled";
+		};
 	};
 };
-- 
1.7.10.4

Re: [PATCH 1/3] pwm: add Rockchip SoC PWM support

[Thierry Reding]

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

On Thu, May 08, 2014 at 01:08:33AM +0200, Beniamino Galvani wrote:
> This commit adds a driver for the PWM controller found on Rockchip
> RK29, RK30 and RK31 SoCs.
> 
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> ---
>  drivers/pwm/Kconfig        |    8 ++
>  drivers/pwm/Makefile       |    1 +
>  drivers/pwm/pwm-rockchip.c |  180 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 189 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rockchip.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 5b34ff2..2e92245 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -197,6 +197,14 @@ config PWM_RENESAS_TPU
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-renesas-tpu.
>  
> +config PWM_ROCKCHIP
> +	tristate "Rockchip PWM support"
> +	depends on ARCH_ROCKCHIP
> +	depends on OF

It seems like ARCH_ROCKCHIP depends on OF already (via ARCH_MULTI_V7 and
ARCH_MULTIPLATFORM), so having the dependency explicitly here seems
redundant.

> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
[...]
> +#define PRESCALER		2
> +#define NSECS_PER_SEC		1000000000

You should use NSEC_PER_SEC from include/linux/time.h.

> +struct rockchip_pwm_chip {
> +	struct pwm_chip	chip;
> +	struct clk	*clk;
> +	void __iomem	*base;
> +};

I prefer no artificial padding within structure definitions.

> +static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       int duty_ns, int period_ns)
> +{
> +	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> +	unsigned long clk_rate, period, duty;
> +	u64 div;
> +	int ret;
> +
> +	clk_rate = clk_get_rate(pc->clk);
> +
> +	/*
> +	 * Since period and duty cycle registers have a width of 32
> +	 * bits, every possible input period can be obtained using the
> +	 * default prescaler value for all practical clock rate values.
> +	 */
> +	div = clk_rate;
> +	div *= period_ns;

Perhaps shorten this to "div = clk_rate * period_ns;"?

> +	do_div(div, PRESCALER * NSECS_PER_SEC);
> +	period = div;
> +
> +	div = clk_rate;
> +	div *= duty_ns;

And this to "div = clk_rate * duty_ns;"?

> +	do_div(div, PRESCALER * NSECS_PER_SEC);
> +	duty = div;
> +
> +	ret = clk_enable(pc->clk);
> +	if (ret)
> +		return ret;
> +
> +	writel(period, pc->base + PWM_LRC);
> +	writel(duty, pc->base + PWM_HRC);
> +	writel(0, pc->base + PWM_CNTR);
> +
> +	clk_disable(pc->clk);
> +
> +	return 0;
> +}
[...]
> +static struct pwm_ops rockchip_pwm_ops = {

static const please.

> +	.config	= rockchip_pwm_config,

There's a tab between .config and = above. It should be a space.

> +static const struct of_device_id rockchip_pwm_dt_ids[] = {
> +	{ .compatible = "rockchip,rk2928-pwm" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rockchip_pwm_id_ids);

The name in the MODULE_DEVICE_TABLE doesn't match the array name above.
Does this even build?

> +static struct platform_driver rockchip_pwm_driver = {
> +	.driver = {
> +		.name = "rockchip-pwm",
> +		.owner = THIS_MODULE,

You no longer need to initialize this explicitly, module_platform_driver
does it for you.

> +		.of_match_table = rockchip_pwm_dt_ids,
> +	},
> +	.probe	= rockchip_pwm_probe,

There's another tab instead of space here.

> +	.remove = rockchip_pwm_remove,
> +};
> +module_platform_driver(rockchip_pwm_driver);
> +
> +MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>");
> +MODULE_DESCRIPTION("Rockchip PWM driver");

Perhaps "Rockchip SoC PWM driver"?

Thierry

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

Re: [PATCH 2/3] pwm: rockchip: document device tree bindings

[Thierry Reding]

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

On Thu, May 08, 2014 at 01:08:34AM +0200, Beniamino Galvani wrote:
[...]
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
> new file mode 100644
> index 0000000..35deba4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
> @@ -0,0 +1,17 @@
> +Rockchip PWM controller
> +
> +Required properties:
> + - compatible: should be "rockchip,rk2928-pwm"
> + - reg: physical base address and length of the controller's registers
> + - clocks: phandle and clock specifier of the PWM reference clock
> + - #pwm-cells: should be 2. See pwm.txt in this directory for a
> +   description of the cell format

Nit: this sentence should have a full stop at the end.

Thierry

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

Re: [PATCH 1/3] pwm: add Rockchip SoC PWM support

[Beniamino Galvani]

On Tue, Jun 17, 2014 at 11:42:58PM +0200, Thierry Reding wrote:
> On Thu, May 08, 2014 at 01:08:33AM +0200, Beniamino Galvani wrote:
> > This commit adds a driver for the PWM controller found on Rockchip
> > RK29, RK30 and RK31 SoCs.
> > 
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> > ---
> >  drivers/pwm/Kconfig        |    8 ++
> >  drivers/pwm/Makefile       |    1 +
> >  drivers/pwm/pwm-rockchip.c |  180 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 189 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-rockchip.c
> > 
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 5b34ff2..2e92245 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -197,6 +197,14 @@ config PWM_RENESAS_TPU
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called pwm-renesas-tpu.
> >  
> > +config PWM_ROCKCHIP
> > +	tristate "Rockchip PWM support"
> > +	depends on ARCH_ROCKCHIP
> > +	depends on OF
> 
> It seems like ARCH_ROCKCHIP depends on OF already (via ARCH_MULTI_V7 and
> ARCH_MULTIPLATFORM), so having the dependency explicitly here seems
> redundant.

Right, I will remove the dependency.

> 
> > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> [...]
> > +#define PRESCALER		2
> > +#define NSECS_PER_SEC		1000000000
> 
> You should use NSEC_PER_SEC from include/linux/time.h.

Ok.

> 
> > +struct rockchip_pwm_chip {
> > +	struct pwm_chip	chip;
> > +	struct clk	*clk;
> > +	void __iomem	*base;
> > +};
> 
> I prefer no artificial padding within structure definitions.
> 
> > +static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			       int duty_ns, int period_ns)
> > +{
> > +	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> > +	unsigned long clk_rate, period, duty;
> > +	u64 div;
> > +	int ret;
> > +
> > +	clk_rate = clk_get_rate(pc->clk);
> > +
> > +	/*
> > +	 * Since period and duty cycle registers have a width of 32
> > +	 * bits, every possible input period can be obtained using the
> > +	 * default prescaler value for all practical clock rate values.
> > +	 */
> > +	div = clk_rate;
> > +	div *= period_ns;
> 
> Perhaps shorten this to "div = clk_rate * period_ns;"?

I will change this, adding a cast to avoid the truncation of the
result to 32 bits: "div = (u64)clk_rate * period_ns;"

> 
> > +	do_div(div, PRESCALER * NSECS_PER_SEC);
> > +	period = div;
> > +
> > +	div = clk_rate;
> > +	div *= duty_ns;
> 
> And this to "div = clk_rate * duty_ns;"?

Ditto.

> 
> > +	do_div(div, PRESCALER * NSECS_PER_SEC);
> > +	duty = div;
> > +
> > +	ret = clk_enable(pc->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	writel(period, pc->base + PWM_LRC);
> > +	writel(duty, pc->base + PWM_HRC);
> > +	writel(0, pc->base + PWM_CNTR);
> > +
> > +	clk_disable(pc->clk);
> > +
> > +	return 0;
> > +}
> [...]
> > +static struct pwm_ops rockchip_pwm_ops = {
> 
> static const please.
> 
> > +	.config	= rockchip_pwm_config,
> 
> There's a tab between .config and = above. It should be a space.
> 
> > +static const struct of_device_id rockchip_pwm_dt_ids[] = {
> > +	{ .compatible = "rockchip,rk2928-pwm" },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rockchip_pwm_id_ids);
> 
> The name in the MODULE_DEVICE_TABLE doesn't match the array name above.
> Does this even build?

Sorry, I didn't test the driver as a module, which indeed doesn't
compile.

> 
> > +static struct platform_driver rockchip_pwm_driver = {
> > +	.driver = {
> > +		.name = "rockchip-pwm",
> > +		.owner = THIS_MODULE,
> 
> You no longer need to initialize this explicitly, module_platform_driver
> does it for you.
> 
> > +		.of_match_table = rockchip_pwm_dt_ids,
> > +	},
> > +	.probe	= rockchip_pwm_probe,
> 
> There's another tab instead of space here.
> 
> > +	.remove = rockchip_pwm_remove,
> > +};
> > +module_platform_driver(rockchip_pwm_driver);
> > +
> > +MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>");
> > +MODULE_DESCRIPTION("Rockchip PWM driver");
> 
> Perhaps "Rockchip SoC PWM driver"?

Seems reasonable; I will fix this and the other issues in v2, thanks.

Beniamino

Re: [PATCH 1/3] pwm: add Rockchip SoC PWM support

[Thierry Reding]

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

On Sat, Jun 21, 2014 at 12:00:36AM +0200, Beniamino Galvani wrote:
> On Tue, Jun 17, 2014 at 11:42:58PM +0200, Thierry Reding wrote:
> > On Thu, May 08, 2014 at 01:08:33AM +0200, Beniamino Galvani wrote:
[...]
> > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
[...]
> > > +static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			       int duty_ns, int period_ns)
> > > +{
> > > +	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> > > +	unsigned long clk_rate, period, duty;
> > > +	u64 div;
> > > +	int ret;
> > > +
> > > +	clk_rate = clk_get_rate(pc->clk);
> > > +
> > > +	/*
> > > +	 * Since period and duty cycle registers have a width of 32
> > > +	 * bits, every possible input period can be obtained using the
> > > +	 * default prescaler value for all practical clock rate values.
> > > +	 */
> > > +	div = clk_rate;
> > > +	div *= period_ns;
> > 
> > Perhaps shorten this to "div = clk_rate * period_ns;"?
> 
> I will change this, adding a cast to avoid the truncation of the
> result to 32 bits: "div = (u64)clk_rate * period_ns;"

Alternatively you could simply make clk_rate a u64 since it's only used
in this context anyway.

Thierry

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